From 46c80509770cf42b60b56dd17d16c6b2a8210823 Mon Sep 17 00:00:00 2001 From: Jacek Sieka Date: Thu, 5 Dec 2024 11:49:10 +0100 Subject: [PATCH] Interpreter dispatch cleanups * `shouldPrepareTracer` always true * simple `pop` should not copy value (reading the memory shows up in a profiler) * continuation code simplified * remove some unnecessary EH --- nimbus/core/chain/persist_blocks.nim | 7 +- nimbus/core/withdrawals.nim | 7 +- .../interpreter/op_handlers/oph_memory.nim | 4 +- nimbus/evm/interpreter_dispatch.nim | 65 +++++++------------ nimbus/evm/stack.nim | 5 ++ nimbus/evm/tracer/json_tracer.nim | 33 ++++------ tests/test_evm_support.nim | 8 +++ 7 files changed, 55 insertions(+), 74 deletions(-) diff --git a/nimbus/core/chain/persist_blocks.nim b/nimbus/core/chain/persist_blocks.nim index 4d69511d6..179c841b1 100644 --- a/nimbus/core/chain/persist_blocks.nim +++ b/nimbus/core/chain/persist_blocks.nim @@ -70,11 +70,8 @@ proc purgeOlderBlocksFromHistory(db: CoreDbRef, bn: BlockNumber) = if 0 < bn: var blkNum = bn - 1 while 0 < blkNum: - try: - if not db.forgetHistory blkNum: - break - except RlpError as exc: - warn "Error forgetting history", err = exc.msg + if not db.forgetHistory blkNum: + break blkNum = blkNum - 1 proc persistBlocksImpl( diff --git a/nimbus/core/withdrawals.nim b/nimbus/core/withdrawals.nim index 9184ad6c6..56c4db4f8 100644 --- a/nimbus/core/withdrawals.nim +++ b/nimbus/core/withdrawals.nim @@ -22,11 +22,8 @@ proc validateWithdrawals*( elif withdrawals.isNone: return err("Post-Shanghai block body must have withdrawals") else: - try: - if withdrawals.get.calcWithdrawalsRoot != header.withdrawalsRoot.get: - return err("Mismatched withdrawalsRoot blockNumber =" & $header.number) - except RlpError as ex: - return err(ex.msg) + if withdrawals.get.calcWithdrawalsRoot != header.withdrawalsRoot.get: + return err("Mismatched withdrawalsRoot blockNumber =" & $header.number) else: if header.withdrawalsRoot.isSome: return err("Pre-Shanghai block header must not have withdrawalsRoot") diff --git a/nimbus/evm/interpreter/op_handlers/oph_memory.nim b/nimbus/evm/interpreter/op_handlers/oph_memory.nim index ae0364d08..fbc80f2ba 100644 --- a/nimbus/evm/interpreter/op_handlers/oph_memory.nim +++ b/nimbus/evm/interpreter/op_handlers/oph_memory.nim @@ -122,9 +122,7 @@ func jumpImpl(c: Computation; jumpTarget: UInt256): EvmResultVoid = proc popOp(cpt: VmCpt): EvmResultVoid = ## 0x50, Remove item from stack. - cpt.stack.popInt.isOkOr: - return err(error) - ok() + cpt.stack.pop() proc mloadOp(cpt: VmCpt): EvmResultVoid = ## 0x51, Load word from memory diff --git a/nimbus/evm/interpreter_dispatch.nim b/nimbus/evm/interpreter_dispatch.nim index b99f5e8c9..b0ccd06a3 100644 --- a/nimbus/evm/interpreter_dispatch.nim +++ b/nimbus/evm/interpreter_dispatch.nim @@ -27,7 +27,6 @@ logScope: proc runVM( c: VmCpt, - shouldPrepareTracer: bool, fork: static EVMFork, tracingEnabled: static bool, ): EvmResultVoid = @@ -35,17 +34,8 @@ proc runVM( ## this function is instantiated so that selection of fork-specific ## versions of functions happens only once - # It's important not to re-prepare the tracer after - # an async operation, only after a call/create. - # - # That is, tracingEnabled is checked in many places, and - # indicates something like, "Do we want tracing to be - # enabled?", whereas shouldPrepareTracer is more like, - # "Are we at a spot right now where we want to re-initialize - # the tracer?" when tracingEnabled: - if shouldPrepareTracer: - c.prepareTracer() + c.prepareTracer() while true: {.computedGoto.} @@ -55,7 +45,7 @@ proc runVM( ok() -macro selectVM(v: VmCpt, shouldPrepareTracer: bool, fork: EVMFork, tracingEnabled: bool): EvmResultVoid = +macro selectVM(v: VmCpt, fork: EVMFork, tracingEnabled: bool): EvmResultVoid = # Generate opcode dispatcher that calls selectVM with a literal for each fork: # # case fork @@ -69,8 +59,8 @@ macro selectVM(v: VmCpt, shouldPrepareTracer: bool, fork: EVMFork, tracingEnable `fork` call = quote: case `tracingEnabled` - of false: runVM(`v`, `shouldPrepareTracer`, `forkVal`, `false`) - of true: runVM(`v`, `shouldPrepareTracer`, `forkVal`, `true`) + of false: runVM(`v`, `fork`, false) + of true: runVM(`v`, `fork`, true) caseStmt.add nnkOfBranch.newTree(forkVal, call) caseStmt @@ -193,43 +183,38 @@ template handleEvmError(x: EvmErrorObj) = depth = $(c.msg.depth + 1) # plus one to match tracer depth, and avoid confusion c.setError("Opcode Dispatch Error: " & msg & ", depth=" & depth, true) -proc executeOpcodes*(c: Computation, shouldPrepareTracer: bool = true) = +proc executeOpcodes*(c: Computation) = let fork = c.fork block blockOne: - if c.continuation.isNil: + let cont = c.continuation + if cont.isNil: let precompile = c.fork.getPrecompile(c.msg.codeAddress) if precompile.isSome: c.execPrecompile(precompile[]) break blockOne - - let cont = c.continuation - if not cont.isNil: + else: c.continuation = nil cont().isOkOr: handleEvmError(error) break blockOne - let nextCont = c.continuation - if not nextCont.isNil: - # Return up to the caller, which will run the child - # and then call this proc again. - break blockOne + let nextCont = c.continuation + if not nextCont.isNil: + # Return up to the caller, which will run the child + # and then call this proc again. + break blockOne - # FIXME-Adam: I hate how convoluted this is. See also the comment in - # op_dispatcher.nim. The idea here is that we need to call - # traceOpCodeEnded at the end of the opcode (and only if there - # hasn't been an exception thrown); otherwise we run into problems - # if an exception (e.g. out of gas) is thrown during a continuation. - # So this code says, "If we've just run a continuation, but there's - # no *subsequent* continuation, then the opcode is done." - if c.tracingEnabled and not (cont.isNil) and nextCont.isNil: - c.traceOpCodeEnded(c.instr, c.opIndex) + # traceOpCodeEnded is normally called directly after opcode execution + # but in the case that a continuation is created, it must run after that + # continuation has finished + if c.tracingEnabled: + c.traceOpCodeEnded(c.instr, c.opIndex) if c.instr == Return or c.instr == Revert or c.instr == SelfDestruct: break blockOne - c.selectVM(shouldPrepareTracer, fork, c.tracingEnabled).isOkOr: + c.selectVM(fork, c.tracingEnabled).isOkOr: handleEvmError(error) break blockOne # this break is not needed but make the flow clear @@ -256,24 +241,24 @@ when vm_use_recursion: else: proc execCallOrCreate*(cParam: Computation) = - var (c, before, shouldPrepareTracer) = (cParam, true, true) + var (c, before) = (cParam, true) # No actual recursion, but simulate recursion including before/after/dispose. while true: while true: if before and c.beforeExec(): break - c.executeOpcodes(shouldPrepareTracer) + c.executeOpcodes() if c.continuation.isNil: c.afterExec() break - (before, shouldPrepareTracer, c.child, c, c.parent) = - (true, true, nil.Computation, c.child, c) + (before, c.child, c, c.parent) = + (true, nil.Computation, c.child, c) if c.parent.isNil: break c.dispose() - (before, shouldPrepareTracer, c.parent, c) = - (false, true, nil.Computation, c.parent) + (before, c.parent, c) = + (false, nil.Computation, c.parent) while not c.isNil: c.dispose() diff --git a/nimbus/evm/stack.nim b/nimbus/evm/stack.nim index 52e8a46c7..b476f59f6 100644 --- a/nimbus/evm/stack.nim +++ b/nimbus/evm/stack.nim @@ -132,6 +132,11 @@ func popInt*(stack: EvmStack): EvmResult[UInt256] = func popAddress*(stack: EvmStack): EvmResult[Address] = popAux(stack, Address) +func pop*(stack: EvmStack): EvmResult[void] = + ? ensurePop(stack, 1) + stack.len -= 1 + ok() + proc init*(_: type EvmStack): EvmStack = let memory = c_malloc(evmStackSize * sizeof(EvmStackElement) + 31) diff --git a/nimbus/evm/tracer/json_tracer.nim b/nimbus/evm/tracer/json_tracer.nim index a85ade93f..c4363dfff 100644 --- a/nimbus/evm/tracer/json_tracer.nim +++ b/nimbus/evm/tracer/json_tracer.nim @@ -8,6 +8,8 @@ # at your option. This file may not be copied, modified, or distributed except # according to those terms. +{.push raises: [].} + import std/[json, sets, streams, strutils], eth/common/eth_types, @@ -24,7 +26,6 @@ type stream: Stream pretty: bool gas: GasInt - pc: int stack: JsonNode storageKeys: seq[HashSet[UInt256]] index: int @@ -75,7 +76,7 @@ proc captureOpImpl(ctx: JsonTracer, c: Computation, pc: int, gasCost = ctx.gas - gas var res = %{ - "pc": %(ctx.pc), + "pc": %(pc), "op": %(op.int), "gas": encodeHex(ctx.gas), "gasCost": encodeHex(gasCost), @@ -156,7 +157,6 @@ method captureOpStart*(ctx: JsonTracer, c: Computation, fixed: bool, pc: int, op: Op, gas: GasInt, depth: int): int {.gcsafe.} = ctx.gas = gas - ctx.pc = pc if TracerFlags.DisableStack notin ctx.flags: ctx.stack = newJArray() @@ -164,17 +164,11 @@ method captureOpStart*(ctx: JsonTracer, c: Computation, ctx.stack.add(%(v.encodeHex)) if TracerFlags.DisableStorage notin ctx.flags and op == Sstore: - try: - if c.stack.len > 1: - ctx.rememberStorageKey(c.msg.depth, - c.stack[^1, UInt256].expect("stack constains more than 2 elements")) - except ValueError as ex: - error "JsonTracer captureOpStart", msg=ex.msg + if c.stack.len > 1: + ctx.rememberStorageKey(c.msg.depth, + c.stack[^1, UInt256].expect("stack constains more than 2 elements")) - try: - ctx.captureOpImpl(c, pc, op, 0, 0, [], depth, Opt.none(string)) - except RlpError as ex: - error "JsonTracer captureOpStart", msg=ex.msg + ctx.captureOpImpl(c, pc, op, 0, 0, [], depth, Opt.none(string)) # make sure captureOpEnd get the right opIndex result = ctx.index @@ -229,13 +223,10 @@ method captureFault*(ctx: JsonTracer, comp: Computation, ctx.node = nil return - try: - ctx.captureOpImpl(comp, pc, op, gas, refund, rData, depth, error) - doAssert(ctx.node.isNil.not) - ctx.writeJson(ctx.node) - ctx.node = nil - except RlpError as ex: - error "JsonTracer captureOpFault", msg=ex.msg + ctx.captureOpImpl(comp, pc, op, gas, refund, rData, depth, error) + doAssert(ctx.node.isNil.not) + ctx.writeJson(ctx.node) + ctx.node = nil -proc close*(ctx: JsonTracer) = +proc close*(ctx: JsonTracer) {.raises: [IOError, OSError].} = ctx.stream.close() diff --git a/tests/test_evm_support.nim b/tests/test_evm_support.nim index 3142a148b..1c3a2e0da 100644 --- a/tests/test_evm_support.nim +++ b/tests/test_evm_support.nim @@ -61,6 +61,14 @@ proc runStackTests() = check stack.push(element).isOk check(stack.popInt.get == 3.u256) + test "pop requires stack item": + var stack = EvmStack.init() + defer: stack.dispose() + check: + stack.pop().isErr() + stack.push(1'u).isOk() + stack.pop().isOk() + test "swap correct": privateAccess(EvmStack) var stack = EvmStack.init()