Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Revert middlewares to antehandler (part 2/2: posthandler) #11985

Merged
merged 34 commits into from May 23, 2022
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
1d8abfb
Make baseapp changes
amaury1093 May 17, 2022
2b32ad5
Revert middlewares
amaury1093 May 17, 2022
abd3ca6
Fix baseapp_test build
amaury1093 May 17, 2022
43b5a4a
Fix one test
amaury1093 May 17, 2022
fbc63dd
Fix simapp
amaury1093 May 17, 2022
0a07181
Fix more tests
amaury1093 May 17, 2022
f0a6d11
Fix another one?
amaury1093 May 17, 2022
659c2bd
Fix ante test build
amaury1093 May 17, 2022
99c764b
Fix amino tests
amaury1093 May 17, 2022
04c72a3
Fix test
amaury1093 May 17, 2022
6fd336e
Fix block gas test
amaury1093 May 17, 2022
09fd262
Small changes
amaury1093 May 17, 2022
6c78ed3
Add posthandler
amaury1093 May 18, 2022
9e414ee
Add tips, priority
amaury1093 May 18, 2022
b07ba75
Add simapp posthandler
amaury1093 May 18, 2022
c622e9c
put priority under deduct
amaury1093 May 18, 2022
2724975
Fix sigverify
amaury1093 May 18, 2022
801a7d6
Make tips test pass
amaury1093 May 20, 2022
c4e7d3e
Merge branch 'main' of ssh://github.com/cosmos/cosmos-sdk into am/rev…
amaury1093 May 20, 2022
e542ea0
Add extension options
amaury1093 May 20, 2022
8f180cf
Add TxFeeChecker
amaury1093 May 20, 2022
a8c8ce8
Add comment
amaury1093 May 20, 2022
417cee3
Fix fee test
amaury1093 May 20, 2022
5f96e31
Add changelog
amaury1093 May 20, 2022
2460fd6
Merge branch 'main' of ssh://github.com/cosmos/cosmos-sdk into am/rev…
amaury1093 May 20, 2022
d48d253
Add comment on posthandler
amaury1093 May 20, 2022
46e2102
Run postHandler on simulate
amaury1093 May 20, 2022
aff7e01
Update baseapp/baseapp.go
amaury1093 May 20, 2022
35866ff
Add priority back to checkTx
amaury1093 May 20, 2022
1880bc3
Merge branch 'am/revert-045-part2' of ssh://github.com/cosmos/cosmos-…
amaury1093 May 20, 2022
9a3cecb
Merge branch 'main' into am/revert-045-part2
julienrbrt May 21, 2022
1a39948
Add comment for skipped test
amaury1093 May 23, 2022
be08978
Add tips comments
amaury1093 May 23, 2022
92c1f0b
Merge branch 'main' into am/revert-045-part2
amaury1093 May 23, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
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) {
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
// 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)
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
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
32 changes: 32 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,24 @@ 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. 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()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any opinions if we should enable Tips by default on simapp (note: all chains that will copy-paste simapp might also enable them)? Is it a use case that's generic enough to be enabled by default on all cosmos chains?

I'd personally say no, and it's more a specific use-case, but would like to hear other opinions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go with a yes, when we built craft, one of the discoveries that I think that we had is that an application template with all of the features turned on would be useful. Also, if we have this turned on in simapp, it gets tested

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if it’s there and come and to down at least it can be used. Just so that you know, craft began as an implementation of simapp.

basically I’d like to see a Nonsimapp app template in the sdk

I’ve been calling that tiny.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For various reasons Gaia is no longer a great template. Unfortunately the same can be said of starport.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like yourself I think it’s not too much of a big deal in either direction if this is present directly in the simulation app or not but I would lean toward including it


if loadLatest {
if err := app.LoadLatestVersion(); err != nil {
Expand Down Expand Up @@ -475,6 +494,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