From 15b6fa0959206cb5d61cca80b0b567bc771bf024 Mon Sep 17 00:00:00 2001 From: Alexander Bezobchuk Date: Fri, 16 Nov 2018 13:33:47 -0500 Subject: [PATCH] Cache-wrap context during ante handler exec (#2781) * Use cache-wrapped multi-store in ante * Implement TestBaseAppAnteHandler * Add reference documentation for BaseApp/CheckTx/DeliverTx --- PENDING.md | 7 ++- baseapp/baseapp.go | 69 ++++++++++++++------- baseapp/baseapp_test.go | 109 +++++++++++++++++++++++++++++++--- cmd/gaia/cli_test/cli_test.go | 4 ++ docs/reference/baseapp.md | 68 ++++++++++++++++++--- 5 files changed, 214 insertions(+), 43 deletions(-) diff --git a/PENDING.md b/PENDING.md index a3034c01e..6295d5572 100644 --- a/PENDING.md +++ b/PENDING.md @@ -53,9 +53,10 @@ IMPROVEMENTS * [\#2749](https://github.com/cosmos/cosmos-sdk/pull/2749) Add --chain-id flag to gaiad testnet * Gaia - - #2773 Require moniker to be provided on `gaiad init`. - - #2672 [Makefile] Updated for better Windows compatibility and ledger support logic, get_tools was rewritten as a cross-compatible Makefile. - - [#110](https://github.com/tendermint/devops/issues/110) Updated CircleCI job to trigger website build when cosmos docs are updated. + - #2772 Update BaseApp to not persist state when the ante handler fails on DeliverTx. + - #2773 Require moniker to be provided on `gaiad init`. + - #2672 [Makefile] Updated for better Windows compatibility and ledger support logic, get_tools was rewritten as a cross-compatible Makefile. + - [#110](https://github.com/tendermint/devops/issues/110) Updated CircleCI job to trigger website build when cosmos docs are updated. * SDK - [x/mock/simulation] [\#2720] major cleanup, introduction of helper objects, reorganization diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 34041dff2..0d5571024 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -10,7 +10,6 @@ import ( abci "github.com/tendermint/tendermint/abci/types" "github.com/tendermint/tendermint/crypto/tmhash" - cmn "github.com/tendermint/tendermint/libs/common" dbm "github.com/tendermint/tendermint/libs/db" "github.com/tendermint/tendermint/libs/log" @@ -505,11 +504,11 @@ func validateBasicTxMsgs(msgs []sdk.Msg) sdk.Error { // retrieve the context for the ante handler and store the tx bytes; store // the vote infos if the tx runs within the deliverTx() state. func (app *BaseApp) getContextForAnte(mode runTxMode, txBytes []byte) (ctx sdk.Context) { - // Get the context - ctx = getState(app, mode).ctx.WithTxBytes(txBytes) + ctx = app.getState(mode).ctx.WithTxBytes(txBytes) if mode == runTxModeDeliver { ctx = ctx.WithVoteInfos(app.voteInfos) } + return } @@ -571,7 +570,7 @@ func (app *BaseApp) runMsgs(ctx sdk.Context, msgs []sdk.Msg, mode runTxMode) (re // Returns the applicantion's deliverState if app is in runTxModeDeliver, // otherwise it returns the application's checkstate. -func getState(app *BaseApp, mode runTxMode) *state { +func (app *BaseApp) getState(mode runTxMode) *state { if mode == runTxModeCheck || mode == runTxModeSimulate { return app.checkState } @@ -581,20 +580,42 @@ func getState(app *BaseApp, mode runTxMode) *state { func (app *BaseApp) initializeContext(ctx sdk.Context, mode runTxMode) sdk.Context { if mode == runTxModeSimulate { - ctx = ctx.WithMultiStore(getState(app, runTxModeSimulate).CacheMultiStore()) + ctx = ctx.WithMultiStore(app.getState(runTxModeSimulate).CacheMultiStore()) } return ctx } +// cacheTxContext returns a new context based off of the provided context with a +// cache wrapped multi-store and the store itself to allow the caller to write +// changes from the cached multi-store. +func (app *BaseApp) cacheTxContext( + ctx sdk.Context, txBytes []byte, mode runTxMode, +) (sdk.Context, sdk.CacheMultiStore) { + + msCache := app.getState(mode).CacheMultiStore() + if msCache.TracingEnabled() { + msCache = msCache.WithTracingContext( + sdk.TraceContext( + map[string]interface{}{ + "txHash": fmt.Sprintf("%X", tmhash.Sum(txBytes)), + }, + ), + ).(sdk.CacheMultiStore) + } + + return ctx.WithMultiStore(msCache), msCache +} + // runTx processes a transaction. The transactions is proccessed via an -// anteHandler. txBytes may be nil in some cases, eg. in tests. Also, in the -// future we may support "internal" transactions. +// anteHandler. The provided txBytes may be nil in some cases, eg. in tests. For +// further details on transaction execution, reference the BaseApp SDK +// documentation. func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk.Result) { // 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. var gasWanted int64 - var msCache sdk.CacheMultiStore + ctx := app.getContextForAnte(mode, txBytes) ctx = app.initializeContext(ctx, mode) @@ -619,16 +640,27 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk return err.Result() } - // run the ante handler + // Execute the ante handler if one is defined. if app.anteHandler != nil { - newCtx, result, abort := app.anteHandler(ctx, tx, (mode == runTxModeSimulate)) + var anteCtx sdk.Context + var msCache sdk.CacheMultiStore + + // Cache wrap context before anteHandler call in case it aborts. + // This is required for both CheckTx and DeliverTx. + // https://github.com/cosmos/cosmos-sdk/issues/2772 + // NOTE: Alternatively, we could require that anteHandler ensures that + // writes do not happen if aborted/failed. This may have some + // performance benefits, but it'll be more difficult to get right. + anteCtx, msCache = app.cacheTxContext(ctx, txBytes, mode) + + newCtx, result, abort := app.anteHandler(anteCtx, tx, (mode == runTxModeSimulate)) if abort { return result } if !newCtx.IsZero() { ctx = newCtx } - + msCache.Write() gasWanted = result.GasWanted } @@ -638,17 +670,10 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk return } - // Keep the state in a transient CacheWrap in case processing the messages - // fails. - msCache = getState(app, mode).CacheMultiStore() - if msCache.TracingEnabled() { - msCache = msCache.WithTracingContext(sdk.TraceContext( - map[string]interface{}{"txHash": cmn.HexBytes(tmhash.Sum(txBytes)).String()}, - )).(sdk.CacheMultiStore) - } - - ctx = ctx.WithMultiStore(msCache) - result = app.runMsgs(ctx, msgs, mode) + // Create a new context based off of the existing context with a cache wrapped + // multi-store in case message processing fails. + runMsgCtx, msCache := app.cacheTxContext(ctx, txBytes, mode) + result = app.runMsgs(runMsgCtx, msgs, mode) result.GasWanted = gasWanted // only update state if all messages pass diff --git a/baseapp/baseapp_test.go b/baseapp/baseapp_test.go index 35f9fa424..9c2e7e1a0 100644 --- a/baseapp/baseapp_test.go +++ b/baseapp/baseapp_test.go @@ -282,8 +282,19 @@ func TestInitChainer(t *testing.T) { // Simple tx with a list of Msgs. type txTest struct { - Msgs []sdk.Msg - Counter int64 + Msgs []sdk.Msg + Counter int64 + FailOnAnte bool +} + +func (tx *txTest) setFailOnAnte(fail bool) { + tx.FailOnAnte = fail +} + +func (tx *txTest) setFailOnHandler(fail bool) { + for i, msg := range tx.Msgs { + tx.Msgs[i] = msgCounter{msg.(msgCounter).Counter, fail} + } } // Implements Tx @@ -297,7 +308,8 @@ const ( // ValidateBasic() fails on negative counters. // Otherwise it's up to the handlers type msgCounter struct { - Counter int64 + Counter int64 + FailOnHandler bool } // Implements Msg @@ -315,9 +327,9 @@ func (msg msgCounter) ValidateBasic() sdk.Error { func newTxCounter(txInt int64, msgInts ...int64) *txTest { var msgs []sdk.Msg for _, msgInt := range msgInts { - msgs = append(msgs, msgCounter{msgInt}) + msgs = append(msgs, msgCounter{msgInt, false}) } - return &txTest{msgs, txInt} + return &txTest{msgs, txInt, false} } // a msg we dont know how to route @@ -369,8 +381,13 @@ func testTxDecoder(cdc *codec.Codec) sdk.TxDecoder { func anteHandlerTxTest(t *testing.T, capKey *sdk.KVStoreKey, storeKey []byte) sdk.AnteHandler { return func(ctx sdk.Context, tx sdk.Tx, simulate bool) (newCtx sdk.Context, res sdk.Result, abort bool) { store := ctx.KVStore(capKey) - msgCounter := tx.(txTest).Counter - res = incrementingCounter(t, store, storeKey, msgCounter) + txTest := tx.(txTest) + + if txTest.FailOnAnte { + return newCtx, sdk.ErrInternal("ante handler failure").Result(), true + } + + res = incrementingCounter(t, store, storeKey, txTest.Counter) return } } @@ -381,10 +398,15 @@ func handlerMsgCounter(t *testing.T, capKey *sdk.KVStoreKey, deliverKey []byte) var msgCount int64 switch m := msg.(type) { case *msgCounter: + if m.FailOnHandler { + return sdk.ErrInternal("message handler failure").Result() + } + msgCount = m.Counter case *msgCounter2: msgCount = m.Counter } + return incrementingCounter(t, store, deliverKey, msgCount) } } @@ -712,12 +734,12 @@ func TestRunInvalidTransaction(t *testing.T) { // Transaction with no known route { - unknownRouteTx := txTest{[]sdk.Msg{msgNoRoute{}}, 0} + unknownRouteTx := txTest{[]sdk.Msg{msgNoRoute{}}, 0, false} err := app.Deliver(unknownRouteTx) require.EqualValues(t, sdk.CodeUnknownRequest, err.Code) require.EqualValues(t, sdk.CodespaceRoot, err.Codespace) - unknownRouteTx = txTest{[]sdk.Msg{msgCounter{}, msgNoRoute{}}, 0} + unknownRouteTx = txTest{[]sdk.Msg{msgCounter{}, msgNoRoute{}}, 0, false} err = app.Deliver(unknownRouteTx) require.EqualValues(t, sdk.CodeUnknownRequest, err.Code) require.EqualValues(t, sdk.CodespaceRoot, err.Codespace) @@ -829,3 +851,72 @@ func TestTxGasLimits(t *testing.T) { } } } + +func TestBaseAppAnteHandler(t *testing.T) { + anteKey := []byte("ante-key") + anteOpt := func(bapp *BaseApp) { + bapp.SetAnteHandler(anteHandlerTxTest(t, capKey1, anteKey)) + } + + deliverKey := []byte("deliver-key") + routerOpt := func(bapp *BaseApp) { + bapp.Router().AddRoute(routeMsgCounter, handlerMsgCounter(t, capKey1, deliverKey)) + } + + cdc := codec.New() + app := setupBaseApp(t, anteOpt, routerOpt) + + app.InitChain(abci.RequestInitChain{}) + registerTestCodec(cdc) + app.BeginBlock(abci.RequestBeginBlock{}) + + // execute a tx that will fail ante handler execution + // + // NOTE: State should not be mutated here. This will be implicitly checked by + // the next txs ante handler execution (anteHandlerTxTest). + tx := newTxCounter(0, 0) + tx.setFailOnAnte(true) + txBytes, err := cdc.MarshalBinaryLengthPrefixed(tx) + require.NoError(t, err) + res := app.DeliverTx(txBytes) + require.False(t, res.IsOK(), fmt.Sprintf("%v", res)) + + ctx := app.getState(runTxModeDeliver).ctx + store := ctx.KVStore(capKey1) + require.Equal(t, int64(0), getIntFromStore(store, anteKey)) + + // execute at tx that will pass the ante handler (the checkTx state should + // mutate) but will fail the message handler + tx = newTxCounter(0, 0) + tx.setFailOnHandler(true) + + txBytes, err = cdc.MarshalBinaryLengthPrefixed(tx) + require.NoError(t, err) + + res = app.DeliverTx(txBytes) + require.False(t, res.IsOK(), fmt.Sprintf("%v", res)) + + ctx = app.getState(runTxModeDeliver).ctx + store = ctx.KVStore(capKey1) + require.Equal(t, int64(1), getIntFromStore(store, anteKey)) + require.Equal(t, int64(0), getIntFromStore(store, deliverKey)) + + // execute a successful ante handler and message execution where state is + // implicitly checked by previous tx executions + tx = newTxCounter(1, 0) + + txBytes, err = cdc.MarshalBinaryLengthPrefixed(tx) + require.NoError(t, err) + + res = app.DeliverTx(txBytes) + require.True(t, res.IsOK(), fmt.Sprintf("%v", res)) + + ctx = app.getState(runTxModeDeliver).ctx + store = ctx.KVStore(capKey1) + require.Equal(t, int64(2), getIntFromStore(store, anteKey)) + require.Equal(t, int64(1), getIntFromStore(store, deliverKey)) + + // commit + app.EndBlock(abci.RequestEndBlock{}) + app.Commit() +} diff --git a/cmd/gaia/cli_test/cli_test.go b/cmd/gaia/cli_test/cli_test.go index 874c2a29a..62f4d0d64 100644 --- a/cmd/gaia/cli_test/cli_test.go +++ b/cmd/gaia/cli_test/cli_test.go @@ -537,16 +537,20 @@ func TestGaiaCLISendGenerateSignAndBroadcast(t *testing.T) { success, stdout, _ = executeWriteRetStdStreams(t, fmt.Sprintf( "gaiacli tx broadcast %v --json %v", flags, signedTxFile.Name())) require.True(t, success) + var result struct { Response abci.ResponseDeliverTx } + require.Nil(t, app.MakeCodec().UnmarshalJSON([]byte(stdout), &result)) require.Equal(t, msg.Fee.Gas, result.Response.GasUsed) require.Equal(t, msg.Fee.Gas, result.Response.GasWanted) + tests.WaitForNextNBlocksTM(2, port) barAcc := executeGetAccount(t, fmt.Sprintf("gaiacli query account %s %v", barAddr, flags)) require.Equal(t, int64(10), barAcc.GetCoins().AmountOf(stakeTypes.DefaultBondDenom).Int64()) + fooAcc = executeGetAccount(t, fmt.Sprintf("gaiacli query account %s %v", fooAddr, flags)) require.Equal(t, int64(40), fooAcc.GetCoins().AmountOf(stakeTypes.DefaultBondDenom).Int64()) } diff --git a/docs/reference/baseapp.md b/docs/reference/baseapp.md index 1828021c2..e1a80e293 100644 --- a/docs/reference/baseapp.md +++ b/docs/reference/baseapp.md @@ -1,13 +1,63 @@ -# baseApp +# BaseApp -`baseApp` requires stores to be mounted via capabilities keys - handlers can only access stores they're given the key to. The `baseApp` ensures all stores are properly loaded, cached, and committed. One mounted store is considered the "main" - it holds the latest block header, from which we can find and load the most recent state. +The BaseApp defines the foundational implementation for a basic ABCI application +so that your Cosmos-SDK application can communicate with an underlying +Tendermint node. -`baseApp` distinguishes between two handler types: `AnteHandler` and `MsgHandler`. Whilst the former is a global validity check that applies to all transactions from all modules, i.e. it checks nonces and whether balances are sufficient to pay fees, validates signatures and ensures that transactions don't carry too many signatures, the latter is the full state transition function. -During CheckTx the state transition function is only applied to the checkTxState and should return -before any expensive state transitions are run (this is up to each developer). It also needs to return the estimated -gas cost. +The BaseApp is composed of many internal components. Some of the most important +include the `CommitMultiStore` and its internal state. The internal state is +essentially two sub-states, both of which are used for transaction execution +during different phases, `CheckTx` and `DeliverTx` respectively. During block +commitment, only the `DeliverTx` is persisted. -During DeliverTx the state transition function is applied to the blockchain state and the transactions -need to be fully executed. +The BaseApp requires stores to be mounted via capabilities keys - handlers can +only access stores they're given the key to. The `baseApp` ensures all stores are +properly loaded, cached, and committed. One mounted store is considered the +"main" - it holds the latest block header, from which we can find and load the +most recent state. -BaseApp is responsible for managing the context passed into handlers - it makes the block header available and provides the right stores for CheckTx and DeliverTx. BaseApp is completely agnostic to serialization formats. \ No newline at end of file +The BaseApp distinguishes between two handler types - the `AnteHandler` and the +`MsgHandler`. The former is a global validity check (checking nonces, sigs and +sufficient balances to pay fees, e.g. things that apply to all transaction from +all modules), the later is the full state transition function. + +During `CheckTx` the state transition function is only applied to the `checkTxState` +and should return before any expensive state transitions are run +(this is up to each developer). It also needs to return the estimated gas cost. + +During `DeliverTx` the state transition function is applied to the blockchain +state and the transactions need to be fully executed. + +The BaseApp is responsible for managing the context passed into handlers - +it makes the block header available and provides the right stores for `CheckTx` +and `DeliverTx`. BaseApp is completely agnostic to serialization formats. + +## Transaction Life Cycle + +During the execution of a transaction, it may pass through both `CheckTx` and +`DeliverTx` as defined in the ABCI specification. `CheckTx` is executed by the +proposing validator and is used for the Tendermint mempool for all full nodes. + +Both `CheckTx` and `DeliverTx` execute the application's AnteHandler (if +defined), where the AnteHandler is responsible for pre-message validation +checks such as account and signature validation, fee deduction and collection, +and incrementing sequence numbers. + +### CheckTx + +During the execution of `CheckTx`, only the AnteHandler is executed. + +State transitions due to the AnteHandler are persisted between subsequent calls +of `CheckTx` in the check-tx state, unless the AnteHandler fails and aborts. + +### DeliverTx + +During the execution of `DeliverTx`, the AnteHandler and Handler is executed. + +The transaction execution during `DeliverTx` operates in a similar fashion to +`CheckTx`. However, state transitions that occur during the AnteHandler are +persisted even when the following Handler processing logic fails. + +It is possible that a malicious proposer may include a transaction in a block +that fails the AnteHandler. In this case, all state transitions for the +offending transaction are discarded.