From e3ad23cd39356fcc03bc980e2bc3e19b489eee62 Mon Sep 17 00:00:00 2001 From: Yacov Manevich Date: Wed, 31 Jul 2024 20:52:32 +0200 Subject: [PATCH] Address code review comments Signed-off-by: Yacov Manevich --- chains/manager.go | 13 ++++++++++-- snow/engine/snowman/block/test_vm.go | 2 +- snow/engine/snowman/bootstrap/acceptor.go | 11 +++++----- snow/engine/snowman/bootstrap/bootstrapper.go | 20 +++++-------------- .../snowman/bootstrap/bootstrapper_test.go | 2 +- snow/engine/snowman/bootstrap/config.go | 2 +- snow/engine/snowman/bootstrap/storage.go | 9 +++++---- snow/engine/snowman/bootstrap/storage_test.go | 17 ++++++++-------- vms/platformvm/vm_test.go | 8 ++++---- vms/proposervm/block/block.go | 4 ++-- vms/proposervm/block/option.go | 2 +- vms/proposervm/block/parse.go | 4 ++-- vms/proposervm/vm.go | 18 +++++++++++------ 13 files changed, 60 insertions(+), 52 deletions(-) diff --git a/chains/manager.go b/chains/manager.go index 55ad9cc0bd4d..ccdf454a7982 100644 --- a/chains/manager.go +++ b/chains/manager.go @@ -954,7 +954,7 @@ func (m *manager) createAvalancheChain( // create bootstrap gear bootstrapCfg := smbootstrap.Config{ - Appraiser: proposerVM, + Parser: &localParser{VM: proposerVM}, AllGetsServer: snowGetHandler, Ctx: ctx, Beacons: vdrs, @@ -1356,7 +1356,7 @@ func (m *manager) createSnowmanChain( // create bootstrap gear bootstrapCfg := smbootstrap.Config{ - Appraiser: proposerVM, + Parser: &localParser{VM: proposerVM}, AllGetsServer: snowGetHandler, Ctx: ctx, Beacons: beacons, @@ -1588,3 +1588,12 @@ func (m *manager) getOrMakeVMRegisterer(vmID ids.ID, chainAlias string) (metrics ) return chainReg, err } + +// localParser intercepts invocations to ParseBlock and re-routes them to ParseLocalBlock +type localParser struct { + *proposervm.VM +} + +func (lp *localParser) ParseBlock(ctx context.Context, blockBytes []byte) (smcon.Block, error) { + return lp.ParseLocalBlock(ctx, blockBytes) +} diff --git a/snow/engine/snowman/block/test_vm.go b/snow/engine/snowman/block/test_vm.go index f6dd62df4a20..af77a0a924ca 100644 --- a/snow/engine/snowman/block/test_vm.go +++ b/snow/engine/snowman/block/test_vm.go @@ -75,7 +75,7 @@ func (vm *TestVM) ParseBlock(ctx context.Context, b []byte) (snowman.Block, erro return nil, errParseBlock } -func (vm *TestVM) AppraiseBlock(ctx context.Context, b []byte) (snowman.Block, error) { +func (vm *TestVM) ParseLocalBlock(ctx context.Context, b []byte) (snowman.Block, error) { return vm.ParseBlock(ctx, b) } diff --git a/snow/engine/snowman/bootstrap/acceptor.go b/snow/engine/snowman/bootstrap/acceptor.go index b46962e64bcc..f5451ad4d3cb 100644 --- a/snow/engine/snowman/bootstrap/acceptor.go +++ b/snow/engine/snowman/bootstrap/acceptor.go @@ -10,21 +10,22 @@ import ( "github.com/ava-labs/avalanchego/snow" "github.com/ava-labs/avalanchego/snow/consensus/snowman" + "github.com/ava-labs/avalanchego/snow/engine/snowman/block" ) var ( - _ Appraiser = (*appraiseAcceptor)(nil) + _ block.Parser = (*parseAcceptor)(nil) _ snowman.Block = (*blockAcceptor)(nil) ) -type appraiseAcceptor struct { - appraiser Appraiser +type parseAcceptor struct { + parser block.Parser ctx *snow.ConsensusContext numAccepted prometheus.Counter } -func (a *appraiseAcceptor) AppraiseBlock(ctx context.Context, bytes []byte) (snowman.Block, error) { - blk, err := a.appraiser.AppraiseBlock(ctx, bytes) +func (a *parseAcceptor) ParseBlock(ctx context.Context, bytes []byte) (snowman.Block, error) { + blk, err := a.parser.ParseBlock(ctx, bytes) if err != nil { return nil, err } diff --git a/snow/engine/snowman/bootstrap/bootstrapper.go b/snow/engine/snowman/bootstrap/bootstrapper.go index e721b3a2dcd7..c76b83a4f432 100644 --- a/snow/engine/snowman/bootstrap/bootstrapper.go +++ b/snow/engine/snowman/bootstrap/bootstrapper.go @@ -114,13 +114,13 @@ type Bootstrapper struct { // Called when bootstrapping is done on a specific chain onFinished func(ctx context.Context, lastReqID uint32) error - appraiser Appraiser + parser block.Parser } func New(config Config, onFinished func(ctx context.Context, lastReqID uint32) error) (*Bootstrapper, error) { metrics, err := newMetrics(config.Ctx.Registerer) return &Bootstrapper{ - appraiser: config.Appraiser, + parser: config.Parser, Config: config, metrics: metrics, StateSummaryFrontierHandler: common.NewNoOpStateSummaryFrontierHandler(config.Ctx.Log), @@ -181,7 +181,7 @@ func (b *Bootstrapper) Start(ctx context.Context, startReqID uint32) error { return fmt.Errorf("failed to initialize interval tree: %w", err) } - b.missingBlockIDs, err = getMissingBlockIDs(ctx, b.DB, b.appraiser, b.tree, b.startingHeight) + b.missingBlockIDs, err = getMissingBlockIDs(ctx, b.DB, b.parser, b.tree, b.startingHeight) if err != nil { return fmt.Errorf("failed to initialize missing block IDs: %w", err) } @@ -652,8 +652,8 @@ func (b *Bootstrapper) tryStartExecuting(ctx context.Context) error { b, log, b.DB, - &appraiseAcceptor{ - appraiser: b.appraiser, + &parseAcceptor{ + parser: b.parser, ctx: b.Ctx, numAccepted: b.numAccepted, }, @@ -777,13 +777,3 @@ func (b *Bootstrapper) Shutdown(ctx context.Context) error { func (*Bootstrapper) Gossip(context.Context) error { return nil } - -// Appraiser defines functionality for appraising the raw representation of a snowman block -type Appraiser interface { - // Attempt to create a block from a stream of bytes. - // - // The block should be represented by the full byte array, without extra - // bytes. - // - AppraiseBlock(ctx context.Context, blockBytes []byte) (snowman.Block, error) -} diff --git a/snow/engine/snowman/bootstrap/bootstrapper_test.go b/snow/engine/snowman/bootstrap/bootstrapper_test.go index 7120de1c69de..b3d9f3e4a888 100644 --- a/snow/engine/snowman/bootstrap/bootstrapper_test.go +++ b/snow/engine/snowman/bootstrap/bootstrapper_test.go @@ -90,7 +90,7 @@ func newConfig(t *testing.T) (Config, ids.NodeID, *common.SenderTest, *block.Tes peerTracker.Connected(peer, version.CurrentApp) return Config{ - Appraiser: vm, + Parser: vm, AllGetsServer: snowGetHandler, Ctx: ctx, Beacons: vdrs, diff --git a/snow/engine/snowman/bootstrap/config.go b/snow/engine/snowman/bootstrap/config.go index fbefedacb6dc..6f37c457a535 100644 --- a/snow/engine/snowman/bootstrap/config.go +++ b/snow/engine/snowman/bootstrap/config.go @@ -38,7 +38,7 @@ type Config struct { VM block.ChainVM - Appraiser Appraiser + Parser block.Parser Bootstrapped func() } diff --git a/snow/engine/snowman/bootstrap/storage.go b/snow/engine/snowman/bootstrap/storage.go index 15ee21bafbf5..7dafc3a40225 100644 --- a/snow/engine/snowman/bootstrap/storage.go +++ b/snow/engine/snowman/bootstrap/storage.go @@ -14,6 +14,7 @@ import ( "github.com/ava-labs/avalanchego/ids" "github.com/ava-labs/avalanchego/snow/consensus/snowman" "github.com/ava-labs/avalanchego/snow/engine/common" + "github.com/ava-labs/avalanchego/snow/engine/snowman/block" "github.com/ava-labs/avalanchego/snow/engine/snowman/bootstrap/interval" "github.com/ava-labs/avalanchego/utils/logging" "github.com/ava-labs/avalanchego/utils/set" @@ -37,7 +38,7 @@ const ( func getMissingBlockIDs( ctx context.Context, db database.KeyValueReader, - appraiser Appraiser, + parser block.Parser, tree *interval.Tree, lastAcceptedHeight uint64, ) (set.Set[ids.ID], error) { @@ -56,7 +57,7 @@ func getMissingBlockIDs( return nil, err } - blk, err := appraiser.AppraiseBlock(ctx, blkBytes) + blk, err := parser.ParseBlock(ctx, blkBytes) if err != nil { return nil, err } @@ -129,7 +130,7 @@ func execute( haltable common.Haltable, log logging.Func, db database.Database, - appraiser Appraiser, + parser block.Parser, tree *interval.Tree, lastAcceptedHeight uint64, ) error { @@ -197,7 +198,7 @@ func execute( for !haltable.Halted() && iterator.Next() { blkBytes := iterator.Value() - blk, err := appraiser.AppraiseBlock(ctx, blkBytes) + blk, err := parser.ParseBlock(ctx, blkBytes) if err != nil { return err } diff --git a/snow/engine/snowman/bootstrap/storage_test.go b/snow/engine/snowman/bootstrap/storage_test.go index f184e04015b3..44ac1621226d 100644 --- a/snow/engine/snowman/bootstrap/storage_test.go +++ b/snow/engine/snowman/bootstrap/storage_test.go @@ -16,17 +16,18 @@ import ( "github.com/ava-labs/avalanchego/snow/consensus/snowman" "github.com/ava-labs/avalanchego/snow/consensus/snowman/snowmantest" "github.com/ava-labs/avalanchego/snow/engine/common" + "github.com/ava-labs/avalanchego/snow/engine/snowman/block" "github.com/ava-labs/avalanchego/snow/engine/snowman/bootstrap/interval" "github.com/ava-labs/avalanchego/snow/snowtest" "github.com/ava-labs/avalanchego/utils/logging" "github.com/ava-labs/avalanchego/utils/set" ) -var _ Appraiser = testAppraiser(nil) +var _ block.Parser = testParser(nil) func TestGetMissingBlockIDs(t *testing.T) { blocks := snowmantest.BuildChain(7) - appraiser := makeAppraiser(blocks) + parser := makeParser(blocks) tests := []struct { name string @@ -92,7 +93,7 @@ func TestGetMissingBlockIDs(t *testing.T) { missingBlockIDs, err := getMissingBlockIDs( context.Background(), db, - appraiser, + parser, tree, test.lastAcceptedHeight, ) @@ -260,7 +261,7 @@ func TestExecute(t *testing.T) { require.NoError(err) blocks := snowmantest.BuildChain(numBlocks) - parser := makeAppraiser(blocks) + parser := makeParser(blocks) for _, blk := range blocks { _, err := interval.Add(db, tree, 0, blk.Height(), blk.Bytes()) require.NoError(err) @@ -293,14 +294,14 @@ func TestExecute(t *testing.T) { } } -type testAppraiser func(context.Context, []byte) (snowman.Block, error) +type testParser func(context.Context, []byte) (snowman.Block, error) -func (f testAppraiser) AppraiseBlock(ctx context.Context, bytes []byte) (snowman.Block, error) { +func (f testParser) ParseBlock(ctx context.Context, bytes []byte) (snowman.Block, error) { return f(ctx, bytes) } -func makeAppraiser(blocks []*snowmantest.Block) Appraiser { - return testAppraiser(func(_ context.Context, b []byte) (snowman.Block, error) { +func makeParser(blocks []*snowmantest.Block) block.Parser { + return testParser(func(_ context.Context, b []byte) (snowman.Block, error) { for _, block := range blocks { if bytes.Equal(b, block.Bytes()) { return block, nil diff --git a/vms/platformvm/vm_test.go b/vms/platformvm/vm_test.go index 8427b1b6f0a0..e831ce57a198 100644 --- a/vms/platformvm/vm_test.go +++ b/vms/platformvm/vm_test.go @@ -1513,7 +1513,7 @@ func TestBootstrapPartiallyAccepted(t *testing.T) { require.NoError(err) bootstrapConfig := bootstrap.Config{ - Appraiser: &appraiser{VM: vm}, + Parser: &parser{VM: vm}, AllGetsServer: snowGetHandler, Ctx: consensusCtx, Beacons: beacons, @@ -2085,7 +2085,7 @@ func TestUptimeDisallowedAfterNeverConnecting(t *testing.T) { require.NoError(abort.Accept(context.Background())) require.NoError(vm.SetPreference(context.Background(), vm.manager.LastAccepted())) - // Verify that rewarded validator has been removed. + // verify that rewarded validator has been removed. // Note that test genesis has multiple validators // terminating at the same time. The rewarded validator // will the first by txID. To make the test more stable @@ -2526,10 +2526,10 @@ func TestPruneMempool(t *testing.T) { require.True(ok) } -type appraiser struct { +type parser struct { *VM } -func (a *appraiser) AppraiseBlock(ctx context.Context, blockBytes []byte) (smcon.Block, error) { +func (a *parser) ParseLocalBlock(ctx context.Context, blockBytes []byte) (smcon.Block, error) { return a.VM.ParseBlock(ctx, blockBytes) } diff --git a/vms/proposervm/block/block.go b/vms/proposervm/block/block.go index 660d4b164e36..68da910e1dbd 100644 --- a/vms/proposervm/block/block.go +++ b/vms/proposervm/block/block.go @@ -28,7 +28,7 @@ type Block interface { Bytes() []byte initialize(bytes []byte) error - Verify(chainID ids.ID) error + verify(chainID ids.ID) error } type SignedBlock interface { @@ -102,7 +102,7 @@ func (b *statelessBlock) initialize(bytes []byte) error { return nil } -func (b *statelessBlock) Verify(chainID ids.ID) error { +func (b *statelessBlock) verify(chainID ids.ID) error { if len(b.StatelessBlock.Certificate) == 0 { if len(b.Signature) > 0 { return errUnexpectedSignature diff --git a/vms/proposervm/block/option.go b/vms/proposervm/block/option.go index 3b054569c923..115b6d0b9f99 100644 --- a/vms/proposervm/block/option.go +++ b/vms/proposervm/block/option.go @@ -38,6 +38,6 @@ func (b *option) initialize(bytes []byte) error { return nil } -func (*option) Verify(ids.ID) error { +func (*option) verify(ids.ID) error { return nil } diff --git a/vms/proposervm/block/parse.go b/vms/proposervm/block/parse.go index 0e8a4e7427b0..fb0542af3f43 100644 --- a/vms/proposervm/block/parse.go +++ b/vms/proposervm/block/parse.go @@ -35,14 +35,14 @@ func ParseBlocks(blks [][]byte, chainID ids.ID) []ParseResult { return results } -// Parse a block and Verify that the signature attached to the block is valid +// Parse a block and verify that the signature attached to the block is valid // for the certificate provided in the block. func Parse(bytes []byte, chainID ids.ID) (Block, error) { block, err := ParseWithoutVerification(bytes) if err != nil { return nil, err } - return block, block.Verify(chainID) + return block, block.verify(chainID) } // ParseWithoutVerification parses a block without verifying that the signature diff --git a/vms/proposervm/vm.go b/vms/proposervm/vm.go index c0aee55024bc..4bf9cd7313ab 100644 --- a/vms/proposervm/vm.go +++ b/vms/proposervm/vm.go @@ -292,7 +292,7 @@ func (vm *VM) ParseBlock(ctx context.Context, b []byte) (snowman.Block, error) { return vm.parsePreForkBlock(ctx, b) } -func (vm *VM) AppraiseBlock(ctx context.Context, b []byte) (snowman.Block, error) { +func (vm *VM) ParseLocalBlock(ctx context.Context, b []byte) (snowman.Block, error) { if blk, err := vm.parsePostForkBlock(ctx, b, false); err == nil { return blk, nil } @@ -532,13 +532,19 @@ func (vm *VM) setLastAcceptedMetadata(ctx context.Context) error { } func (vm *VM) parsePostForkBlock(ctx context.Context, b []byte, verifySignature bool) (PostForkBlock, error) { - statelessBlock, err := statelessblock.ParseWithoutVerification(b) - if err != nil { - return nil, err - } + var ( + statelessBlock statelessblock.Block + err error + ) if verifySignature { - if err := statelessBlock.Verify(vm.ctx.ChainID); err != nil { + statelessBlock, err = statelessblock.Parse(b, vm.ctx.ChainID) + if err != nil { + return nil, err + } + } else { + statelessBlock, err = statelessblock.ParseWithoutVerification(b) + if err != nil { return nil, err } }