move node types and handlings to separate packages #729
Conversation
…reth into move-atomic-network-handlers
…reth into move-atomic-network-handlers
| return nil | ||
| } | ||
|
|
||
| func (vm *VM) RegisterLeafRequestHandler(nodeType message.NodeType, metricName string, trieDB *triedb.Database, trieKeyLen int, useSnapshot bool) error { |
There was a problem hiding this comment.
Hmm I wonder if we should register an interface rather than nodeType directly. I feel like there is definitely room to improve this PR in general, but did not want to break it anything further.
plugin/evm/vm.go
Outdated
| @@ -1218,15 +1226,41 @@ func (vm *VM) setAppRequestHandlers() { | |||
| }, | |||
| }, | |||
| ) | |||
| if err := vm.RegisterLeafRequestHandler(message.StateTrieNode, "sync_state_trie_leaves", evmTrieDB, message.StateTrieKeyLength, true); err != nil { | |||
| return err | |||
There was a problem hiding this comment.
nit
| return err | |
| return fmt.Errorf("failed registering state trie database: %s", err) |
maybe %w is needed though
plugin/evm/vm.go
Outdated
| } | ||
| // Register atomic trieDB for serving atomic leafs requests. | ||
| if err := vm.RegisterLeafRequestHandler(atomic.AtomicTrieNode, "sync_atomic_trie_leaves", vm.atomicTrie.TrieDB(), atomic.AtomicTrieKeyLength, false); err != nil { | ||
| return err |
There was a problem hiding this comment.
nit
| return err | |
| return fmt.Errorf("failed registering atomic trie database: %s", err) |
maybe %w is needed though
sync/README.md
Outdated
| @@ -34,7 +34,7 @@ State sync code is structured as follows: | |||
| - `plugin/evm/`: The engine expects the VM to implement `StateSyncableVM` interface, | |||
| - `StateSyncServer`: Contains methods executed on nodes _serving_ state sync requests. | |||
| - `StateSyncClient`: Contains methods executed on nodes joining the network via state sync, and orchestrates the top level steps of the sync. | |||
| - `peer`: Contains abstractions used by `sync/statesync` to send requests to peers (`AppRequest`) and receive responses from peers (`AppResponse`). | |||
| - `peer`: Contains abstractions used by `sync/statesync` to send requests to peers (`AppRequest`) and receive responses from peers (`AppResponse`). | |||
There was a problem hiding this comment.
nit revert this, the trailing space is a bit out of scope 😄 Although we probably want to setup CI markdown checks at some point to have consistent markdown.
sync/handlers/leafs_request.go
Outdated
| if len(leafsRequest.Start) != 0 && len(leafsRequest.Start) != lrh.trieKeyLength || | ||
| len(leafsRequest.End) != 0 && len(leafsRequest.End) != lrh.trieKeyLength { |
There was a problem hiding this comment.
shouldnt' we use parentheses in those conditions?? 🤔
| StateTrieNode NodeType = iota + 1 | ||
| // AtomicTrieNode represents a leaf node that belongs to the coreth evm.AtomicTrie | ||
| AtomicTrieNode | ||
| StateTrieNode = NodeType(0) |
There was a problem hiding this comment.
should we keep this as 1 for compatibility?
| warpBackend warp.Backend, | ||
| networkCodec codec.Manager, | ||
| leafRequesTypeConfigs map[message.NodeType]LeafRequestTypeConfig, |
There was a problem hiding this comment.
should we just take map[message.NodeType]*syncHandlers.LeafsRequestHandler here possibly it can be shortened using a type declaration like type LeafHandlers map[message.NodeType]*syncHandlers.LeafsRequestHandler
| vm.warpBackend, | ||
| vm.networkCodec, | ||
| vm.leafRequestTypeConfigs, |
There was a problem hiding this comment.
could we return the map from RegisterLeafRequestHandler and perhaps rename it to makeLeafRequestHandlers to avoid a one-time use field on vm? seems like it doesn't need to be defined on vm
There was a problem hiding this comment.
This was in fact to use it from outside this pkg and let an external wrapping VM to register it's own handler.
| func (n networkHandler) HandleLeafsRequest(ctx context.Context, nodeID ids.NodeID, requestID uint32, leafsRequest message.LeafsRequest) ([]byte, error) { | ||
| handler, ok := n.leafRequestHandlers[leafsRequest.NodeType] | ||
| if !ok { | ||
| return nil, fmt.Errorf("unknown node type %d", leafsRequest.NodeType) |
There was a problem hiding this comment.
I think this error will be fatal so we should log debug and return nil as before
Why this should be merged
Moves declaration of message.NodeType to separate pkgs
How this works
This pull request includes several changes to the
plugin/evmpackage, primarily focusing on the atomic trie synchronization and request handling.Atomic Trie Synchronization:
plugin/evm/atomic/atomic_syncer.go: IntroducedAtomicTrieNodeconstant and replacedatomicKeyLengthwithAtomicTrieKeyLengththroughout the file. [1] [2] [3] [4]Request Handling:
plugin/evm/message/handler.go: Simplified theRequestHandlerinterface by mergingHandleStateTrieLeafsRequestandHandleAtomicTrieLeafsRequestinto a singleHandleLeafsRequestmethod. [1] [2]plugin/evm/message/leafs_request.go: UpdatedLeafsRequestto use a singleNodeTypefield and removed the distinction between state and atomic trie nodes. [1] [2]plugin/evm/vm.go: IntroducedRegisterLeafRequestHandlerthat can register differentmessage.NodeTypes and passes configs tonetworkHandlerto handle registered NodeTypes accordingly by creating leaf request handlers for each type.Test Updates:
plugin/evm/atomic/atomic_syncer_test.go: Updated tests to useAtomicTrieKeyLengthinstead ofatomicKeyLength. [1] [2] [3] [4]plugin/evm/message/leafs_request_test.go: Removed tests that validated the old node type-specific request handling.Additional Changes:
plugin/evm/atomic/atomic_trie.go: ReplacedatomicKeyLengthwithAtomicTrieKeyLength. [1] [2]plugin/evm/atomic/atomic_trie_iterator.go: Removed theatomicTrieKeyLenconstant and replaced its usage withAtomicTrieKeyLength. [1] [2]These changes collectively simplify the codebase and improve the maintainability of the atomic trie synchronization and request handling logic.
How this was tested
Existing UTs
Need to be documented?
No
Need to update RELEASES.md?
No