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

RecheckTx Optimizations #5196

Merged
merged 23 commits into from
Oct 23, 2019
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
420faef
Introduce Recheck optimizations
AdityaSripal Oct 15, 2019
dc98bb2
Merge branch 'master' into aditya/recheck
alexanderbez Oct 15, 2019
0310b36
fix tests
AdityaSripal Oct 15, 2019
42ddba9
Merge branch 'aditya/recheck' of https://github.com/cosmos/cosmos-sdk…
AdityaSripal Oct 15, 2019
17b47bf
add unit tests
AdityaSripal Oct 15, 2019
4eb7dc8
add integration tests
AdityaSripal Oct 15, 2019
48bc5cb
update changelog
AdityaSripal Oct 15, 2019
af3b24d
Update x/auth/ante/ante_test.go
AdityaSripal Oct 15, 2019
93fd162
Merge branch 'master' into aditya/recheck
fedekunze Oct 16, 2019
2e24c37
Merge branch 'master' into aditya/recheck
alexanderbez Oct 22, 2019
32f087b
enforce invariant, that recheck mode has IsCheckTx() = true
AdityaSripal Oct 22, 2019
3f94b1c
Merge branch 'aditya/recheck' of https://github.com/cosmos/cosmos-sdk…
AdityaSripal Oct 22, 2019
14f1939
address reviews
AdityaSripal Oct 22, 2019
81a5996
update changelog
alexanderbez Oct 22, 2019
b5232ee
lint: update godoc
alexanderbez Oct 22, 2019
8d2e54b
docs: update checktx section in baseapp doc
alexanderbez Oct 22, 2019
27d0393
doc: update IncrementSequenceDecorator godoc
alexanderbez Oct 22, 2019
08a44d6
cleanup iota const
alexanderbez Oct 23, 2019
39b91bf
remove named return
alexanderbez Oct 23, 2019
0af80d5
Update docs/core/baseapp.md
alexanderbez Oct 23, 2019
a0f895f
Update docs/core/baseapp.md
alexanderbez Oct 23, 2019
da856be
Merge branch 'master' into aditya/recheck
alexanderbez Oct 23, 2019
d14f4ee
Merge branch 'master' into aditya/recheck
alexanderbez Oct 23, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,9 @@ that allows for arbitrary vesting periods.
* `SigVerificationDecorator`: Verify each signature is valid, return if there is an error.
* `ValidateSigCountDecorator`: Validate the number of signatures in tx based on app-parameters.
* `IncrementSequenceDecorator`: Increments the account sequence for each signer to prevent replay attacks.
* Baseapp has a new `runTxModeReCheck` to allow applications to skip expensive and unnecessary re-checking of transaction
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
* Context has new `IsRecheckTx() bool` and `WithIsReCheckTx(bool) Context` methods to allow recheck boolean to be available for antehandler
* AnteDecorators have been updated to avoid unnecessary checks when `ctx.IsReCheckTx() == true`

### Improvements

Expand Down
4 changes: 3 additions & 1 deletion baseapp/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,10 @@ func (app *BaseApp) CheckTx(req abci.RequestCheckTx) (res abci.ResponseCheckTx)
tx, err := app.txDecoder(req.Tx)
if err != nil {
result = err.Result()
} else {
} else if req.Type == abci.CheckTxType_New {
result = app.runTx(runTxModeCheck, req.Tx, tx)
} else {
result = app.runTx(runTxModeReCheck, req.Tx, tx)
}
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved

return abci.ResponseCheckTx{
Expand Down
9 changes: 7 additions & 2 deletions baseapp/baseapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import (
const (
// Check a transaction
runTxModeCheck runTxMode = iota
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
// Recheck a transaction
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
runTxModeReCheck runTxMode = iota
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
// Simulate a transaction
runTxModeSimulate runTxMode = iota
// Deliver a transaction
Expand Down Expand Up @@ -481,6 +483,9 @@ func (app *BaseApp) getContextForTx(mode runTxMode, txBytes []byte) (ctx sdk.Con
WithVoteInfos(app.voteInfos).
WithConsensusParams(app.consensusParams)

AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
if mode == runTxModeReCheck {
ctx = ctx.WithIsReCheckTx(true)
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
}
if mode == runTxModeSimulate {
ctx, _ = ctx.CacheContext()
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
}
Expand Down Expand Up @@ -653,8 +658,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)
}

Expand Down
7 changes: 7 additions & 0 deletions types/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ type Context struct {
gasMeter GasMeter
blockGasMeter GasMeter
checkTx bool
recheckTx bool
minGasPrice DecCoins
consParams *abci.ConsensusParams
eventManager *EventManager
Expand All @@ -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 }

Expand Down Expand Up @@ -152,6 +154,11 @@ func (c Context) WithIsCheckTx(isCheckTx bool) Context {
return c
}

func (c Context) WithIsReCheckTx(isRecheckTx bool) Context {
c.recheckTx = isRecheckTx
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
return c
}

func (c Context) WithMinGasPrices(gasPrices DecCoins) Context {
c.minGasPrice = gasPrices
return c
Expand Down
91 changes: 87 additions & 4 deletions x/auth/ante/ante_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package ante_test

import (
"encoding/json"
"fmt"
"math/rand"
"strings"
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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")
}
5 changes: 5 additions & 0 deletions x/auth/ante/basic.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,18 @@ var (

// ValidateBasicDecorator will call tx.ValidateBasic and return any non-nil error.
// If ValidateBasic passes, decorator calls next AnteHandler in chain.
// This decorator will not get run on ReCheck since it is not dependent on application state
type ValidateBasicDecorator struct{}

func NewValidateBasicDecorator() ValidateBasicDecorator {
return 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
}
Expand Down
8 changes: 8 additions & 0 deletions x/auth/ante/basic_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
10 changes: 10 additions & 0 deletions x/auth/ante/sigverify.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ func (sgcd SigGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
// 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
// This decorator will not get run on ReCheck since it is not dependent on application state
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
// CONTRACT: Pubkeys are set in context for all signers before this decorator runs
// CONTRACT: Tx must implement SigVerifiableTx interface
type SigVerificationDecorator struct {
Expand All @@ -169,6 +170,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")
Expand Down Expand Up @@ -214,6 +219,7 @@ func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simul

// Increments sequences of all signers.
// Use this decorator to prevent replay attacks
// No need to run this decorator on CheckTx since it is merely updating nonce
// CONTRACT: Tx must implement SigVerifiableTx interface
type IncrementSequenceDecorator struct {
ak keeper.AccountKeeper
Expand All @@ -226,6 +232,10 @@ 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 check tx
if ctx.IsCheckTx() {
return next(ctx, tx, simulate)
}
sigTx, ok := tx.(SigVerifiableTx)
if !ok {
return ctx, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type")
Expand Down
36 changes: 31 additions & 5 deletions x/auth/ante/sigverify_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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) {
Expand Down