diff --git a/CHANGELOG.md b/CHANGELOG.md index c008cf3d109d..29f3267e7cc8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (abci): [#19200](https://github.com/cosmos/cosmos-sdk/pull/19200) Ensure that sdk side ve math matches cometbft * [#19106](https://github.com/cosmos/cosmos-sdk/pull/19106) Allow empty public keys when setting signatures. Public keys aren't needed for every transaction. +* (baseapp) [#19198](https://github.com/cosmos/cosmos-sdk/pull/19198) Remove usage of pointers in logs in all OE goroutines. +* (baseapp) [#19177](https://github.com/cosmos/cosmos-sdk/pull/19177) Fix baseapp DefaultProposalHandler same-sender non-sequential sequence ## [v0.50.3](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.3) - 2023-01-15 @@ -63,7 +65,6 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes -* (baseapp) [#19198](https://github.com/cosmos/cosmos-sdk/pull/19198) Remove usage of pointers in logs in all OE goroutines. * (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 module's beginBlock and before DeliverTx, ensuring transaction processing always starts with the expected zeroed out block gas meter. * (baseapp) [#18895](https://github.com/cosmos/cosmos-sdk/pull/18895) Fix de-duplicating vote extensions during validation in ValidateVoteExtensions. diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go index b7e7eadb971f..0d8fabe77197 100644 --- a/baseapp/abci_utils.go +++ b/baseapp/abci_utils.go @@ -154,17 +154,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(), } } @@ -224,8 +226,40 @@ 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[signer.Signer.String()] + if !ok { + txSignersSeqs[signer.Signer.String()] = 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[signer.Signer.String()] = 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 @@ -242,6 +276,16 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan if stop { break } + + txsLen := len(h.txSelector.SelectedTxs(ctx)) + for sender, seq := range txSignersSeqs { + if txsLen != selectedTxsNums { + selectedTxsSignersSeqs[sender] = seq + } else if _, ok := selectedTxsSignersSeqs[sender]; !ok { + selectedTxsSignersSeqs[sender] = seq - 1 + } + } + selectedTxsNums = txsLen } iterator = iterator.Next() diff --git a/baseapp/abci_utils_test.go b/baseapp/abci_utils_test.go index 9624156c21fa..84589b554a74 100644 --- a/baseapp/abci_utils_test.go +++ b/baseapp/abci_utils_test.go @@ -2,16 +2,19 @@ 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" 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" @@ -19,10 +22,13 @@ 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/crypto/keys/secp256k1" "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" authtx "github.com/cosmos/cosmos-sdk/x/auth/tx" ) @@ -33,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{ @@ -403,6 +409,162 @@ 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) + 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") + 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}) + + 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", + } + testCases := map[string]struct { + ctx sdk.Context + req *abci.RequestPrepareProposal + handler sdk.PrepareProposalHandler + 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, + }, + handler: handler1, + expectedTxs: [][]byte{txBz1, txBz4}, + }, + "skip multi-signers msg non-sequential sequence": { + ctx: s.ctx, + req: &abci.RequestPrepareProposal{ + Txs: [][]byte{txBz5, txBz6, txBz7, txBz8, txBz9}, + MaxTxBytes: 195 + 196, + }, + handler: handler2, + expectedTxs: [][]byte{txBz5, txBz9}, + }, + } + + for name, tc := range testCases { + s.Run(name, func() { + resp, err := tc.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 { @@ -411,3 +573,41 @@ func marshalDelimitedFn(msg proto.Message) ([]byte, error) { return buf.Bytes(), nil } + +func buildMsg(t *testing.T, txConfig client.TxConfig, value []byte, secrets [][]byte, nonces []uint64) sdk.Tx { + t.Helper() + builder := txConfig.NewTxBuilder() + _ = builder.SetMsgs( + &baseapptestutil.MsgKeyValue{Value: value}, + ) + require.Equal(t, len(secrets), len(nonces)) + signatures := make([]signingtypes.SignatureV2, 0) + for index, secret := range secrets { + nonce := nonces[index] + privKey := secp256k1.GenPrivKeyFromSecret(secret) + pubKey := privKey.PubKey() + signatures = append(signatures, signingtypes.SignatureV2{ + PubKey: pubKey, + Sequence: nonce, + Data: &signingtypes.SingleSignatureData{}, + }) + } + setTxSignatureWithSecret(t, builder, signatures...) + 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( + signatures..., + ) + require.NoError(t, err) +}