test(baseapp): Refactor tx selector tests + better comments (backport #19284)

baseapp/abci_utils.go

@@ -105,8 +105,8 @@
 				panic(fmt.Errorf("failed to get signatures: %w", err))
 			}
 
-			// if the signers aren't in selectedTxsSignersSeqs then we haven't seen them before
-			// so we add them and return true so this tx gets selected.
+			// If the signers aren't in selectedTxsSignersSeqs then we haven't seen them before
+			// so we add them and continue given that we don't need to check the sequence.
 			shouldAdd := true
 			txSignersSeqs := make(map[string]uint64)
 			for _, sig := range sigs {
@@ -117,11 +117,18 @@
 					continue
 				}
 
+<<<<<<< HEAD
 				// if we have seen this signer before we check if the sequence we just got is
 				// seq+1 and if it is we update the sequence and return true so this tx gets
 				// selected. If it isn't seq+1 we return false so this tx doesn't get
 				// selected (it could be the same sequence or seq+2 which are invalid).
 				if seq+1 != sig.Sequence {
+=======
+				// If we have seen this signer before in this block, we must make
+				// sure that the current sequence is seq+1; otherwise is invalid
+				// and we skip it.
+				if seq+1 != signer.Sequence {
+>>>>>>> a86a83f76 (test(baseapp): Refactor tx selector tests + better comments  (#19284))
 					shouldAdd = false
 					break
 				}
@@ -150,9 +157,16 @@
 
 				txsLen := len(h.txSelector.SelectedTxs())
 				for sender, seq := range txSignersSeqs {
+					// If txsLen != selectedTxsNums is true, it means that we've
+					// added a new tx to the selected txs, so we need to update
+					// the sequence of the sender.
 					if txsLen != selectedTxsNums {
 						selectedTxsSignersSeqs[sender] = seq
 					} else if _, ok := selectedTxsSignersSeqs[sender]; !ok {
+						// The transaction hasn't been added but it passed the
+						// verification, so we know that the sequence is correct.
+						// So we set this sender's sequence to seq-1, in order
+						// to avoid unnecessary calls to PrepareProposalVerifyTx.
 						selectedTxsSignersSeqs[sender] = seq - 1
 					}
 				}
@@ -162,9 +176,9 @@
 			iterator = iterator.Next()
 		}
 
 		return abci.ResponsePrepareProposal{Txs: h.txSelector.SelectedTxs()}
 	}
 }
 
 // ProcessProposalHandler returns the default implementation for processing an
 // ABCI proposal. Every transaction in the proposal must pass 2 conditions:
@@ -177,12 +191,12 @@
 // DefaultPrepareProposal. It is very important that the same validation logic
 // is used in both steps, and applications must ensure that this is the case in
 // non-default handlers.
 func (h *DefaultProposalHandler) ProcessProposalHandler() sdk.ProcessProposalHandler {
 	// If the mempool is nil or NoOp we simply return ACCEPT,
 	// because PrepareProposal may have included txs that could fail verification.
 	_, isNoOp := h.mempool.(mempool.NoOpMempool)
 	if h.mempool == nil || isNoOp {
 		return NoOpProcessProposal()
 	}
 
 	return func(ctx sdk.Context, req abci.RequestProcessProposal) abci.ResponseProcessProposal {

baseapp/abci_utils_test.go

@@ -2,7 +2,6 @@
 
 import (
 	"bytes"
-	"strings"
 	"testing"
 
 	abci "github.com/cometbft/cometbft/abci/types"
@@ -94,6 +93,7 @@
 	cdc := codec.NewProtoCodec(codectypes.NewInterfaceRegistry())
 	baseapptestutil.RegisterInterfaces(cdc.InterfaceRegistry())
 	txConfig := authtx.NewTxConfig(cdc, authtx.DefaultSignModes)
+<<<<<<< HEAD
 	ctrl := gomock.NewController(s.T())
 	app := mock.NewMockProposalTxVerifier(ctrl)
 	mp1 := mempool.NewPriorityMempool()
@@ -102,133 +102,161 @@
 	handler1 := ph1.PrepareProposalHandler()
 	ph2 := baseapp.NewDefaultProposalHandler(mp2, app)
 	handler2 := ph2.PrepareProposalHandler()
+=======
+
+>>>>>>> a86a83f76 (test(baseapp): Refactor tx selector tests + better comments  (#19284))
 	var (
 		secret1 = []byte("secret1")
 		secret2 = []byte("secret2")
 		secret3 = []byte("secret3")
 		secret4 = []byte("secret4")
 		secret5 = []byte("secret5")
 		secret6 = []byte("secret6")
-		ctx1    = s.ctx.WithPriority(10)
-		ctx2    = s.ctx.WithPriority(8)
 	)
 
-	tx1 := buildMsg(s.T(), txConfig, []byte(`1`), [][]byte{secret1}, []uint64{1})
-	tx2 := buildMsg(s.T(), txConfig, []byte(`12345678910`), [][]byte{secret1}, []uint64{2})
-	tx3 := buildMsg(s.T(), txConfig, []byte(`12`), [][]byte{secret1}, []uint64{3})
-	tx4 := buildMsg(s.T(), txConfig, []byte(`12`), [][]byte{secret2}, []uint64{1})
-	err := mp1.Insert(ctx1, tx1)
-	s.Require().NoError(err)
-	err = mp1.Insert(ctx1, tx2)
-	s.Require().NoError(err)
-	err = mp1.Insert(ctx1, tx3)
-	s.Require().NoError(err)
-	err = mp1.Insert(ctx2, tx4)
-	s.Require().NoError(err)
-	txBz1, err := txConfig.TxEncoder()(tx1)
-	s.Require().NoError(err)
-	txBz2, err := txConfig.TxEncoder()(tx2)
-	s.Require().NoError(err)
-	txBz3, err := txConfig.TxEncoder()(tx3)
-	s.Require().NoError(err)
-	txBz4, err := txConfig.TxEncoder()(tx4)
-	s.Require().NoError(err)
-	app.EXPECT().PrepareProposalVerifyTx(tx1).Return(txBz1, nil).AnyTimes()
-	app.EXPECT().PrepareProposalVerifyTx(tx2).Return(txBz2, nil).AnyTimes()
-	app.EXPECT().PrepareProposalVerifyTx(tx3).Return(txBz3, nil).AnyTimes()
-	app.EXPECT().PrepareProposalVerifyTx(tx4).Return(txBz4, nil).AnyTimes()
-	txDataSize1 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz1}))
-	txDataSize2 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz2}))
-	txDataSize3 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz3}))
-	txDataSize4 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz4}))
-	s.Require().Equal(txDataSize1, 111)
-	s.Require().Equal(txDataSize2, 121)
-	s.Require().Equal(txDataSize3, 112)
-	s.Require().Equal(txDataSize4, 112)
-
-	tx5 := buildMsg(s.T(), txConfig, []byte(`1`), [][]byte{secret1, secret2}, []uint64{1, 1})
-	tx6 := buildMsg(s.T(), txConfig, []byte(`12345678910`), [][]byte{secret1, secret3}, []uint64{2, 1})
-	tx7 := buildMsg(s.T(), txConfig, []byte(`12`), [][]byte{secret1, secret4}, []uint64{3, 1})
-	tx8 := buildMsg(s.T(), txConfig, []byte(`12`), [][]byte{secret3, secret5}, []uint64{2, 1})
-	tx9 := buildMsg(s.T(), txConfig, []byte(`12`), [][]byte{secret2, secret6}, []uint64{2, 1})
+	type testTx struct {
+		tx       sdk.Tx
+		priority int64
+		bz       []byte
+		size     int
+	}
 
-	err = mp2.Insert(ctx1, tx5)
-	s.Require().NoError(err)
-	err = mp2.Insert(ctx1, tx6)
-	s.Require().NoError(err)
-	err = mp2.Insert(ctx2, tx7)
-	s.Require().NoError(err)
-	err = mp2.Insert(ctx2, tx8)
-	s.Require().NoError(err)
-	err = mp2.Insert(ctx2, tx9)
-	s.Require().NoError(err)
-	txBz5, err := txConfig.TxEncoder()(tx5)
-	s.Require().NoError(err)
-	txBz6, err := txConfig.TxEncoder()(tx6)
-	s.Require().NoError(err)
-	txBz7, err := txConfig.TxEncoder()(tx7)
-	s.Require().NoError(err)
-	txBz8, err := txConfig.TxEncoder()(tx8)
-	s.Require().NoError(err)
-	txBz9, err := txConfig.TxEncoder()(tx9)
-	s.Require().NoError(err)
-	app.EXPECT().PrepareProposalVerifyTx(tx5).Return(txBz5, nil).AnyTimes()
-	app.EXPECT().PrepareProposalVerifyTx(tx6).Return(txBz6, nil).AnyTimes()
-	app.EXPECT().PrepareProposalVerifyTx(tx7).Return(txBz7, nil).AnyTimes()
-	app.EXPECT().PrepareProposalVerifyTx(tx8).Return(txBz8, nil).AnyTimes()
-	app.EXPECT().PrepareProposalVerifyTx(tx9).Return(txBz9, nil).AnyTimes()
-	txDataSize5 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz5}))
-	txDataSize6 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz6}))
-	txDataSize7 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz7}))
-	txDataSize8 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz8}))
-	txDataSize9 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz9}))
-	s.Require().Equal(txDataSize5, 195)
-	s.Require().Equal(txDataSize6, 205)
-	s.Require().Equal(txDataSize7, 196)
-	s.Require().Equal(txDataSize8, 196)
-	s.Require().Equal(txDataSize9, 196)
+	testTxs := []testTx{
+		// test 1
+		{tx: buildMsg(s.T(), txConfig, []byte(`0`), [][]byte{secret1}, []uint64{1}), priority: 10},
+		{tx: buildMsg(s.T(), txConfig, []byte(`12345678910`), [][]byte{secret1}, []uint64{2}), priority: 10},
+		{tx: buildMsg(s.T(), txConfig, []byte(`22`), [][]byte{secret1}, []uint64{3}), priority: 10},
+		{tx: buildMsg(s.T(), txConfig, []byte(`32`), [][]byte{secret2}, []uint64{1}), priority: 8},
+		// test 2
+		{tx: buildMsg(s.T(), txConfig, []byte(`4`), [][]byte{secret1, secret2}, []uint64{3, 3}), priority: 10},
+		{tx: buildMsg(s.T(), txConfig, []byte(`52345678910`), [][]byte{secret1, secret3}, []uint64{4, 3}), priority: 10},
+		{tx: buildMsg(s.T(), txConfig, []byte(`62`), [][]byte{secret1, secret4}, []uint64{5, 3}), priority: 8},
+		{tx: buildMsg(s.T(), txConfig, []byte(`72`), [][]byte{secret3, secret5}, []uint64{4, 3}), priority: 8},
+		{tx: buildMsg(s.T(), txConfig, []byte(`82`), [][]byte{secret2, secret6}, []uint64{4, 3}), priority: 8},
+		// test 3
+		{tx: buildMsg(s.T(), txConfig, []byte(`9`), [][]byte{secret3, secret4}, []uint64{3, 3}), priority: 10},
+		{tx: buildMsg(s.T(), txConfig, []byte(`1052345678910`), [][]byte{secret1, secret2}, []uint64{4, 4}), priority: 8},
+		{tx: buildMsg(s.T(), txConfig, []byte(`11`), [][]byte{secret1, secret2}, []uint64{5, 5}), priority: 8},
+		// test 4
+		{tx: buildMsg(s.T(), txConfig, []byte(`1252345678910`), [][]byte{secret1}, []uint64{3}), priority: 10},
+		{tx: buildMsg(s.T(), txConfig, []byte(`13`), [][]byte{secret1}, []uint64{5}), priority: 10},
+		{tx: buildMsg(s.T(), txConfig, []byte(`14`), [][]byte{secret1}, []uint64{6}), priority: 8},
+	}
 
-	mapTxs := map[string]string{
-		string(txBz1): "1",
-		string(txBz2): "2",
-		string(txBz3): "3",
-		string(txBz4): "4",
-		string(txBz5): "5",
-		string(txBz6): "6",
-		string(txBz7): "7",
-		string(txBz8): "8",
-		string(txBz9): "9",
+	for i := range testTxs {
+		bz, err := txConfig.TxEncoder()(testTxs[i].tx)
+		s.Require().NoError(err)
+		testTxs[i].bz = bz
+		testTxs[i].size = int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{bz}))
 	}
+
+	s.Require().Equal(testTxs[0].size, 111)
+	s.Require().Equal(testTxs[1].size, 121)
+	s.Require().Equal(testTxs[2].size, 112)
+	s.Require().Equal(testTxs[3].size, 112)
+	s.Require().Equal(testTxs[4].size, 195)
+	s.Require().Equal(testTxs[5].size, 205)
+	s.Require().Equal(testTxs[6].size, 196)
+	s.Require().Equal(testTxs[7].size, 196)
+	s.Require().Equal(testTxs[8].size, 196)
+
 	testCases := map[string]struct {
 		ctx         sdk.Context
+<<<<<<< HEAD
 		req         abci.RequestPrepareProposal
+=======
+		txInputs    []testTx
+		req         *abci.RequestPrepareProposal
+>>>>>>> a86a83f76 (test(baseapp): Refactor tx selector tests + better comments  (#19284))
 		handler     sdk.PrepareProposalHandler
-		expectedTxs [][]byte
+		expectedTxs []int
 	}{
 		"skip same-sender non-sequential sequence and then add others txs": {
+<<<<<<< HEAD
 			ctx: s.ctx,
 			req: abci.RequestPrepareProposal{
 				Txs:        [][]byte{txBz1, txBz2, txBz3, txBz4},
+=======
+			ctx:      s.ctx,
+			txInputs: []testTx{testTxs[0], testTxs[1], testTxs[2], testTxs[3]},
+			req: &abci.RequestPrepareProposal{
+>>>>>>> a86a83f76 (test(baseapp): Refactor tx selector tests + better comments  (#19284))
 				MaxTxBytes: 111 + 112,
 			},
-			handler:     handler1,
-			expectedTxs: [][]byte{txBz1, txBz4},
+			expectedTxs: []int{0, 3},
 		},
 		"skip multi-signers msg non-sequential sequence": {
+<<<<<<< HEAD
 			ctx: s.ctx,
 			req: abci.RequestPrepareProposal{
 				Txs:        [][]byte{txBz5, txBz6, txBz7, txBz8, txBz9},
+=======
+			ctx:      s.ctx,
+			txInputs: []testTx{testTxs[4], testTxs[5], testTxs[6], testTxs[7], testTxs[8]},
+			req: &abci.RequestPrepareProposal{
+>>>>>>> a86a83f76 (test(baseapp): Refactor tx selector tests + better comments  (#19284))
+				MaxTxBytes: 195 + 196,
+			},
+			expectedTxs: []int{4, 8},
+		},
+		"only the first tx is added": {
+			// Because tx 10 is valid, tx 11 can't be valid as they have higher sequence numbers.
+			ctx:      s.ctx,
+			txInputs: []testTx{testTxs[9], testTxs[10], testTxs[11]},
+			req: &abci.RequestPrepareProposal{
 				MaxTxBytes: 195 + 196,
 			},
-			handler:     handler2,
-			expectedTxs: [][]byte{txBz5, txBz9},
+			expectedTxs: []int{9},
+		},
+		"no txs added": {
+			// Becasuse the first tx was deemed valid but too big, the next expected valid sequence is tx[0].seq (3), so
+			// the rest of the txs fail because they have a seq of 4.
+			ctx:      s.ctx,
+			txInputs: []testTx{testTxs[12], testTxs[13], testTxs[14]},
+			req: &abci.RequestPrepareProposal{
+				MaxTxBytes: 112,
+			},
+			expectedTxs: []int{},
 		},
 	}
 
 	for name, tc := range testCases {
 		s.Run(name, func() {
+<<<<<<< HEAD
 			resp := tc.handler(tc.ctx, tc.req)
 			s.Require().EqualValues(toHumanReadable(mapTxs, resp.Txs), toHumanReadable(mapTxs, tc.expectedTxs))
+=======
+			ctrl := gomock.NewController(s.T())
+			app := mock.NewMockProposalTxVerifier(ctrl)
+			mp := mempool.NewPriorityMempool(
+				mempool.PriorityNonceMempoolConfig[int64]{
+					TxPriority:      mempool.NewDefaultTxPriority(),
+					MaxTx:           0,
+					SignerExtractor: mempool.NewDefaultSignerExtractionAdapter(),
+				},
+			)
+
+			ph := baseapp.NewDefaultProposalHandler(mp, app)
+
+			for _, v := range tc.txInputs {
+				app.EXPECT().PrepareProposalVerifyTx(v.tx).Return(v.bz, nil).AnyTimes()
+				s.NoError(mp.Insert(s.ctx.WithPriority(v.priority), v.tx))
+				tc.req.Txs = append(tc.req.Txs, v.bz)
+			}
+
+			resp, err := ph.PrepareProposalHandler()(tc.ctx, tc.req)
+			s.Require().NoError(err)
+			respTxIndexes := []int{}
+			for _, tx := range resp.Txs {
+				for i, v := range testTxs {
+					if bytes.Equal(tx, v.bz) {
+						respTxIndexes = append(respTxIndexes, i)
+					}
+				}
+			}
+
+			s.Require().EqualValues(tc.expectedTxs, respTxIndexes)
+>>>>>>> a86a83f76 (test(baseapp): Refactor tx selector tests + better comments  (#19284))
 		})
 	}
 }
@@ -264,14 +292,6 @@
 	return builder.GetTx()
 }
 
-func toHumanReadable(mapTxs map[string]string, txs [][]byte) string {
-	strs := []string{}
-	for _, v := range txs {
-		strs = append(strs, mapTxs[string(v)])
-	}
-	return strings.Join(strs, ",")
-}
-
 func setTxSignatureWithSecret(t *testing.T, builder client.TxBuilder, signatures ...signingtypes.SignatureV2) {
 	t.Helper()
 	err := builder.SetSignatures(