move syncers to atomic vm #1009
Conversation
| trie: trie, | ||
| requestSize: requestSize, | ||
| } | ||
| func NewExtender() *extender { |
There was a problem hiding this comment.
Why is there a constructor if it doesn't do anything?
There was a problem hiding this comment.
bc it was using an unexported struct
| requestSize: requestSize, | ||
| } | ||
| func NewExtender() *extender { | ||
| return &extender{} |
There was a problem hiding this comment.
Returning non-exported types can cause problems at the call sites as they're really only useable as interfaces, but because they're not exported there's no way to check the documentation to see if they implement the interface of interest.
| a.backend = backend | ||
| a.trie = trie | ||
| a.requestSize = requestSize |
There was a problem hiding this comment.
In fact yes, but we need to initialize this after calling the innerVM.Initialize() (see #998)
| func NewSummaryProvider() *summaryProvider { | ||
| return &summaryProvider{} | ||
| } | ||
|
|
||
| func (a *summaryProvider) Initialize(trie *state.AtomicTrie) { | ||
| a.trie = trie |
There was a problem hiding this comment.
All my comments on *extender apply here. Non-exported type, no-op constructor, and an initialiser that's just a constructor.
| // Initialize initializes the leafHandler with the provided atomicTrieDB, trieKeyLength, and networkCodec | ||
| func NewLeafHandler(atomicTrieDB *triedb.Database, trieKeyLength int, networkCodec codec.Manager) *leafHandler { | ||
| handlerStats := stats.GetOrRegisterHandlerStats(metrics.Enabled) | ||
| // NewAtomicLeafHandler returns a new uninitialzied atomicLeafHandler that can be later initialized |
There was a problem hiding this comment.
that can be later initialized
It can't be. It always returns errUninitialized.
EDIT: looking at the code below, I think you mean "can be later replaced".
| syncProvider.Initialize(vm.AtomicBackend().AtomicTrie()) | ||
| syncExtender.Initialize(vm.AtomicBackend(), vm.AtomicBackend().AtomicTrie(), vm.Config().StateSyncRequestSize) | ||
| leafHandler.Initialize(vm.AtomicBackend().AtomicTrie().TrieDB(), state.TrieKeyLength, message.Codec) | ||
|
|
There was a problem hiding this comment.
This is confusing because of the embedded InnerVM and took me about 10 minutes to figure out, including opening the code locally. As it was merged it suggests to the reader that there was no need to already initialise the InnerVM; I suggest making explicit that you are using the InnerVM's method.
atomicBE := vm.InnerVM.AtomicBackend()
atomicTrie := atomicBE.AtomicTrie()
syncProvider.Initialize(atomicTrie)
syncExtender.Initialize(atomicBE, atomicTrie, vm.Config().StateSyncRequestSize)
leafHandler.Initialize(atomicTrie.TrieDB(), state.TrieKeyLength, message.Codec)I appreciate that this is a means to an end, but I'm still really concerned about this pattern. The PR chain needs to eventually simplify construction and initialisation.
There was a problem hiding this comment.
In one of the previous PRs it was suggested to call InnerVM methods implicitly to reduce the complexity, but I agree it's easier to figure them out if they were explicit.
There was a problem hiding this comment.
On the other hand atomicBackend will be moved to atomic/VM in #1011
|
Responded reviews here in #1015 |
Replaces: #985
Why this should be merged
This moves initialization of atomic syncers and sync extender to atomic/vm pkg
How this works
This pull request introduces changes to improve the initialization and configuration flexibility of multiple components in the EVM plugin. The key updates involve refactoring constructors to support deferred initialization, enhancing extensibility in the VM configuration, and updating state sync logic to better integrate with the new extension framework.
Refactoring Constructors for Deferred Initialization:
plugin/evm/atomic/sync/extender.go: Refactored theNewExtenderfunction to return an uninitializedextenderinstance and added anInitializemethod for deferred setup.plugin/evm/atomic/sync/leaf_handler.go: Introduced anuninitializedHandlertype to handle uninitialized leaf requests, refactoredNewLeafHandlerto return an uninitializedleafHandler, and added anInitializemethod for setup.plugin/evm/atomic/sync/summary_provider.go: RefactoredNewSummaryProviderto return an uninitializedsummaryProviderand added anInitializemethod for deferred initialization.Enhancements to VM Configuration:
plugin/evm/extension/config.go: Extended theConfigstruct to include new fields forSyncSummaryProvider,SyncExtender,SyncableParser, andExtraSyncLeafHandlerConfig. Added validation for required fields and support for an optionalClock.plugin/evm/vm.go: Updated theVMstruct and initialization logic to integrate the extended configuration, including support for custom sync handlers and parsers. [1] [2]State Sync Logic Updates:
plugin/evm/vm.go: Refactored state sync initialization to leverage the new extension configuration for handlers, parser, and extender, ensuring better modularity and flexibility. [1] [2]How this was tested
Existing tests
Need to be documented?
No
Need to update RELEASES.md?
No