Decision txs in proposal blocks

vms/platformvm/block/executor/proposal_block_test.go

@@ -18,7 +18,9 @@ import (
 	"github.com/ava-labs/avalanchego/snow/consensus/snowman"
 	"github.com/ava-labs/avalanchego/utils/constants"
 	"github.com/ava-labs/avalanchego/utils/crypto/secp256k1"
+	"github.com/ava-labs/avalanchego/utils/timer/mockable"
 	"github.com/ava-labs/avalanchego/vms/components/avax"
+	"github.com/ava-labs/avalanchego/vms/components/verify"
 	"github.com/ava-labs/avalanchego/vms/platformvm/block"
 	"github.com/ava-labs/avalanchego/vms/platformvm/reward"
 	"github.com/ava-labs/avalanchego/vms/platformvm/state"
@@ -151,7 +153,8 @@ func TestBanffProposalBlockTimeVerification(t *testing.T) {
 		require.NoError(shutdownEnvironment(env))
 	}()
 	env.clk.Set(defaultGenesisTime)
-	env.config.BanffTime = time.Time{} // activate Banff
+	env.config.BanffTime = time.Time{} // activate Banff, but not Durango
+	env.config.DurangoTime = mockable.MaxTime
 
 	// create parentBlock. It's a standard one for simplicity
 	parentTime := defaultGenesisTime
@@ -1319,3 +1322,114 @@ func TestBanffProposalBlockDelegatorStakers(t *testing.T) {
 	vdrWeight = env.config.Validators.GetWeight(constants.PrimaryNetworkID, nodeID)
 	require.Equal(env.config.MinDelegatorStake+env.config.MinValidatorStake, vdrWeight)
 }
+
+func TestDurangoVerifierVisitProposalBlockWithStandarTxs(t *testing.T) {
+	require := require.New(t)
+	ctrl := gomock.NewController(t)
+
+	env := newEnvironment(t, ctrl)
+	defer func() {
+		require.NoError(shutdownEnvironment(env))
+	}()
+	env.clk.Set(defaultGenesisTime)
+	env.config.DurangoTime = time.Time{} // activate Durango
+
+	// create parentBlock. It's a standard one for simplicity
+	parentTime := defaultGenesisTime
+	parentHeight := uint64(2022)
+
+	parentBlk, err := block.NewBanffStandardBlock(
+		parentTime,
+		genesisBlkID, // does not matter
+		parentHeight,
+		nil, // txs do not matter in this test
+	)
+	require.NoError(err)
+	parentBlkID := parentBlk.ID()
+
+	// store parent block, with relevant quantities
+	chainTime := parentTime
+	env.mockedState.EXPECT().GetTimestamp().Return(chainTime).AnyTimes()
+
+	onParentAccept := state.NewMockDiff(ctrl)
+	onParentAccept.EXPECT().GetTimestamp().Return(parentTime).AnyTimes()
+	onParentAccept.EXPECT().GetCurrentSupply(constants.PrimaryNetworkID).Return(uint64(1000), nil).AnyTimes()
+
+	env.blkManager.(*manager).blkIDToState[parentBlkID] = &blockState{
+		statelessBlock: parentBlk,
+		onAcceptState:  onParentAccept,
+		timestamp:      parentTime,
+	}
+	env.blkManager.(*manager).lastAccepted = parentBlkID
+	env.mockedState.EXPECT().GetLastAccepted().Return(parentBlkID).AnyTimes()
+	env.mockedState.EXPECT().GetStatelessBlock(gomock.Any()).DoAndReturn(
+		func(blockID ids.ID) (block.Block, error) {
+			if blockID == parentBlkID {
+				return parentBlk, nil
+			}
+			return nil, database.ErrNotFound
+		}).AnyTimes()
+
+	// build proposal block with decision txs
+	proposalUnsignedMock := txs.NewMockUnsignedTx(ctrl)
+	proposalUnsignedMock.EXPECT().Visit(gomock.AssignableToTypeOf(&executor.ProposalTxExecutor{})).Return(nil).Times(1)
+	proposalTx := &txs.Tx{
+		Unsigned: proposalUnsignedMock,
+	}
+
+	decisionUnsignedMock := txs.NewMockUnsignedTx(ctrl)
+	decisionUnsignedMock.EXPECT().Visit(gomock.AssignableToTypeOf(&executor.StandardTxExecutor{})).Return(nil).Times(2) // Two times because my dummy implementation sucks. TODO: make this 1
+	decisionTx := &txs.Tx{
+		Unsigned: decisionUnsignedMock,
+	}
+
+	// We can't serialize [blkTx] because it isn't registered with the blocks.Codec.
+	// Serialize this block with a dummy txs and replace it after creation with the mock tx.
+	var (
+		blkTimestamp = chainTime.Add(time.Second)
+		blkHeight    = parentHeight + 1
+	)
+	durangoProposalBlk, err := block.NewBanffProposalBlock(
+		blkTimestamp,
+		parentBlkID,
+		blkHeight,
+		&txs.Tx{
+			Unsigned: &txs.AdvanceTimeTx{},
+			Creds:    []verify.Verifiable{},
+		},
+		[]*txs.Tx{
+			{
+				Unsigned: &txs.ImportTx{}, // a dummy tx that passes marshalling even if empty
+				Creds:    []verify.Verifiable{},
+			},
+		},
+	)
+	require.NoError(err)
+	durangoProposalBlk.ApricotProposalBlock.Tx = proposalTx
+	durangoProposalBlk.Transactions = []*txs.Tx{decisionTx}
+
+	// setup state to validate proposal block transactions
+	var (
+		nextStakerTime = chainTime.Add(time.Second)
+		nextStakerTxID = ids.GenerateTestID()
+	)
+	currentStakersIt := state.NewMockStakerIterator(ctrl)
+	currentStakersIt.EXPECT().Next().Return(true).AnyTimes()
+	currentStakersIt.EXPECT().Value().Return(&state.Staker{
+		TxID:     nextStakerTxID,
+		EndTime:  nextStakerTime,
+		NextTime: nextStakerTime,
+		Priority: txs.PrimaryNetworkValidatorCurrentPriority,
+	}).AnyTimes()
+	currentStakersIt.EXPECT().Release().AnyTimes()
+	onParentAccept.EXPECT().GetCurrentStakerIterator().Return(currentStakersIt, nil).AnyTimes()
+
+	pendingStakersIt := state.NewMockStakerIterator(ctrl)
+	pendingStakersIt.EXPECT().Next().Return(false).AnyTimes() // no pending stakers
+	pendingStakersIt.EXPECT().Release().AnyTimes()
+	onParentAccept.EXPECT().GetPendingStakerIterator().Return(pendingStakersIt, nil).AnyTimes()
+
+	// test: verify the block
+	blk := env.blkManager.NewBlock(durangoProposalBlk)
+	require.NoError(blk.Verify(context.Background()))
+}

vms/platformvm/block/executor/verifier.go

@@ -6,6 +6,7 @@ package executor
 import (
 	"errors"
 	"fmt"
+	reflect "reflect"
 
 	"github.com/ava-labs/avalanchego/chains/atomic"
 	"github.com/ava-labs/avalanchego/ids"
@@ -49,7 +50,8 @@ func (v *verifier) BanffCommitBlock(b *block.BanffCommitBlock) error {
 }
 
 func (v *verifier) BanffProposalBlock(b *block.BanffProposalBlock) error {
-	if len(b.Transactions) != 0 {
+	nextChainTime := b.Timestamp()
+	if !v.txExecutorBackend.Config.IsDurangoActivated(nextChainTime) && len(b.Transactions) != 0 {
 		return errBanffProposalBlockWithMultipleTransactions
 	}
 
@@ -67,8 +69,7 @@ func (v *verifier) BanffProposalBlock(b *block.BanffProposalBlock) error {
 		return err
 	}
 
-	// Apply the changes, if any, from advancing the chain time.
-	nextChainTime := b.Timestamp()
+	// Step 1: advance timestamp and apply resulting changes, if any
 	changes, err := executor.AdvanceTimeTo(
 		v.txExecutorBackend,
 		onCommitState,
@@ -84,7 +85,38 @@ func (v *verifier) BanffProposalBlock(b *block.BanffProposalBlock) error {
 	onAbortState.SetTimestamp(nextChainTime)
 	changes.Apply(onAbortState)
 
-	return v.proposalBlock(&b.ApricotProposalBlock, onCommitState, onAbortState)
+	// Step 2: process standard transactions, if any
+	var atomicRequests map[ids.ID]*atomic.Requests
+	if len(b.Transactions) != 0 {
+		// NOTE: this sucks! Find a way to execute standardTxs once (at least for the same proposal block)
+
+		onCommitBlkState := &blockState{
+			statelessBlock: b,
+			onAcceptState:  onCommitState,
+			timestamp:      onCommitState.GetTimestamp(),
+			atomicRequests: make(map[ids.ID]*atomic.Requests),
+		}
+		if err := v.processStandardTxs(b.Transactions, onCommitBlkState); err != nil {
+			return err
+		}
+
+		onAbortBlkState := &blockState{
+			statelessBlock: b,
+			onAcceptState:  onCommitState,
+			timestamp:      onCommitState.GetTimestamp(),
+			atomicRequests: make(map[ids.ID]*atomic.Requests),
+		}
+		if err := v.processStandardTxs(b.Transactions, onAbortBlkState); err != nil {
+			return err
+		}
+		if !reflect.DeepEqual(onCommitBlkState.atomicRequests, onAbortBlkState.atomicRequests) {
+			return errors.New("this should really never ever happen, so gotta find a structural way to avoid it")
+		}
+		atomicRequests = onCommitBlkState.atomicRequests
+	}
+
+	// Step 3: finally process the proposal transaction
+	return v.proposalBlock(&b.ApricotProposalBlock, onCommitState, onAbortState, atomicRequests)
 }
 
 func (v *verifier) BanffStandardBlock(b *block.BanffStandardBlock) error {
@@ -150,7 +182,7 @@ func (v *verifier) ApricotProposalBlock(b *block.ApricotProposalBlock) error {
 		return err
 	}
 
-	return v.proposalBlock(b, onCommitState, onAbortState)
+	return v.proposalBlock(b, onCommitState, onAbortState, nil)
 }
 
 func (v *verifier) ApricotStandardBlock(b *block.ApricotStandardBlock) error {
@@ -354,6 +386,7 @@ func (v *verifier) proposalBlock(
 	b *block.ApricotProposalBlock,
 	onCommitState state.Diff,
 	onAbortState state.Diff,
+	atomicRequests map[ids.ID]*atomic.Requests,
 ) error {
 	txExecutor := executor.ProposalTxExecutor{
 		OnCommitState: onCommitState,
@@ -382,7 +415,8 @@ func (v *verifier) proposalBlock(
 		// It is safe to use [b.onAbortState] here because the timestamp will
 		// never be modified by an Apricot Abort block and the timestamp will
 		// always be the same as the Banff Proposal Block.
-		timestamp: onAbortState.GetTimestamp(),
+		timestamp:      onAbortState.GetTimestamp(),
+		atomicRequests: atomicRequests,
 	}
 
 	v.Mempool.Remove([]*txs.Tx{b.Tx})
@@ -401,12 +435,22 @@ func (v *verifier) standardBlock(
 		atomicRequests: make(map[ids.ID]*atomic.Requests),
 	}
 
-	// Finally we process the transactions
-	funcs := make([]func(), 0, len(b.Transactions))
-	for _, tx := range b.Transactions {
+	return v.processStandardTxs(b.Txs(), blkState)
+}
+
+func (v *verifier) processStandardTxs(transactions []*txs.Tx, blkState *blockState) error {
+	// We pass explicitly transactions instead of calling b.Txs(), since in proposal blocks
+	// with decision txs, b.Txs() returns the proposal txs which we don't want to validate here.
+	var (
+		parentBlkID = blkState.statelessBlock.Parent()
+		blkID       = blkState.statelessBlock.ID()
+		funcs       = make([]func(), 0, len(transactions))
+	)
+
+	for _, tx := range transactions {
 		txExecutor := executor.StandardTxExecutor{
 			Backend: v.txExecutorBackend,
-			State:   onAcceptState,
+			State:   blkState.onAcceptState,
 			Tx:      tx,
 		}
 		if err := tx.Unsigned.Visit(&txExecutor); err != nil {
@@ -421,7 +465,7 @@ func (v *verifier) standardBlock(
 		// Add UTXOs to batch
 		blkState.inputs.Union(txExecutor.Inputs)
 
-		onAcceptState.AddTx(tx, status.Committed)
+		blkState.onAcceptState.AddTx(tx, status.Committed)
 		if txExecutor.OnAccept != nil {
 			funcs = append(funcs, txExecutor.OnAccept)
 		}
@@ -439,7 +483,7 @@ func (v *verifier) standardBlock(
 		}
 	}
 
-	if err := v.verifyUniqueInputs(b.Parent(), blkState.inputs); err != nil {
+	if err := v.verifyUniqueInputs(parentBlkID, blkState.inputs); err != nil {
 		return err
 	}
 
@@ -453,9 +497,8 @@ func (v *verifier) standardBlock(
 		}
 	}
 
-	blkID := b.ID()
 	v.blkIDToState[blkID] = blkState
 
-	v.Mempool.Remove(b.Transactions)
+	v.Mempool.Remove(transactions)
 	return nil
 }