From 23dab6a38b7f56d11a10fb1fdf9095bd5d2416b9 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 15 Sep 2023 10:40:30 -0700 Subject: [PATCH 01/11] updates --- baseapp/abci_utils.go | 129 ++++++++++++++++++++++++++++-------------- 1 file changed, 88 insertions(+), 41 deletions(-) diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go index 4b2568d57dd7..a86468fbb8b2 100644 --- a/baseapp/abci_utils.go +++ b/baseapp/abci_utils.go @@ -178,26 +178,33 @@ 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() @@ -205,40 +212,15 @@ func (h DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHand // 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 } } @@ -246,7 +228,7 @@ func (h DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHand iterator = iterator.Next() } - return &abci.ResponsePrepareProposal{Txs: selectedTxs}, nil + return &abci.ResponsePrepareProposal{Txs: txSelector.SelectedTxs()}, nil } } @@ -330,3 +312,68 @@ func NoOpVerifyVoteExtensionHandler() sdk.VerifyVoteExtensionHandler { return &abci.ResponseVerifyVoteExtension{Status: abci.ResponseVerifyVoteExtension_ACCEPT}, nil } } + +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, + } +} + +func (ts *TxSelector) SelectedTxs() [][]byte { + txs := make([][]byte, len(ts.selectedTxs)) + copy(txs, ts.selectedTxs) + return txs +} + +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 if the caller should halt the transaction selection loop +// (typically over a mempool) or 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 +} From 70588e9b0abb8e759472c35b3a027c71a126c2d4 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 15 Sep 2023 10:46:39 -0700 Subject: [PATCH 02/11] updates --- baseapp/abci_utils.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go index a86468fbb8b2..fa1ce10aa76b 100644 --- a/baseapp/abci_utils.go +++ b/baseapp/abci_utils.go @@ -313,6 +313,10 @@ func NoOpVerifyVoteExtensionHandler() sdk.VerifyVoteExtensionHandler { } } +// 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 From 3503c1036ea788549f98c01b3245626ceddf092b Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 15 Sep 2023 10:59:55 -0700 Subject: [PATCH 03/11] updates --- baseapp/abci_utils.go | 2 +- baseapp/abci_utils_test.go | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go index fa1ce10aa76b..de1e274435e7 100644 --- a/baseapp/abci_utils.go +++ b/baseapp/abci_utils.go @@ -359,7 +359,7 @@ func (ts *TxSelector) SelectTxForProposal(memTx sdk.Tx, txBz []byte) bool { } // only add the transaction to the proposal if we have enough capacity - if (txSize + ts.totalTxBytes) < ts.maxTxBytes { + 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 { diff --git a/baseapp/abci_utils_test.go b/baseapp/abci_utils_test.go index 8c7fda91de95..d9a6183c1c70 100644 --- a/baseapp/abci_utils_test.go +++ b/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,25 @@ func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsTwoVotesNilAbsent() { s.Require().Error(baseapp.ValidateVoteExtensions(s.ctx, s.valStore, 3, chainID, llc)) } +func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_NoMempoolSelection() { + ph := baseapp.NewDefaultProposalHandler(mempool.NoOpMempool{}, nil) + handler := ph.PrepareProposalHandler() + + // request PrepareProposal with 5 txs, 5 bytes each, with a max size of 15 + 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 { From ab7351da56cdabd28cee4209e11dff3b7c872bd6 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 15 Sep 2023 11:00:34 -0700 Subject: [PATCH 04/11] updates --- baseapp/abci_utils_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/baseapp/abci_utils_test.go b/baseapp/abci_utils_test.go index d9a6183c1c70..8094600b7eaa 100644 --- a/baseapp/abci_utils_test.go +++ b/baseapp/abci_utils_test.go @@ -275,7 +275,7 @@ func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsTwoVotesNilAbsent() { s.Require().Error(baseapp.ValidateVoteExtensions(s.ctx, s.valStore, 3, chainID, llc)) } -func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_NoMempoolSelection() { +func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_NoOpMempoolTxSelection() { ph := baseapp.NewDefaultProposalHandler(mempool.NoOpMempool{}, nil) handler := ph.PrepareProposalHandler() From b111c403ddf8d8f8533324fe52d37cdadf467f6f Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 15 Sep 2023 11:01:16 -0700 Subject: [PATCH 05/11] updates --- baseapp/abci_utils_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/baseapp/abci_utils_test.go b/baseapp/abci_utils_test.go index 8094600b7eaa..eb6bd2fd3787 100644 --- a/baseapp/abci_utils_test.go +++ b/baseapp/abci_utils_test.go @@ -279,7 +279,8 @@ 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 + // 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}, From 179511805a17ae12936256f9b01d4a808de9ba2b Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 15 Sep 2023 11:04:57 -0700 Subject: [PATCH 06/11] cl++ --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 761f8b1ae538..2108bed98334 100644 --- a/CHANGELOG.md +++ b/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`. From 88a059f163b6fb4ea18ef635c8e0dfe19eb50b72 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 15 Sep 2023 11:05:56 -0700 Subject: [PATCH 07/11] cl++ --- baseapp/abci_utils.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go index de1e274435e7..1655e7293a2d 100644 --- a/baseapp/abci_utils.go +++ b/baseapp/abci_utils.go @@ -332,12 +332,14 @@ func NewTxSelector(maxTxBytes, maxBlockGas uint64) *TxSelector { } } +// 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 From 28fcae08d327b5c1ef9b806f63cffaa8f256d53c Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Mon, 18 Sep 2023 10:15:00 -0700 Subject: [PATCH 08/11] updates --- baseapp/abci_utils.go | 59 ++++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 23 deletions(-) diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go index 1655e7293a2d..1e14fbd958d4 100644 --- a/baseapp/abci_utils.go +++ b/baseapp/abci_utils.go @@ -146,16 +146,25 @@ type ( 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,14 +185,14 @@ func NewDefaultProposalHandler(mp mempool.Mempool, txVerifier ProposalTxVerifier // - 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 := NewTxSelector(uint64(req.MaxTxBytes), maxBlockGas) + txSelector := h.txSelector(uint64(req.MaxTxBytes), maxBlockGas) defer txSelector.Clear() // If the mempool is nil or NoOp we simply return the transactions @@ -243,7 +252,7 @@ func (h DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHand // 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) @@ -317,7 +326,21 @@ func NoOpVerifyVoteExtensionHandler() sdk.VerifyVoteExtensionHandler { // 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 { +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(memTx sdk.Tx, txBz []byte) bool +} + +type defaultTxSelector struct { maxTxBytes uint64 maxBlockGas uint64 totalTxBytes uint64 @@ -325,32 +348,26 @@ type TxSelector struct { selectedTxs [][]byte } -func NewTxSelector(maxTxBytes, maxBlockGas uint64) *TxSelector { - return &TxSelector{ +func NewDefaultTxSelector(maxTxBytes, maxBlockGas uint64) TxSelector { + return &defaultTxSelector{ maxTxBytes: maxTxBytes, maxBlockGas: maxBlockGas, } } -// SelectedTxs returns a copy of the selected transactions. -func (ts *TxSelector) SelectedTxs() [][]byte { +func (ts *defaultTxSelector) 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() { +func (ts *defaultTxSelector) 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 if the caller should halt the transaction selection loop -// (typically over a mempool) or otherwise. -func (ts *TxSelector) SelectTxForProposal(memTx sdk.Tx, txBz []byte) bool { +func (ts *defaultTxSelector) SelectTxForProposal(memTx sdk.Tx, txBz []byte) bool { txSize := uint64(len(txBz)) var txGasLimit uint64 @@ -376,10 +393,6 @@ func (ts *TxSelector) SelectTxForProposal(memTx sdk.Tx, txBz []byte) bool { } } - // 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 + // 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)) } From 11ae5ab313605c165a6ad87fa6230ef545a42a69 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Mon, 18 Sep 2023 12:31:35 -0700 Subject: [PATCH 09/11] updates --- baseapp/abci_utils.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go index 1e14fbd958d4..89f65d7e28f6 100644 --- a/baseapp/abci_utils.go +++ b/baseapp/abci_utils.go @@ -154,9 +154,7 @@ func NewDefaultProposalHandler(mp mempool.Mempool, txVerifier ProposalTxVerifier return &DefaultProposalHandler{ mempool: mp, txVerifier: txVerifier, - txSelector: func(maxTxBytes, maxBlockGas uint64) TxSelector { - return NewDefaultTxSelector(maxTxBytes, maxBlockGas) - }, + txSelector: NewDefaultTxSelector, } } From 0d47da65142388294f9c2b52ade6fb15629761b4 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 22 Sep 2023 09:52:26 -0400 Subject: [PATCH 10/11] updates --- baseapp/abci_utils.go | 49 +++++++++--------- baseapp/abci_utils_test.go | 101 ++++++++++++++++++++++++++++++++----- baseapp/baseapp.go | 8 +++ 3 files changed, 119 insertions(+), 39 deletions(-) diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go index 89f65d7e28f6..9a8da22c191c 100644 --- a/baseapp/abci_utils.go +++ b/baseapp/abci_utils.go @@ -139,6 +139,8 @@ type ( ProposalTxVerifier interface { PrepareProposalVerifyTx(tx sdk.Tx) ([]byte, error) ProcessProposalVerifyTx(txBz []byte) (sdk.Tx, error) + TxDecode(txBz []byte) (sdk.Tx, error) + TxEncode(tx sdk.Tx) ([]byte, error) } // DefaultProposalHandler defines the default ABCI PrepareProposal and @@ -146,7 +148,7 @@ type ( DefaultProposalHandler struct { mempool mempool.Mempool txVerifier ProposalTxVerifier - txSelector func(maxTxBytes, maxBlockGas uint64) TxSelector + txSelector TxSelector } ) @@ -154,13 +156,13 @@ func NewDefaultProposalHandler(mp mempool.Mempool, txVerifier ProposalTxVerifier return &DefaultProposalHandler{ mempool: mp, txVerifier: txVerifier, - txSelector: NewDefaultTxSelector, + txSelector: NewDefaultTxSelector(), } } // SetTxSelector sets the TxSelector function on the DefaultProposalHandler. -func (h *DefaultProposalHandler) SetTxSelector(fn func(maxTxBytes, maxBlockGas uint64) TxSelector) { - h.txSelector = fn +func (h *DefaultProposalHandler) SetTxSelector(ts TxSelector) { + h.txSelector = ts } // PrepareProposalHandler returns the default implementation for processing an @@ -190,8 +192,7 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan maxBlockGas = uint64(b.MaxGas) } - txSelector := h.txSelector(uint64(req.MaxTxBytes), maxBlockGas) - defer txSelector.Clear() + 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. @@ -200,15 +201,18 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan _, 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. - stop := txSelector.SelectTxForProposal(nil, txBz) + tx, err := h.txVerifier.TxDecode(txBz) + if err != nil { + return nil, err + } + + stop := h.txSelector.SelectTxForProposal(uint64(req.MaxTxBytes), maxBlockGas, tx, txBz) if stop { break } } - return &abci.ResponsePrepareProposal{Txs: txSelector.SelectedTxs()}, nil + return &abci.ResponsePrepareProposal{Txs: h.txSelector.SelectedTxs()}, nil } iterator := h.mempool.Select(ctx, req.Txs) @@ -226,7 +230,7 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan return nil, err } } else { - stop := txSelector.SelectTxForProposal(memTx, txBz) + stop := h.txSelector.SelectTxForProposal(uint64(req.MaxTxBytes), maxBlockGas, memTx, txBz) if stop { break } @@ -235,7 +239,7 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan iterator = iterator.Next() } - return &abci.ResponsePrepareProposal{Txs: txSelector.SelectedTxs()}, nil + return &abci.ResponsePrepareProposal{Txs: h.txSelector.SelectedTxs()}, nil } } @@ -335,22 +339,17 @@ type TxSelector interface { // 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(memTx sdk.Tx, txBz []byte) bool + SelectTxForProposal(maxTxBytes, maxBlockGas uint64, 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 NewDefaultTxSelector() TxSelector { + return &defaultTxSelector{} } func (ts *defaultTxSelector) SelectedTxs() [][]byte { @@ -365,7 +364,7 @@ func (ts *defaultTxSelector) Clear() { ts.selectedTxs = nil } -func (ts *defaultTxSelector) SelectTxForProposal(memTx sdk.Tx, txBz []byte) bool { +func (ts *defaultTxSelector) SelectTxForProposal(maxTxBytes, maxBlockGas uint64, memTx sdk.Tx, txBz []byte) bool { txSize := uint64(len(txBz)) var txGasLimit uint64 @@ -376,11 +375,11 @@ func (ts *defaultTxSelector) SelectTxForProposal(memTx sdk.Tx, txBz []byte) bool } // only add the transaction to the proposal if we have enough capacity - if (txSize + ts.totalTxBytes) <= ts.maxTxBytes { + 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 ts.maxBlockGas > 0 { - if (txGasLimit + ts.totalTxGas) <= ts.maxBlockGas { + if maxBlockGas > 0 { + if (txGasLimit + ts.totalTxGas) <= maxBlockGas { ts.totalTxGas += txGasLimit ts.totalTxBytes += txSize ts.selectedTxs = append(ts.selectedTxs, txBz) @@ -392,5 +391,5 @@ func (ts *defaultTxSelector) SelectTxForProposal(memTx sdk.Tx, txBz []byte) bool } // 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)) + return ts.totalTxBytes >= maxTxBytes || (maxBlockGas > 0 && (ts.totalTxGas >= maxBlockGas)) } diff --git a/baseapp/abci_utils_test.go b/baseapp/abci_utils_test.go index eb6bd2fd3787..00eb3e2fa32b 100644 --- a/baseapp/abci_utils_test.go +++ b/baseapp/abci_utils_test.go @@ -4,19 +4,25 @@ import ( "bytes" "testing" + "cosmossdk.io/log" abci "github.com/cometbft/cometbft/abci/types" "github.com/cometbft/cometbft/crypto/secp256k1" cmtprotocrypto "github.com/cometbft/cometbft/proto/tendermint/crypto" cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" + dbm "github.com/cosmos/cosmos-db" protoio "github.com/cosmos/gogoproto/io" "github.com/cosmos/gogoproto/proto" "github.com/golang/mock/gomock" "github.com/stretchr/testify/suite" "github.com/cosmos/cosmos-sdk/baseapp" + baseapptestutil "github.com/cosmos/cosmos-sdk/baseapp/testutil" "github.com/cosmos/cosmos-sdk/baseapp/testutil/mock" + 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" + authtx "github.com/cosmos/cosmos-sdk/x/auth/tx" ) const ( @@ -276,23 +282,90 @@ func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsTwoVotesNilAbsent() { } func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_NoOpMempoolTxSelection() { - ph := baseapp.NewDefaultProposalHandler(mempool.NoOpMempool{}, nil) + // create a codec for marshaling + cdc := codectestutil.CodecOptions{}.NewCodec() + baseapptestutil.RegisterInterfaces(cdc.InterfaceRegistry()) + + // create a baseapp along with a tx config for tx generation + txConfig := authtx.NewTxConfig(cdc, authtx.DefaultSignModes) + app := baseapp.NewBaseApp(s.T().Name(), log.NewNopLogger(), dbm.NewMemDB(), txConfig.TxDecoder()) + + // create a proposal handler + ph := baseapp.NewDefaultProposalHandler(mempool.NoOpMempool{}, app) 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, - }) + // build a tx + _, _, addr := testdata.KeyTestPubAddr() + builder := txConfig.NewTxBuilder() + s.Require().NoError(builder.SetMsgs( + &baseapptestutil.MsgCounter{Counter: 0, FailOnHandler: false, Signer: addr.String()}, + )) + builder.SetGasLimit(100) + setTxSignature(s.T(), builder, 0) + + // encode the tx to be used in the proposal request + tx := builder.GetTx() + txBz, err := txConfig.TxEncoder()(tx) s.Require().NoError(err) - s.Require().Len(resp.Txs, 3) + s.Require().Len(txBz, 152) + + testCases := map[string]struct { + ctx sdk.Context + req *abci.RequestPrepareProposal + expectedTxs int + }{ + "small max tx bytes": { + ctx: s.ctx, + req: &abci.RequestPrepareProposal{ + Txs: [][]byte{txBz, txBz, txBz, txBz, txBz}, + MaxTxBytes: 10, + }, + expectedTxs: 0, + }, + "small max gas": { + ctx: s.ctx.WithConsensusParams(cmtproto.ConsensusParams{ + Block: &cmtproto.BlockParams{ + MaxGas: 10, + }, + }), + req: &abci.RequestPrepareProposal{ + Txs: [][]byte{txBz, txBz, txBz, txBz, txBz}, + MaxTxBytes: 456, + }, + expectedTxs: 0, + }, + "large max tx bytes": { + ctx: s.ctx, + req: &abci.RequestPrepareProposal{ + Txs: [][]byte{txBz, txBz, txBz, txBz, txBz}, + MaxTxBytes: 456, + }, + expectedTxs: 3, + }, + "max gas and tx bytes": { + ctx: s.ctx.WithConsensusParams(cmtproto.ConsensusParams{ + Block: &cmtproto.BlockParams{ + MaxGas: 200, + }, + }), + req: &abci.RequestPrepareProposal{ + Txs: [][]byte{txBz, txBz, txBz, txBz, txBz}, + MaxTxBytes: 456, + }, + expectedTxs: 2, + }, + } + + for name, tc := range testCases { + s.Run(name, func() { + // iterate multiple times to ensure the tx selector is cleared each time + for i := 0; i < 5; i++ { + resp, err := handler(tc.ctx, tc.req) + s.Require().NoError(err) + s.Require().Len(resp.Txs, tc.expectedTxs) + } + }) + } } func marshalDelimitedFn(msg proto.Message) ([]byte, error) { diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 2fba770458bf..f46aa8735daf 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -1092,6 +1092,14 @@ func (app *BaseApp) ProcessProposalVerifyTx(txBz []byte) (sdk.Tx, error) { return tx, nil } +func (app *BaseApp) TxDecode(txBytes []byte) (sdk.Tx, error) { + return app.txDecoder(txBytes) +} + +func (app *BaseApp) TxEncode(tx sdk.Tx) ([]byte, error) { + return app.txEncoder(tx) +} + // Close is called in start cmd to gracefully cleanup resources. func (app *BaseApp) Close() error { var errs []error From f142781b7c21214702628dbbf82b9694d7abfb0d Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 22 Sep 2023 10:03:47 -0400 Subject: [PATCH 11/11] lint++ --- baseapp/abci_utils_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/baseapp/abci_utils_test.go b/baseapp/abci_utils_test.go index 00eb3e2fa32b..8919ee81ba8c 100644 --- a/baseapp/abci_utils_test.go +++ b/baseapp/abci_utils_test.go @@ -4,7 +4,6 @@ import ( "bytes" "testing" - "cosmossdk.io/log" abci "github.com/cometbft/cometbft/abci/types" "github.com/cometbft/cometbft/crypto/secp256k1" cmtprotocrypto "github.com/cometbft/cometbft/proto/tendermint/crypto" @@ -15,6 +14,8 @@ import ( "github.com/golang/mock/gomock" "github.com/stretchr/testify/suite" + "cosmossdk.io/log" + "github.com/cosmos/cosmos-sdk/baseapp" baseapptestutil "github.com/cosmos/cosmos-sdk/baseapp/testutil" "github.com/cosmos/cosmos-sdk/baseapp/testutil/mock"