certusone
/
cosmos-sdk
mirror of https://github.com/certusone/cosmos-sdk.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

fix(baseapp): select txs correctly with no-op mempool (backport #17769) (#17848) 

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Aleksandr Bezobchuk <aleks.bezobchuk@gmail.com>
This commit is contained in:
mergify[bot] 2023-09-25 21:41:43 +02:00 committed by GitHub
parent c65feecc83
commit a6f79a9117
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 346 additions and 165 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

@ -43,6 +43,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
### Bug Fixes ### 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`. * (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. * (mempool) [#17668](https://github.com/cosmos/cosmos-sdk/pull/17668) Fix `PriorityNonceIterator.Next()` nil pointer ref for min priority at the end of iteration.

256
baseapp/abci_utils.go Normal file
View File

@ -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 <true> if the caller should halt the transaction selection loop
// (typically over a mempool) or <false> 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))
}

165
baseapp/baseapp.go
View File

@ -906,171 +906,6 @@ func (app *BaseApp) ProcessProposalVerifyTx(txBz []byte) (sdk.Tx, error) {
return tx, nil 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. // Close is called in start cmd to gracefully cleanup resources.
func (app *BaseApp) Close() error { func (app *BaseApp) Close() error {
return nil return nil

89
baseapp/baseapp_test.go
View File

@ -25,6 +25,7 @@ import (
"github.com/cosmos/cosmos-sdk/testutil" "github.com/cosmos/cosmos-sdk/testutil"
sdk "github.com/cosmos/cosmos-sdk/types" sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/cosmos-sdk/types/mempool"
authtx "github.com/cosmos/cosmos-sdk/x/auth/tx" authtx "github.com/cosmos/cosmos-sdk/x/auth/tx"
) )
@ -654,3 +655,91 @@ func TestLoadVersionPruning(t *testing.T) {
require.Nil(t, err) require.Nil(t, err)
testLoadVersionHelper(t, app, int64(7), lastCommitID) 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)
}
})
}
}