From fe5fefbd8e82ce5b2529607043e9cc786be268a7 Mon Sep 17 00:00:00 2001 From: Thomas Nguy <81727899+thomas-nguy@users.noreply.github.com> Date: Fri, 8 Oct 2021 20:29:40 +0900 Subject: [PATCH] evm: fix panic when transaction is reverted (#650) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix panic when transaction is reverted * update changelog * Update x/evm/keeper/context_stack.go Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com> --- CHANGELOG.md | 3 ++- x/evm/keeper/context_stack.go | 11 ++++++----- x/evm/keeper/keeper.go | 2 +- x/evm/keeper/state_transition.go | 34 +++++++++++++++++++++----------- 4 files changed, 31 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a47cfb6..1b7f6b93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,7 +58,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes -* (rpc) [tharsis#642](https://github.com/tharsis/ethermint/issues/642) Fix `eth_getLogs` when string is specified in filter's from or to fields +* (evm) [tharsis#650](https://github.com/tharsis/ethermint/pull/650) Fix panic when flattening the cache context in case transaction is reverted. +* (rpc) [tharsis#642](https://github.com/tharsis/ethermint/issues/642) Fix `eth_getLogs` when string is specified in filter's from or to fields. * (evm) [tharsis#616](https://github.com/tharsis/ethermint/issues/616) Fix halt on deeply nested stack of cache context. Stack is now flattened before iterating over the tx logs. * (rpc, evm) [tharsis#614](https://github.com/tharsis/ethermint/issues/614) Use JSON for (un)marshaling tx `Log`s from events. * (rpc) [tharsis#611](https://github.com/tharsis/ethermint/pull/611) Fix panic on JSON-RPC when querying for an invalid block height. diff --git a/x/evm/keeper/context_stack.go b/x/evm/keeper/context_stack.go index f2a41917..f832bd1e 100644 --- a/x/evm/keeper/context_stack.go +++ b/x/evm/keeper/context_stack.go @@ -62,9 +62,9 @@ func (cs *ContextStack) Commit() { // CommitToRevision commit the cache after the target revision, // to improve efficiency of db operations. -func (cs *ContextStack) CommitToRevision(target int) { +func (cs *ContextStack) CommitToRevision(target int) error { if target < 0 || target >= len(cs.cachedContexts) { - panic(fmt.Errorf("snapshot index %d out of bound [%d..%d)", target, 0, len(cs.cachedContexts))) + return fmt.Errorf("snapshot index %d out of bound [%d..%d)", target, 0, len(cs.cachedContexts)) } targetCtx := cs.cachedContexts[target].ctx @@ -73,12 +73,13 @@ func (cs *ContextStack) CommitToRevision(target int) { // keep all the cosmos events targetCtx.EventManager().EmitEvents(cs.cachedContexts[i].ctx.EventManager().Events()) if cs.cachedContexts[i].commit == nil { - panic(fmt.Sprintf("commit function at index %d should not be nil", i)) - } else { - cs.cachedContexts[i].commit() + return fmt.Errorf("commit function at index %d should not be nil", i) } + cs.cachedContexts[i].commit() } cs.cachedContexts = cs.cachedContexts[0 : target+1] + + return nil } // Snapshot pushes a new cached context to the stack, diff --git a/x/evm/keeper/keeper.go b/x/evm/keeper/keeper.go index 1ef5fddb..08a4a5f8 100644 --- a/x/evm/keeper/keeper.go +++ b/x/evm/keeper/keeper.go @@ -98,7 +98,7 @@ func NewKeeper( } } -// Ctx returns the current context from the context stack +// Ctx returns the current context from the context stack. func (k Keeper) Ctx() sdk.Context { return k.ctxStack.CurrentContext() } diff --git a/x/evm/keeper/state_transition.go b/x/evm/keeper/state_transition.go index 07db5a53..2b695c60 100644 --- a/x/evm/keeper/state_transition.go +++ b/x/evm/keeper/state_transition.go @@ -186,7 +186,8 @@ func (k *Keeper) ApplyTransaction(tx *ethtypes.Transaction) (*types.MsgEthereumT panic("context stack shouldn't be dirty before apply message") } - var revision int + // revision is -1 for empty stack + revision := -1 if k.hooks != nil { // snapshot to contain the tx processing and post processing in same scope revision = k.Snapshot() @@ -197,15 +198,23 @@ func (k *Keeper) ApplyTransaction(tx *ethtypes.Transaction) (*types.MsgEthereumT if err != nil { return nil, stacktrace.Propagate(err, "failed to apply ethereum core message") } - - // flatten the cache contexts to improve efficiency of following db operations - // the reason is some operations under deep context stack is extremely slow, - // refer to `benchmark_test.go:BenchmarkDeepContextStack13`. - k.ctxStack.CommitToRevision(revision) - - k.IncreaseTxIndexTransient() - res.Hash = txHash.Hex() + + // The state is reverted (i.e `RevertToSnapshot`) for the VM error cases, so it's safe to call the commit here. + // NOTE: revision is >= 0 only when the EVM hooks are not empty + if revision >= 0 { + // Flatten the cache contexts to improve the efficiency of following DB operations. + // Only commit the cache layers created by the EVM contract execution + // FIXME: some operations under deep context stack are extremely slow, + // see `benchmark_test.go:BenchmarkDeepContextStack13`. + if err = k.ctxStack.CommitToRevision(revision); err != nil { + return nil, stacktrace.Propagate(err, "failed to commit ethereum core message") + } + } else { + // All cache layers are created by the EVM contract execution. So it is safe to commit them all + k.CommitCachedContexts() + } + logs := k.GetTxLogsTransient(txHash) if !res.Failed() { @@ -215,6 +224,9 @@ func (k *Keeper) ApplyTransaction(tx *ethtypes.Transaction) (*types.MsgEthereumT k.RevertToSnapshot(revision) res.VmError = types.ErrPostTxProcessing.Error() k.Logger(ctx).Error("tx post processing failed", "error", err) + } else { + // PostTxProcessing is successful, commit the leftover contexts + k.CommitCachedContexts() } } @@ -226,9 +238,7 @@ func (k *Keeper) ApplyTransaction(tx *ethtypes.Transaction) (*types.MsgEthereumT k.SetBlockBloomTransient(bloom) } - // Since we've implemented `RevertToSnapshot` api, so for the vm error cases, - // the state is reverted, so it's ok to call the commit here anyway. - k.CommitCachedContexts() + k.IncreaseTxIndexTransient() // update the gas used after refund k.resetGasMeterAndConsumeGas(res.GasUsed)