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

CHANGELOG.md

@@ -60,6 +60,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

@@ -178,75 +178,57 @@ func NewDefaultProposalHandler(mp mempool.Mempool, txVerifier ProposalTxVerifier
 // FIFO order.
 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 := NewTxSelector(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
 	}
 }
 
@@ -330,3 +312,74 @@ func NoOpVerifyVoteExtensionHandler() sdk.VerifyVoteExtensionHandler {
 		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 struct {
+	maxTxBytes   uint64
+	maxBlockGas  uint64
+	totalTxBytes uint64
+	totalTxGas   uint64
+	selectedTxs  [][]byte
+}
+
+func NewTxSelector(maxTxBytes, maxBlockGas uint64) *TxSelector {
+	return &TxSelector{
+		maxTxBytes:  maxTxBytes,
+		maxBlockGas: maxBlockGas,
+	}
+}
+
+// SelectedTxs returns a copy of the selected transactions.
+func (ts *TxSelector) SelectedTxs() [][]byte {
+	txs := make([][]byte, len(ts.selectedTxs))
+	copy(txs, ts.selectedTxs)
+	return txs
+}
+
+// Clear clears the TxSelector, nulling out all fields.
+func (ts *TxSelector) Clear() {
+	ts.totalTxBytes = 0
+	ts.totalTxGas = 0
+	ts.selectedTxs = nil
+}
+
+// SelectTxForProposal selects a transaction for inclusion in a proposal. It will
+// only select the provided transaction if there is enough capacity in the block.
+// It will return <true> if the caller should halt the transaction selection loop
+// (typically over a mempool) or <false> otherwise.
+func (ts *TxSelector) 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.
+	if ts.totalTxBytes >= ts.maxTxBytes || (ts.maxBlockGas > 0 && (ts.totalTxGas >= ts.maxBlockGas)) {
+		return true
+	}
+
+	return false
+}

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 {