diff --git a/CHANGELOG.md b/CHANGELOG.md index 882ee49e9d64..79f4307928da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -100,6 +100,9 @@ correct version via: `pkgutil --pkg-info=com.apple.pkg.CLTools_Executables`. to the new keyring. * [\#4486](https://github.com/cosmos/cosmos-sdk/issues/4486) Introduce new `PeriodicVestingAccount` vesting account type that allows for arbitrary vesting periods. +* (baseapp) [\#5196](https://github.com/cosmos/cosmos-sdk/pull/5196) Baseapp has a new `runTxModeReCheck` to allow applications to skip expensive and unnecessary re-checking of transactions. +* (types) [\#5196](https://github.com/cosmos/cosmos-sdk/pull/5196) Context has new `IsRecheckTx() bool` and `WithIsReCheckTx(bool) Context` methods to to be used in the `AnteHandler`. +* (x/auth/ante) [\#5196](https://github.com/cosmos/cosmos-sdk/pull/5196) AnteDecorators have been updated to avoid unnecessary checks when `ctx.IsReCheckTx() == true` * (x/auth) [\#5006](https://github.com/cosmos/cosmos-sdk/pull/5006) Modular `AnteHandler` via composable decorators: * The `AnteDecorator` interface has been introduced to allow users to implement modular `AnteHandler` functionality that can be composed together to create a single `AnteHandler` rather than implementing diff --git a/baseapp/abci.go b/baseapp/abci.go index e843b99c756b..ef8c71d4222a 100644 --- a/baseapp/abci.go +++ b/baseapp/abci.go @@ -162,10 +162,15 @@ func (app *BaseApp) CheckTx(req abci.RequestCheckTx) (res abci.ResponseCheckTx) var result sdk.Result tx, err := app.txDecoder(req.Tx) - if err != nil { + switch { + case err != nil: result = err.Result() - } else { + case req.Type == abci.CheckTxType_New: result = app.runTx(runTxModeCheck, req.Tx, tx) + case req.Type == abci.CheckTxType_Recheck: + result = app.runTx(runTxModeReCheck, req.Tx, tx) + default: + panic(fmt.Sprintf("Unknown RequestCheckTx Type: %v", req.Type)) } return abci.ResponseCheckTx{ diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 5e1c465393bc..ae1ca19db7f7 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -22,12 +22,10 @@ import ( ) const ( - // Check a transaction - runTxModeCheck runTxMode = iota - // Simulate a transaction - runTxModeSimulate runTxMode = iota - // Deliver a transaction - runTxModeDeliver runTxMode = iota + runTxModeCheck runTxMode = iota // Check a transaction + runTxModeReCheck // Recheck a (pending) transaction after a commit + runTxModeSimulate // Simulate a transaction + runTxModeDeliver // Deliver a transaction // MainStoreKey is the string representation of the main store MainStoreKey = "main" @@ -467,25 +465,28 @@ func validateBasicTxMsgs(msgs []sdk.Msg) sdk.Error { // Returns the applications's deliverState if app is in runTxModeDeliver, // otherwise it returns the application's checkstate. func (app *BaseApp) getState(mode runTxMode) *state { - if mode == runTxModeCheck || mode == runTxModeSimulate { - return app.checkState + if mode == runTxModeDeliver { + return app.deliverState } - return app.deliverState + return app.checkState } // retrieve the context for the tx w/ txBytes and other memoized values. -func (app *BaseApp) getContextForTx(mode runTxMode, txBytes []byte) (ctx sdk.Context) { - ctx = app.getState(mode).ctx. +func (app *BaseApp) getContextForTx(mode runTxMode, txBytes []byte) sdk.Context { + ctx := app.getState(mode).ctx. WithTxBytes(txBytes). WithVoteInfos(app.voteInfos). WithConsensusParams(app.consensusParams) + if mode == runTxModeReCheck { + ctx = ctx.WithIsReCheckTx(true) + } if mode == runTxModeSimulate { ctx, _ = ctx.CacheContext() } - return + return ctx } // cacheTxContext returns a new context based off of the provided context with @@ -653,8 +654,8 @@ func (app *BaseApp) runMsgs(ctx sdk.Context, msgs []sdk.Msg, mode runTxMode) (re var msgResult sdk.Result - // skip actual execution for CheckTx mode - if mode != runTxModeCheck { + // skip actual execution for CheckTx and ReCheckTx mode + if mode != runTxModeCheck && mode != runTxModeReCheck { msgResult = handler(ctx, msg) } diff --git a/docs/core/baseapp.md b/docs/core/baseapp.md index 72dafa94e059..2a0a06a4a836 100644 --- a/docs/core/baseapp.md +++ b/docs/core/baseapp.md @@ -27,6 +27,7 @@ functionalities of an SDK application. - [Query Routing](#query-routing) - [Main ABCI Messages](#main-abci-messages) - [CheckTx](#checktx-1) + - [RecheckTx](#rechecktx) - [DeliverTx](#delivertx-1) - [RunTx, AnteHandler and RunMsgs](#runtx-antehandler-and-runmsgs) - [RunTx](#runtx) @@ -103,7 +104,7 @@ raw transaction bytes relayed by the underlying Tendermint engine. used to persist data related to the core of the application, like consensus parameters. - [`AnteHandler`](#antehandler): This handler is used to handle signature verification, fee payment, and other pre-message execution checks when a transaction is received. It's executed during -[`CheckTx`](#checktx-1) and [`DeliverTx`](#delivertx-1). +[`CheckTx/RecheckTx`](#checktx-1) and [`DeliverTx`](#delivertx-1). - [`InitChainer`](../basics/app-anatomy.md#initchainer), [`BeginBlocker` and `EndBlocker`](../basics/app-anatomy.md#beginblocker-and-endblocker): These are the functions executed when the application receives the `InitChain`, `BeginBlock` and `EndBlock` @@ -240,18 +241,43 @@ Developers building on top of the Cosmos SDK need not implement the ABCI themsel ### CheckTx -`CheckTx` is sent by the underlying consensus engine when a new unconfirmed (i.e. not yet included in a valid block) transaction is received by a full-node. The role of `CheckTx` is to guard the full-node's mempool (where unconfirmed transactions are stored until they are included in a block) from spam transactions. Unconfirmed transactions are relayed to peers only if they pass `CheckTx`. +`CheckTx` is sent by the underlying consensus engine when a new unconfirmed (i.e. not yet included in a valid block) +transaction is received by a full-node. The role of `CheckTx` is to guard the full-node's mempool +(where unconfirmed transactions are stored until they are included in a block) from spam transactions. +Unconfirmed transactions are relayed to peers only if they pass `CheckTx`. -`CheckTx()` can perform both _stateful_ and _stateless_ checks, but developers should strive to make them lightweight. In the Cosmos SDK, after decoding transactions, `CheckTx()` is implemented to do the following checks: +`CheckTx()` can perform both _stateful_ and _stateless_ checks, but developers should strive to +make them lightweight. In the Cosmos SDK, after decoding transactions, `CheckTx()` is implemented +to do the following checks: 1. Extract the `message`s from the transaction. -2. Perform _stateless_ checks by calling `ValidateBasic()` on each of the `messages`. This is done first, as _stateless_ checks are less computationally expensive than _stateful_ checks. If `ValidateBasic()` fail, `CheckTx` returns before running _stateful_ checks, which saves resources. -3. Perform non-module related _stateful_ checks on the account. This step is mainly about checking that the `message` signatures are valid, that enough fees are provided and that the sending account has enough funds to pay for said fees. Note that no precise `gas` counting occurs here, as `message`s are not processed. Usually, the `anteHandler` will check that the `gas` provided with the transaction is superior to a minimum reference gas amount based on the raw transaction size, in order to avoid spam with transactions that provide 0 gas. -4. Ensure that a [`Route`](#message-routing) exists for each `message`, but do **not** actually process `message`s. `Message`s only need to be processed when the canonical state need to be updated, which happens during `DeliverTx`. - -Steps 2. and 3. are performed by the `anteHandler` in the [`RunTx()`](<#runtx()-,antehandler-and-runmsgs()>) function, which `CheckTx()` calls with the `runTxModeCheck` mode. During each step of `CheckTx()`, a special [volatile state](#volatile-states) called `checkState` is updated. This state is used to keep track of the temporary changes triggered by the `CheckTx()` calls of each transaction without modifying the [main canonical state](#main-state) . For example, when a transaction goes through `CheckTx()`, the transaction's fees are deducted from the sender's account in `checkState`. If a second transaction is received from the same account before the first is processed, and the account has consumed all its funds in `checkState` during the first transaction, the second transaction will fail `CheckTx`() and be rejected. In any case, the sender's account will not actually pay the fees until the transaction is actually included in a block, because `checkState` never gets committed to the main state. `checkState` is reset to the latest state of the main state each time a blocks gets [committed](#commit). - -`CheckTx` returns a response to the underlying consensus engine of type [`abci.ResponseCheckTx`](https://tendermint.com/docs/spec/abci/abci.html#messages). The response contains: +2. Perform _stateless_ checks by calling `ValidateBasic()` on each of the `messages`. This is done + first, as _stateless_ checks are less computationally expensive than _stateful_ checks. If + `ValidateBasic()` fail, `CheckTx` returns before running _stateful_ checks, which saves resources. +3. Perform non-module related _stateful_ checks on the account. This step is mainly about checking + that the `message` signatures are valid, that enough fees are provided and that the sending account + has enough funds to pay for said fees. Note that no precise `gas` counting occurs here, + as `message`s are not processed. Usually, the `AnteHandler` will check that the `gas` provided + with the transaction is superior to a minimum reference gas amount based on the raw transaction size, + in order to avoid spam with transactions that provide 0 gas. +4. Ensure that a [`Route`](#message-routing) exists for each `message`, but do **not** actually + process `message`s. `Message`s only need to be processed when the canonical state need to be updated, + which happens during `DeliverTx`. + +Steps 2. and 3. are performed by the `AnteHandler` in the [`RunTx()`](<#runtx()-,antehandler-and-runmsgs()>) +function, which `CheckTx()` calls with the `runTxModeCheck` mode. During each step of `CheckTx()`, a +special [volatile state](#volatile-states) called `checkState` is updated. This state is used to keep +track of the temporary changes triggered by the `CheckTx()` calls of each transaction without modifying +the [main canonical state](#main-state) . For example, when a transaction goes through `CheckTx()`, the +transaction's fees are deducted from the sender's account in `checkState`. If a second transaction is +received from the same account before the first is processed, and the account has consumed all its +funds in `checkState` during the first transaction, the second transaction will fail `CheckTx`() and +be rejected. In any case, the sender's account will not actually pay the fees until the transaction +is actually included in a block, because `checkState` never gets committed to the main state. The +`checkState` is reset to the latest state of the main state each time a blocks gets [committed](#commit). + +`CheckTx` returns a response to the underlying consensus engine of type [`abci.ResponseCheckTx`](https://tendermint.com/docs/spec/abci/abci.html#messages). +The response contains: - `Code (uint32)`: Response Code. `0` if successful. - `Data ([]byte)`: Result bytes, if any. @@ -259,9 +285,18 @@ Steps 2. and 3. are performed by the `anteHandler` in the [`RunTx()`](<#runtx()- - `Info (string):` Additional information. May be non-deterministic. - `GasWanted (int64)`: Amount of gas requested for transaction. It is provided by users when they generate the transaction. - `GasUsed (int64)`: Amount of gas consumed by transaction. During `CheckTx`, this value is computed by multiplying the standard cost of a transaction byte by the size of the raw transaction (click [here](https://github.com/cosmos/cosmos-sdk/blob/master/x/auth/ante.go#L101) for an example). -- `Tags ([]cmn.KVPair)`: Key-Value tags for filtering and indexing transactions (eg. by account). +- `Events ([]Event)`: Key-Value events for filtering and indexing transactions (eg. by account or message type). - `Codespace (string)`: Namespace for the Code. +#### RecheckTx + +After `Commit`, `CheckTx` is run again on all transactions that remain in the node's local mempool +after filtering those included in the block. To prevent the mempool from rechecking all transactions +every time a block is committed, the configuration option `mempool.recheck=false` can be set. As of +Tendermint v0.32.1, an additional `Type` parameter is made available to the `CheckTx` function that +indicates whether an incoming transaction is new (`CheckTxType_New`), or a recheck (`CheckTxType_Recheck`). +This allows certain checks like signature verification can be skipped during `CheckTxType_Recheck`. + ### DeliverTx When the underlying consensus engine receives a block proposal, each transaction in the block needs to be processed by the application. To that end, the underlying consensus engine sends a `DeliverTx` message to the application for each transaction in a sequential order. @@ -270,8 +305,8 @@ Before the first transaction of a given block is processed, a [volatile state](# `DeliverTx` performs the **exact same steps as `CheckTx`**, with a little caveat at step 3 and the addition of a fifth step: -3. The `anteHandler` does **not** check that the transaction's `gas-prices` is sufficient. That is because the `min-gas-prices` value `gas-prices` is checked against is local to the node, and therefore what is enough for one full-node might not be for another. This means that the proposer can potentially include transactions for free, although they are not incentivised to do so, as they earn a bonus on the total fee of the block they propose. -4. For each `message` in the transaction, route to the appropriate module's `handler`. Additional _stateful_ checks are performed, and the cache-wrapped multistore held in `deliverState`'s `context` is updated by the module's `keeper`. If the `handler` returns successfully, the cache-wrapped multistore held in `context` is written to `deliverState` `CacheMultiStore`. +1. The `AnteHandler` does **not** check that the transaction's `gas-prices` is sufficient. That is because the `min-gas-prices` value `gas-prices` is checked against is local to the node, and therefore what is enough for one full-node might not be for another. This means that the proposer can potentially include transactions for free, although they are not incentivised to do so, as they earn a bonus on the total fee of the block they propose. +2. For each `message` in the transaction, route to the appropriate module's `handler`. Additional _stateful_ checks are performed, and the cache-wrapped multistore held in `deliverState`'s `context` is updated by the module's `keeper`. If the `handler` returns successfully, the cache-wrapped multistore held in `context` is written to `deliverState` `CacheMultiStore`. During step 5., each read/write to the store increases the value of `GasConsumed`. You can find the default cost of each operation [here](https://github.com/cosmos/cosmos-sdk/blob/master/store/types/gas.go#L142-L150). At any point, if `GasConsumed > GasWanted`, the function returns with `Code != 0` and `DeliverTx()` fails. diff --git a/types/context.go b/types/context.go index 9fe8f2b67437..ed4b693c9770 100644 --- a/types/context.go +++ b/types/context.go @@ -31,6 +31,7 @@ type Context struct { gasMeter GasMeter blockGasMeter GasMeter checkTx bool + recheckTx bool // if recheckTx == true, then checkTx must also be true minGasPrice DecCoins consParams *abci.ConsensusParams eventManager *EventManager @@ -51,6 +52,7 @@ func (c Context) VoteInfos() []abci.VoteInfo { return c.voteInfo } func (c Context) GasMeter() GasMeter { return c.gasMeter } func (c Context) BlockGasMeter() GasMeter { return c.blockGasMeter } 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 } @@ -152,6 +154,16 @@ func (c Context) WithIsCheckTx(isCheckTx bool) Context { return c } +// WithIsRecheckTx called with true will also set true on checkTx in order to +// enforce the invariant that if recheckTx = true then checkTx = true as well. +func (c Context) WithIsReCheckTx(isRecheckTx bool) Context { + if isRecheckTx { + c.checkTx = true + } + c.recheckTx = isRecheckTx + return c +} + func (c Context) WithMinGasPrices(gasPrices DecCoins) Context { c.minGasPrice = gasPrices return c diff --git a/x/auth/ante/ante_test.go b/x/auth/ante/ante_test.go index 1cbc7419c6c0..b6803c174189 100644 --- a/x/auth/ante/ante_test.go +++ b/x/auth/ante/ante_test.go @@ -1,6 +1,7 @@ package ante_test import ( + "encoding/json" "fmt" "math/rand" "strings" @@ -138,7 +139,7 @@ func TestAnteHandlerSigErrors(t *testing.T) { // Test logic around account number checking with one signer and many signers. func TestAnteHandlerAccountNumbers(t *testing.T) { // setup - app, ctx := createTestApp(true) + app, ctx := createTestApp(false) ctx = ctx.WithBlockHeight(1) anteHandler := ante.NewAnteHandler(app.AccountKeeper, app.SupplyKeeper, ante.DefaultSigVerificationGasConsumer) @@ -195,7 +196,7 @@ func TestAnteHandlerAccountNumbers(t *testing.T) { // Test logic around account number checking with many signers when BlockHeight is 0. func TestAnteHandlerAccountNumbersAtBlockHeightZero(t *testing.T) { // setup - app, ctx := createTestApp(true) + app, ctx := createTestApp(false) ctx = ctx.WithBlockHeight(0) anteHandler := ante.NewAnteHandler(app.AccountKeeper, app.SupplyKeeper, ante.DefaultSigVerificationGasConsumer) @@ -251,7 +252,7 @@ func TestAnteHandlerAccountNumbersAtBlockHeightZero(t *testing.T) { // Test logic around sequence checking with one signer and many signers. func TestAnteHandlerSequences(t *testing.T) { // setup - app, ctx := createTestApp(true) + app, ctx := createTestApp(false) ctx = ctx.WithBlockHeight(1) anteHandler := ante.NewAnteHandler(app.AccountKeeper, app.SupplyKeeper, ante.DefaultSigVerificationGasConsumer) @@ -407,7 +408,7 @@ func TestAnteHandlerMemoGas(t *testing.T) { func TestAnteHandlerMultiSigner(t *testing.T) { // setup - app, ctx := createTestApp(true) + app, ctx := createTestApp(false) ctx = ctx.WithBlockHeight(1) anteHandler := ante.NewAnteHandler(app.AccountKeeper, app.SupplyKeeper, ante.DefaultSigVerificationGasConsumer) @@ -733,3 +734,85 @@ func TestCustomSignatureVerificationGasConsumer(t *testing.T) { tx = types.NewTestTx(ctx, msgs, privs, accnums, seqs, fee) checkValidTx(t, anteHandler, ctx, tx, false) } + +func TestAnteHandlerReCheck(t *testing.T) { + // setup + app, ctx := createTestApp(true) + // set blockheight and recheck=true + ctx = ctx.WithBlockHeight(1) + ctx = ctx.WithIsReCheckTx(true) + + // keys and addresses + priv1, _, addr1 := types.KeyTestPubAddr() + // priv2, _, addr2 := types.KeyTestPubAddr() + + // set the accounts + acc1 := app.AccountKeeper.NewAccountWithAddress(ctx, addr1) + acc1.SetCoins(types.NewTestCoins()) + require.NoError(t, acc1.SetAccountNumber(0)) + app.AccountKeeper.SetAccount(ctx, acc1) + + antehandler := ante.NewAnteHandler(app.AccountKeeper, app.SupplyKeeper, ante.DefaultSigVerificationGasConsumer) + + // test that operations skipped on recheck do not run + + msg := types.NewTestMsg(addr1) + msgs := []sdk.Msg{msg} + fee := types.NewTestStdFee() + + privs, accnums, seqs := []crypto.PrivKey{priv1}, []uint64{0}, []uint64{0} + tx := types.NewTestTxWithMemo(ctx, msgs, privs, accnums, seqs, fee, "thisisatestmemo") + + // make signature array empty which would normally cause ValidateBasicDecorator and SigVerificationDecorator fail + // since these decorators don't run on recheck, the tx should pass the antehandler + stdTx := tx.(types.StdTx) + stdTx.Signatures = []types.StdSignature{} + + _, err := antehandler(ctx, stdTx, false) + require.Nil(t, err, "AnteHandler errored on recheck unexpectedly: %v", err) + + tx = types.NewTestTxWithMemo(ctx, msgs, privs, accnums, seqs, fee, "thisisatestmemo") + txBytes, err := json.Marshal(tx) + require.Nil(t, err, "Error marshalling tx: %v", err) + ctx = ctx.WithTxBytes(txBytes) + + // require that state machine param-dependent checking is still run on recheck since parameters can change between check and recheck + testCases := []struct { + name string + params types.Params + }{ + {"memo size check", types.NewParams(0, types.DefaultTxSigLimit, types.DefaultTxSizeCostPerByte, types.DefaultSigVerifyCostED25519, types.DefaultSigVerifyCostSecp256k1)}, + {"tx sig limit check", types.NewParams(types.DefaultMaxMemoCharacters, 0, types.DefaultTxSizeCostPerByte, types.DefaultSigVerifyCostED25519, types.DefaultSigVerifyCostSecp256k1)}, + {"txsize check", types.NewParams(types.DefaultMaxMemoCharacters, types.DefaultTxSigLimit, 10000000, types.DefaultSigVerifyCostED25519, types.DefaultSigVerifyCostSecp256k1)}, + {"sig verify cost check", types.NewParams(types.DefaultMaxMemoCharacters, types.DefaultTxSigLimit, types.DefaultTxSizeCostPerByte, types.DefaultSigVerifyCostED25519, 100000000)}, + } + for _, tc := range testCases { + // set testcase parameters + app.AccountKeeper.SetParams(ctx, tc.params) + + _, err := antehandler(ctx, tx, false) + + require.NotNil(t, err, "tx does not fail on recheck with updated params in test case: %s", tc.name) + + // reset parameters to default values + app.AccountKeeper.SetParams(ctx, types.DefaultParams()) + } + + // require that local mempool fee check is still run on recheck since validator may change minFee between check and recheck + // create new minimum gas price so antehandler fails on recheck + ctx = ctx.WithMinGasPrices([]sdk.DecCoin{{ + Denom: "dnecoin", // fee does not have this denom + Amount: sdk.NewDec(5), + }}) + _, err = antehandler(ctx, tx, false) + require.NotNil(t, err, "antehandler on recheck did not fail when mingasPrice was changed") + // reset min gasprice + ctx = ctx.WithMinGasPrices(sdk.DecCoins{}) + + // remove funds for account so antehandler fails on recheck + acc1.SetCoins(sdk.Coins{}) + app.AccountKeeper.SetAccount(ctx, acc1) + + _, err = antehandler(ctx, tx, false) + require.NotNil(t, err, "antehandler on recheck did not fail once feePayer no longer has sufficient funds") +} diff --git a/x/auth/ante/basic.go b/x/auth/ante/basic.go index d14f720b2a0d..0e999a9d4781 100644 --- a/x/auth/ante/basic.go +++ b/x/auth/ante/basic.go @@ -17,7 +17,9 @@ var ( ) // ValidateBasicDecorator will call tx.ValidateBasic and return any non-nil error. -// If ValidateBasic passes, decorator calls next AnteHandler in chain. +// 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 ValidateBasicDecorator struct{} func NewValidateBasicDecorator() ValidateBasicDecorator { @@ -25,6 +27,10 @@ func NewValidateBasicDecorator() ValidateBasicDecorator { } func (vbd ValidateBasicDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { + // no need to validate basic on recheck tx, call next antehandler + if ctx.IsReCheckTx() { + return next(ctx, tx, simulate) + } if err := tx.ValidateBasic(); err != nil { return ctx, err } diff --git a/x/auth/ante/basic_test.go b/x/auth/ante/basic_test.go index 90e7e8c1c067..145137f4cb9f 100644 --- a/x/auth/ante/basic_test.go +++ b/x/auth/ante/basic_test.go @@ -40,6 +40,14 @@ func TestValidateBasic(t *testing.T) { _, err = antehandler(ctx, validTx, false) require.Nil(t, err, "ValidateBasicDecorator returned error on valid tx. err: %v", err) + + // test decorator skips on recheck + ctx = ctx.WithIsReCheckTx(true) + + // decorator should skip processing invalidTx on recheck and thus return nil-error + _, err = antehandler(ctx, invalidTx, false) + + require.Nil(t, err, "ValidateBasicDecorator ran on ReCheck") } func TestValidateMemo(t *testing.T) { diff --git a/x/auth/ante/sigverify.go b/x/auth/ante/sigverify.go index f166ad4aaf39..da987527f943 100644 --- a/x/auth/ante/sigverify.go +++ b/x/auth/ante/sigverify.go @@ -153,9 +153,9 @@ func (sgcd SigGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula return next(ctx, tx, simulate) } -// Verify all signatures for tx and return error if any are invalid -// increment sequence of each signer and set updated account back in store -// Call next AnteHandler +// Verify all signatures for a tx and return an error if any are invalid. Note, +// the SigVerificationDecorator decorator will not get executed on ReCheck. +// // CONTRACT: Pubkeys are set in context for all signers before this decorator runs // CONTRACT: Tx must implement SigVerifiableTx interface type SigVerificationDecorator struct { @@ -169,6 +169,10 @@ func NewSigVerificationDecorator(ak keeper.AccountKeeper) SigVerificationDecorat } func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) { + // no need to verify signatures on recheck tx + if ctx.IsReCheckTx() { + return next(ctx, tx, simulate) + } sigTx, ok := tx.(SigVerifiableTx) if !ok { return ctx, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type") @@ -212,9 +216,15 @@ func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simul return next(ctx, tx, simulate) } -// Increments sequences of all signers. -// Use this decorator to prevent replay attacks -// CONTRACT: Tx must implement SigVerifiableTx interface +// IncrementSequenceDecorator handles incrementing sequences of all signers. +// Use the IncrementSequenceDecorator decorator to prevent replay attacks. Note, +// there is no need to execute IncrementSequenceDecorator on CheckTx or RecheckTX +// since it is merely updating the nonce. As a result, this has the side effect +// that subsequent and sequential txs orginating from the same account cannot be +// handled correctly in a reliable way. To send sequential txs orginating from the +// same account, it is recommended to instead use multiple messages in a tx. +// +// CONTRACT: The tx must implement the SigVerifiableTx interface. type IncrementSequenceDecorator struct { ak keeper.AccountKeeper } @@ -226,6 +236,11 @@ func NewIncrementSequenceDecorator(ak keeper.AccountKeeper) IncrementSequenceDec } func (isd IncrementSequenceDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { + // no need to increment sequence on CheckTx or RecheckTx + if ctx.IsCheckTx() { + return next(ctx, tx, simulate) + } + sigTx, ok := tx.(SigVerifiableTx) if !ok { return ctx, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type") diff --git a/x/auth/ante/sigverify_test.go b/x/auth/ante/sigverify_test.go index 85ed6911ebe1..58bcef1e2811 100644 --- a/x/auth/ante/sigverify_test.go +++ b/x/auth/ante/sigverify_test.go @@ -102,6 +102,8 @@ func TestConsumeSignatureVerificationGas(t *testing.T) { func TestSigVerification(t *testing.T) { // setup app, ctx := createTestApp(true) + // make block height non-zero to ensure account numbers part of signBytes + ctx = ctx.WithBlockHeight(1) // keys and addresses priv1, _, addr1 := types.KeyTestPubAddr() @@ -121,15 +123,39 @@ func TestSigVerification(t *testing.T) { fee := types.NewTestStdFee() - privs, accNums, seqs := []crypto.PrivKey{priv3, priv2, priv1}, []uint64{2, 1, 0}, []uint64{0, 0, 0} - tx := types.NewTestTx(ctx, msgs, privs, accNums, seqs, fee) - spkd := ante.NewSetPubKeyDecorator(app.AccountKeeper) svd := ante.NewSigVerificationDecorator(app.AccountKeeper) antehandler := sdk.ChainAnteDecorators(spkd, svd) - _, err := antehandler(ctx, tx, false) - require.NotNil(t, err) + type testCase struct { + name string + privs []crypto.PrivKey + accNums []uint64 + seqs []uint64 + recheck bool + shouldErr bool + } + testCases := []testCase{ + {"no signers", []crypto.PrivKey{}, []uint64{}, []uint64{}, false, true}, + {"not enough signers", []crypto.PrivKey{priv1, priv2}, []uint64{0, 1}, []uint64{0, 0}, false, true}, + {"wrong order signers", []crypto.PrivKey{priv3, priv2, priv1}, []uint64{2, 1, 0}, []uint64{0, 0, 0}, false, true}, + {"wrong accnums", []crypto.PrivKey{priv1, priv2, priv3}, []uint64{7, 8, 9}, []uint64{0, 0, 0}, false, true}, + {"wrong sequences", []crypto.PrivKey{priv1, priv2, priv3}, []uint64{0, 1, 2}, []uint64{3, 4, 5}, false, true}, + {"valid tx", []crypto.PrivKey{priv1, priv2, priv3}, []uint64{0, 1, 2}, []uint64{0, 0, 0}, false, false}, + {"no err on recheck", []crypto.PrivKey{}, []uint64{}, []uint64{}, true, false}, + } + for i, tc := range testCases { + ctx = ctx.WithIsReCheckTx(tc.recheck) + + tx := types.NewTestTx(ctx, msgs, tc.privs, tc.accNums, tc.seqs, fee) + + _, err := antehandler(ctx, tx, false) + if tc.shouldErr { + require.NotNil(t, err, "TestCase %d: %s did not error as expected", i, tc.name) + } else { + require.Nil(t, err, "TestCase %d: %s errored unexpectedly. Err: %v", i, tc.name, err) + } + } } func TestSigIntegration(t *testing.T) {