Move statesync to separate package #1759
Conversation
aaronbuchwald
force-pushed
the
repackage-state-sync
branch
from
2a923e6 to
bdfaf03
Compare
statesync/client.go
Outdated
| 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?
There was a problem hiding this comment.
This needs to match the function signature defined in vm/resolutions.go and return the stateful block, so can't conflict with the other functions present there
| 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?
There was a problem hiding this comment.
Will need to change this in the future when we support multiple block types, but for now all of these functions are implemented on StatefulBlock via embedding
statesync/handler.go
Outdated
| ) | ||
|
|
||
| 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
There was a problem hiding this comment.
Will use this here since it's a limited case. I think we should keep using wrappers.Errs{} where there's a large number of errors and a string concatenation of all of them would create a worse error message than returning the first error.
|
|
||
| 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.
There was a problem hiding this comment.
We should never call Accept on the server side, so I think we should log an error rather than pass a no-op if we change this.
| // 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.
There was a problem hiding this comment.
To provide a parse function that returns our own concrete type. We use this in the state sync client.
vm/vm.go
Outdated
| *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.