diff --git a/CHANGELOG.md b/CHANGELOG.md index 661d890e2091..d55c2e85c12a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,6 +80,7 @@ Every Module contains its own CHANGELOG.md. Please refer to the module you are i ### Bug Fixes +* (baseapp) [#19119](https://github.com/cosmos/cosmos-sdk/pull/19119) Fix baseapp DefaultProposalHandler same-sender non-sequential sequence * (baseapp) [#19058](https://github.com/cosmos/cosmos-sdk/pull/19058) Fix baseapp posthandler branch would fail if the `runMsgs` had returned an error. * (baseapp) [#18609](https://github.com/cosmos/cosmos-sdk/issues/18609) Fixed accounting in the block gas meter after BeginBlock and before DeliverTx, ensuring transaction processing always starts with the expected zeroed out block gas meter. * (baseapp) [#18727](https://github.com/cosmos/cosmos-sdk/pull/18727) Ensure that `BaseApp.Init` firstly returns any errors from a nil commit multistore instead of panicking on nil dereferencing and before sealing the app. diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go index b09a22b33ecf..09a4e11de72b 100644 --- a/baseapp/abci_utils.go +++ b/baseapp/abci_utils.go @@ -157,17 +157,19 @@ type ( // DefaultProposalHandler defines the default ABCI PrepareProposal and // ProcessProposal handlers. DefaultProposalHandler struct { - mempool mempool.Mempool - txVerifier ProposalTxVerifier - txSelector TxSelector + mempool mempool.Mempool + txVerifier ProposalTxVerifier + txSelector TxSelector + signerExtAdapter mempool.SignerExtractionAdapter } ) func NewDefaultProposalHandler(mp mempool.Mempool, txVerifier ProposalTxVerifier) *DefaultProposalHandler { return &DefaultProposalHandler{ - mempool: mp, - txVerifier: txVerifier, - txSelector: NewDefaultTxSelector(), + mempool: mp, + txVerifier: txVerifier, + txSelector: NewDefaultTxSelector(), + signerExtAdapter: mempool.NewDefaultSignerExtractionAdapter(), } } @@ -227,8 +229,38 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan } iterator := h.mempool.Select(ctx, req.Txs) + selectedTxsSignersSeqs := make(map[string]uint64) + var selectedTxsNums int for iterator != nil { memTx := iterator.Tx() + signerData, err := h.signerExtAdapter.GetSigners(memTx) + if err != nil { + 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. + shouldAdd := true + txSignersSeqs := make(map[string]uint64) + for _, signer := range signerData { + seq, ok := selectedTxsSignersSeqs[string(signer.Signer)] + if !ok { + txSignersSeqs[string(signer.Signer)] = signer.Sequence + 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 seq+1 != signer.Sequence { + shouldAdd = false + break + } + txSignersSeqs[string(signer.Signer)] = signer.Sequence + } + if !shouldAdd { + iterator = iterator.Next() + continue + } // NOTE: Since transaction verification was already executed in CheckTx, // which calls mempool.Insert, in theory everything in the pool should be @@ -245,6 +277,13 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan if stop { break } + txsLen := len(h.txSelector.SelectedTxs(ctx)) + if txsLen != selectedTxsNums { + for sender, seq := range txSignersSeqs { + selectedTxsSignersSeqs[sender] = seq + } + } + selectedTxsNums = txsLen } iterator = iterator.Next() diff --git a/baseapp/abci_utils_test.go b/baseapp/abci_utils_test.go index ac72b90e8e26..e476de8b0a19 100644 --- a/baseapp/abci_utils_test.go +++ b/baseapp/abci_utils_test.go @@ -2,17 +2,20 @@ package baseapp_test import ( "bytes" + "strings" "testing" abci "github.com/cometbft/cometbft/abci/types" - "github.com/cometbft/cometbft/crypto/secp256k1" + cmtsecp256k1 "github.com/cometbft/cometbft/crypto/secp256k1" cmtprotocrypto "github.com/cometbft/cometbft/proto/tendermint/crypto" cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" cmttypes "github.com/cometbft/cometbft/types" dbm "github.com/cosmos/cosmos-db" + "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" protoio "github.com/cosmos/gogoproto/io" "github.com/cosmos/gogoproto/proto" "github.com/golang/mock/gomock" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "cosmossdk.io/log" @@ -21,10 +24,12 @@ import ( "github.com/cosmos/cosmos-sdk/baseapp" baseapptestutil "github.com/cosmos/cosmos-sdk/baseapp/testutil" "github.com/cosmos/cosmos-sdk/baseapp/testutil/mock" + "github.com/cosmos/cosmos-sdk/client" codectestutil "github.com/cosmos/cosmos-sdk/codec/testutil" "github.com/cosmos/cosmos-sdk/testutil/testdata" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/mempool" + signingtypes "github.com/cosmos/cosmos-sdk/types/tx/signing" ) const ( @@ -34,11 +39,11 @@ const ( type testValidator struct { consAddr sdk.ConsAddress tmPk cmtprotocrypto.PublicKey - privKey secp256k1.PrivKey + privKey cmtsecp256k1.PrivKey } func newTestValidator() testValidator { - privkey := secp256k1.GenPrivKey() + privkey := cmtsecp256k1.GenPrivKey() pubkey := privkey.PubKey() tmPk := cmtprotocrypto.PublicKey{ Sum: &cmtprotocrypto.PublicKey_Secp256K1{ @@ -415,6 +420,93 @@ func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_NoOpMempoolTxSelection() } } +func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_PriorityNonceMempoolTxSelection() { + cdc := codectestutil.CodecOptions{}.NewCodec() + baseapptestutil.RegisterInterfaces(cdc.InterfaceRegistry()) + txConfig := authtx.NewTxConfig(cdc, authtx.DefaultSignModes) + 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) + handler := ph.PrepareProposalHandler() + var ( + secret1 = []byte("secret1") + secret2 = []byte("secret2") + tx1 = buildMsg(s.T(), txConfig, []byte(`1`), secret1, 1) + ctx1 = s.ctx.WithPriority(10) + tx2 = buildMsg(s.T(), txConfig, []byte(`12345678910`), secret1, 2) + tx3 = buildMsg(s.T(), txConfig, []byte(`12`), secret1, 3) + + ctx2 = s.ctx.WithPriority(8) + tx4 = buildMsg(s.T(), txConfig, []byte(`12`), secret2, 1) + ) + err := mp.Insert(ctx1, tx1) + s.Require().NoError(err) + err = mp.Insert(ctx1, tx2) + s.Require().NoError(err) + err = mp.Insert(ctx1, tx3) + s.Require().NoError(err) + err = mp.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) + mapTxs := map[string]string{ + string(txBz1): "1", + string(txBz2): "2", + string(txBz3): "3", + string(txBz4): "4", + } + testCases := map[string]struct { + ctx sdk.Context + req *abci.RequestPrepareProposal + expectedTxs [][]byte + }{ + "skip same-sender non-sequential sequence and then add others txs": { + ctx: s.ctx, + req: &abci.RequestPrepareProposal{ + Txs: [][]byte{txBz1, txBz2, txBz3, txBz4}, + MaxTxBytes: 111 + 112, + }, + expectedTxs: [][]byte{txBz1, txBz4}, + }, + } + + for name, tc := range testCases { + s.Run(name, func() { + resp, err := handler(tc.ctx, tc.req) + s.Require().NoError(err) + s.Require().EqualValues(toHumanReadable(mapTxs, resp.Txs), toHumanReadable(mapTxs, tc.expectedTxs)) + }) + } +} + func marshalDelimitedFn(msg proto.Message) ([]byte, error) { var buf bytes.Buffer if err := protoio.NewDelimitedWriter(&buf).WriteMsg(msg); err != nil { @@ -423,3 +515,35 @@ func marshalDelimitedFn(msg proto.Message) ([]byte, error) { return buf.Bytes(), nil } + +func buildMsg(t *testing.T, txConfig client.TxConfig, value, secret []byte, nonce uint64) sdk.Tx { + t.Helper() + builder := txConfig.NewTxBuilder() + _ = builder.SetMsgs( + &baseapptestutil.MsgKeyValue{Value: value}, + ) + setTxSignatureWithSecret(t, builder, nonce, secret) + 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, nonce uint64, secret []byte) { + t.Helper() + privKey := secp256k1.GenPrivKeyFromSecret(secret) + pubKey := privKey.PubKey() + err := builder.SetSignatures( + signingtypes.SignatureV2{ + PubKey: pubKey, + Sequence: nonce, + Data: &signingtypes.SingleSignatureData{}, + }, + ) + require.NoError(t, err) +}