Provide same P-chain to inner vm as proposervm verification (#2330)

Co-authored-by: Dan Laine <daniel.laine@avalabs.org>

snow/engine/snowman/block/block_context_vm.go

@@ -13,6 +13,9 @@ import (
 // proposervm to an underlying vm.
 type Context struct {
 <<<<<<< HEAD
+<<<<<<< HEAD
+=======
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 	// PChainHeight is the height that this block will use to verify it's state.
 	// In the proposervm, blocks verify the proposer based on the P-chain height
 	// recorded in the parent block. The P-chain height provided here is also
@@ -20,8 +23,11 @@ type Context struct {
 	//
 	// Because PreForkBlocks and PostForkOptions do not verify their execution
 	// against the P-chain's state, this context is undefined for those blocks.
+<<<<<<< HEAD
 =======
 >>>>>>> 37ccd9a48 (Add BuildBlockWithContext as an optional VM method (#2210))
+=======
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 	PChainHeight uint64
 }
 
@@ -30,10 +36,14 @@ type Context struct {
 type BuildBlockWithContextChainVM interface {
 	// Attempt to build a new block given that the P-Chain height is
 	// [blockCtx.PChainHeight].
+<<<<<<< HEAD
 <<<<<<< HEAD
 	//
 =======
 >>>>>>> 37ccd9a48 (Add BuildBlockWithContext as an optional VM method (#2210))
+=======
+	//
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 	// This method will be called if and only if the proposervm is activated.
 	// Otherwise [BuildBlock] will be called.
 	BuildBlockWithContext(ctx context.Context, blockCtx *Context) (snowman.Block, error)
@@ -46,12 +56,18 @@ type BuildBlockWithContextChainVM interface {
 type WithVerifyContext interface {
 	// Returns true if [VerifyWithContext] should be called.
 	// Returns false if [Verify] should be called.
+<<<<<<< HEAD
 <<<<<<< HEAD
 	//
 	// This method will be called if and only if the proposervm is activated.
 	// Otherwise [Verify] will be called.
 =======
 >>>>>>> 552ae0539 (Add optional VerifyWithContext to block (#2145))
+=======
+	//
+	// This method will be called if and only if the proposervm is activated.
+	// Otherwise [Verify] will be called.
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 	ShouldVerifyWithContext(context.Context) (bool, error)
 
 	// Verify that the state transition this block would make if accepted is

vms/proposervm/block.go

@@ -165,6 +165,9 @@ func (p *postForkCommonComponents) Verify(
 	}
 
 <<<<<<< HEAD
+<<<<<<< HEAD
+=======
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 	return p.vm.verifyAndRecordInnerBlk(
 		ctx,
 		&smblock.Context{
@@ -172,9 +175,12 @@ func (p *postForkCommonComponents) Verify(
 		},
 		child,
 	)
+<<<<<<< HEAD
 =======
 	return p.vm.verifyAndRecordInnerBlk(ctx, child)
 >>>>>>> 5be92660b (Pass message context through the VM interface (#2219))
+=======
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 }
 
 // Return the child (a *postForkBlock) of this block
@@ -235,8 +241,12 @@ func (p *postForkCommonComponents) buildChild(
 	var innerBlock snowman.Block
 	if p.vm.blockBuilderVM != nil {
 		innerBlock, err = p.vm.blockBuilderVM.BuildBlockWithContext(ctx, &smblock.Context{
+<<<<<<< HEAD
 			PChainHeight: pChainHeight,
 >>>>>>> 37ccd9a48 (Add BuildBlockWithContext as an optional VM method (#2210))
+=======
+			PChainHeight: parentPChainHeight,
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 		})
 	} else {
 		innerBlock, err = p.vm.ChainVM.BuildBlock(ctx)

vms/proposervm/block_test.go

@@ -49,11 +49,15 @@ func TestPostForkCommonComponents_buildChild(t *testing.T) {
 	innerVM := mocks.NewMockChainVM(ctrl)
 	innerBlockBuilderVM := mocks.NewMockBuildBlockWithContextChainVM(ctrl)
 	innerBlockBuilderVM.EXPECT().BuildBlockWithContext(gomock.Any(), &block.Context{
+<<<<<<< HEAD
 <<<<<<< HEAD
 		PChainHeight: pChainHeight - 1,
 =======
 		PChainHeight: pChainHeight,
 >>>>>>> 37ccd9a48 (Add BuildBlockWithContext as an optional VM method (#2210))
+=======
+		PChainHeight: pChainHeight - 1,
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 	}).Return(builtBlk, nil).AnyTimes()
 	vdrState := validators.NewMockState(ctrl)
 	vdrState.EXPECT().GetMinimumHeight(context.Background()).Return(pChainHeight, nil).AnyTimes()

vms/proposervm/post_fork_block.go

@@ -160,11 +160,15 @@ func (b *postForkBlock) verifyPostForkOption(ctx context.Context, child *postFor
 		return errInnerParentMismatch
 	}
 
+<<<<<<< HEAD
 <<<<<<< HEAD
 	return child.vm.verifyAndRecordInnerBlk(ctx, nil, child)
 =======
 	return child.vm.verifyAndRecordInnerBlk(ctx, child)
 >>>>>>> 5be92660b (Pass message context through the VM interface (#2219))
+=======
+	return child.vm.verifyAndRecordInnerBlk(ctx, nil, child)
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 }
 
 // Return the child (a *postForkBlock) of this block

vms/proposervm/pre_fork_block.go

@@ -13,8 +13,6 @@ import (
 	"github.com/ava-labs/avalanchego/snow/choices"
 	"github.com/ava-labs/avalanchego/snow/consensus/snowman"
 	"github.com/ava-labs/avalanchego/vms/proposervm/block"
-
-	smblock "github.com/ava-labs/avalanchego/snow/engine/snowman/block"
 )
 
 var _ Block = (*preForkBlock)(nil)
@@ -155,6 +153,7 @@ func (b *preForkBlock) verifyPostForkChild(ctx context.Context, child *postForkB
 	}
 
 	// Verify the inner block and track it as verified
+<<<<<<< HEAD
 <<<<<<< HEAD
 	return b.vm.verifyAndRecordInnerBlk(ctx, nil, child)
 }
@@ -170,6 +169,9 @@ func (*preForkBlock) verifyPostForkOption(*postForkOption) error {
 >>>>>>> 3a7ebb1da (Add UnusedParameter linter (#2226))
 =======
 	return b.vm.verifyAndRecordInnerBlk(ctx, child)
+=======
+	return b.vm.verifyAndRecordInnerBlk(ctx, nil, child)
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 }
 
 func (*preForkBlock) verifyPostForkOption(context.Context, *postForkOption) error {
@@ -213,14 +215,7 @@ func (b *preForkBlock) buildChild(ctx context.Context) (Block, error) {
 		return nil, err
 	}
 
-	var innerBlock snowman.Block
-	if b.vm.blockBuilderVM != nil {
-		innerBlock, err = b.vm.blockBuilderVM.BuildBlockWithContext(ctx, &smblock.Context{
-			PChainHeight: pChainHeight,
-		})
-	} else {
-		innerBlock, err = b.vm.ChainVM.BuildBlock(ctx)
-	}
+	innerBlock, err := b.vm.ChainVM.BuildBlock(ctx)
 	if err != nil {
 		return nil, err
 	}

vms/proposervm/pre_fork_block_test.go

@@ -24,8 +24,6 @@ import (
 	"github.com/ava-labs/avalanchego/utils/timer/mockable"
 	"github.com/ava-labs/avalanchego/vms/proposervm/block"
 	"github.com/ava-labs/avalanchego/vms/proposervm/proposer"
-
-	smblock "github.com/ava-labs/avalanchego/snow/engine/snowman/block"
 )
 
 func TestOracle_PreForkBlkImplementsInterface(t *testing.T) {
@@ -962,6 +960,7 @@ func TestBlockVerify_ForkBlockIsOracleBlockButChildrenAreSigned(t *testing.T) {
 
 // Assert that when the underlying VM implements ChainVMWithBuildBlockContext
 <<<<<<< HEAD
+<<<<<<< HEAD
 // and the proposervm is activated, we only call the VM's BuildBlockWithContext
 // when a P-chain height can be correctly provided from the parent block.
 =======
@@ -969,6 +968,10 @@ func TestBlockVerify_ForkBlockIsOracleBlockButChildrenAreSigned(t *testing.T) {
 // method to build a block rather than BuildBlockWithContext. If the proposervm
 // isn't activated, we should call BuildBlock rather than BuildBlockWithContext.
 >>>>>>> 37ccd9a48 (Add BuildBlockWithContext as an optional VM method (#2210))
+=======
+// and the proposervm is activated, we only call the VM's BuildBlockWithContext
+// when a P-chain height can be correctly provided from the parent block.
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 func TestPreForkBlock_BuildBlockWithContext(t *testing.T) {
 	require := require.New(t)
 	ctrl := gomock.NewController(t)
@@ -984,6 +987,7 @@ func TestPreForkBlock_BuildBlockWithContext(t *testing.T) {
 	builtBlk.EXPECT().ID().Return(ids.GenerateTestID()).AnyTimes()
 	builtBlk.EXPECT().Height().Return(pChainHeight).AnyTimes()
 	innerVM := mocks.NewMockChainVM(ctrl)
+<<<<<<< HEAD
 <<<<<<< HEAD
 	innerVM.EXPECT().BuildBlock(gomock.Any()).Return(builtBlk, nil).AnyTimes()
 =======
@@ -992,16 +996,23 @@ func TestPreForkBlock_BuildBlockWithContext(t *testing.T) {
 		PChainHeight: pChainHeight,
 	}).Return(builtBlk, nil).AnyTimes()
 >>>>>>> 37ccd9a48 (Add BuildBlockWithContext as an optional VM method (#2210))
+=======
+	innerVM.EXPECT().BuildBlock(gomock.Any()).Return(builtBlk, nil).AnyTimes()
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 	vdrState := validators.NewMockState(ctrl)
 	vdrState.EXPECT().GetMinimumHeight(context.Background()).Return(pChainHeight, nil).AnyTimes()
 
 	vm := &VM{
+<<<<<<< HEAD
 <<<<<<< HEAD
 		ChainVM: innerVM,
 =======
 		ChainVM:        innerVM,
 		blockBuilderVM: innerBlockBuilderVM,
 >>>>>>> 37ccd9a48 (Add BuildBlockWithContext as an optional VM method (#2210))
+=======
+		ChainVM: innerVM,
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 		ctx: &snow.Context{
 			ValidatorState: vdrState,
 			Log:            logging.NoLog{},
@@ -1013,12 +1024,16 @@ func TestPreForkBlock_BuildBlockWithContext(t *testing.T) {
 		vm:    vm,
 	}
 
+<<<<<<< HEAD
 <<<<<<< HEAD
 	// Should call BuildBlock since proposervm won't have a P-chain height
 =======
 	// Should call BuildBlockWithContext since proposervm is activated
 	// (timestamp is after activation time)
 >>>>>>> 37ccd9a48 (Add BuildBlockWithContext as an optional VM method (#2210))
+=======
+	// Should call BuildBlock since proposervm won't have a P-chain height
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 	gotChild, err := blk.buildChild(context.Background())
 	require.NoError(err)
 	require.Equal(builtBlk, gotChild.(*postForkBlock).innerBlk)
@@ -1027,9 +1042,12 @@ func TestPreForkBlock_BuildBlockWithContext(t *testing.T) {
 	innerBlk.EXPECT().Timestamp().Return(time.Time{})
 	vm.activationTime = mockable.MaxTime
 <<<<<<< HEAD
+<<<<<<< HEAD
 =======
 	innerVM.EXPECT().BuildBlock(context.Background()).Return(builtBlk, nil)
 >>>>>>> 37ccd9a48 (Add BuildBlockWithContext as an optional VM method (#2210))
+=======
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 
 	gotChild, err = blk.buildChild(context.Background())
 	require.NoError(err)

vms/proposervm/vm.go

@@ -770,6 +770,7 @@ func (vm *VM) storePostForkBlock(blk PostForkBlock) error {
 	return vm.db.Commit()
 }
 
+<<<<<<< HEAD
 <<<<<<< HEAD
 func (vm *VM) verifyAndRecordInnerBlk(ctx context.Context, blockCtx *block.Context, postFork PostForkBlock) error {
 	innerBlk := postFork.getInnerBlk()
@@ -797,6 +798,9 @@ func (vm *VM) verifyAndRecordInnerBlk(ctx context.Context, blockCtx *block.Conte
 			}
 =======
 func (vm *VM) verifyAndRecordInnerBlk(ctx context.Context, postFork PostForkBlock) error {
+=======
+func (vm *VM) verifyAndRecordInnerBlk(ctx context.Context, blockCtx *block.Context, postFork PostForkBlock) error {
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 	innerBlk := postFork.getInnerBlk()
 	postForkID := postFork.ID()
 	originalInnerBlock, previouslyVerified := vm.Tree.Get(innerBlk)
@@ -808,13 +812,25 @@ func (vm *VM) verifyAndRecordInnerBlk(ctx context.Context, postFork PostForkBloc
 		vm.innerBlkCache.Put(postForkID, originalInnerBlock)
 	}
 
-	var err error
-	blkWithCtx, shouldVerifyWithCtx := innerBlk.(block.WithVerifyContext)
+	var (
+		shouldVerifyWithCtx = blockCtx != nil
+		blkWithCtx          block.WithVerifyContext
+		err                 error
+	)
 	if shouldVerifyWithCtx {
+<<<<<<< HEAD
 		shouldVerifyWithCtx, err = blkWithCtx.ShouldVerifyWithContext(ctx)
 		if err != nil {
 			return err
 >>>>>>> 5be92660b (Pass message context through the VM interface (#2219))
+=======
+		blkWithCtx, shouldVerifyWithCtx = innerBlk.(block.WithVerifyContext)
+		if shouldVerifyWithCtx {
+			shouldVerifyWithCtx, err = blkWithCtx.ShouldVerifyWithContext(ctx)
+			if err != nil {
+				return err
+			}
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 		}
 	}
 
@@ -827,6 +843,7 @@ func (vm *VM) verifyAndRecordInnerBlk(ctx context.Context, postFork PostForkBloc
 		// Note that [VerifyWithContext] with context may be called multiple
 		// times with multiple contexts.
 <<<<<<< HEAD
+<<<<<<< HEAD
 =======
 		blockCtx := &block.Context{}
 		blockCtx.PChainHeight, err = postFork.pChainHeight(ctx)
@@ -834,6 +851,8 @@ func (vm *VM) verifyAndRecordInnerBlk(ctx context.Context, postFork PostForkBloc
 			return err
 		}
 >>>>>>> 552ae0539 (Add optional VerifyWithContext to block (#2145))
+=======
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 		err = blkWithCtx.VerifyWithContext(ctx, blockCtx)
 	} else if !previouslyVerified {
 		// This isn't a [block.WithVerifyContext] so we only call [Verify] once.

vms/proposervm/vm_test.go

@@ -3222,6 +3222,7 @@ func TestVM_VerifyBlockWithContext(t *testing.T) {
 		blk.EXPECT().getInnerBlk().Return(innerBlk).AnyTimes()
 		blkID := ids.GenerateTestID()
 		blk.EXPECT().ID().Return(blkID).AnyTimes()
+<<<<<<< HEAD
 <<<<<<< HEAD
 
 		err = vm.verifyAndRecordInnerBlk(
@@ -3236,22 +3237,8 @@ func TestVM_VerifyBlockWithContext(t *testing.T) {
 
 		err = vm.verifyAndRecordInnerBlk(context.Background(), blk)
 >>>>>>> 552ae0539 (Add optional VerifyWithContext to block (#2145))
-		require.NoError(err)
-
-		// Call VerifyWithContext again but with a different P-Chain height
-		blk.EXPECT().setInnerBlk(innerBlk).AnyTimes()
-		pChainHeight++
-<<<<<<< HEAD
 =======
-		blk.EXPECT().pChainHeight(context.Background()).Return(pChainHeight, nil)
->>>>>>> 552ae0539 (Add optional VerifyWithContext to block (#2145))
-		innerBlk.MockWithVerifyContext.EXPECT().VerifyWithContext(context.Background(),
-			&block.Context{
-				PChainHeight: pChainHeight,
-			},
-		).Return(nil)
 
-<<<<<<< HEAD
 		err = vm.verifyAndRecordInnerBlk(
 			context.Background(),
 			&block.Context{
@@ -3259,6 +3246,37 @@ func TestVM_VerifyBlockWithContext(t *testing.T) {
 			},
 			blk,
 		)
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
+		require.NoError(err)
+
+		// Call VerifyWithContext again but with a different P-Chain height
+		blk.EXPECT().setInnerBlk(innerBlk).AnyTimes()
+		pChainHeight++
+<<<<<<< HEAD
+<<<<<<< HEAD
+=======
+		blk.EXPECT().pChainHeight(context.Background()).Return(pChainHeight, nil)
+>>>>>>> 552ae0539 (Add optional VerifyWithContext to block (#2145))
+=======
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
+		innerBlk.MockWithVerifyContext.EXPECT().VerifyWithContext(context.Background(),
+			&block.Context{
+				PChainHeight: pChainHeight,
+			},
+		).Return(nil)
+
+<<<<<<< HEAD
+<<<<<<< HEAD
+=======
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
+		err = vm.verifyAndRecordInnerBlk(
+			context.Background(),
+			&block.Context{
+				PChainHeight: pChainHeight,
+			},
+			blk,
+		)
+<<<<<<< HEAD
 		require.NoError(err)
 	}
 
@@ -3305,15 +3323,53 @@ func TestVM_VerifyBlockWithContext(t *testing.T) {
 	}
 =======
 		err = vm.verifyAndRecordInnerBlk(context.Background(), blk)
+=======
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 		require.NoError(err)
 	}
 
-	// Ensure we call Verify on a block that returns
-	// false for ShouldVerifyWithContext
-	innerBlk := blockWithVerifyContext{
-		MockBlock:             snowman.NewMockBlock(ctrl),
-		MockWithVerifyContext: mocks.NewMockWithVerifyContext(ctrl),
+	{
+		// Ensure we call Verify on a block that returns
+		// false for ShouldVerifyWithContext
+		innerBlk := blockWithVerifyContext{
+			MockBlock:             snowman.NewMockBlock(ctrl),
+			MockWithVerifyContext: mocks.NewMockWithVerifyContext(ctrl),
+		}
+		innerBlk.MockWithVerifyContext.EXPECT().ShouldVerifyWithContext(gomock.Any()).Return(false, nil)
+		innerBlk.MockBlock.EXPECT().Verify(gomock.Any()).Return(nil)
+		innerBlk.MockBlock.EXPECT().Parent().Return(ids.GenerateTestID()).AnyTimes()
+		innerBlk.MockBlock.EXPECT().ID().Return(ids.GenerateTestID()).AnyTimes()
+		blk := NewMockPostForkBlock(ctrl)
+		blk.EXPECT().getInnerBlk().Return(innerBlk).AnyTimes()
+		blkID := ids.GenerateTestID()
+		blk.EXPECT().ID().Return(blkID).AnyTimes()
+		err = vm.verifyAndRecordInnerBlk(
+			context.Background(),
+			&block.Context{
+				PChainHeight: 1,
+			},
+			blk,
+		)
+		require.NoError(err)
 	}
+
+	{
+		// Ensure we call Verify on a block that doesn't have a valid context
+		innerBlk := blockWithVerifyContext{
+			MockBlock:             snowman.NewMockBlock(ctrl),
+			MockWithVerifyContext: mocks.NewMockWithVerifyContext(ctrl),
+		}
+		innerBlk.MockBlock.EXPECT().Verify(gomock.Any()).Return(nil)
+		innerBlk.MockBlock.EXPECT().Parent().Return(ids.GenerateTestID()).AnyTimes()
+		innerBlk.MockBlock.EXPECT().ID().Return(ids.GenerateTestID()).AnyTimes()
+		blk := NewMockPostForkBlock(ctrl)
+		blk.EXPECT().getInnerBlk().Return(innerBlk).AnyTimes()
+		blkID := ids.GenerateTestID()
+		blk.EXPECT().ID().Return(blkID).AnyTimes()
+		err = vm.verifyAndRecordInnerBlk(context.Background(), nil, blk)
+		require.NoError(err)
+	}
+<<<<<<< HEAD
 	innerBlk.MockWithVerifyContext.EXPECT().ShouldVerifyWithContext(gomock.Any()).Return(false, nil)
 	innerBlk.MockBlock.EXPECT().Verify(gomock.Any()).Return(nil)
 	innerBlk.MockBlock.EXPECT().Parent().Return(ids.GenerateTestID()).AnyTimes()
@@ -3325,4 +3381,6 @@ func TestVM_VerifyBlockWithContext(t *testing.T) {
 	err = vm.verifyAndRecordInnerBlk(context.Background(), blk)
 	require.NoError(err)
 >>>>>>> 552ae0539 (Add optional VerifyWithContext to block (#2145))
+=======
+>>>>>>> f083e702f (Provide same P-chain to inner vm as proposervm verification (#2330))
 }