More trace fixes (#386)

* pop flat trace context when handling halts
* Better detection of precompiled and non-executed contracts
* correct from address when calling in init code
* fix some exotic nesting cases
* correct from field for init code calls at depth >1
* correct cost on a non-call
* changelog and notes

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

CHANGELOG.md

@@ -1,5 +1,13 @@
 # Changelog
 
+## 1.4 RC 
+
+### Additions and Improvements 
+
+- New`trace_replayBlockTransactions` JSON-RPC API
+
+This can be enabled using the `--rpc-http-api TRACE` CLI flag.  There are some philosophical differences between Besu and other implementations that are outlined in the `[trace_rpc_apis.md](./docs/trace_rpc_apis.md)` documentation.
+
 ## 1.4 Beta 3 
 
 ### Additions and Improvements 

docs/trace_rpc_apis.md

@@ -0,0 +1,39 @@
+# Trace RPC API Notes
+
+This document outlines major differences for `trace_replayBlockTransactions` 
+compared to other implementations.
+
+## `stateDiff` 
+
+No major differences were observed in the `stateDiff` field.
+
+## `trace`
+
+Besu reports `gasUsed` after applying the effects of gas refunds.  Future
+implementations of Besu might track gas refunds separately.
+
+## `vmTrace`
+
+### Returned Memory from Calls
+
+In the `vmTrace` `ope.ex.mem` fields Besu only reports actual data returned
+from a `RETURN` opcode.  Other implementations return the contents of the 
+reserved output space for the call operations.  Note two major differences:
+
+1. Besu reports `null` when a call operation ends because of a `STOP`,  `HALT`, 
+   `REVERT`, running out of instructions, or any exceptional halts.
+2. When a `RETURN` operation returns data of a different length than the space
+   reserved by the call only the data passed to the `RETURN` operation is 
+   reported.  Other implementations will include pre-existing memory data or 
+   trim the returned data.
+
+### Precompiled Contracts Calls
+
+Besu reports only the actual cost of the precompiled contract call in the 
+`cost` field. 
+
+### Out of Gas 
+
+Besu reports the operation that causes out fo gas exceptions, including 
+calculated gas cost.  The operation is not executed so no `ex` values are 
+reported.

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/results/tracing/flat/FlatTrace.java

@@ -187,6 +187,10 @@ public class FlatTrace implements Trace {
       return this;
     }
 
+    public String getType() {
+      return type;
+    }
+
     public Builder error(final Optional<String> error) {
       this.error = error;
       return this;

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/results/tracing/flat/FlatTraceGenerator.java

@@ -18,7 +18,6 @@ import org.hyperledger.besu.ethereum.api.jsonrpc.internal.processor.TransactionT
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.results.Quantity;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.results.tracing.Trace;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.results.tracing.TracingUtils;
-import org.hyperledger.besu.ethereum.api.jsonrpc.internal.results.tracing.flat.FlatTrace.Context;
 import org.hyperledger.besu.ethereum.core.Address;
 import org.hyperledger.besu.ethereum.core.Gas;
 import org.hyperledger.besu.ethereum.core.Transaction;
@@ -30,6 +29,7 @@ import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Deque;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Locale;
 import java.util.Optional;
@@ -98,9 +98,12 @@ public class FlatTraceGenerator {
     // declare the first transactionTrace context as the previous transactionTrace context
     long cumulativeGasCost = 0;
 
-    int traceFrameIndex = 0;
+    final Iterator<TraceFrame> iter = transactionTrace.getTraceFrames().iterator();
-    final List<TraceFrame> traceFrames = transactionTrace.getTraceFrames();
+    Optional<TraceFrame> nextTraceFrame =
-    for (final TraceFrame traceFrame : traceFrames) {
+        iter.hasNext() ? Optional.of(iter.next()) : Optional.empty();
+    while (nextTraceFrame.isPresent()) {
+      final TraceFrame traceFrame = nextTraceFrame.get();
+      nextTraceFrame = iter.hasNext() ? Optional.of(iter.next()) : Optional.empty();
       cumulativeGasCost +=
           traceFrame.getGasCost().orElse(Gas.ZERO).toLong()
               + traceFrame.getPrecompiledGasCost().orElse(Gas.ZERO).toLong();
@@ -113,12 +116,10 @@ public class FlatTraceGenerator {
             handleCall(
                 transactionTrace,
                 traceFrame,
-                smartContractAddress,
+                nextTraceFrame,
                 flatTraces,
                 cumulativeGasCost,
                 tracesContexts,
-                traceFrameIndex,
-                traceFrames,
                 opcodeString.toLowerCase(Locale.US));
       } else if ("RETURN".equals(opcodeString) || "STOP".equals(opcodeString)) {
         if (currentContext != null) {
@@ -136,19 +137,12 @@ public class FlatTraceGenerator {
                 flatTraces,
                 tracesContexts,
                 cumulativeGasCost,
-                traceFrameIndex,
+                nextTraceFrame);
-                traceFrames);
       } else if ("REVERT".equals(opcodeString)) {
         currentContext = handleRevert(tracesContexts, currentContext);
       } else if (!traceFrame.getExceptionalHaltReasons().isEmpty()) {
-        currentContext
+        currentContext = handleHalt(tracesContexts, currentContext, traceFrame);
-            .getBuilder()
-            .error(
-                traceFrame.getExceptionalHaltReasons().stream()
-                    .map(ExceptionalHaltReason::getDescription)
-                    .reduce((a, b) -> a + ", " + b));
       }
-      traceFrameIndex++;
     }
 
     return flatTraces.stream().map(FlatTrace.Builder::build);
@@ -157,21 +151,18 @@ public class FlatTraceGenerator {
   private static FlatTrace.Context handleCall(
       final TransactionTrace transactionTrace,
       final TraceFrame traceFrame,
-      final Optional<String> smartContractAddress,
+      final Optional<TraceFrame> nextTraceFrame,
       final List<FlatTrace.Builder> flatTraces,
       final long cumulativeGasCost,
       final Deque<FlatTrace.Context> tracesContexts,
-      final int traceFrameIndex,
-      final List<TraceFrame> traceFrames,
       final String opcodeString) {
-    final TraceFrame nextTraceFrame = traceFrames.get(traceFrameIndex + 1);
     final Bytes32[] stack = traceFrame.getStack().orElseThrow();
     final Address contractCallAddress = toAddress(stack[stack.length - 2]);
     final FlatTrace.Context lastContext = tracesContexts.peekLast();
     final String callingAddress = calculateCallingAddress(lastContext);
 
-    if (contractCallAddress.numberOfLeadingZeroBytes() >= 19) {
+    if (traceFrame.getDepth() >= nextTraceFrame.map(TraceFrame::getDepth).orElse(0)) {
-      // don't log calls to precompiles
+      // don't log calls to calls that don't execute, such as insufficient value and precompiles
       return tracesContexts.peekLast();
     }
 
@@ -181,13 +172,11 @@ public class FlatTraceGenerator {
             .resultBuilder(Result.builder());
     final Action.Builder subTraceActionBuilder =
         Action.builder()
-            .from(smartContractAddress.orElse(callingAddress))
+            .from(callingAddress)
             .to(contractCallAddress.toString())
             .input(
-                Optional.ofNullable(nextTraceFrame.getInputData())
+                nextTraceFrame.map(TraceFrame::getInputData).map(Bytes::toHexString).orElse(null))
-                    .map(Bytes::toHexString)
+            .gas(nextTraceFrame.map(TraceFrame::getGasRemaining).orElse(Gas.ZERO).toHexString())
-                    .orElse(null))
-            .gas(nextTraceFrame.getGasRemaining().toHexString())
             .callType(opcodeString.toLowerCase(Locale.US))
             .value(Quantity.create(transactionTrace.getTransaction().getValue()));
 
@@ -218,7 +207,7 @@ public class FlatTraceGenerator {
     // set value for contract creation TXes, CREATE, and CREATE2
     if (actionBuilder.getCallType() == null && traceFrame.getMaybeCode().isPresent()) {
       actionBuilder.init(traceFrame.getMaybeCode().get().getBytes().toHexString());
-      resultBuilder.code(outputData.toHexString()).address(traceFrame.getRecipient().toHexString());
+      resultBuilder.code(outputData.toHexString());
       if (currentContext.isCreateOp()) {
         // this is from a CREATE/CREATE2, so add code deposit cost.
         currentContext.incGasUsed(outputData.size() * 200L);
@@ -275,9 +264,7 @@ public class FlatTraceGenerator {
       final List<FlatTrace.Builder> flatTraces,
       final Deque<FlatTrace.Context> tracesContexts,
       final long cumulativeGasCost,
-      final int traceFrameIndex,
+      final Optional<TraceFrame> nextTraceFrame) {
-      final List<TraceFrame> traceFrames) {
-    final TraceFrame nextTraceFrame = traceFrames.get(traceFrameIndex + 1);
     final FlatTrace.Context lastContext = tracesContexts.peekLast();
     final String callingAddress = calculateCallingAddress(lastContext);
 
@@ -289,11 +276,15 @@ public class FlatTraceGenerator {
     final Action.Builder subTraceActionBuilder =
         Action.builder()
             .from(smartContractAddress.orElse(callingAddress))
-            .gas(nextTraceFrame.getGasRemaining().toHexString())
+            .gas(nextTraceFrame.map(TraceFrame::getGasRemaining).orElse(Gas.ZERO).toHexString())
-            .value(Quantity.create(nextTraceFrame.getValue()));
+            .value(Quantity.create(nextTraceFrame.map(TraceFrame::getValue).orElse(Wei.ZERO)));
 
     final FlatTrace.Context currentContext =
         new FlatTrace.Context(subTraceBuilder.actionBuilder(subTraceActionBuilder));
+    currentContext
+        .getBuilder()
+        .getResultBuilder()
+        .address(nextTraceFrame.map(TraceFrame::getRecipient).orElse(Address.ZERO).toHexString());
     currentContext.setCreateOp(true);
     currentContext.decGasUsed(cumulativeGasCost);
     tracesContexts.addLast(currentContext);
@@ -301,9 +292,33 @@ public class FlatTraceGenerator {
     return currentContext;
   }
 
-  private static Context handleRevert(
+  private static FlatTrace.Context handleHalt(
-      final Deque<Context> tracesContexts, final FlatTrace.Context currentContext) {
+      final Deque<FlatTrace.Context> tracesContexts,
-    currentContext.getBuilder().error(Optional.of("Reverted"));
+      final FlatTrace.Context currentContext,
+      final TraceFrame traceFrame) {
+    final FlatTrace.Builder traceFrameBuilder = currentContext.getBuilder();
+    traceFrameBuilder.error(
+        traceFrame.getExceptionalHaltReasons().stream()
+            .map(ExceptionalHaltReason::getDescription)
+            .reduce((a, b) -> a + ", " + b));
+    if (tracesContexts.size() > 1) {
+      traceFrameBuilder.getActionBuilder().value("0x0");
+    }
+    tracesContexts.removeLast();
+    final FlatTrace.Context nextContext = tracesContexts.peekLast();
+    if (nextContext != null) {
+      nextContext.getBuilder().incSubTraces();
+    }
+    return nextContext;
+  }
+
+  private static FlatTrace.Context handleRevert(
+      final Deque<FlatTrace.Context> tracesContexts, final FlatTrace.Context currentContext) {
+    final FlatTrace.Builder traceFrameBuilder = currentContext.getBuilder();
+    traceFrameBuilder.error(Optional.of("Reverted"));
+    if (tracesContexts.size() > 1) {
+      traceFrameBuilder.getActionBuilder().value("0x0");
+    }
     tracesContexts.removeLast();
     final FlatTrace.Context nextContext = tracesContexts.peekLast();
     if (nextContext != null) {
@@ -313,16 +328,25 @@ public class FlatTraceGenerator {
   }
 
   private static String calculateCallingAddress(final FlatTrace.Context lastContext) {
-    if (lastContext.getBuilder().getActionBuilder().getCallType() == null) {
+    final FlatTrace.Builder lastContextBuilder = lastContext.getBuilder();
-      return ZERO_ADDRESS_STRING;
+    final Action.Builder lastActionBuilder = lastContextBuilder.getActionBuilder();
+    if (lastActionBuilder.getCallType() == null) {
+      if ("create".equals(lastContextBuilder.getType())) {
+        return lastContextBuilder.getResultBuilder().getAddress();
+      } else {
+        return ZERO_ADDRESS_STRING;
+      }
     }
-    switch (lastContext.getBuilder().getActionBuilder().getCallType()) {
+    switch (lastActionBuilder.getCallType()) {
       case "call":
       case "staticcall":
-        return lastContext.getBuilder().getActionBuilder().getTo();
+        return lastActionBuilder.getTo();
       case "delegatecall":
       case "callcode":
-        return lastContext.getBuilder().getActionBuilder().getFrom();
+        return lastActionBuilder.getFrom();
+      case "create":
+      case "create2":
+        return lastContextBuilder.getResultBuilder().getAddress();
       default:
         return ZERO_ADDRESS_STRING;
     }

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/results/tracing/flat/Result.java

@@ -89,6 +89,10 @@ public class Result {
       return this;
     }
 
+    public String getAddress() {
+      return address;
+    }
+
     public static Builder of(final Result result) {
       final Builder builder = new Builder();
       if (result != null) {

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/results/tracing/vm/VmTraceGenerator.java

@@ -24,6 +24,8 @@ import org.hyperledger.besu.ethereum.vm.ExceptionalHaltReason;
 
 import java.util.ArrayDeque;
 import java.util.Deque;
+import java.util.EnumSet;
+import java.util.Iterator;
 import java.util.Optional;
 import java.util.stream.IntStream;
 import java.util.stream.Stream;
@@ -40,7 +42,7 @@ public class VmTraceGenerator {
   private final TransactionTrace transactionTrace;
   private final VmTrace rootVmTrace = new VmTrace();
   private final Deque<VmTrace> parentTraces = new ArrayDeque<>();
-  int lastDepth = 0;
+  private int lastDepth = 0;
 
   public VmTraceGenerator(final TransactionTrace transactionTrace) {
     this.transactionTrace = transactionTrace;
@@ -68,7 +70,14 @@ public class VmTraceGenerator {
           .getInit()
           .map(Bytes::toHexString)
           .ifPresent(rootVmTrace::setCode);
-      transactionTrace.getTraceFrames().forEach(this::addFrame);
+      final Iterator<TraceFrame> iter = transactionTrace.getTraceFrames().iterator();
+      Optional<TraceFrame> nextTraceFrame =
+          iter.hasNext() ? Optional.of(iter.next()) : Optional.empty();
+      while (nextTraceFrame.isPresent()) {
+        final TraceFrame currentTraceFrame = nextTraceFrame.get();
+        nextTraceFrame = iter.hasNext() ? Optional.of(iter.next()) : Optional.empty();
+        addFrame(currentTraceFrame, nextTraceFrame);
+      }
     }
     return rootVmTrace;
   }
@@ -78,7 +87,7 @@ public class VmTraceGenerator {
    *
    * @param frame the current trace frame
    */
-  private void addFrame(final TraceFrame frame) {
+  private void addFrame(final TraceFrame frame, final Optional<TraceFrame> nextTraceFrame) {
     handleDepthDecreased(frame);
     if (!mustIgnore(frame)) {
       initStep(frame);
@@ -87,8 +96,8 @@ public class VmTraceGenerator {
       generateTracingMemory(report);
       generateTracingPush(report);
       generateTracingStorage(report);
-      handleDepthIncreased(op, report);
+      handleDepthIncreased(op, report, nextTraceFrame);
-      completeStep(op, report);
+      completeStep(frame, op, report);
       lastDepth = frame.getDepth();
     }
   }
@@ -96,26 +105,33 @@ public class VmTraceGenerator {
   private boolean mustIgnore(final TraceFrame frame) {
     if ("STOP".equals(frame.getOpcode()) && transactionTrace.getTraceFrames().size() == 1) {
       return true;
-    } else if (!frame.getExceptionalHaltReasons().isEmpty()
+    } else if (!frame.getExceptionalHaltReasons().isEmpty()) {
-        && !frame
+      final EnumSet<ExceptionalHaltReason> haltReasons = frame.getExceptionalHaltReasons();
-            .getExceptionalHaltReasons()
+      return !haltReasons.contains(ExceptionalHaltReason.INVALID_JUMP_DESTINATION)
-            .contains(ExceptionalHaltReason.INVALID_JUMP_DESTINATION)) {
+          && !haltReasons.contains(ExceptionalHaltReason.INSUFFICIENT_GAS);
-      return true;
     } else {
       return frame.isVirtualOperation();
     }
   }
 
-  private void completeStep(final VmOperation op, final VmOperationExecutionReport report) {
+  private void completeStep(
+      final TraceFrame frame, final VmOperation op, final VmOperationExecutionReport report) {
     // add the operation representation to the list of traces
-    op.setVmOperationExecutionReport(report);
+    if (frame.getExceptionalHaltReasons().contains(ExceptionalHaltReason.INSUFFICIENT_GAS)) {
+      op.setVmOperationExecutionReport(null);
+    } else {
+      op.setVmOperationExecutionReport(report);
+    }
     if (currentTrace != null) {
       currentTrace.add(op);
     }
     currentIndex++;
   }
 
-  private void handleDepthIncreased(final VmOperation op, final VmOperationExecutionReport report) {
+  private void handleDepthIncreased(
+      final VmOperation op,
+      final VmOperationExecutionReport report,
+      final Optional<TraceFrame> nextTraceFrame) {
     // check if next frame depth has increased i.e the current operation is a call
     switch (currentOperation) {
       case "STATICCALL":
@@ -144,10 +160,15 @@ public class VmTraceGenerator {
                   }
                 });
         if (currentTraceFrame.getMaybeCode().map(Code::getSize).orElse(0) > 0) {
-          op.setCost(currentTraceFrame.getGasRemainingPostExecution().toLong() + op.getCost());
+          if (nextTraceFrame.map(TraceFrame::getDepth).orElse(0) > currentTraceFrame.getDepth()) {
-          final VmTrace newSubTrace = new VmTrace();
+            op.setCost(currentTraceFrame.getGasRemainingPostExecution().toLong() + op.getCost());
-          parentTraces.addLast(newSubTrace);
+            final VmTrace newSubTrace = new VmTrace();
-          op.setSub(newSubTrace);
+            parentTraces.addLast(newSubTrace);
+            op.setSub(newSubTrace);
+          } else {
+            op.setCost(op.getCost());
+            op.setSub(null);
+          }
         } else {
           if (currentTraceFrame.getPrecompiledGasCost().isPresent()) {
             op.setCost(op.getCost() + currentTraceFrame.getPrecompiledGasCost().get().toLong());

ethereum/api/src/test/resources/org/hyperledger/besu/ethereum/api/jsonrpc/trace/specs/all/trace_replayBlockTransactions_all_0xD.json

@@ -91,6 +91,12 @@
               },
               "pc": 33,
               "sub": null
+            },
+            {
+              "cost": 9223372036854775807,
+              "ex": null,
+              "pc": 66,
+              "sub": null
             }
           ]
         }

ethereum/api/src/test/resources/org/hyperledger/besu/ethereum/api/jsonrpc/trace/specs/vm-trace/trace_replayBlockTransactions_0xD.json

@@ -46,6 +46,12 @@
               },
               "pc": 33,
               "sub": null
+            },
+            {
+              "cost": 9223372036854775807,
+              "ex": null,
+              "pc": 66,
+              "sub": null
             }
           ]
         }