Interpreter dispatch cleanups (#2913)

* `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

@@ -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
       blkNum = blkNum - 1
 
 proc persistBlocksImpl(

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)
   else:
     if header.withdrawalsRoot.isSome:
       return err("Pre-Shanghai block header must not have withdrawalsRoot")

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

nimbus/evm/interpreter_dispatch.nim

@@ -27,7 +27,6 @@ logScope:
 
 proc runVM(
     c: VmCpt,
-    shouldPrepareTracer: bool,
     fork: static EVMFork,
     tracingEnabled: static bool,
 ): EvmResultVoid =
@@ -35,16 +34,7 @@ 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()
 
   while true:
@@ -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,18 +183,17 @@ 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)
@@ -216,20 +205,16 @@ proc executeOpcodes*(c: Computation, shouldPrepareTracer: bool = true) =
         # 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:
+      # 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()

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)
 

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,
@@ -164,17 +166,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
 
-  try:
   ctx.captureOpImpl(c, pc, op, 0, 0, [], depth, Opt.none(string))
-  except RlpError as ex:
-    error "JsonTracer captureOpStart", msg=ex.msg
 
   # make sure captureOpEnd get the right opIndex
   result = ctx.index
@@ -229,13 +225,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
 
-proc close*(ctx: JsonTracer) =
+proc close*(ctx: JsonTracer) {.raises: [IOError, OSError].} =
   ctx.stream.close()

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