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 25 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
12 changes: 12 additions & 0 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 @@ -688,6 +689,17 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re
// 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 {
// Run optional postHandlers
if app.postHandler != nil {
newCtx, err := app.postHandler(runMsgCtx, tx, mode == runTxModeSimulate)
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor

Choose a reason for hiding this comment

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

So to be clear, if the post-handler fails, we don't write ANY of the message execution's state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I added a comment to clarify.

Rationale is to mimic how tips were implemented in the previous middlewares:

see tips details
  • in antehandlers, deduct fees from feepayer
  • run msgs
  • if msgs are succesful, transfer tip to feepayer
  • if that transfer is not successful (e.g. if tipper doesn't have enough tip balance), then revert the msg, because it's the tipper's fault, so his msgs shouldn't be executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, we could decide to do 3 store branches: one ante, one runMsgs, one post. What's your opinion on that @alexanderbez?

If we do that, the tip posthandler would need to be rewritten.

Copy link
Contributor

Choose a reason for hiding this comment

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

@AmauryM, this is how I see it:

  1. IFF you treat runMsgs and postHandlers as atomic, then they should use the same branch.
  2. If, on the other hand, they are NOT atomic, i.e. runMsgs passes and postHandlers fails but still commit, then they should use different branches.

Being that you've implemented it as atomic, i.e. (1), then it makes sense to use the same branch as you do now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. The more general question is: should the SDK treat runMsgs and postHandlers as atomic?

My take is yes, but I wanted to hear other opinions.

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

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

// When block gas exceeds, it'll panic and won't commit the cached store.
consumeBlockGas()

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
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
49 changes: 39 additions & 10 deletions x/auth/ante/ext.go
Expand Up @@ -2,7 +2,7 @@ package ante

import (
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"github.com/cosmos/cosmos-sdk/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)

Expand All @@ -11,26 +11,55 @@ type HasExtensionOptionsTx interface {
GetNonCriticalExtensionOptions() []*codectypes.Any
}

// ExtensionOptionChecker is a function that returns true if the extension option is accepted.
type ExtensionOptionChecker func(*codectypes.Any) bool

// rejectExtensionOption is the default extension check that reject all tx
// extensions.
func rejectExtensionOption(*codectypes.Any) bool {
return false
}

// RejectExtensionOptionsDecorator is an AnteDecorator that rejects all extension
// options which can optionally be included in protobuf transactions. Users that
// need extension options should create a custom AnteHandler chain that handles
// needed extension options properly and rejects unknown ones.
type RejectExtensionOptionsDecorator struct{}
type RejectExtensionOptionsDecorator struct {
checker ExtensionOptionChecker
}

// NewExtensionOptionsDecorator creates a new antehandler that rejects all extension
// options which can optionally be included in protobuf transactions that don't pass the checker.
// Users that need extension options should pass a custom checker that returns true for the
// needed extension options.
func NewExtensionOptionsDecorator(checker ExtensionOptionChecker) sdk.AnteDecorator {
if checker == nil {
checker = rejectExtensionOption
}

// NewRejectExtensionOptionsDecorator creates a new RejectExtensionOptionsDecorator
func NewRejectExtensionOptionsDecorator() RejectExtensionOptionsDecorator {
return RejectExtensionOptionsDecorator{}
return RejectExtensionOptionsDecorator{checker: checker}
}

var _ types.AnteDecorator = RejectExtensionOptionsDecorator{}
var _ sdk.AnteDecorator = RejectExtensionOptionsDecorator{}

// AnteHandle implements the AnteDecorator.AnteHandle method
func (r RejectExtensionOptionsDecorator) AnteHandle(ctx types.Context, tx types.Tx, simulate bool, next types.AnteHandler) (newCtx types.Context, err error) {
func (r RejectExtensionOptionsDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) {
err = checkExtOpts(tx, r.checker)
if err != nil {
return ctx, err
}

return next(ctx, tx, simulate)
}

func checkExtOpts(tx sdk.Tx, checker ExtensionOptionChecker) error {
if hasExtOptsTx, ok := tx.(HasExtensionOptionsTx); ok {
if len(hasExtOptsTx.GetExtensionOptions()) != 0 {
return ctx, sdkerrors.ErrUnknownExtensionOptions
for _, opt := range hasExtOptsTx.GetExtensionOptions() {
if !checker(opt) {
return sdkerrors.ErrUnknownExtensionOptions
}
}
}

return next(ctx, tx, simulate)
return nil
}
58 changes: 38 additions & 20 deletions x/auth/ante/ext_test.go
@@ -1,7 +1,7 @@
package ante_test

import (
"github.com/cosmos/cosmos-sdk/codec/types"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"github.com/cosmos/cosmos-sdk/testutil/testdata"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/auth/ante"
Expand All @@ -10,27 +10,45 @@ import (

func (suite *AnteTestSuite) TestRejectExtensionOptionsDecorator() {
suite.SetupTest(true) // setup
suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder()

reod := ante.NewRejectExtensionOptionsDecorator()
antehandler := sdk.ChainAnteDecorators(reod)
testCases := []struct {
msg string
allow bool
}{
{"allow extension", true},
{"reject extension", false},
}
for _, tc := range testCases {
suite.Run(tc.msg, func() {
txBuilder := suite.clientCtx.TxConfig.NewTxBuilder()

// no extension options should not trigger an error
theTx := suite.txBuilder.GetTx()
_, err := antehandler(suite.ctx, theTx, false)
suite.Require().NoError(err)
reod := ante.NewExtensionOptionsDecorator(func(_ *codectypes.Any) bool {
return tc.allow
})
antehandler := sdk.ChainAnteDecorators(reod)

extOptsTxBldr, ok := suite.txBuilder.(tx.ExtensionOptionsTxBuilder)
if !ok {
// if we can't set extension options, this decorator doesn't apply and we're done
return
}
// no extension options should not trigger an error
theTx := txBuilder.GetTx()
_, err := antehandler(suite.ctx, theTx, false)
suite.Require().NoError(err)

// setting any extension option should cause an error
any, err := types.NewAnyWithValue(testdata.NewTestMsg())
suite.Require().NoError(err)
extOptsTxBldr.SetExtensionOptions(any)
theTx = suite.txBuilder.GetTx()
_, err = antehandler(suite.ctx, theTx, false)
suite.Require().EqualError(err, "unknown extension options")
extOptsTxBldr, ok := txBuilder.(tx.ExtensionOptionsTxBuilder)
if !ok {
// if we can't set extension options, this decorator doesn't apply and we're done
return
}

// set an extension option and check
any, err := codectypes.NewAnyWithValue(testdata.NewTestMsg())
suite.Require().NoError(err)
extOptsTxBldr.SetExtensionOptions(any)
theTx = txBuilder.GetTx()
_, err = antehandler(suite.ctx, theTx, false)
if tc.allow {
suite.Require().NoError(err)
} else {
suite.Require().EqualError(err, "unknown extension options")
}
})
}
}