From f8711ce5a06d8d6ebb751885cbf8e517ed1aa3a9 Mon Sep 17 00:00:00 2001 From: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Date: Tue, 30 Jan 2024 10:16:46 +0100 Subject: [PATCH] test(baseapp): Refactor tx selector tests + better comments (#19284) Co-authored-by: Aleksandr Bezobchuk (cherry picked from commit a86a83f761383c1ea434925cddd199cd5a271303) # Conflicts: # baseapp/abci_utils.go # baseapp/abci_utils_test.go --- baseapp/abci_utils.go | 18 ++- baseapp/abci_utils_test.go | 218 ++++++++++++++++++++----------------- 2 files changed, 135 insertions(+), 101 deletions(-) diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go index 8d218949cc9d..f383e6ecc8ed 100644 --- a/baseapp/abci_utils.go +++ b/baseapp/abci_utils.go @@ -105,8 +105,8 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan 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 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan 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 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan 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 } } diff --git a/baseapp/abci_utils_test.go b/baseapp/abci_utils_test.go index 273935817a15..cb62c908cf3f 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" @@ -94,6 +93,7 @@ func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_PriorityNonceMempoolTxSe 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,6 +102,9 @@ func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_PriorityNonceMempoolTxSe 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") @@ -109,126 +112,151 @@ 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}) + 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 @@ 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(