Move statesync to separate package #1759
Conversation
vm/vm.go
Outdated
| vm.genesis.GetStateBranchFactor(), | ||
| vm.config.StateSyncMinBlocks, | ||
| vm.config.StateSyncParallelism, | ||
| ) | ||
|
|
||
| if err := vm.network.AddHandler( | ||
| rangeProofHandlerID, |
There was a problem hiding this comment.
It may use the rangeProofHandlerID from the statesync package?
There was a problem hiding this comment.
Ah good catch!
There was a problem hiding this comment.
Moving these into the statesync/ package as well
aaronbuchwald
force-pushed
the
repackage-state-sync
branch
from
2a923e6
to
bdfaf03
Compare
|
|
||
| var _ block.StateSummary = (*SyncableBlock[StateSummaryContainer])(nil) | ||
|
|
||
| type StateSummaryContainer interface { |
There was a problem hiding this comment.
Container IIRC is a legacy term form when we supported vertices... should we just call this Block?
| RangeProofHandlerID = 0 | ||
| ChangeProofHandlerID = 1 |
There was a problem hiding this comment.
IMO these handler ids should be defined by the vm - this package has no idea if it's colliding with other handler ids or not.
| var _ Accepter[StateSummaryContainer] = (*Client[StateSummaryContainer])(nil) | ||
|
|
||
| type ChainClient[T StateSummaryContainer] interface { | ||
| LastAcceptedStatefulBlock() T |
There was a problem hiding this comment.
Naming here seems weird because this package doesn't know about StatefulBlocks, as that's the concrete type defined by the consumer of this package. Maybe something like Block would make more sense?
| func (b *StatelessBlock) ID() ids.ID { return b.id } | ||
| func (b *StatelessBlock) Bytes() []byte { return b.bytes } | ||
| func (b *StatelessBlock) Size() int { return len(b.bytes) } | ||
| func (b *StatelessBlock) GetStateRoot() ids.ID { return b.StateRoot } |
There was a problem hiding this comment.
Shouldn't this function be on the StatefulBlock type since that's the one we care about implementing the statesync interface?
| Bytes() []byte | ||
| GetStateRoot() ids.ID | ||
| MarkAccepted(context.Context) | ||
| fmt.Stringer |
There was a problem hiding this comment.
nit: Generally embedding is done by composing stuff at the top of the interface/struct as opposed to at the bottom
| ) | ||
|
|
||
| func RegisterHandlers(log logging.Logger, network *p2p.Network, db merkledb.MerkleDB) error { | ||
| errs := wrappers.Errs{} |
There was a problem hiding this comment.
I think we can actually just use the stdlib errors.Join(...) here instead of the avalanchego wrapper
|
|
||
| type SyncableBlock[T StateSummaryContainer] struct { | ||
| container T | ||
| accepter Accepter[T] // accepter is nil if the SyncableBlock is constructed by the server |
There was a problem hiding this comment.
Another idea instead of relying on not ever calling Accept to avoid panic'ing would be to just pass in a no-op implementation of accept so you don't have to document this edge-case at all.
| // implements "block.ChainVM.commom.VM.Parser" | ||
| // replaces "core.SnowmanVM.ParseBlock" | ||
| func (vm *VM) ParseBlock(ctx context.Context, source []byte) (snowman.Block, error) { | ||
| func (vm *VM) ParseStatefulBlock(ctx context.Context, source []byte) (*StatefulBlock, error) { |
There was a problem hiding this comment.
Why did we rename this? Wondering if we need this + the ParseBlock function... I guess this one is nice because it returns the concrete type but it's also not something we're using right now.
| *statesync.Client[*StatefulBlock] | ||
| *statesync.Server[*StatefulBlock] |
There was a problem hiding this comment.
Prefer not embedding here... it can be annoying because now we can collide across functions/fields that the embedded types have.
This PR separates state sync from the VM package.