fix(baseapp): select txs correctly with no-op mempool

CHANGELOG.md

@@ -62,6 +62,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Bug Fixes
 
+* (baseapp) [#17769](https://github.com/cosmos/cosmos-sdk/pull/17769) Ensure we respect block size constraints in the `DefaultProposalHandler`'s `PrepareProposal` handler when a nil or no-op mempool is used. We provide a `TxSelector` type to assist in making transaction selection generalized. We also fix a comparison bug in tx selection when `req.maxTxBytes` is reached.
 * (types) [#16583](https://github.com/cosmos/cosmos-sdk/pull/16583), [#17372](https://github.com/cosmos/cosmos-sdk/pull/17372), [#17421](https://github.com/cosmos/cosmos-sdk/pull/17421), [#17713](https://github.com/cosmos/cosmos-sdk/pull/17713) Introduce `PreBlock`, which executes in `FinalizeBlock` before `BeginBlock`. It allows the application to modify consensus parameters and have access to VE state. Note, `PreFinalizeBlockHook` is replaced by`PreBlocker`.
 * (baseapp) [#17518](https://github.com/cosmos/cosmos-sdk/pull/17518) Utilizing voting power from vote extensions (CometBFT) instead of the current bonded tokens (x/staking) to determine if a set of vote extensions are valid.
 * (config) [#17649](https://github.com/cosmos/cosmos-sdk/pull/17649) Fix `mempool.max-txs` configuration is invalid in `app.config`.

baseapp/abci_utils.go

@@ -146,16 +146,25 @@
 	DefaultProposalHandler struct {
 		mempool    mempool.Mempool
 		txVerifier ProposalTxVerifier
+		txSelector func(maxTxBytes, maxBlockGas uint64) TxSelector
 	}
 )
 
-func NewDefaultProposalHandler(mp mempool.Mempool, txVerifier ProposalTxVerifier) DefaultProposalHandler {
-	return DefaultProposalHandler{
+func NewDefaultProposalHandler(mp mempool.Mempool, txVerifier ProposalTxVerifier) *DefaultProposalHandler {
+	return &DefaultProposalHandler{
 		mempool:    mp,
 		txVerifier: txVerifier,
+		txSelector: func(maxTxBytes, maxBlockGas uint64) TxSelector {
+			return NewDefaultTxSelector(maxTxBytes, maxBlockGas)
+		},
 	}
 }
 
+// SetTxSelector sets the TxSelector function on the DefaultProposalHandler.
+func (h *DefaultProposalHandler) SetTxSelector(fn func(maxTxBytes, maxBlockGas uint64) TxSelector) {
+	h.txSelector = fn
+}
+
 // PrepareProposalHandler returns the default implementation for processing an
 // ABCI proposal. The application's mempool is enumerated and all valid
 // transactions are added to the proposal. Transactions are valid if they:
@@ -176,77 +185,59 @@
 // - If no mempool is set or if the mempool is a no-op mempool, the transactions
 // requested from CometBFT will simply be returned, which, by default, are in
 // FIFO order.
-func (h DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHandler {
+func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHandler {
 	return func(ctx sdk.Context, req *abci.RequestPrepareProposal) (*abci.ResponsePrepareProposal, error) {
+		var maxBlockGas uint64
+		if b := ctx.ConsensusParams().Block; b != nil {
+			maxBlockGas = uint64(b.MaxGas)
+		}
+
+		txSelector := h.txSelector(uint64(req.MaxTxBytes), maxBlockGas)
+		defer txSelector.Clear()
+
 		// If the mempool is nil or NoOp we simply return the transactions
 		// requested from CometBFT, which, by default, should be in FIFO order.
+		//
+		// Note, we still need to ensure the transactions returned respect req.MaxTxBytes.
 		_, isNoOp := h.mempool.(mempool.NoOpMempool)
 		if h.mempool == nil || isNoOp {
-			return &abci.ResponsePrepareProposal{Txs: req.Txs}, nil
-		}
+			for _, txBz := range req.Txs {
+				// XXX: We pass nil as the memTx because we have no way of decoding the
+				// txBz. We'd need to break (update) the ProposalTxVerifier interface.
+				stop := txSelector.SelectTxForProposal(nil, txBz)
+				if stop {
+					break
+				}
+			}
 
-		var maxBlockGas int64
-		if b := ctx.ConsensusParams().Block; b != nil {
-			maxBlockGas = b.MaxGas
+			return &abci.ResponsePrepareProposal{Txs: txSelector.SelectedTxs()}, nil
 		}
 
-		var (
-			selectedTxs  [][]byte
-			totalTxBytes int64
-			totalTxGas   uint64
-		)
-
 		iterator := h.mempool.Select(ctx, req.Txs)
-
 		for iterator != nil {
 			memTx := iterator.Tx()
 
 			// NOTE: Since transaction verification was already executed in CheckTx,
 			// which calls mempool.Insert, in theory everything in the pool should be
 			// valid. But some mempool implementations may insert invalid txs, so we
 			// check again.
-			bz, err := h.txVerifier.PrepareProposalVerifyTx(memTx)
+			txBz, err := h.txVerifier.PrepareProposalVerifyTx(memTx)
 			if err != nil {
 				err := h.mempool.Remove(memTx)
 				if err != nil && !errors.Is(err, mempool.ErrTxNotFound) {
 					return nil, err
 				}
 			} else {
-				var txGasLimit uint64
-				txSize := int64(len(bz))
-
-				gasTx, ok := memTx.(GasTx)
-				if ok {
-					txGasLimit = gasTx.GetGas()
-				}
-
-				// only add the transaction to the proposal if we have enough capacity
-				if (txSize + totalTxBytes) < req.MaxTxBytes {
-					// If there is a max block gas limit, add the tx only if the limit has
-					// not been met.
-					if maxBlockGas > 0 {
-						if (txGasLimit + totalTxGas) <= uint64(maxBlockGas) {
-							totalTxGas += txGasLimit
-							totalTxBytes += txSize
-							selectedTxs = append(selectedTxs, bz)
-						}
-					} else {
-						totalTxBytes += txSize
-						selectedTxs = append(selectedTxs, bz)
-					}
-				}
-
-				// Check if we've reached capacity. If so, we cannot select any more
-				// transactions.
-				if totalTxBytes >= req.MaxTxBytes || (maxBlockGas > 0 && (totalTxGas >= uint64(maxBlockGas))) {
+				stop := txSelector.SelectTxForProposal(memTx, txBz)
+				if stop {
 					break
 				}
 			}
 
 			iterator = iterator.Next()
 		}
 
-		return &abci.ResponsePrepareProposal{Txs: selectedTxs}, nil
+		return &abci.ResponsePrepareProposal{Txs: txSelector.SelectedTxs()}, nil
 	}
 }
 
@@ -261,7 +252,7 @@
 // DefaultPrepareProposal. It is very important that the same validation logic
 // is used in both steps, and applications must ensure that this is the case in
 // non-default handlers.
-func (h DefaultProposalHandler) ProcessProposalHandler() sdk.ProcessProposalHandler {
+func (h *DefaultProposalHandler) ProcessProposalHandler() sdk.ProcessProposalHandler {
 	// If the mempool is nil or NoOp we simply return ACCEPT,
 	// because PrepareProposal may have included txs that could fail verification.
 	_, isNoOp := h.mempool.(mempool.NoOpMempool)
@@ -330,3 +321,78 @@
 		return &abci.ResponseVerifyVoteExtension{Status: abci.ResponseVerifyVoteExtension_ACCEPT}, nil
 	}
 }
+
+// TxSelector defines a helper type that assists in selecting transactions during
+// mempool transaction selection in PrepareProposal. It keeps track of the total
+// number of bytes and total gas of the selected transactions. It also keeps
+// track of the selected transactions themselves.
+type TxSelector interface {
+	// SelectedTxs should return a copy of the selected transactions.
+	SelectedTxs() [][]byte
+
+	// Clear should clear the TxSelector, nulling out all relevant fields.
+	Clear()
+
+	// SelectTxForProposal should attempt to select a transaction for inclusion in
+	// a proposal based on inclusion criteria defined by the TxSelector. It must
+	// return <true> if the caller should halt the transaction selection loop
+	// (typically over a mempool) or <false> otherwise.
+	SelectTxForProposal(memTx sdk.Tx, txBz []byte) bool
+}
+
+type defaultTxSelector struct {
+	maxTxBytes   uint64
+	maxBlockGas  uint64
+	totalTxBytes uint64
+	totalTxGas   uint64
+	selectedTxs  [][]byte
+}
+
+func NewDefaultTxSelector(maxTxBytes, maxBlockGas uint64) TxSelector {
+	return &defaultTxSelector{
+		maxTxBytes:  maxTxBytes,
+		maxBlockGas: maxBlockGas,
+	}
+}
+
+func (ts *defaultTxSelector) SelectedTxs() [][]byte {
+	txs := make([][]byte, len(ts.selectedTxs))
+	copy(txs, ts.selectedTxs)
+	return txs
+}
+
+func (ts *defaultTxSelector) Clear() {
+	ts.totalTxBytes = 0
+	ts.totalTxGas = 0
+	ts.selectedTxs = nil
+}
+
+func (ts *defaultTxSelector) SelectTxForProposal(memTx sdk.Tx, txBz []byte) bool {
+	txSize := uint64(len(txBz))
+
+	var txGasLimit uint64
+	if memTx != nil {
+		if gasTx, ok := memTx.(GasTx); ok {
+			txGasLimit = gasTx.GetGas()
+		}
+	}
+
+	// only add the transaction to the proposal if we have enough capacity
+	if (txSize + ts.totalTxBytes) <= ts.maxTxBytes {
+		// If there is a max block gas limit, add the tx only if the limit has
+		// not been met.
+		if ts.maxBlockGas > 0 {
+			if (txGasLimit + ts.totalTxGas) <= ts.maxBlockGas {
+				ts.totalTxGas += txGasLimit
+				ts.totalTxBytes += txSize
+				ts.selectedTxs = append(ts.selectedTxs, txBz)
+			}
+		} else {
+			ts.totalTxBytes += txSize
+			ts.selectedTxs = append(ts.selectedTxs, txBz)
+		}
+	}
+
+	// check if we've reached capacity; if so, we cannot select any more transactions
+	return ts.totalTxBytes >= ts.maxTxBytes || (ts.maxBlockGas > 0 && (ts.totalTxGas >= ts.maxBlockGas))
+}

baseapp/abci_utils_test.go

@@ -16,6 +16,7 @@ import (
 	"github.com/cosmos/cosmos-sdk/baseapp"
 	"github.com/cosmos/cosmos-sdk/baseapp/testutil/mock"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/cosmos-sdk/types/mempool"
 )
 
 const (
@@ -274,6 +275,26 @@ func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsTwoVotesNilAbsent() {
 	s.Require().Error(baseapp.ValidateVoteExtensions(s.ctx, s.valStore, 3, chainID, llc))
 }
 
+func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_NoOpMempoolTxSelection() {
+	ph := baseapp.NewDefaultProposalHandler(mempool.NoOpMempool{}, nil)
+	handler := ph.PrepareProposalHandler()
+
+	// Request PrepareProposal with 5 txs, 5 bytes each, with a max size of 15.
+	// We should only select the first 3 txs.
+	resp, err := handler(s.ctx, &abci.RequestPrepareProposal{
+		Txs: [][]byte{
+			{0xFF, 0xFF, 0xFF, 0xFF, 0xFF},
+			{0xFF, 0xFF, 0xFF, 0xFF, 0xFF},
+			{0xFF, 0xFF, 0xFF, 0xFF, 0xFF},
+			{0xFF, 0xFF, 0xFF, 0xFF, 0xFF},
+			{0xFF, 0xFF, 0xFF, 0xFF, 0xFF},
+		},
+		MaxTxBytes: 15,
+	})
+	s.Require().NoError(err)
+	s.Require().Len(resp.Txs, 3)
+}
+
 func marshalDelimitedFn(msg proto.Message) ([]byte, error) {
 	var buf bytes.Buffer
 	if err := protoio.NewDelimitedWriter(&buf).WriteMsg(msg); err != nil {