skip same-sender non-sequential sequence and then add others txs

CHANGELOG.md

@@ -80,6 +80,7 @@ Every Module contains its own CHANGELOG.md. Please refer to the module you are i
 
 ### Bug Fixes
 
+* (baseapp) [#19177](https://github.com/cosmos/cosmos-sdk/pull/19177) 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.

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()

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)
+}