Skip to content

Commit

Permalink
refactor: Revert middlewares to antehandler (part 2/2: posthandler) (#…
Browse files Browse the repository at this point in the history
…11985)

## Description

We decided to remove middlewares, and revert to antehandlers (exactly like in v045) for this release. A better middleware solution will be implemented after v046.

ref: #11955

This PR is part 2 of 2:

- part 1: Revert baseapp and middlewares to v0.45.4
- part 2: Add posthandler, tips, priority

Depends on:
- [x] #11979 

---

Suggestion for reviewers:

- Apart from correctness, I would also like someone to review **exhaustiveness**. I.e. all changes we made in v046 into the [middleware folder](https://github.com/cosmos/cosmos-sdk/tree/v0.46.0-beta2/x/auth/middleware) are reflected in this PR, and that I didn't forget anything. I found the following ones:
  - add a TxFeeChecker in DeductFee
  - add a ExtensionChecker in ExtCheckerDecorator
  - add a TipDecorator



---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
  • Loading branch information
amaury1093 committed May 23, 2022
1 parent b2af716 commit d416ee8
Show file tree
Hide file tree
Showing 19 changed files with 392 additions and 142 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Expand Up @@ -39,6 +39,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Features

* (types) [#11985](https://github.com/cosmos/cosmos-sdk/pull/11985) Add a `Priority` field on `sdk.Context`, which represents the CheckTx priority field. It is only used during CheckTx.
* (gRPC) [#11889](https://github.com/cosmos/cosmos-sdk/pull/11889) Support custom read and write gRPC options in `app.toml`. See `max-recv-msg-size` and `max-send-msg-size` respectively.
* (cli) [\#11738](https://github.com/cosmos/cosmos-sdk/pull/11738) Add `tx auth multi-sign` as alias of `tx auth multisign` for consistency with `multi-send`.
* (cli) [\#11738](https://github.com/cosmos/cosmos-sdk/pull/11738) Add `tx bank multi-send` command for bulk send of coins to multiple accounts.
Expand Down Expand Up @@ -90,6 +91,8 @@ Ref: https://keepachangelog.com/en/1.0.0/

### API Breaking Changes

* (x/auth/ante) [#11985](https://github.com/cosmos/cosmos-sdk/pull/11985) The `MempoolFeeDecorator` has been removed. Instead, the `DeductFeeDecorator` takes a new argument of type `TxFeeChecker`, to define custom fee models. If `nil` is passed to this `TxFeeChecker` argument, then it will default to `checkTxFeeWithValidatorMinGasPrices`, which is the exact same behavior as the old `MempoolFeeDecorator` (i.e. checking fees against validator's own min gas price).
* (x/auth/ante) [#11985](https://github.com/cosmos/cosmos-sdk/pull/11985) The `ExtensionOptionsDecorator` takes an argument of type `ExtensionOptionChecker`. For backwards-compatibility, you can pass `nil`, which defaults to the old behavior of rejecting all tx extensions.
* (crypto/keyring) [#11932](https://github.com/cosmos/cosmos-sdk/pull/11932) Remove `Unsafe*` interfaces from keyring package. Please use interface casting if you wish to access those unsafe functions.
* (types) [#11881](https://github.com/cosmos/cosmos-sdk/issues/11881) Rename `AccAddressFromHex` to `AccAddressFromHexUnsafe`.
* (types) [#11788](https://github.com/cosmos/cosmos-sdk/pull/11788) The `Int` and `Uint` types have been moved to their own dedicated module, `math`. Aliases are kept in the SDK's root `types` package, however, it is encouraged to utilize the new `math` module. As a result, the `Int#ToDec` API has been removed.
Expand Down Expand Up @@ -277,6 +280,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### State Machine Breaking

* (baseapp) [\#11985](https://github.com/cosmos/cosmos-sdk/pull/11985) Add a `postHandler` to baseapp. This `postHandler` is like antehandler, but is run _after_ the `runMsgs` execution. It is in the same store branch that `runMsgs`, meaning that both `runMsgs` and `postHandler`
* (x/gov) [#11998](https://github.com/cosmos/cosmos-sdk/pull/11998) Tweak the `x/gov` `ModuleAccountInvariant` invariant to ensure deposits are `<=` total module account balance instead of strictly equal.
* (x/upgrade) [\#11800](https://github.com/cosmos/cosmos-sdk/pull/11800) Fix `GetLastCompleteUpgrade` to properly return the latest upgrade.
* [\#10564](https://github.com/cosmos/cosmos-sdk/pull/10564) Fix bug when updating allowance inside AllowedMsgAllowance
Expand Down
5 changes: 3 additions & 2 deletions baseapp/abci.go
Expand Up @@ -251,7 +251,7 @@ func (app *BaseApp) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx {
panic(fmt.Sprintf("unknown RequestCheckTx type: %s", req.Type))
}

gInfo, result, anteEvents, err := app.runTx(mode, req.Tx)
gInfo, result, anteEvents, priority, err := app.runTx(mode, req.Tx)
if err != nil {
return sdkerrors.ResponseCheckTxWithEvents(err, gInfo.GasWanted, gInfo.GasUsed, anteEvents, app.trace)
}
Expand All @@ -262,6 +262,7 @@ func (app *BaseApp) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx {
Log: result.Log,
Data: result.Data,
Events: sdk.MarkEventsToIndex(result.Events, app.indexEvents),
Priority: priority,
}
}

Expand All @@ -281,7 +282,7 @@ func (app *BaseApp) DeliverTx(req abci.RequestDeliverTx) abci.ResponseDeliverTx
telemetry.SetGauge(float32(gInfo.GasWanted), "tx", "gas", "wanted")
}()

gInfo, result, anteEvents, err := app.runTx(runTxModeDeliver, req.Tx)
gInfo, result, anteEvents, _, err := app.runTx(runTxModeDeliver, req.Tx)
if err != nil {
resultStr = "failed"
return sdkerrors.ResponseDeliverTxWithEvents(err, gInfo.GasWanted, gInfo.GasUsed, anteEvents, app.trace)
Expand Down
42 changes: 29 additions & 13 deletions baseapp/baseapp.go
Expand Up @@ -58,6 +58,7 @@ type BaseApp struct { // nolint: maligned
txDecoder sdk.TxDecoder // unmarshal []byte into sdk.Tx

anteHandler sdk.AnteHandler // ante handler for fee and auth
postHandler sdk.AnteHandler // post handler, optional, e.g. for tips
initChainer sdk.InitChainer // initialize state with validators and state blob
beginBlocker sdk.BeginBlocker // logic to run before any txs
endBlocker sdk.EndBlocker // logic to run after all txs, and to determine valset changes
Expand Down Expand Up @@ -584,7 +585,7 @@ func (app *BaseApp) cacheTxContext(ctx sdk.Context, txBytes []byte) (sdk.Context
// Note, gas execution info is always returned. A reference to a Result is
// returned if the tx does not run out of gas and if all the messages are valid
// and execute successfully. An error is returned otherwise.
func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, result *sdk.Result, anteEvents []abci.Event, err error) {
func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, result *sdk.Result, anteEvents []abci.Event, priority int64, err error) {
// NOTE: GasWanted should be returned by the AnteHandler. GasUsed is
// determined by the GasMeter. We need access to the context to get the gas
// meter so we initialize upfront.
Expand All @@ -595,7 +596,7 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re

// only run the tx if there is block gas remaining
if mode == runTxModeDeliver && ctx.BlockGasMeter().IsOutOfGas() {
return gInfo, nil, nil, sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "no block gas left to run tx")
return gInfo, nil, nil, 0, sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "no block gas left to run tx")
}

defer func() {
Expand Down Expand Up @@ -630,12 +631,12 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re

tx, err := app.txDecoder(txBytes)
if err != nil {
return sdk.GasInfo{}, nil, nil, err
return sdk.GasInfo{}, nil, nil, 0, err
}

msgs := tx.GetMsgs()
if err := validateBasicTxMsgs(msgs); err != nil {
return sdk.GasInfo{}, nil, nil, err
return sdk.GasInfo{}, nil, nil, 0, err
}

if app.anteHandler != nil {
Expand Down Expand Up @@ -671,9 +672,10 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re
gasWanted = ctx.GasMeter().Limit()

if err != nil {
return gInfo, nil, nil, err
return gInfo, nil, nil, 0, err
}

priority = ctx.Priority()
msCache.Write()
anteEvents = events.ToABCIEvents()
}
Expand All @@ -687,19 +689,33 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re
// and we're in DeliverTx. Note, runMsgs will never return a reference to a
// Result if any single message fails or does not have a registered Handler.
result, err = app.runMsgs(runMsgCtx, msgs, mode)
if err == nil && mode == runTxModeDeliver {
// When block gas exceeds, it'll panic and won't commit the cached store.
consumeBlockGas()
if err == nil {
// Run optional postHandlers.
//
// Note: If the postHandler fails, we also revert the runMsgs state.
if app.postHandler != nil {
newCtx, err := app.postHandler(runMsgCtx, tx, mode == runTxModeSimulate)
if err != nil {
return gInfo, nil, nil, priority, err
}

msCache.Write()
result.Events = append(result.Events, newCtx.EventManager().ABCIEvents()...)
}

if len(anteEvents) > 0 {
// append the events in the order of occurrence
result.Events = append(anteEvents, result.Events...)
if mode == runTxModeDeliver {
// When block gas exceeds, it'll panic and won't commit the cached store.
consumeBlockGas()

msCache.Write()

if len(anteEvents) > 0 {
// append the events in the order of occurrence
result.Events = append(anteEvents, result.Events...)
}
}
}

return gInfo, result, anteEvents, err
return gInfo, result, anteEvents, priority, err
}

// runMsgs iterates through a list of messages and executes them with the provided
Expand Down
7 changes: 7 additions & 0 deletions baseapp/baseapp_test.go
Expand Up @@ -36,6 +36,10 @@ import (
var (
capKey1 = sdk.NewKVStoreKey("key1")
capKey2 = sdk.NewKVStoreKey("key2")

// testTxPriority is the CheckTx priority that we set in the test
// antehandler.
testTxPriority = int64(42)
)

type paramStore struct {
Expand Down Expand Up @@ -826,6 +830,8 @@ func anteHandlerTxTest(t *testing.T, capKey storetypes.StoreKey, storeKey []byte
counterEvent("ante_handler", txTest.Counter),
)

ctx = ctx.WithPriority(testTxPriority)

return ctx, nil
}
}
Expand Down Expand Up @@ -933,6 +939,7 @@ func TestCheckTx(t *testing.T) {
txBytes, err := codec.Marshal(tx)
require.NoError(t, err)
r := app.CheckTx(abci.RequestCheckTx{Tx: txBytes})
require.Equal(t, testTxPriority, r.Priority)
require.Empty(t, r.GetEvents())
require.True(t, r.IsOK(), fmt.Sprintf("%v", r))
}
Expand Down
8 changes: 8 additions & 0 deletions baseapp/options.go
Expand Up @@ -153,6 +153,14 @@ func (app *BaseApp) SetAnteHandler(ah sdk.AnteHandler) {
app.anteHandler = ah
}

func (app *BaseApp) SetPostHandler(ph sdk.AnteHandler) {
if app.sealed {
panic("SetPostHandler() on sealed BaseApp")
}

app.postHandler = ph
}

func (app *BaseApp) SetAddrPeerFilter(pf sdk.PeerFilter) {
if app.sealed {
panic("SetAddrPeerFilter() on sealed BaseApp")
Expand Down
6 changes: 3 additions & 3 deletions baseapp/test_helpers.go
Expand Up @@ -16,13 +16,13 @@ func (app *BaseApp) SimCheck(txEncoder sdk.TxEncoder, tx sdk.Tx) (sdk.GasInfo, *
if err != nil {
return sdk.GasInfo{}, nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "%s", err)
}
gasInfo, result, _, err := app.runTx(runTxModeCheck, bz)
gasInfo, result, _, _, err := app.runTx(runTxModeCheck, bz)
return gasInfo, result, err
}

// Simulate executes a tx in simulate mode to get result and gas info.
func (app *BaseApp) Simulate(txBytes []byte) (sdk.GasInfo, *sdk.Result, error) {
gasInfo, result, _, err := app.runTx(runTxModeSimulate, txBytes)
gasInfo, result, _, _, err := app.runTx(runTxModeSimulate, txBytes)
return gasInfo, result, err
}

Expand All @@ -32,7 +32,7 @@ func (app *BaseApp) SimDeliver(txEncoder sdk.TxEncoder, tx sdk.Tx) (sdk.GasInfo,
if err != nil {
return sdk.GasInfo{}, nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "%s", err)
}
gasInfo, result, _, err := app.runTx(runTxModeDeliver, bz)
gasInfo, result, _, _, err := app.runTx(runTxModeDeliver, bz)
return gasInfo, result, err
}

Expand Down
35 changes: 35 additions & 0 deletions simapp/app.go
Expand Up @@ -34,6 +34,7 @@ import (
"github.com/cosmos/cosmos-sdk/x/auth"
"github.com/cosmos/cosmos-sdk/x/auth/ante"
authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper"
"github.com/cosmos/cosmos-sdk/x/auth/posthandler"
authsims "github.com/cosmos/cosmos-sdk/x/auth/simulation"
authtx "github.com/cosmos/cosmos-sdk/x/auth/tx"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
Expand Down Expand Up @@ -443,6 +444,27 @@ func NewSimApp(
app.SetBeginBlocker(app.BeginBlocker)
app.SetEndBlocker(app.EndBlocker)
app.setAnteHandler(encodingConfig.TxConfig, cast.ToStringSlice(appOpts.Get(server.FlagIndexEvents)))
// In v0.46, the SDK introduces _postHandlers_. PostHandlers are like
// antehandlers, but are run _after_ the `runMsgs` execution. They are also
// defined as a chain, and have the same signature as antehandlers.
//
// In baseapp, postHandlers are run in the same store branch as `runMsgs`,
// meaning that both `runMsgs` and `postHandler` state will be committed if
// both are successful, and both will be reverted if any of the two fails.
//
// The SDK exposes a default postHandlers chain, which comprises of only
// one decorator: the Transaction Tips decorator. However, some chains do
// not need it by default, so the following line is commented out. You can
// uncomment it to include the tips decorator, or define your own
// postHandler chain. To read more about tips:
// https://docs.cosmos.network/main/core/tips.html
//
// Please note that changing any of the anteHandler or postHandler chain is
// likely to be a state-machine breaking change, which needs a coordinated
// upgrade.
//
// Uncomment to enable postHandlers:
// app.setPostHandler()

if loadLatest {
if err := app.LoadLatestVersion(); err != nil {
Expand Down Expand Up @@ -475,6 +497,19 @@ func (app *SimApp) setAnteHandler(txConfig client.TxConfig, indexEventsStr []str
app.SetAnteHandler(anteHandler)
}

func (app *SimApp) setPostHandler() {
postHandler, err := posthandler.NewPostHandler(
posthandler.HandlerOptions{
BankKeeper: app.BankKeeper,
},
)
if err != nil {
panic(err)
}

app.SetPostHandler(postHandler)
}

// Name returns the name of the App
func (app *SimApp) Name() string { return app.BaseApp.Name() }

Expand Down
8 changes: 8 additions & 0 deletions types/context.go
Expand Up @@ -38,6 +38,7 @@ type Context struct {
minGasPrice DecCoins
consParams *tmproto.ConsensusParams
eventManager *EventManager
priority int64 // The tx priority, only relevant in CheckTx
}

// Proposed rename, not done to avoid API breakage
Expand All @@ -58,6 +59,7 @@ func (c Context) IsCheckTx() bool { return c.checkTx }
func (c Context) IsReCheckTx() bool { return c.recheckTx }
func (c Context) MinGasPrices() DecCoins { return c.minGasPrice }
func (c Context) EventManager() *EventManager { return c.eventManager }
func (c Context) Priority() int64 { return c.priority }

// clone the header before returning
func (c Context) BlockHeader() tmproto.Header {
Expand Down Expand Up @@ -226,6 +228,12 @@ func (c Context) WithEventManager(em *EventManager) Context {
return c
}

// WithEventManager returns a Context with an updated tx priority
func (c Context) WithPriority(p int64) Context {
c.priority = p
return c
}

// TODO: remove???
func (c Context) IsZero() bool {
return c.ms == nil
Expand Down
24 changes: 10 additions & 14 deletions x/auth/ante/ante.go
Expand Up @@ -10,11 +10,13 @@ import (

// HandlerOptions are the options required for constructing a default SDK AnteHandler.
type HandlerOptions struct {
AccountKeeper AccountKeeper
BankKeeper types.BankKeeper
FeegrantKeeper FeegrantKeeper
SignModeHandler authsigning.SignModeHandler
SigGasConsumer func(meter sdk.GasMeter, sig signing.SignatureV2, params types.Params) error
AccountKeeper AccountKeeper
BankKeeper types.BankKeeper
ExtensionOptionChecker ExtensionOptionChecker
FeegrantKeeper FeegrantKeeper
SignModeHandler authsigning.SignModeHandler
SigGasConsumer func(meter sdk.GasMeter, sig signing.SignatureV2, params types.Params) error
TxFeeChecker TxFeeChecker
}

// NewAnteHandler returns an AnteHandler that checks and increments sequence
Expand All @@ -33,23 +35,17 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) {
return nil, sdkerrors.Wrap(sdkerrors.ErrLogic, "sign mode handler is required for ante builder")
}

var sigGasConsumer = options.SigGasConsumer
if sigGasConsumer == nil {
sigGasConsumer = DefaultSigVerificationGasConsumer
}

anteDecorators := []sdk.AnteDecorator{
NewSetUpContextDecorator(), // outermost AnteDecorator. SetUpContext must be called first
NewRejectExtensionOptionsDecorator(),
NewMempoolFeeDecorator(),
NewExtensionOptionsDecorator(options.ExtensionOptionChecker),
NewValidateBasicDecorator(),
NewTxTimeoutHeightDecorator(),
NewValidateMemoDecorator(options.AccountKeeper),
NewConsumeGasForTxSizeDecorator(options.AccountKeeper),
NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper),
NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.TxFeeChecker),
NewSetPubKeyDecorator(options.AccountKeeper), // SetPubKeyDecorator must be called before all signature verification decorators
NewValidateSigCountDecorator(options.AccountKeeper),
NewSigGasConsumeDecorator(options.AccountKeeper, sigGasConsumer),
NewSigGasConsumeDecorator(options.AccountKeeper, options.SigGasConsumer),
NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler),
NewIncrementSequenceDecorator(options.AccountKeeper),
}
Expand Down

0 comments on commit d416ee8

Please sign in to comment.