CreateSpecificBlockchains #467
Conversation
patrick-ogrady
added
the
DO NOT MERGE
patrick-ogrady
removed
the
DO NOT MERGE
patrick-ogrady
added
the
DO NOT MERGE
patrick-ogrady
removed
the
DO NOT MERGE
There was a problem hiding this comment.
I am ok to merge this, with comments addressed. Probably with all names changed to FoorBarExperimental. Probably also keeping server async, unless we want that change in server behaviour.
And as soon as posible, we may want to migrate this to local lib, following the style there. Probably overload CreateSubnets a little more with the funcitonality. Also avoid server to block, unless we will want server to be sync now. I am also tempted to enable AddNode to create multiple nodes, and to be more flexible regarding the options (not to ask for a full config). But this is the least idea.
rpcpb/rpc.proto
Outdated
|
|
||
| message CreateSpecificBlockchainsResponse { | ||
| ClusterInfo cluster_info = 1; | ||
| repeated CustomChainInfo chains = 2; |
There was a problem hiding this comment.
Isn't this included in cluster_info? custom_chains
| @@ -481,3 +489,13 @@ message GetSnapshotNamesRequest { | |||
| message GetSnapshotNamesResponse { | |||
| repeated string snapshot_names = 1; | |||
| } | |||
|
|
|||
| message CreateSpecificBlockchainsRequest { | |||
| string exec_path = 1; | |||
There was a problem hiding this comment.
the function should take default network exec path probably, and have this one optional
| @@ -481,3 +489,13 @@ message GetSnapshotNamesRequest { | |||
| message GetSnapshotNamesResponse { | |||
| repeated string snapshot_names = 1; | |||
| } | |||
|
|
|||
| message CreateSpecificBlockchainsRequest { | |||
There was a problem hiding this comment.
this functionality should belong to CreateBlockchains, maybe we can rename it to something-experimental to avoid user confusion
| // validators, they will be added. This function blocks until all blockchains | ||
| // have been created and are healthy. This function returns the chains | ||
| // created and the participants that are on each chain. | ||
| // |
There was a problem hiding this comment.
Probably also comment here that the nodes are also created
There was a problem hiding this comment.
I would have added the node creation and subnet participation logic to CreateSubnetsExperimental, with multiple subnets configuration at a time. That is to be clear in breaking a 1-1 subnet-blockchain relation
There was a problem hiding this comment.
Maybe we can call it CreateBlockchainsExperimental ?
| defer ln.lock.Unlock() | ||
|
|
||
| // Create new subnets and blockchains | ||
| chainSpecs, chainIDs, err := experimental.CreateSpecificBlockchains(ctx, ln, execPath, chainSpecs, redirectOutput) |
There was a problem hiding this comment.
we can take execPath from ln.binaryPath, if not given, and let it be optional at a server level
| if ready { | ||
| break | ||
| } | ||
| time.Sleep(1 * time.Second) |
There was a problem hiding this comment.
maybe we can use a select and time.After
| return err | ||
| } | ||
| nodeIDs := make([]ids.NodeID, len(spec.Participants)) | ||
| for i, nodeName := range spec.Participants { |
There was a problem hiding this comment.
maybe we can have a wallet function to add subnet validators also
| ) | ||
| } | ||
|
|
||
| // Wait for validators to become active |
There was a problem hiding this comment.
also maybe this can be split, as a network may want to do something in the middle
| if ready { | ||
| break | ||
| } | ||
| time.Sleep(1 * time.Second) |
| // if err := ln.ReloadVMPluginsUnsafe(ctx); err != nil { | ||
| // return nil, nil, err | ||
| // } | ||
| if err := wallet.createBlockchains(ctx, chainSpecs, blockchainTxs); err != nil { |
There was a problem hiding this comment.
it is nice to see the same wallet can be used to issue the txs after it was also used to create those same tx with UTXOs and tx tracking
| "github.com/ava-labs/avalanche-network-runner/rpcpb" | ||
| ) | ||
|
|
||
| func deepCopy(i *rpcpb.ClusterInfo) (*rpcpb.ClusterInfo, error) { |
There was a problem hiding this comment.
Testing this now in HyperSDK: ava-labs/hypersdk#95
There was a problem hiding this comment.
It may only be necessary to clone the strings in the chains field in CreateSpecificBlockchainsResponse (would reference same byte arrays as chainInfo).
Still researching why any of this would be related (assuming this actually solves the issue)...seems like gRPC should handle byte reuse in the marshaled response without issue and this is a bug elsewhere.
There was a problem hiding this comment.
It is also possible that there is a race such that clusterInfo changes while it is being encoded? (this seems unlikely given we aren't making calls concurrently)
| } | ||
| return &rpcpb.CreateSpecificBlockchainsResponse{ClusterInfo: s.clusterInfo, Chains: chainIDs}, nil | ||
| return &rpcpb.CreateSpecificBlockchainsResponse{ClusterInfo: clusterInfo, Chains: chainIDs}, nil |
There was a problem hiding this comment.
Technically, returning s.clusterInfo in this response is not safe as it may be modified as soon as the lock is released.
|
Closing. Pretty important PR with lots of new implementations! All of them where added to local/blockchain.go on v1.4.0. Thanks! |
Replaces: #463