diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go index 1c42411d2e75..f31446d540b7 100644 --- a/baseapp/abci_utils.go +++ b/baseapp/abci_utils.go @@ -236,8 +236,8 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan return nil, 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 _, signer := range signerData { @@ -247,10 +247,9 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan continue } - // 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 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 { shouldAdd = false break @@ -280,9 +279,16 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan txsLen := len(h.txSelector.SelectedTxs(ctx)) 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 } } diff --git a/baseapp/abci_utils_test.go b/baseapp/abci_utils_test.go index d5de8e2e7b45..82562c45916b 100644 --- a/baseapp/abci_utils_test.go +++ b/baseapp/abci_utils_test.go @@ -2,7 +2,6 @@ package baseapp_test import ( "bytes" - "strings" "testing" abci "github.com/cometbft/cometbft/abci/types" @@ -424,26 +423,7 @@ func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_PriorityNonceMempoolTxSe cdc := codectestutil.CodecOptions{}.NewCodec() baseapptestutil.RegisterInterfaces(cdc.InterfaceRegistry()) txConfig := authtx.NewTxConfig(cdc, authtx.DefaultSignModes) - ctrl := gomock.NewController(s.T()) - app := mock.NewMockProposalTxVerifier(ctrl) - mp1 := mempool.NewPriorityMempool( - mempool.PriorityNonceMempoolConfig[int64]{ - TxPriority: mempool.NewDefaultTxPriority(), - MaxTx: 0, - SignerExtractor: mempool.NewDefaultSignerExtractionAdapter(), - }, - ) - mp2 := mempool.NewPriorityMempool( - mempool.PriorityNonceMempoolConfig[int64]{ - TxPriority: mempool.NewDefaultTxPriority(), - MaxTx: 0, - SignerExtractor: mempool.NewDefaultSignerExtractionAdapter(), - }, - ) - ph1 := baseapp.NewDefaultProposalHandler(mp1, app) - handler1 := ph1.PrepareProposalHandler() - ph2 := baseapp.NewDefaultProposalHandler(mp2, app) - handler2 := ph2.PrepareProposalHandler() + var ( secret1 = []byte("secret1") secret2 = []byte("secret2") @@ -451,127 +431,130 @@ func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_PriorityNonceMempoolTxSe 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}) - - 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) - - 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", + type testTx struct { + tx sdk.Tx + priority int64 + bz []byte + size int + } + + 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}, + } + + 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 + txInputs []testTx req *abci.RequestPrepareProposal handler sdk.PrepareProposalHandler - expectedTxs [][]byte + expectedTxs []int }{ "skip same-sender non-sequential sequence and then add others txs": { - ctx: s.ctx, + ctx: s.ctx, + txInputs: []testTx{testTxs[0], testTxs[1], testTxs[2], testTxs[3]}, req: &abci.RequestPrepareProposal{ - Txs: [][]byte{txBz1, txBz2, txBz3, txBz4}, MaxTxBytes: 111 + 112, }, - handler: handler1, - expectedTxs: [][]byte{txBz1, txBz4}, + expectedTxs: []int{0, 3}, }, "skip multi-signers msg non-sequential sequence": { - ctx: s.ctx, + ctx: s.ctx, + txInputs: []testTx{testTxs[4], testTxs[5], testTxs[6], testTxs[7], testTxs[8]}, + req: &abci.RequestPrepareProposal{ + 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{ - Txs: [][]byte{txBz5, txBz6, txBz7, txBz8, txBz9}, 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() { - resp, err := tc.handler(tc.ctx, tc.req) + 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) - s.Require().EqualValues(toHumanReadable(mapTxs, resp.Txs), toHumanReadable(mapTxs, tc.expectedTxs)) + 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) }) } } @@ -607,14 +590,6 @@ func buildMsg(t *testing.T, txConfig client.TxConfig, value []byte, secrets [][] 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(