Address code review comments

Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>

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)
+}

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)
 }
 

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
 	}

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)
-}

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,

snow/engine/snowman/bootstrap/config.go

@@ -38,7 +38,7 @@ type Config struct {
 
 	VM block.ChainVM
 
-	Appraiser Appraiser
+	Parser block.Parser
 
 	Bootstrapped func()
 }

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
 		}

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

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)
 }

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

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
 }

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

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
 		}
 	}