From 26ea71bb54eda0c9babf6103cc725282a7f89cfc Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Mon, 18 Sep 2023 22:07:12 +0200 Subject: [PATCH 01/10] feat(gov): handle panics when executing x/gov proposals (backport #17780) (#17793) Co-authored-by: Robert Zaremba Co-authored-by: Julien Robert --- .github/workflows/dependencies-review.yml | 2 +- CHANGELOG.md | 4 +++ x/gov/abci.go | 16 ++++++++++-- x/gov/abci_internal_test.go | 32 +++++++++++++++++++++++ 4 files changed, 51 insertions(+), 3 deletions(-) create mode 100644 x/gov/abci_internal_test.go diff --git a/.github/workflows/dependencies-review.yml b/.github/workflows/dependencies-review.yml index 52b58c3a486f..7503a9718030 100644 --- a/.github/workflows/dependencies-review.yml +++ b/.github/workflows/dependencies-review.yml @@ -10,7 +10,7 @@ jobs: steps: - uses: actions/setup-go@v3 with: - go-version: "1.19" + go-version: "1.21" check-latest: true - name: "Checkout Repository" uses: actions/checkout@v3 diff --git a/CHANGELOG.md b/CHANGELOG.md index e38a365dbcaa..1f66baa53589 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,10 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [Unreleased] +### Improvements + +* (x/gov) [#17780](https://github.com/cosmos/cosmos-sdk/pull/17780) Recover panics and turn them into errors when executing x/gov proposals. + ### Bug Fixes * (config) [#17649](https://github.com/cosmos/cosmos-sdk/pull/17649) Fix `mempool.max-txs` configuration is invalid in `app.config`. diff --git a/x/gov/abci.go b/x/gov/abci.go index fd0df2981779..8fc06069f886 100644 --- a/x/gov/abci.go +++ b/x/gov/abci.go @@ -4,6 +4,7 @@ import ( "fmt" "time" + "github.com/cosmos/cosmos-sdk/baseapp" "github.com/cosmos/cosmos-sdk/telemetry" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/gov/keeper" @@ -78,9 +79,8 @@ func EndBlocker(ctx sdk.Context, keeper *keeper.Keeper) { if err == nil { for idx, msg = range messages { handler := keeper.Router().Handler(msg) - var res *sdk.Result - res, err = handler(cacheCtx, msg) + res, err = safeExecuteHandler(cacheCtx, msg, handler) if err != nil { break } @@ -136,3 +136,15 @@ func EndBlocker(ctx sdk.Context, keeper *keeper.Keeper) { return false }) } + +// executes handle(msg) and recovers from panic. +func safeExecuteHandler(ctx sdk.Context, msg sdk.Msg, handler baseapp.MsgServiceHandler, +) (res *sdk.Result, err error) { + defer func() { + if r := recover(); r != nil { + err = fmt.Errorf("handling x/gov proposal msg [%s] PANICKED: %v", msg, r) + } + }() + res, err = handler(ctx, msg) + return +} diff --git a/x/gov/abci_internal_test.go b/x/gov/abci_internal_test.go new file mode 100644 index 000000000000..1421a81b5cb6 --- /dev/null +++ b/x/gov/abci_internal_test.go @@ -0,0 +1,32 @@ +package gov + +import ( + "testing" + + "github.com/stretchr/testify/require" + + sdk "github.com/cosmos/cosmos-sdk/types" +) + +func failingHandler(_ sdk.Context, _ sdk.Msg) (*sdk.Result, error) { + panic("test-fail") +} + +func okHandler(_ sdk.Context, _ sdk.Msg) (*sdk.Result, error) { + return new(sdk.Result), nil +} + +func TestSafeExecuteHandler(t *testing.T) { + t.Parallel() + + require := require.New(t) + var ctx sdk.Context + + r, err := safeExecuteHandler(ctx, nil, failingHandler) + require.ErrorContains(err, "test-fail") + require.Nil(r) + + r, err = safeExecuteHandler(ctx, nil, okHandler) + require.Nil(err) + require.NotNil(r) +} From 991691b08a3fc9a20c812eb1aa32f8b1a78c1110 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Wed, 20 Sep 2023 13:38:31 +0200 Subject: [PATCH 02/10] refactor(store): add missing error checks in store (#17817) --- store/iavl/store.go | 7 +++++-- store/listenkv/store.go | 4 +++- store/rootmulti/store.go | 17 ++++++++++++----- store/streaming/constructor.go | 8 +++++--- store/tracekv/store.go | 4 +++- 5 files changed, 28 insertions(+), 12 deletions(-) diff --git a/store/iavl/store.go b/store/iavl/store.go index 77f611380535..28e7f68df7d1 100644 --- a/store/iavl/store.go +++ b/store/iavl/store.go @@ -223,7 +223,9 @@ func (st *Store) Has(key []byte) (exists bool) { // Implements types.KVStore. func (st *Store) Delete(key []byte) { defer telemetry.MeasureSince(time.Now(), "store", "iavl", "delete") - st.tree.Remove(key) + if _, _, err := st.tree.Remove(key); err != nil { + panic(err) + } } // DeleteVersions deletes a series of versions from the MutableTree. An error @@ -370,7 +372,8 @@ func (st *Store) Query(req abci.RequestQuery) (res abci.ResponseQuery) { for ; iterator.Valid(); iterator.Next() { pairs.Pairs = append(pairs.Pairs, kv.Pair{Key: iterator.Key(), Value: iterator.Value()}) } - iterator.Close() + + _ = iterator.Close() bz, err := pairs.Marshal() if err != nil { diff --git a/store/listenkv/store.go b/store/listenkv/store.go index 4595d0fe56d1..2ea0e9974c2a 100644 --- a/store/listenkv/store.go +++ b/store/listenkv/store.go @@ -144,6 +144,8 @@ func (s *Store) CacheWrapWithTrace(_ io.Writer, _ types.TraceContext) types.Cach // onWrite writes a KVStore operation to all of the WriteListeners func (s *Store) onWrite(delete bool, key, value []byte) { for _, l := range s.listeners { - l.OnWrite(s.parentStoreKey, key, value, delete) + if err := l.OnWrite(s.parentStoreKey, key, value, delete); err != nil { + panic(err) + } } } diff --git a/store/rootmulti/store.go b/store/rootmulti/store.go index 79865502a33c..bebd6cd1af9a 100644 --- a/store/rootmulti/store.go +++ b/store/rootmulti/store.go @@ -312,7 +312,7 @@ func deleteKVStore(kv types.KVStore) error { keys = append(keys, itr.Key()) itr.Next() } - itr.Close() + _ = itr.Close() for _, k := range keys { kv.Delete(k) @@ -328,7 +328,7 @@ func moveKVStoreData(oldDB types.KVStore, newDB types.KVStore) error { newDB.Set(itr.Key(), itr.Value()) itr.Next() } - itr.Close() + _ = itr.Close() // then delete the old store return deleteKVStore(oldDB) @@ -1068,7 +1068,9 @@ func (rs *Store) GetCommitInfo(ver int64) (*types.CommitInfo, error) { func (rs *Store) flushMetadata(db dbm.DB, version int64, cInfo *types.CommitInfo) { rs.logger.Debug("flushing metadata", "height", version) batch := db.NewBatch() - defer batch.Close() + defer func() { + _ = batch.Close() + }() if cInfo != nil { flushCommitInfo(batch, version, cInfo) @@ -1168,7 +1170,10 @@ func flushCommitInfo(batch dbm.Batch, version int64, cInfo *types.CommitInfo) { } cInfoKey := fmt.Sprintf(commitInfoKeyFmt, version) - batch.Set([]byte(cInfoKey), bz) + + if err := batch.Set([]byte(cInfoKey), bz); err != nil { + panic(err) + } } func flushLatestVersion(batch dbm.Batch, version int64) { @@ -1177,5 +1182,7 @@ func flushLatestVersion(batch dbm.Batch, version int64) { panic(err) } - batch.Set([]byte(latestVersionKey), bz) + if err := batch.Set([]byte(latestVersionKey), bz); err != nil { + panic(err) + } } diff --git a/store/streaming/constructor.go b/store/streaming/constructor.go index b1b822bfa8fa..6693c938cdbb 100644 --- a/store/streaming/constructor.go +++ b/store/streaming/constructor.go @@ -163,7 +163,7 @@ func LoadStreamingServices( // Close any services we may have already spun up before hitting the error // on this one. for _, activeStreamer := range activeStreamers { - activeStreamer.Close() + _ = activeStreamer.Close() } return nil, nil, err @@ -176,7 +176,7 @@ func LoadStreamingServices( // Close any services we may have already spun up before hitting the error // on this one. for _, activeStreamer := range activeStreamers { - activeStreamer.Close() + _ = activeStreamer.Close() } return nil, nil, err @@ -186,7 +186,9 @@ func LoadStreamingServices( bApp.SetStreamingService(streamingService) // kick off the background streaming service loop - streamingService.Stream(wg) + if err := streamingService.Stream(wg); err != nil { + return nil, nil, err + } // add to the list of active streamers activeStreamers = append(activeStreamers, streamingService) diff --git a/store/tracekv/store.go b/store/tracekv/store.go index caf871552f56..d9c637823058 100644 --- a/store/tracekv/store.go +++ b/store/tracekv/store.go @@ -195,5 +195,7 @@ func writeOperation(w io.Writer, op operation, tc types.TraceContext, key, value panic(errors.Wrap(err, "failed to write trace operation")) } - io.WriteString(w, "\n") + if _, err = io.WriteString(w, "\n"); err != nil { + panic(errors.Wrap(err, "failed to write newline")) + } } 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 03/10] 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) + } + }) + } +} From d088a008928b508efe52159b8b9bf1a42cfa4d87 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 26 Sep 2023 15:42:47 +0000 Subject: [PATCH 04/10] docs: small update for routes change (backport #17881) (#17887) Co-authored-by: samricotta <37125168+samricotta@users.noreply.github.com> --- x/README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/x/README.md b/x/README.md index 0f53de0cd4af..ccbe2f4c66a0 100644 --- a/x/README.md +++ b/x/README.md @@ -1,6 +1,5 @@ --- sidebar_position: 0 -slug : /modules --- # List of Modules From 2854212f1f4c74bbb3f75b60e2b33af7f8ab12d9 Mon Sep 17 00:00:00 2001 From: Marko Date: Wed, 27 Sep 2023 14:24:35 +0200 Subject: [PATCH 05/10] chore: remove duplicate check (#17901) --- x/auth/tx/builder.go | 1 - 1 file changed, 1 deletion(-) diff --git a/x/auth/tx/builder.go b/x/auth/tx/builder.go index 2bee58151cff..ed84b6a23ed1 100644 --- a/x/auth/tx/builder.go +++ b/x/auth/tx/builder.go @@ -39,7 +39,6 @@ var ( _ tx.TipTx = &wrapper{} _ ante.HasExtensionOptionsTx = &wrapper{} _ ExtensionOptionsTxBuilder = &wrapper{} - _ tx.TipTx = &wrapper{} ) // ExtensionOptionsTxBuilder defines a TxBuilder that can also set extensions. From 4a7305463753dd65b7f567e8cfbcffa90955eaef Mon Sep 17 00:00:00 2001 From: Marko Date: Wed, 27 Sep 2023 15:10:41 +0200 Subject: [PATCH 06/10] chore: remove tip posthandler (#17902) --- CHANGELOG.md | 1 + x/auth/posthandler/tips.go | 58 -------------------------------------- 2 files changed, 1 insertion(+), 58 deletions(-) delete mode 100644 x/auth/posthandler/tips.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e46c3d941f8f..1ec9277052bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (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. +* (x/auth) [#17902](https://github.com/cosmos/cosmos-sdk/pull/17902) Remove tip posthandler ## [v0.47.5](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.5) - 2023-09-01 diff --git a/x/auth/posthandler/tips.go b/x/auth/posthandler/tips.go deleted file mode 100644 index 33a5f69e9081..000000000000 --- a/x/auth/posthandler/tips.go +++ /dev/null @@ -1,58 +0,0 @@ -package posthandler - -import ( - "fmt" - - sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/types/tx" - "github.com/cosmos/cosmos-sdk/x/auth/types" -) - -// ValidateBasicDecorator will call tx.ValidateBasic and return any non-nil error. -// If ValidateBasic passes, decorator calls next AnteHandler in chain. Note, -// ValidateBasicDecorator decorator will not get executed on ReCheckTx since it -// is not dependent on application state. -type tipDecorator struct { - bankKeeper types.BankKeeper -} - -// NewTipDecorator returns a new decorator for handling transactions with -// tips. -// -// IMPORTANT: This decorator is still in beta, please use it at your own risk. -func NewTipDecorator(bankKeeper types.BankKeeper) sdk.AnteDecorator { - return tipDecorator{ - bankKeeper: bankKeeper, - } -} - -func (d tipDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { - err := d.transferTip(ctx, tx) - if err != nil { - return ctx, err - } - - return next(ctx, tx, simulate) -} - -// transferTip transfers the tip from the tipper to the fee payer. -func (d tipDecorator) transferTip(ctx sdk.Context, sdkTx sdk.Tx) error { - tipTx, ok := sdkTx.(tx.TipTx) - - // No-op if the tx doesn't have tips. - if !ok || tipTx.GetTip() == nil { - return nil - } - - tipper, err := sdk.AccAddressFromBech32(tipTx.GetTip().Tipper) - if err != nil { - return err - } - - coins := tipTx.GetTip().Amount - if err := d.bankKeeper.IsSendEnabledCoins(ctx, coins...); err != nil { - return fmt.Errorf("cannot tip these coins: %w", err) - } - - return d.bankKeeper.SendCoins(ctx, tipper, tipTx.FeePayer(), coins) -} From 22f68a42589a30731e26ee07ca02e98aaa23b993 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Wed, 27 Sep 2023 15:44:25 +0200 Subject: [PATCH 07/10] chore(auth): return accId in error (backport #17903) (#17906) Co-authored-by: Marko Co-authored-by: Julien Robert --- x/auth/keeper/grpc_query.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/auth/keeper/grpc_query.go b/x/auth/keeper/grpc_query.go index 7acb77134c68..f81991af3478 100644 --- a/x/auth/keeper/grpc_query.go +++ b/x/auth/keeper/grpc_query.go @@ -33,7 +33,7 @@ func (ak AccountKeeper) AccountAddressByID(c context.Context, req *types.QueryAc ctx := sdk.UnwrapSDKContext(c) address := ak.GetAccountAddressByID(ctx, accId) if len(address) == 0 { - return nil, status.Errorf(codes.NotFound, "account address not found with account number %d", req.Id) + return nil, status.Errorf(codes.NotFound, "account address not found with account number %d", accId) } return &types.QueryAccountAddressByIDResponse{AccountAddress: address}, nil From a354b499aec9fb6d16bbc8bc04288e3abf69ee9b Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Wed, 27 Sep 2023 17:27:16 +0200 Subject: [PATCH 08/10] chore(store): check value (backport #17900) (#17908) Co-authored-by: Marko --- store/dbadapter/store.go | 1 + 1 file changed, 1 insertion(+) diff --git a/store/dbadapter/store.go b/store/dbadapter/store.go index b3dc99847ce9..aa52c95eaea2 100644 --- a/store/dbadapter/store.go +++ b/store/dbadapter/store.go @@ -38,6 +38,7 @@ func (dsa Store) Has(key []byte) bool { // Set wraps the underlying DB's Set method panicing on error. func (dsa Store) Set(key, value []byte) { types.AssertValidKey(key) + types.AssertValidValue(value) if err := dsa.DB.Set(key, value); err != nil { panic(err) } From 60909f3c360a13065e605e0001e8057fac5b1bdb Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Wed, 27 Sep 2023 19:50:32 +0000 Subject: [PATCH 09/10] refactor(x/gov): remove gov vote and proposal based telemetry (backport #17910) (#17916) Co-authored-by: Marko Co-authored-by: Julien Robert --- CHANGELOG.md | 6 +++++- x/gov/keeper/msg_server.go | 30 ------------------------------ 2 files changed, 5 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ec9277052bd..c71eb8612fd0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,7 +46,11 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (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. -* (x/auth) [#17902](https://github.com/cosmos/cosmos-sdk/pull/17902) Remove tip posthandler +* (x/auth) [#17902](https://github.com/cosmos/cosmos-sdk/pull/17902) Remove tip posthandler. + +### Client Breaking Changes + +* (x/gov) [#17910](https://github.com/cosmos/cosmos-sdk/pull/17910) Remove telemetry for counting votes and proposals. It was incorrectly counting votes. Use alternatives, such as state streaming. ## [v0.47.5](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.5) - 2023-09-01 diff --git a/x/gov/keeper/msg_server.go b/x/gov/keeper/msg_server.go index c66d011552ca..00a7186a629f 100644 --- a/x/gov/keeper/msg_server.go +++ b/x/gov/keeper/msg_server.go @@ -3,12 +3,8 @@ package keeper import ( "context" "fmt" - "strconv" "cosmossdk.io/errors" - "github.com/armon/go-metrics" - - "github.com/cosmos/cosmos-sdk/telemetry" sdk "github.com/cosmos/cosmos-sdk/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" v1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" @@ -63,8 +59,6 @@ func (k msgServer) SubmitProposal(goCtx context.Context, msg *v1.MsgSubmitPropos "submit proposal", ) - defer telemetry.IncrCounter(1, govtypes.ModuleName, "proposal") - votingStarted, err := k.Keeper.AddDeposit(ctx, proposal.Id, proposer, msg.GetInitialDeposit()) if err != nil { return nil, err @@ -122,14 +116,6 @@ func (k msgServer) Vote(goCtx context.Context, msg *v1.MsgVote) (*v1.MsgVoteResp return nil, err } - defer telemetry.IncrCounterWithLabels( - []string{govtypes.ModuleName, "vote"}, - 1, - []metrics.Label{ - telemetry.NewLabel("proposal_id", strconv.FormatUint(msg.ProposalId, 10)), - }, - ) - return &v1.MsgVoteResponse{}, nil } @@ -145,14 +131,6 @@ func (k msgServer) VoteWeighted(goCtx context.Context, msg *v1.MsgVoteWeighted) return nil, err } - defer telemetry.IncrCounterWithLabels( - []string{govtypes.ModuleName, "vote"}, - 1, - []metrics.Label{ - telemetry.NewLabel("proposal_id", strconv.FormatUint(msg.ProposalId, 10)), - }, - ) - return &v1.MsgVoteWeightedResponse{}, nil } @@ -168,14 +146,6 @@ func (k msgServer) Deposit(goCtx context.Context, msg *v1.MsgDeposit) (*v1.MsgDe return nil, err } - defer telemetry.IncrCounterWithLabels( - []string{govtypes.ModuleName, "deposit"}, - 1, - []metrics.Label{ - telemetry.NewLabel("proposal_id", strconv.FormatUint(msg.ProposalId, 10)), - }, - ) - if votingStarted { ctx.EventManager().EmitEvent( sdk.NewEvent( From 9de3c3fc5fd2823c22d30cbbd39e15461ae8b8a6 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Sun, 1 Oct 2023 06:13:26 +0200 Subject: [PATCH 10/10] docs: Typo on Application - Specific documentation (backport #17933) (#17935) --- docs/docs/intro/01-why-app-specific.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/docs/intro/01-why-app-specific.md b/docs/docs/intro/01-why-app-specific.md index 5830530ae706..0f0c1c64ed42 100644 --- a/docs/docs/intro/01-why-app-specific.md +++ b/docs/docs/intro/01-why-app-specific.md @@ -58,7 +58,7 @@ The list above contains a few examples that show how much flexibility applicatio ### Performance -decentralized applications built with Smart Contracts are inherently capped in performance by the underlying environment. For a decentralized application to optimise performance, it needs to be built as an application-specific blockchain. Next are some of the benefits an application-specific blockchain brings in terms of performance: +Decentralized applications built with Smart Contracts are inherently capped in performance by the underlying environment. For a decentralized application to optimise performance, it needs to be built as an application-specific blockchain. Next are some of the benefits an application-specific blockchain brings in terms of performance: * Developers of application-specific blockchains can choose to operate with a novel consensus engine such as CometBFT BFT. Compared to Proof-of-Work (used by most virtual-machine blockchains today), it offers significant gains in throughput. * An application-specific blockchain only operates a single application, so that the application does not compete with others for computation and storage. This is the opposite of most non-sharded virtual-machine blockchains today, where smart contracts all compete for computation and storage.