From 61ee2401d1394bf0bb861792523967ff0c215087 Mon Sep 17 00:00:00 2001 From: Yacov Manevich Date: Wed, 31 Jul 2024 16:21:10 +0200 Subject: [PATCH 1/6] Minimize signature verification when bootstrapping When bootstrapping blocks of the P-chain, the node verifies blocks when it replicates them from remote nodes, and afterwards it executes them. When executing them, it verifies the blocks once again via parsing them. This is redundant, as it has already parsed them before while replicating them. This change introduces an interface called Appraiser, which similar to the parser found in snow/engine/snowman/block/vm.go, converts the bytes of a block to a snowman.Block. However, unlike the Parser, it is only implemented at the proposerVM, and doesn't verify the signature of the block, reducing the CPU cycles it costs to execute the blocks when bootstrapping. When testing a chunk of 1000 blocks, parsing the blocks without verifying the signature shaves off 93% of the runtime in comparison to just appraising them. Signed-off-by: Yacov Manevich --- chains/manager.go | 10 +- snow/engine/snowman/block/test_vm.go | 4 + snow/engine/snowman/bootstrap/acceptor.go | 15 +- snow/engine/snowman/bootstrap/bootstrapper.go | 19 ++- .../snowman/bootstrap/bootstrapper_test.go | 1 + snow/engine/snowman/bootstrap/config.go | 2 + snow/engine/snowman/bootstrap/storage.go | 9 +- snow/engine/snowman/bootstrap/storage_test.go | 136 ++++++++++++++++-- vms/platformvm/vm_test.go | 9 ++ vms/proposervm/block/block.go | 4 +- vms/proposervm/block/option.go | 2 +- vms/proposervm/block/parse.go | 4 +- vms/proposervm/state_syncable_vm.go | 2 +- vms/proposervm/vm.go | 19 ++- 14 files changed, 201 insertions(+), 35 deletions(-) diff --git a/chains/manager.go b/chains/manager.go index 1980b999a013..382d6e23a651 100644 --- a/chains/manager.go +++ b/chains/manager.go @@ -789,7 +789,7 @@ func (m *manager) createAvalancheChain( // Note: vmWrappingProposerVM is the VM that the Snowman engines should be // using. - var vmWrappingProposerVM block.ChainVM = proposervm.New( + proposerVM := proposervm.New( vmWrappedInsideProposerVM, proposervm.Config{ Upgrades: m.Upgrades, @@ -801,6 +801,8 @@ func (m *manager) createAvalancheChain( }, ) + var vmWrappingProposerVM block.ChainVM = proposerVM + if m.MeterVMEnabled { meterchainvmReg, err := metrics.MakeAndRegister( m.meterChainVMGatherer, @@ -948,6 +950,7 @@ func (m *manager) createAvalancheChain( // create bootstrap gear bootstrapCfg := smbootstrap.Config{ + Appraiser: proposerVM, AllGetsServer: snowGetHandler, Ctx: ctx, Beacons: vdrs, @@ -1184,7 +1187,7 @@ func (m *manager) createSnowmanChain( return nil, err } - vm = proposervm.New( + proposerVM := proposervm.New( vm, proposervm.Config{ Upgrades: m.Upgrades, @@ -1196,6 +1199,8 @@ func (m *manager) createSnowmanChain( }, ) + vm = proposerVM + if m.MeterVMEnabled { meterchainvmReg, err := metrics.MakeAndRegister( m.meterChainVMGatherer, @@ -1345,6 +1350,7 @@ func (m *manager) createSnowmanChain( // create bootstrap gear bootstrapCfg := smbootstrap.Config{ + Appraiser: proposerVM, AllGetsServer: snowGetHandler, Ctx: ctx, Beacons: beacons, diff --git a/snow/engine/snowman/block/test_vm.go b/snow/engine/snowman/block/test_vm.go index cdbeabacc4f1..f6dd62df4a20 100644 --- a/snow/engine/snowman/block/test_vm.go +++ b/snow/engine/snowman/block/test_vm.go @@ -75,6 +75,10 @@ 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) { + return vm.ParseBlock(ctx, b) +} + func (vm *TestVM) GetBlock(ctx context.Context, id ids.ID) (snowman.Block, error) { if vm.GetBlockF != nil { return vm.GetBlockF(ctx, id) diff --git a/snow/engine/snowman/bootstrap/acceptor.go b/snow/engine/snowman/bootstrap/acceptor.go index eae4be879afa..b46962e64bcc 100644 --- a/snow/engine/snowman/bootstrap/acceptor.go +++ b/snow/engine/snowman/bootstrap/acceptor.go @@ -10,29 +10,28 @@ 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 ( - _ block.Parser = (*parseAcceptor)(nil) + _ Appraiser = (*appraiseAcceptor)(nil) _ snowman.Block = (*blockAcceptor)(nil) ) -type parseAcceptor struct { - parser block.Parser +type appraiseAcceptor struct { + appraiser Appraiser ctx *snow.ConsensusContext numAccepted prometheus.Counter } -func (p *parseAcceptor) ParseBlock(ctx context.Context, bytes []byte) (snowman.Block, error) { - blk, err := p.parser.ParseBlock(ctx, bytes) +func (a *appraiseAcceptor) AppraiseBlock(ctx context.Context, bytes []byte) (snowman.Block, error) { + blk, err := a.appraiser.AppraiseBlock(ctx, bytes) if err != nil { return nil, err } return &blockAcceptor{ Block: blk, - ctx: p.ctx, - numAccepted: p.numAccepted, + ctx: a.ctx, + numAccepted: a.numAccepted, }, nil } diff --git a/snow/engine/snowman/bootstrap/bootstrapper.go b/snow/engine/snowman/bootstrap/bootstrapper.go index 6b8462f83f63..e721b3a2dcd7 100644 --- a/snow/engine/snowman/bootstrap/bootstrapper.go +++ b/snow/engine/snowman/bootstrap/bootstrapper.go @@ -113,11 +113,14 @@ type Bootstrapper struct { // Called when bootstrapping is done on a specific chain onFinished func(ctx context.Context, lastReqID uint32) error + + appraiser Appraiser } 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, Config: config, metrics: metrics, StateSummaryFrontierHandler: common.NewNoOpStateSummaryFrontierHandler(config.Ctx.Log), @@ -178,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.VM, b.tree, b.startingHeight) + b.missingBlockIDs, err = getMissingBlockIDs(ctx, b.DB, b.appraiser, b.tree, b.startingHeight) if err != nil { return fmt.Errorf("failed to initialize missing block IDs: %w", err) } @@ -649,8 +652,8 @@ func (b *Bootstrapper) tryStartExecuting(ctx context.Context) error { b, log, b.DB, - &parseAcceptor{ - parser: b.VM, + &appraiseAcceptor{ + appraiser: b.appraiser, ctx: b.Ctx, numAccepted: b.numAccepted, }, @@ -774,3 +777,13 @@ 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 325b7d2221de..7120de1c69de 100644 --- a/snow/engine/snowman/bootstrap/bootstrapper_test.go +++ b/snow/engine/snowman/bootstrap/bootstrapper_test.go @@ -90,6 +90,7 @@ func newConfig(t *testing.T) (Config, ids.NodeID, *common.SenderTest, *block.Tes peerTracker.Connected(peer, version.CurrentApp) return Config{ + Appraiser: vm, AllGetsServer: snowGetHandler, Ctx: ctx, Beacons: vdrs, diff --git a/snow/engine/snowman/bootstrap/config.go b/snow/engine/snowman/bootstrap/config.go index bcf57f02e832..fbefedacb6dc 100644 --- a/snow/engine/snowman/bootstrap/config.go +++ b/snow/engine/snowman/bootstrap/config.go @@ -38,5 +38,7 @@ type Config struct { VM block.ChainVM + Appraiser Appraiser + Bootstrapped func() } diff --git a/snow/engine/snowman/bootstrap/storage.go b/snow/engine/snowman/bootstrap/storage.go index 7dafc3a40225..15ee21bafbf5 100644 --- a/snow/engine/snowman/bootstrap/storage.go +++ b/snow/engine/snowman/bootstrap/storage.go @@ -14,7 +14,6 @@ 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" @@ -38,7 +37,7 @@ const ( func getMissingBlockIDs( ctx context.Context, db database.KeyValueReader, - parser block.Parser, + appraiser Appraiser, tree *interval.Tree, lastAcceptedHeight uint64, ) (set.Set[ids.ID], error) { @@ -57,7 +56,7 @@ func getMissingBlockIDs( return nil, err } - blk, err := parser.ParseBlock(ctx, blkBytes) + blk, err := appraiser.AppraiseBlock(ctx, blkBytes) if err != nil { return nil, err } @@ -130,7 +129,7 @@ func execute( haltable common.Haltable, log logging.Func, db database.Database, - parser block.Parser, + appraiser Appraiser, tree *interval.Tree, lastAcceptedHeight uint64, ) error { @@ -198,7 +197,7 @@ func execute( for !haltable.Halted() && iterator.Next() { blkBytes := iterator.Value() - blk, err := parser.ParseBlock(ctx, blkBytes) + blk, err := appraiser.AppraiseBlock(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 44ac1621226d..adbf53db79bb 100644 --- a/snow/engine/snowman/bootstrap/storage_test.go +++ b/snow/engine/snowman/bootstrap/storage_test.go @@ -6,28 +6,38 @@ package bootstrap import ( "bytes" "context" + "crypto" + "encoding/binary" "testing" + "time" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/require" "github.com/ava-labs/avalanchego/database" "github.com/ava-labs/avalanchego/database/memdb" + "github.com/ava-labs/avalanchego/database/prefixdb" "github.com/ava-labs/avalanchego/ids" + "github.com/ava-labs/avalanchego/snow" "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/staking" "github.com/ava-labs/avalanchego/utils/logging" "github.com/ava-labs/avalanchego/utils/set" + "github.com/ava-labs/avalanchego/vms/proposervm" + + blockbuilder "github.com/ava-labs/avalanchego/vms/proposervm/block" ) -var _ block.Parser = testParser(nil) +var _ Appraiser = testAppraiser(nil) func TestGetMissingBlockIDs(t *testing.T) { blocks := snowmantest.BuildChain(7) - parser := makeParser(blocks) + appraiser := makeAppraiser(blocks) tests := []struct { name string @@ -93,7 +103,7 @@ func TestGetMissingBlockIDs(t *testing.T) { missingBlockIDs, err := getMissingBlockIDs( context.Background(), db, - parser, + appraiser, tree, test.lastAcceptedHeight, ) @@ -216,6 +226,116 @@ func TestProcess(t *testing.T) { } } +func TestMeasureExecute(t *testing.T) { + const numBlocks = 1000 + + halted := &common.Halter{} + halted.Halt(context.Background()) + + parentID := ids.ID{1} + timestamp := time.Unix(123, 0) + pChainHeight := uint64(2) + chainID := ids.GenerateTestID() + + tlsCert, err := staking.NewTLSCert() + require.NoError(t, err) + + cert, err := staking.ParseCertificate(tlsCert.Leaf.Raw) + require.NoError(t, err) + key := tlsCert.PrivateKey.(crypto.Signer) + + buff := binary.AppendVarint(nil, int64(42)) + + signedBlock, err := blockbuilder.Build( + parentID, + timestamp, + pChainHeight, + cert, + buff, + chainID, + key, + ) + require.NoError(t, err) + + rawBlock := signedBlock.Bytes() + + blocks := make([][]byte, numBlocks) + + for i := 0; i < numBlocks; i++ { + blocks[i] = rawBlock + } + + conf := proposervm.Config{ + ActivationTime: time.Unix(0, 0), + DurangoTime: time.Unix(0, 0), + MinimumPChainHeight: 0, + Registerer: prometheus.NewRegistry(), + } + + innerVM := &block.TestVM{ + ParseBlockF: func(_ context.Context, rawBlock []byte) (snowman.Block, error) { + return &snowmantest.Block{BytesV: rawBlock}, nil + }, + } + + vm := proposervm.New(innerVM, conf) + + db := prefixdb.New([]byte{}, memdb.New()) + + ctx := snowtest.Context(t, snowtest.CChainID) + ctx.NodeID = ids.NodeIDFromCert(cert) + + _ = vm.Initialize(context.Background(), &snow.Context{ + Log: logging.NoLog{}, + ChainID: ids.GenerateTestID(), + }, db, nil, nil, nil, nil, nil, nil) + appraiseVM := &block.TestVM{ + ParseBlockF: vm.AppraiseBlock, + } + + parseVM := &block.TestVM{ + ParseBlockF: vm.ParseBlock, + } + + tests := []struct { + name string + vm *block.TestVM + }{ + { + name: "appraise", + vm: appraiseVM, + }, + { + name: "parse", + vm: parseVM, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + db := memdb.New() + tree, err := interval.NewTree(db) + require.NoError(t, err) + + for i, blk := range blocks { + _, err := interval.Add(db, tree, 0, uint64(i), blk) + require.NoError(t, err) + } + + t1 := time.Now() + require.NoError(t, execute( + context.Background(), + &common.Halter{}, + logging.NoLog{}.Info, + db, + test.vm, + tree, + numBlocks, + )) + t.Log(time.Since(t1)) + }) + } +} + func TestExecute(t *testing.T) { const numBlocks = 7 @@ -261,7 +381,7 @@ func TestExecute(t *testing.T) { require.NoError(err) blocks := snowmantest.BuildChain(numBlocks) - parser := makeParser(blocks) + parser := makeAppraiser(blocks) for _, blk := range blocks { _, err := interval.Add(db, tree, 0, blk.Height(), blk.Bytes()) require.NoError(err) @@ -294,14 +414,14 @@ func TestExecute(t *testing.T) { } } -type testParser func(context.Context, []byte) (snowman.Block, error) +type testAppraiser func(context.Context, []byte) (snowman.Block, error) -func (f testParser) ParseBlock(ctx context.Context, bytes []byte) (snowman.Block, error) { +func (f testAppraiser) AppraiseBlock(ctx context.Context, bytes []byte) (snowman.Block, error) { return f(ctx, bytes) } -func makeParser(blocks []*snowmantest.Block) block.Parser { - return testParser(func(_ context.Context, b []byte) (snowman.Block, error) { +func makeAppraiser(blocks []*snowmantest.Block) Appraiser { + return testAppraiser(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 726c4a8b0397..660c60f5a112 100644 --- a/vms/platformvm/vm_test.go +++ b/vms/platformvm/vm_test.go @@ -1513,6 +1513,7 @@ func TestBootstrapPartiallyAccepted(t *testing.T) { require.NoError(err) bootstrapConfig := bootstrap.Config{ + Appraiser: &appraiser{VM: vm}, AllGetsServer: snowGetHandler, Ctx: consensusCtx, Beacons: beacons, @@ -2524,3 +2525,11 @@ func TestPruneMempool(t *testing.T) { _, ok = vm.Builder.Get(baseTxID) require.True(ok) } + +type appraiser struct { + *VM +} + +func (a *appraiser) AppraiseBlock(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 68da910e1dbd..660d4b164e36 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 115b6d0b9f99..3b054569c923 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 fb0542af3f43..0e8a4e7427b0 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/state_syncable_vm.go b/vms/proposervm/state_syncable_vm.go index 12f61e057396..e4b462c10955 100644 --- a/vms/proposervm/state_syncable_vm.go +++ b/vms/proposervm/state_syncable_vm.go @@ -66,7 +66,7 @@ func (vm *VM) ParseStateSummary(ctx context.Context, summaryBytes []byte) (block if err != nil { return nil, fmt.Errorf("could not parse inner summary due to: %w", err) } - block, err := vm.parsePostForkBlock(ctx, statelessSummary.BlockBytes()) + block, err := vm.parsePostForkBlock(ctx, statelessSummary.BlockBytes(), true) if err != nil { return nil, fmt.Errorf("could not parse proposervm block bytes from summary due to: %w", err) } diff --git a/vms/proposervm/vm.go b/vms/proposervm/vm.go index 6853507b1a9e..e934a1b6c403 100644 --- a/vms/proposervm/vm.go +++ b/vms/proposervm/vm.go @@ -286,7 +286,14 @@ func (vm *VM) BuildBlock(ctx context.Context) (snowman.Block, error) { } func (vm *VM) ParseBlock(ctx context.Context, b []byte) (snowman.Block, error) { - if blk, err := vm.parsePostForkBlock(ctx, b); err == nil { + if blk, err := vm.parsePostForkBlock(ctx, b, true); err == nil { + return blk, nil + } + return vm.parsePreForkBlock(ctx, b) +} + +func (vm *VM) AppraiseBlock(ctx context.Context, b []byte) (snowman.Block, error) { + if blk, err := vm.parsePostForkBlock(ctx, b, false); err == nil { return blk, nil } return vm.parsePreForkBlock(ctx, b) @@ -524,12 +531,18 @@ func (vm *VM) setLastAcceptedMetadata(ctx context.Context) error { return nil } -func (vm *VM) parsePostForkBlock(ctx context.Context, b []byte) (PostForkBlock, error) { - statelessBlock, err := statelessblock.Parse(b, vm.ctx.ChainID) +func (vm *VM) parsePostForkBlock(ctx context.Context, b []byte, verifySignature bool) (PostForkBlock, error) { + statelessBlock, err := statelessblock.ParseWithoutVerification(b) if err != nil { return nil, err } + if verifySignature { + if err := statelessBlock.Verify(vm.ctx.ChainID); err != nil { + return nil, err + } + } + blkID := statelessBlock.ID() innerBlkBytes := statelessBlock.Block() innerBlk, err := vm.parseInnerBlock(ctx, blkID, innerBlkBytes) From 1d863b0a9414316af4314ec197b377e648c58eda Mon Sep 17 00:00:00 2001 From: Yacov Manevich Date: Wed, 31 Jul 2024 18:35:30 +0200 Subject: [PATCH 2/6] Remove comparison test Signed-off-by: Yacov Manevich --- snow/engine/snowman/bootstrap/storage_test.go | 121 ------------------ 1 file changed, 121 deletions(-) diff --git a/snow/engine/snowman/bootstrap/storage_test.go b/snow/engine/snowman/bootstrap/storage_test.go index adbf53db79bb..f184e04015b3 100644 --- a/snow/engine/snowman/bootstrap/storage_test.go +++ b/snow/engine/snowman/bootstrap/storage_test.go @@ -6,31 +6,20 @@ package bootstrap import ( "bytes" "context" - "crypto" - "encoding/binary" "testing" - "time" - "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/require" "github.com/ava-labs/avalanchego/database" "github.com/ava-labs/avalanchego/database/memdb" - "github.com/ava-labs/avalanchego/database/prefixdb" "github.com/ava-labs/avalanchego/ids" - "github.com/ava-labs/avalanchego/snow" "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/staking" "github.com/ava-labs/avalanchego/utils/logging" "github.com/ava-labs/avalanchego/utils/set" - "github.com/ava-labs/avalanchego/vms/proposervm" - - blockbuilder "github.com/ava-labs/avalanchego/vms/proposervm/block" ) var _ Appraiser = testAppraiser(nil) @@ -226,116 +215,6 @@ func TestProcess(t *testing.T) { } } -func TestMeasureExecute(t *testing.T) { - const numBlocks = 1000 - - halted := &common.Halter{} - halted.Halt(context.Background()) - - parentID := ids.ID{1} - timestamp := time.Unix(123, 0) - pChainHeight := uint64(2) - chainID := ids.GenerateTestID() - - tlsCert, err := staking.NewTLSCert() - require.NoError(t, err) - - cert, err := staking.ParseCertificate(tlsCert.Leaf.Raw) - require.NoError(t, err) - key := tlsCert.PrivateKey.(crypto.Signer) - - buff := binary.AppendVarint(nil, int64(42)) - - signedBlock, err := blockbuilder.Build( - parentID, - timestamp, - pChainHeight, - cert, - buff, - chainID, - key, - ) - require.NoError(t, err) - - rawBlock := signedBlock.Bytes() - - blocks := make([][]byte, numBlocks) - - for i := 0; i < numBlocks; i++ { - blocks[i] = rawBlock - } - - conf := proposervm.Config{ - ActivationTime: time.Unix(0, 0), - DurangoTime: time.Unix(0, 0), - MinimumPChainHeight: 0, - Registerer: prometheus.NewRegistry(), - } - - innerVM := &block.TestVM{ - ParseBlockF: func(_ context.Context, rawBlock []byte) (snowman.Block, error) { - return &snowmantest.Block{BytesV: rawBlock}, nil - }, - } - - vm := proposervm.New(innerVM, conf) - - db := prefixdb.New([]byte{}, memdb.New()) - - ctx := snowtest.Context(t, snowtest.CChainID) - ctx.NodeID = ids.NodeIDFromCert(cert) - - _ = vm.Initialize(context.Background(), &snow.Context{ - Log: logging.NoLog{}, - ChainID: ids.GenerateTestID(), - }, db, nil, nil, nil, nil, nil, nil) - appraiseVM := &block.TestVM{ - ParseBlockF: vm.AppraiseBlock, - } - - parseVM := &block.TestVM{ - ParseBlockF: vm.ParseBlock, - } - - tests := []struct { - name string - vm *block.TestVM - }{ - { - name: "appraise", - vm: appraiseVM, - }, - { - name: "parse", - vm: parseVM, - }, - } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - db := memdb.New() - tree, err := interval.NewTree(db) - require.NoError(t, err) - - for i, blk := range blocks { - _, err := interval.Add(db, tree, 0, uint64(i), blk) - require.NoError(t, err) - } - - t1 := time.Now() - require.NoError(t, execute( - context.Background(), - &common.Halter{}, - logging.NoLog{}.Info, - db, - test.vm, - tree, - numBlocks, - )) - t.Log(time.Since(t1)) - }) - } -} - func TestExecute(t *testing.T) { const numBlocks = 7 From dd8c58fb96c84c8fe3063bbaad6d1ff0ad918c67 Mon Sep 17 00:00:00 2001 From: Yacov Manevich Date: Wed, 31 Jul 2024 20:52:32 +0200 Subject: [PATCH 3/6] 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 382d6e23a651..7815a7344629 100644 --- a/chains/manager.go +++ b/chains/manager.go @@ -950,7 +950,7 @@ func (m *manager) createAvalancheChain( // create bootstrap gear bootstrapCfg := smbootstrap.Config{ - Appraiser: proposerVM, + Parser: &localParser{VM: proposerVM}, AllGetsServer: snowGetHandler, Ctx: ctx, Beacons: vdrs, @@ -1350,7 +1350,7 @@ func (m *manager) createSnowmanChain( // create bootstrap gear bootstrapCfg := smbootstrap.Config{ - Appraiser: proposerVM, + Parser: &localParser{VM: proposerVM}, AllGetsServer: snowGetHandler, Ctx: ctx, Beacons: beacons, @@ -1582,3 +1582,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 660c60f5a112..1ab53b761b48 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 e934a1b6c403..759ee8205983 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 } } From 1307e25e79bf355063eb2becb08375615b96ae70 Mon Sep 17 00:00:00 2001 From: Yacov Manevich Date: Thu, 1 Aug 2024 15:06:33 +0200 Subject: [PATCH 4/6] Address code review comments Signed-off-by: Yacov Manevich --- chains/manager.go | 4 +- snow/engine/snowman/bootstrap/acceptor.go | 8 +- vms/platformvm/vm_test.go | 2 +- vms/proposervm/vm.go | 9 +-- vms/proposervm/vm_test.go | 91 +++++++++++++++++++++++ 5 files changed, 101 insertions(+), 13 deletions(-) diff --git a/chains/manager.go b/chains/manager.go index 7815a7344629..924ee345b135 100644 --- a/chains/manager.go +++ b/chains/manager.go @@ -787,8 +787,6 @@ func (m *manager) createAvalancheChain( return nil, err } - // Note: vmWrappingProposerVM is the VM that the Snowman engines should be - // using. proposerVM := proposervm.New( vmWrappedInsideProposerVM, proposervm.Config{ @@ -801,6 +799,8 @@ func (m *manager) createAvalancheChain( }, ) + // Note: vmWrappingProposerVM is the VM that the Snowman engines should be + // using. var vmWrappingProposerVM block.ChainVM = proposerVM if m.MeterVMEnabled { diff --git a/snow/engine/snowman/bootstrap/acceptor.go b/snow/engine/snowman/bootstrap/acceptor.go index f5451ad4d3cb..eae4be879afa 100644 --- a/snow/engine/snowman/bootstrap/acceptor.go +++ b/snow/engine/snowman/bootstrap/acceptor.go @@ -24,15 +24,15 @@ type parseAcceptor struct { numAccepted prometheus.Counter } -func (a *parseAcceptor) ParseBlock(ctx context.Context, bytes []byte) (snowman.Block, error) { - blk, err := a.parser.ParseBlock(ctx, bytes) +func (p *parseAcceptor) ParseBlock(ctx context.Context, bytes []byte) (snowman.Block, error) { + blk, err := p.parser.ParseBlock(ctx, bytes) if err != nil { return nil, err } return &blockAcceptor{ Block: blk, - ctx: a.ctx, - numAccepted: a.numAccepted, + ctx: p.ctx, + numAccepted: p.numAccepted, }, nil } diff --git a/vms/platformvm/vm_test.go b/vms/platformvm/vm_test.go index 1ab53b761b48..8cbb04ca2815 100644 --- a/vms/platformvm/vm_test.go +++ b/vms/platformvm/vm_test.go @@ -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 diff --git a/vms/proposervm/vm.go b/vms/proposervm/vm.go index 759ee8205983..d0c21971c80d 100644 --- a/vms/proposervm/vm.go +++ b/vms/proposervm/vm.go @@ -539,14 +539,11 @@ func (vm *VM) parsePostForkBlock(ctx context.Context, b []byte, verifySignature if verifySignature { 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 - } + } + if err != nil { + return nil, err } blkID := statelessBlock.ID() diff --git a/vms/proposervm/vm_test.go b/vms/proposervm/vm_test.go index 983759328913..9b1a7924abdb 100644 --- a/vms/proposervm/vm_test.go +++ b/vms/proposervm/vm_test.go @@ -30,6 +30,7 @@ import ( "github.com/ava-labs/avalanchego/staking" "github.com/ava-labs/avalanchego/upgrade" "github.com/ava-labs/avalanchego/utils" + "github.com/ava-labs/avalanchego/utils/logging" "github.com/ava-labs/avalanchego/utils/timer/mockable" "github.com/ava-labs/avalanchego/vms/proposervm/proposer" @@ -2476,3 +2477,93 @@ func TestGetPostDurangoSlotTimeWithNoValidators(t *testing.T) { require.NoError(err) require.Equal(parentTimestamp.Add(proVM.MinBlkDelay), slotTime) } + +func TestLocalParse(t *testing.T) { + innerVM := &block.TestVM{ + ParseBlockF: func(_ context.Context, rawBlock []byte) (snowman.Block, error) { + return &snowmantest.Block{BytesV: rawBlock}, nil + }, + } + + chainID := ids.GenerateTestID() + + tlsCert, err := staking.NewTLSCert() + require.NoError(t, err) + + cert, err := staking.ParseCertificate(tlsCert.Leaf.Raw) + require.NoError(t, err) + key := tlsCert.PrivateKey.(crypto.Signer) + + signedBlock, err := statelessblock.Build( + ids.ID{1}, + time.Unix(123, 0), + uint64(42), + cert, + []byte{1, 2, 3}, + chainID, + key, + ) + require.NoError(t, err) + + properlySignedBlock := signedBlock.Bytes() + + improperlySignedBlock := make([]byte, len(properlySignedBlock)) + copy(improperlySignedBlock, properlySignedBlock) + improperlySignedBlock[len(improperlySignedBlock)-1] = ^improperlySignedBlock[len(improperlySignedBlock)-1] + + conf := Config{ + ActivationTime: time.Unix(0, 0), + DurangoTime: time.Unix(0, 0), + MinimumPChainHeight: 0, + MinBlkDelay: DefaultMinBlockDelay, + NumHistoricalBlocks: DefaultNumHistoricalBlocks, + StakingLeafSigner: pTestSigner, + StakingCertLeaf: pTestCert, + Registerer: prometheus.NewRegistry(), + } + + vm := New(innerVM, conf) + defer func() { + require.NoError(t, vm.Shutdown(context.Background())) + }() + + db := prefixdb.New([]byte{}, memdb.New()) + + _ = vm.Initialize(context.Background(), &snow.Context{ + Log: logging.NoLog{}, + ChainID: chainID, + }, db, nil, nil, nil, nil, nil, nil) + + tests := []struct { + name string + f func(ctx context.Context, b []byte) (snowman.Block, error) + block []byte + resultingBlock interface{} + }{ + { + name: "local parse as post-fork", + f: vm.ParseLocalBlock, + block: improperlySignedBlock, + resultingBlock: &postForkBlock{}, + }, + { + name: "parse as pre-fork", + f: vm.ParseBlock, + block: improperlySignedBlock, + resultingBlock: &preForkBlock{}, + }, + { + name: "parse as post-fork", + f: vm.ParseBlock, + block: properlySignedBlock, + resultingBlock: &postForkBlock{}, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + block, err := test.f(context.Background(), test.block) + require.NoError(t, err) + require.IsType(t, test.resultingBlock, block) + }) + } +} From f1f99c393498a58614cf7abad5d06100bb305777 Mon Sep 17 00:00:00 2001 From: Yacov Manevich Date: Thu, 1 Aug 2024 22:11:27 +0200 Subject: [PATCH 5/6] Address code review comments Signed-off-by: Yacov Manevich --- chains/manager.go | 13 ++----------- snow/engine/snowman/block/test_vm.go | 4 ---- snow/engine/snowman/block/vm.go | 8 ++++++++ snow/engine/snowman/bootstrap/bootstrapper.go | 8 ++++---- snow/engine/snowman/bootstrap/bootstrapper_test.go | 2 +- snow/engine/snowman/bootstrap/config.go | 3 ++- snow/engine/snowman/bootstrap/storage.go | 8 ++++---- vms/platformvm/vm_test.go | 10 +--------- vms/proposervm/vm_test.go | 2 +- 9 files changed, 23 insertions(+), 35 deletions(-) diff --git a/chains/manager.go b/chains/manager.go index 924ee345b135..5cb75c79bc94 100644 --- a/chains/manager.go +++ b/chains/manager.go @@ -950,7 +950,7 @@ func (m *manager) createAvalancheChain( // create bootstrap gear bootstrapCfg := smbootstrap.Config{ - Parser: &localParser{VM: proposerVM}, + NonVerifyingParse: block.ParseFunc(proposerVM.ParseLocalBlock), AllGetsServer: snowGetHandler, Ctx: ctx, Beacons: vdrs, @@ -1350,7 +1350,7 @@ func (m *manager) createSnowmanChain( // create bootstrap gear bootstrapCfg := smbootstrap.Config{ - Parser: &localParser{VM: proposerVM}, + NonVerifyingParse: block.ParseFunc(proposerVM.ParseLocalBlock), AllGetsServer: snowGetHandler, Ctx: ctx, Beacons: beacons, @@ -1582,12 +1582,3 @@ 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 af77a0a924ca..cdbeabacc4f1 100644 --- a/snow/engine/snowman/block/test_vm.go +++ b/snow/engine/snowman/block/test_vm.go @@ -75,10 +75,6 @@ func (vm *TestVM) ParseBlock(ctx context.Context, b []byte) (snowman.Block, erro return nil, errParseBlock } -func (vm *TestVM) ParseLocalBlock(ctx context.Context, b []byte) (snowman.Block, error) { - return vm.ParseBlock(ctx, b) -} - func (vm *TestVM) GetBlock(ctx context.Context, id ids.ID) (snowman.Block, error) { if vm.GetBlockF != nil { return vm.GetBlockF(ctx, id) diff --git a/snow/engine/snowman/block/vm.go b/snow/engine/snowman/block/vm.go index d28dc7ef7a6d..792eda28511d 100644 --- a/snow/engine/snowman/block/vm.go +++ b/snow/engine/snowman/block/vm.go @@ -83,3 +83,11 @@ type Parser interface { // It is expected for all historical blocks to be parseable. ParseBlock(ctx context.Context, blockBytes []byte) (snowman.Block, error) } + +// ParseFunc defines a function that parses raw bytes into a block. +type ParseFunc func(context.Context, []byte) (snowman.Block, error) + +// ParseBlock wraps a ParseFunc into a ParseBlock function, to be used by a Parser interface +func (f ParseFunc) ParseBlock(ctx context.Context, blockBytes []byte) (snowman.Block, error) { + return f(ctx, blockBytes) +} diff --git a/snow/engine/snowman/bootstrap/bootstrapper.go b/snow/engine/snowman/bootstrap/bootstrapper.go index c76b83a4f432..966bdcf67d11 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 - parser block.Parser + nonVerifyingParser block.Parser } func New(config Config, onFinished func(ctx context.Context, lastReqID uint32) error) (*Bootstrapper, error) { metrics, err := newMetrics(config.Ctx.Registerer) return &Bootstrapper{ - parser: config.Parser, + nonVerifyingParser: config.NonVerifyingParse, 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.parser, b.tree, b.startingHeight) + b.missingBlockIDs, err = getMissingBlockIDs(ctx, b.DB, b.nonVerifyingParser, b.tree, b.startingHeight) if err != nil { return fmt.Errorf("failed to initialize missing block IDs: %w", err) } @@ -653,7 +653,7 @@ func (b *Bootstrapper) tryStartExecuting(ctx context.Context) error { log, b.DB, &parseAcceptor{ - parser: b.parser, + parser: b.nonVerifyingParser, ctx: b.Ctx, numAccepted: b.numAccepted, }, diff --git a/snow/engine/snowman/bootstrap/bootstrapper_test.go b/snow/engine/snowman/bootstrap/bootstrapper_test.go index b3d9f3e4a888..6bccfdb58f95 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{ - Parser: vm, + NonVerifyingParse: vm.ParseBlock, AllGetsServer: snowGetHandler, Ctx: ctx, Beacons: vdrs, diff --git a/snow/engine/snowman/bootstrap/config.go b/snow/engine/snowman/bootstrap/config.go index 6f37c457a535..d501e37f5499 100644 --- a/snow/engine/snowman/bootstrap/config.go +++ b/snow/engine/snowman/bootstrap/config.go @@ -38,7 +38,8 @@ type Config struct { VM block.ChainVM - Parser block.Parser + // NonVerifyingParse parses blocks without verifying them. + NonVerifyingParse block.ParseFunc Bootstrapped func() } diff --git a/snow/engine/snowman/bootstrap/storage.go b/snow/engine/snowman/bootstrap/storage.go index 7dafc3a40225..bc5488e3870e 100644 --- a/snow/engine/snowman/bootstrap/storage.go +++ b/snow/engine/snowman/bootstrap/storage.go @@ -38,7 +38,7 @@ const ( func getMissingBlockIDs( ctx context.Context, db database.KeyValueReader, - parser block.Parser, + nonVerifyingParser block.Parser, tree *interval.Tree, lastAcceptedHeight uint64, ) (set.Set[ids.ID], error) { @@ -57,7 +57,7 @@ func getMissingBlockIDs( return nil, err } - blk, err := parser.ParseBlock(ctx, blkBytes) + blk, err := nonVerifyingParser.ParseBlock(ctx, blkBytes) if err != nil { return nil, err } @@ -130,7 +130,7 @@ func execute( haltable common.Haltable, log logging.Func, db database.Database, - parser block.Parser, + nonVerifyingParser block.Parser, tree *interval.Tree, lastAcceptedHeight uint64, ) error { @@ -198,7 +198,7 @@ func execute( for !haltable.Halted() && iterator.Next() { blkBytes := iterator.Value() - blk, err := parser.ParseBlock(ctx, blkBytes) + blk, err := nonVerifyingParser.ParseBlock(ctx, blkBytes) if err != nil { return err } diff --git a/vms/platformvm/vm_test.go b/vms/platformvm/vm_test.go index 8cbb04ca2815..ab2bcb430463 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{ - Parser: &parser{VM: vm}, + NonVerifyingParse: vm.ParseBlock, AllGetsServer: snowGetHandler, Ctx: consensusCtx, Beacons: beacons, @@ -2525,11 +2525,3 @@ func TestPruneMempool(t *testing.T) { _, ok = vm.Builder.Get(baseTxID) require.True(ok) } - -type parser struct { - *VM -} - -func (a *parser) ParseLocalBlock(ctx context.Context, blockBytes []byte) (smcon.Block, error) { - return a.VM.ParseBlock(ctx, blockBytes) -} diff --git a/vms/proposervm/vm_test.go b/vms/proposervm/vm_test.go index 9b1a7924abdb..c6c10056eb56 100644 --- a/vms/proposervm/vm_test.go +++ b/vms/proposervm/vm_test.go @@ -2536,7 +2536,7 @@ func TestLocalParse(t *testing.T) { tests := []struct { name string - f func(ctx context.Context, b []byte) (snowman.Block, error) + f block.ParseFunc block []byte resultingBlock interface{} }{ From 750f0fdd706457ac50e29472e6ea5fda439d1406 Mon Sep 17 00:00:00 2001 From: Yacov Manevich Date: Fri, 2 Aug 2024 00:06:04 +0200 Subject: [PATCH 6/6] Rebase on top of latest master Signed-off-by: Yacov Manevich --- vms/proposervm/vm_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/vms/proposervm/vm_test.go b/vms/proposervm/vm_test.go index c6c10056eb56..9c6a786706a1 100644 --- a/vms/proposervm/vm_test.go +++ b/vms/proposervm/vm_test.go @@ -2512,9 +2512,6 @@ func TestLocalParse(t *testing.T) { improperlySignedBlock[len(improperlySignedBlock)-1] = ^improperlySignedBlock[len(improperlySignedBlock)-1] conf := Config{ - ActivationTime: time.Unix(0, 0), - DurangoTime: time.Unix(0, 0), - MinimumPChainHeight: 0, MinBlkDelay: DefaultMinBlockDelay, NumHistoricalBlocks: DefaultNumHistoricalBlocks, StakingLeafSigner: pTestSigner,