From 5ae4b66845716b18cdeb73645850082875ac0ccc Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Mon, 25 Sep 2023 21:41:43 +0200 Subject: [PATCH] fix(baseapp): select txs correctly with no-op mempool (backport #17769) (#17848) Co-authored-by: Aleksandr Bezobchuk Co-authored-by: Aleksandr Bezobchuk --- CHANGELOG.md | 1 + baseapp/abci_utils.go | 256 ++++++++++++++++++++++++++++++++++++++++ baseapp/baseapp.go | 165 -------------------------- baseapp/baseapp_test.go | 89 ++++++++++++++ 4 files changed, 346 insertions(+), 165 deletions(-) create mode 100644 baseapp/abci_utils.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f66baa53589..e46c3d941f8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,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. * (config) [#17649](https://github.com/cosmos/cosmos-sdk/pull/17649) Fix `mempool.max-txs` configuration is invalid in `app.config`. * (mempool) [#17668](https://github.com/cosmos/cosmos-sdk/pull/17668) Fix `PriorityNonceIterator.Next()` nil pointer ref for min priority at the end of iteration. diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go new file mode 100644 index 000000000000..4d00ce4eb473 --- /dev/null +++ b/baseapp/abci_utils.go @@ -0,0 +1,256 @@ +package baseapp + +import ( + "github.com/cockroachdb/errors" + abci "github.com/cometbft/cometbft/abci/types" + + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/types/mempool" +) + +type ( + // GasTx defines the contract that a transaction with a gas limit must implement. + GasTx interface { + GetGas() uint64 + } + + // ProposalTxVerifier defines the interface that is implemented by BaseApp, + // that any custom ABCI PrepareProposal and ProcessProposal handler can use + // to verify a transaction. + ProposalTxVerifier interface { + PrepareProposalVerifyTx(tx sdk.Tx) ([]byte, error) + ProcessProposalVerifyTx(txBz []byte) (sdk.Tx, error) + } + + // DefaultProposalHandler defines the default ABCI PrepareProposal and + // ProcessProposal handlers. + DefaultProposalHandler struct { + mempool mempool.Mempool + txVerifier ProposalTxVerifier + txSelector TxSelector + } +) + +func NewDefaultProposalHandler(mp mempool.Mempool, txVerifier ProposalTxVerifier) *DefaultProposalHandler { + return &DefaultProposalHandler{ + mempool: mp, + txVerifier: txVerifier, + txSelector: NewDefaultTxSelector(), + } +} + +// SetTxSelector sets the TxSelector function on the DefaultProposalHandler. +func (h *DefaultProposalHandler) SetTxSelector(ts TxSelector) { + h.txSelector = ts +} + +// 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: +// +// 1) Successfully encode to bytes. +// 2) Are valid (i.e. pass runTx, AnteHandler only). +// +// Enumeration is halted once RequestPrepareProposal.MaxBytes of transactions is +// reached or the mempool is exhausted. +// +// Note: +// +// - Step (2) is identical to the validation step performed in +// DefaultProcessProposal. 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. +// +// - 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 { + return func(ctx sdk.Context, req abci.RequestPrepareProposal) abci.ResponsePrepareProposal { + var maxBlockGas uint64 + if b := ctx.ConsensusParams().Block; b != nil { + maxBlockGas = uint64(b.MaxGas) + } + + defer h.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 { + 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. + // As a result, we CANNOT account for block max gas. + stop := h.txSelector.SelectTxForProposal(uint64(req.MaxTxBytes), maxBlockGas, nil, txBz) + if stop { + break + } + } + + return abci.ResponsePrepareProposal{Txs: h.txSelector.SelectedTxs()} + } + + 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. + txBz, err := h.txVerifier.PrepareProposalVerifyTx(memTx) + if err != nil { + err := h.mempool.Remove(memTx) + if err != nil && !errors.Is(err, mempool.ErrTxNotFound) { + panic(err) + } + } else { + stop := h.txSelector.SelectTxForProposal(uint64(req.MaxTxBytes), maxBlockGas, memTx, txBz) + if stop { + break + } + } + + iterator = iterator.Next() + } + + return abci.ResponsePrepareProposal{Txs: h.txSelector.SelectedTxs()} + } +} + +// ProcessProposalHandler returns the default implementation for processing an +// ABCI proposal. Every transaction in the proposal must pass 2 conditions: +// +// 1. The transaction bytes must decode to a valid transaction. +// 2. The transaction must be valid (i.e. pass runTx, AnteHandler only) +// +// If any transaction fails to pass either condition, the proposal is rejected. +// Note that step (2) is identical to the validation step performed in +// 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 { + // 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) + if h.mempool == nil || isNoOp { + return NoOpProcessProposal() + } + + return func(ctx sdk.Context, req abci.RequestProcessProposal) abci.ResponseProcessProposal { + var totalTxGas uint64 + + var maxBlockGas int64 + if b := ctx.ConsensusParams().Block; b != nil { + maxBlockGas = b.MaxGas + } + + for _, txBytes := range req.Txs { + tx, err := h.txVerifier.ProcessProposalVerifyTx(txBytes) + if err != nil { + return abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_REJECT} + } + + if maxBlockGas > 0 { + gasTx, ok := tx.(GasTx) + if ok { + totalTxGas += gasTx.GetGas() + } + + if totalTxGas > uint64(maxBlockGas) { + return abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_REJECT} + } + } + } + + return abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_ACCEPT} + } +} + +// NoOpPrepareProposal defines a no-op PrepareProposal handler. It will always +// return the transactions sent by the client's request. +func NoOpPrepareProposal() sdk.PrepareProposalHandler { + return func(_ sdk.Context, req abci.RequestPrepareProposal) abci.ResponsePrepareProposal { + return abci.ResponsePrepareProposal{Txs: req.Txs} + } +} + +// NoOpProcessProposal defines a no-op ProcessProposal Handler. It will always +// return ACCEPT. +func NoOpProcessProposal() sdk.ProcessProposalHandler { + return func(_ sdk.Context, _ abci.RequestProcessProposal) abci.ResponseProcessProposal { + return abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_ACCEPT} + } +} + +// 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 if the caller should halt the transaction selection loop + // (typically over a mempool) or otherwise. + SelectTxForProposal(maxTxBytes, maxBlockGas uint64, memTx sdk.Tx, txBz []byte) bool +} + +type defaultTxSelector struct { + totalTxBytes uint64 + totalTxGas uint64 + selectedTxs [][]byte +} + +func NewDefaultTxSelector() TxSelector { + return &defaultTxSelector{} +} + +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(maxTxBytes, maxBlockGas uint64, 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) <= 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 + ts.totalTxGas) <= 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 >= maxTxBytes || (maxBlockGas > 0 && (ts.totalTxGas >= maxBlockGas)) +} diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 48205ccfe163..b77d3384a138 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -907,171 +907,6 @@ func (app *BaseApp) ProcessProposalVerifyTx(txBz []byte) (sdk.Tx, error) { return tx, nil } -type ( - // ProposalTxVerifier defines the interface that is implemented by BaseApp, - // that any custom ABCI PrepareProposal and ProcessProposal handler can use - // to verify a transaction. - ProposalTxVerifier interface { - PrepareProposalVerifyTx(tx sdk.Tx) ([]byte, error) - ProcessProposalVerifyTx(txBz []byte) (sdk.Tx, error) - } - - // DefaultProposalHandler defines the default ABCI PrepareProposal and - // ProcessProposal handlers. - DefaultProposalHandler struct { - mempool mempool.Mempool - txVerifier ProposalTxVerifier - } -) - -func NewDefaultProposalHandler(mp mempool.Mempool, txVerifier ProposalTxVerifier) DefaultProposalHandler { - return DefaultProposalHandler{ - mempool: mp, - txVerifier: txVerifier, - } -} - -// 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: -// -// 1) Successfully encode to bytes. -// 2) Are valid (i.e. pass runTx, AnteHandler only). -// -// Enumeration is halted once RequestPrepareProposal.MaxBytes of transactions is -// reached or the mempool is exhausted. -// -// Note: -// -// - Step (2) is identical to the validation step performed in -// DefaultProcessProposal. 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. -// -// - If no mempool is set or if the mempool is a no-op mempool, the transactions -// requested from Tendermint will simply be returned, which, by default, are in -// FIFO order. -func (h DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHandler { - return func(ctx sdk.Context, req abci.RequestPrepareProposal) abci.ResponsePrepareProposal { - // If the mempool is nil or NoOp we simply return the transactions - // requested from CometBFT, which, by default, should be in FIFO order. - _, isNoOp := h.mempool.(mempool.NoOpMempool) - if h.mempool == nil || isNoOp { - return abci.ResponsePrepareProposal{Txs: req.Txs} - } - - var maxBlockGas int64 - if b := ctx.ConsensusParams().Block; b != nil { - maxBlockGas = b.MaxGas - } - - 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) - if err != nil { - err := h.mempool.Remove(memTx) - if err != nil && !errors.Is(err, mempool.ErrTxNotFound) { - panic(err) - } - } else { - var txGasLimit uint64 - txSize := int64(len(bz)) - - gasTx, ok := memTx.(interface{ GetGas() uint64 }) - 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))) { - break - } - } - - iterator = iterator.Next() - } - - return abci.ResponsePrepareProposal{Txs: selectedTxs} - } -} - -// ProcessProposalHandler returns the default implementation for processing an -// ABCI proposal. Every transaction in the proposal must pass 2 conditions: -// -// 1. The transaction bytes must decode to a valid transaction. -// 2. The transaction must be valid (i.e. pass runTx, AnteHandler only) -// -// If any transaction fails to pass either condition, the proposal is rejected. -// Note that step (2) is identical to the validation step performed in -// 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 { - // 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) - if h.mempool == nil || isNoOp { - return NoOpProcessProposal() - } - - return func(ctx sdk.Context, req abci.RequestProcessProposal) abci.ResponseProcessProposal { - for _, txBytes := range req.Txs { - _, err := h.txVerifier.ProcessProposalVerifyTx(txBytes) - if err != nil { - return abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_REJECT} - } - } - - return abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_ACCEPT} - } -} - -// NoOpPrepareProposal defines a no-op PrepareProposal handler. It will always -// return the transactions sent by the client's request. -func NoOpPrepareProposal() sdk.PrepareProposalHandler { - return func(_ sdk.Context, req abci.RequestPrepareProposal) abci.ResponsePrepareProposal { - return abci.ResponsePrepareProposal{Txs: req.Txs} - } -} - -// NoOpProcessProposal defines a no-op ProcessProposal Handler. It will always -// return ACCEPT. -func NoOpProcessProposal() sdk.ProcessProposalHandler { - return func(_ sdk.Context, _ abci.RequestProcessProposal) abci.ResponseProcessProposal { - return abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_ACCEPT} - } -} - // Close is called in start cmd to gracefully cleanup resources. func (app *BaseApp) Close() error { return nil diff --git a/baseapp/baseapp_test.go b/baseapp/baseapp_test.go index 4d64568cf7a8..b0f8b97002bb 100644 --- a/baseapp/baseapp_test.go +++ b/baseapp/baseapp_test.go @@ -25,6 +25,7 @@ import ( "github.com/cosmos/cosmos-sdk/testutil" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + "github.com/cosmos/cosmos-sdk/types/mempool" authtx "github.com/cosmos/cosmos-sdk/x/auth/tx" ) @@ -657,3 +658,91 @@ func TestLoadVersionPruning(t *testing.T) { require.Nil(t, err) testLoadVersionHelper(t, app, int64(7), lastCommitID) } + +func TestDefaultProposalHandler_NoOpMempoolTxSelection(t *testing.T) { + // create a codec for marshaling + cdc := codec.NewProtoCodec(codectypes.NewInterfaceRegistry()) + baseapptestutil.RegisterInterfaces(cdc.InterfaceRegistry()) + + // create a baseapp along with a tx config for tx generation + txConfig := authtx.NewTxConfig(cdc, authtx.DefaultSignModes) + app := baseapp.NewBaseApp(t.Name(), log.NewNopLogger(), dbm.NewMemDB(), txConfig.TxDecoder()) + + // create a proposal handler + ph := baseapp.NewDefaultProposalHandler(mempool.NoOpMempool{}, app) + handler := ph.PrepareProposalHandler() + + // build a tx + builder := txConfig.NewTxBuilder() + require.NoError(t, builder.SetMsgs( + &baseapptestutil.MsgCounter{Counter: 0, FailOnHandler: false}, + )) + builder.SetGasLimit(100) + setTxSignature(t, builder, 0) + + // encode the tx to be used in the proposal request + tx := builder.GetTx() + txBz, err := txConfig.TxEncoder()(tx) + require.NoError(t, err) + require.Len(t, txBz, 103) + + ctx := sdk.NewContext(nil, tmproto.Header{}, false, nil). + WithConsensusParams(&tmproto.ConsensusParams{}) + + testCases := map[string]struct { + ctx sdk.Context + req abci.RequestPrepareProposal + expectedTxs int + }{ + "small max tx bytes": { + ctx: ctx, + req: abci.RequestPrepareProposal{ + Txs: [][]byte{txBz, txBz, txBz, txBz, txBz}, + MaxTxBytes: 10, + }, + expectedTxs: 0, + }, + "small max gas": { + ctx: ctx.WithConsensusParams(&tmproto.ConsensusParams{ + Block: &tmproto.BlockParams{ + MaxGas: 10, + }, + }), + req: abci.RequestPrepareProposal{ + Txs: [][]byte{txBz, txBz, txBz, txBz, txBz}, + MaxTxBytes: 309, + }, + expectedTxs: 3, + }, + "large max tx bytes": { + ctx: ctx, + req: abci.RequestPrepareProposal{ + Txs: [][]byte{txBz, txBz, txBz, txBz, txBz}, + MaxTxBytes: 309, + }, + expectedTxs: 3, + }, + "max gas and tx bytes": { + ctx: ctx.WithConsensusParams(&tmproto.ConsensusParams{ + Block: &tmproto.BlockParams{ + MaxGas: 200, + }, + }), + req: abci.RequestPrepareProposal{ + Txs: [][]byte{txBz, txBz, txBz, txBz, txBz}, + MaxTxBytes: 309, + }, + expectedTxs: 3, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + // iterate multiple times to ensure the tx selector is cleared each time + for i := 0; i < 5; i++ { + resp := handler(tc.ctx, tc.req) + require.Len(t, resp.Txs, tc.expectedTxs) + } + }) + } +}