evm: fix panic when transaction is reverted (#650)

* 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

@@ -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.

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 {
+			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,

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()
 }

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)