Add revert reason to trace calls (hyperledger#1921)

* Add revert reason to trace calls * Add test for revert reason * Updated to add revertReason field Signed-off-by: David Mechler <david.mechler@consensys.net>
atoulme · Feb 18, 2021 · b605bf6 · b605bf6
1 parent bebb671
commit b605bf6
Show file tree

Hide file tree

Showing 10 changed files with 129 additions and 8 deletions.
diff --git a/.../hyperledger/besu/ethereum/api/jsonrpc/internal/methods/TraceReplayBlockTransactions.java b/.../hyperledger/besu/ethereum/api/jsonrpc/internal/methods/TraceReplayBlockTransactions.java
@@ -136,7 +136,8 @@ private void handleTransactionTrace(
     final ObjectNode resultNode = mapper.createObjectNode();
 
     TransactionProcessingResult result = transactionTrace.getResult();
-    resultNode.put("output", result.getRevertReason().orElse(result.getOutput()).toString());
+    resultNode.put("output", result.getOutput().toString());
+    result.getRevertReason().ifPresent(r -> resultNode.put("revertReason", r.toHexString()));
 
     if (traceTypes.contains(TraceType.STATE_DIFF)) {
       generateTracesFromTransactionTrace(

diff --git a/...va/org/hyperledger/besu/ethereum/api/jsonrpc/internal/results/tracing/flat/FlatTrace.java b/...va/org/hyperledger/besu/ethereum/api/jsonrpc/internal/results/tracing/flat/FlatTrace.java
@@ -33,6 +33,7 @@
   "blockNumber",
   "result",
   "error",
+  "revertReason",
   "subtraces",
   "traceAddress",
   "transactionHash",
@@ -47,6 +48,7 @@ public class FlatTrace implements Trace {
   private final Integer transactionPosition;
   private final String transactionHash;
   private final Optional<String> error;
+  private final String revertReason;
   private final int subtraces;
   private final List<Integer> traceAddress;
   private final String type;
@@ -72,7 +74,34 @@ protected FlatTrace(
         blockHash,
         transactionPosition,
         transactionHash,
-        error);
+        error,
+        null);
+  }
+
+  protected FlatTrace(
+      final Action.Builder actionBuilder,
+      final Result.Builder resultBuilder,
+      final int subtraces,
+      final List<Integer> traceAddress,
+      final String type,
+      final Long blockNumber,
+      final String blockHash,
+      final Integer transactionPosition,
+      final String transactionHash,
+      final Optional<String> error,
+      final String revertReason) {
+    this(
+        actionBuilder != null ? actionBuilder.build() : null,
+        resultBuilder != null ? resultBuilder.build() : null,
+        subtraces,
+        traceAddress,
+        type,
+        blockNumber,
+        blockHash,
+        transactionPosition,
+        transactionHash,
+        error,
+        revertReason);
   }
 
   protected FlatTrace(
@@ -85,7 +114,8 @@ protected FlatTrace(
       final String blockHash,
       final Integer transactionPosition,
       final String transactionHash,
-      final Optional<String> error) {
+      final Optional<String> error,
+      final String revertReason) {
     this.action = action;
     this.result = result;
     this.subtraces = subtraces;
@@ -96,6 +126,7 @@ protected FlatTrace(
     this.transactionPosition = transactionPosition;
     this.transactionHash = transactionHash;
     this.error = error;
+    this.revertReason = revertReason;
   }
 
   static Builder freshBuilder(final TransactionTrace transactionTrace) {
@@ -133,6 +164,11 @@ public String getError() {
     return error.orElse(null);
   }
 
+  @JsonInclude(NON_NULL)
+  public String getRevertReason() {
+    return revertReason;
+  }
+
   /**
    * This ridiculous construction returns a true "null" when we have a value in error, resulting in
    * jackson not serializing it, or a wrapped reference of either null or the value, resulting in
@@ -217,6 +253,7 @@ public static class Builder {
     private String transactionHash;
     private Integer transactionPosition;
     private Optional<String> error = Optional.empty();
+    private String revertReason;
 
     protected Builder() {}
 
@@ -265,6 +302,11 @@ public Builder error(final Optional<String> error) {
       return this;
     }
 
+    public Builder revertReason(final String revertReason) {
+      this.revertReason = revertReason;
+      return this;
+    }
+
     public String getType() {
       return type;
     }
@@ -297,6 +339,10 @@ public Optional<String> getError() {
       return error;
     }
 
+    public String getRevertReason() {
+      return revertReason;
+    }
+
     void incSubTraces() {
       this.subtraces++;
     }
@@ -312,7 +358,8 @@ public FlatTrace build() {
           blockHash,
           transactionPosition,
           transactionHash,
-          error);
+          error,
+          revertReason);
     }
 
     public Result.Builder getResultBuilder() {

diff --git a/...perledger/besu/ethereum/api/jsonrpc/internal/results/tracing/flat/FlatTraceGenerator.java b/...perledger/besu/ethereum/api/jsonrpc/internal/results/tracing/flat/FlatTraceGenerator.java
@@ -79,9 +79,11 @@ public static Stream<Trace> generateFromTransactionTrace(
     final Optional<String> smartContractAddress =
         smartContractCode.map(
             __ -> Address.contractAddress(tx.getSender(), tx.getNonce()).toHexString());
+    final Optional<Bytes> revertReason = transactionTrace.getResult().getRevertReason();
 
     // set code field in result node
     smartContractCode.ifPresent(firstFlatTraceBuilder.getResultBuilder()::code);
+    revertReason.ifPresent(r -> firstFlatTraceBuilder.revertReason(r.toHexString()));
 
     // set init field if transaction is a smart contract deployment
     tx.getInit().map(Bytes::toHexString).ifPresent(firstFlatTraceBuilder.getActionBuilder()::init);

diff --git a/...edger/besu/ethereum/api/jsonrpc/internal/results/tracing/flat/FlatTraceGeneratorTest.java b/...edger/besu/ethereum/api/jsonrpc/internal/results/tracing/flat/FlatTraceGeneratorTest.java
@@ -0,0 +1,63 @@
+/*
+ * Copyright ConsenSys AG.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+package org.hyperledger.besu.ethereum.api.jsonrpc.internal.results.tracing.flat;
+
+import org.hyperledger.besu.ethereum.api.jsonrpc.internal.processor.TransactionTrace;
+import org.hyperledger.besu.ethereum.api.jsonrpc.internal.results.tracing.Trace;
+import org.hyperledger.besu.ethereum.core.Address;
+import org.hyperledger.besu.ethereum.core.Transaction;
+import org.hyperledger.besu.ethereum.processing.TransactionProcessingResult;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Optional;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import org.apache.tuweni.bytes.Bytes;
+import org.assertj.core.api.Assertions;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class FlatTraceGeneratorTest {
+  @Mock private Transaction transaction;
+  @Mock private TransactionProcessingResult transactionProcessingResult;
+
+  @Test
+  public void testGenerateFromTransactionTraceWithRevertReason() {
+    final Bytes revertReason = Bytes.random(32);
+    Mockito.when(transaction.getSender()).thenReturn(Address.ZERO);
+    Mockito.when(transactionProcessingResult.getRevertReason())
+        .thenReturn(Optional.of(revertReason));
+
+    final TransactionTrace transactionTrace =
+        new TransactionTrace(transaction, transactionProcessingResult, Collections.emptyList());
+    final Stream<Trace> traceStream =
+        FlatTraceGenerator.generateFromTransactionTrace(
+            null, transactionTrace, null, new AtomicInteger());
+    final List<Trace> traces = traceStream.collect(Collectors.toList());
+
+    Assertions.assertThat(traces.isEmpty()).isFalse();
+    Assertions.assertThat(traces.get(0)).isNotNull();
+    Assertions.assertThat(traces.get(0) instanceof FlatTrace).isTrue();
+    Assertions.assertThat(((FlatTrace) traces.get(0)).getRevertReason())
+        .isEqualTo(revertReason.toHexString());
+  }
+}
diff --git a/...nrpc/trace/specs/replay-trace-transaction/all/trace_replayBlockTransactions_all_0x12.json b/...nrpc/trace/specs/replay-trace-transaction/all/trace_replayBlockTransactions_all_0x12.json
@@ -182,7 +182,8 @@
         }
       },
       {
-        "output": "0x7d88c1856cc95352",
+        "output": "0x",
+        "revertReason": "0x7d88c1856cc95352",
         "stateDiff": {
           "0x0000000000000000000000000000000000000000": {
             "balance": {
@@ -223,6 +224,7 @@
               "value": "0x0"
             },
             "error": "Reverted",
+            "revertReason": "0x7d88c1856cc95352",
             "subtraces": 0,
             "traceAddress": [],
             "type": "call"

diff --git a/...ace/specs/replay-trace-transaction/flat/trace_replayBlockTransactions_traceOnly_0x12.json b/...ace/specs/replay-trace-transaction/flat/trace_replayBlockTransactions_traceOnly_0x12.json
@@ -39,7 +39,8 @@
         "vmTrace": null
       },
       {
-        "output": "0x7d88c1856cc95352",
+        "output": "0x",
+        "revertReason": "0x7d88c1856cc95352",
         "stateDiff": null,
         "trace": [
           {
@@ -52,6 +53,7 @@
               "value": "0x0"
             },
             "error": "Reverted",
+            "revertReason": "0x7d88c1856cc95352",
             "subtraces": 0,
             "traceAddress": [],
             "type": "call"

diff --git a/...specs/replay-trace-transaction/statediff/trace_replayBlockTransactions_diffOnly_0x12.json b/...specs/replay-trace-transaction/statediff/trace_replayBlockTransactions_diffOnly_0x12.json
@@ -49,7 +49,8 @@
         "vmTrace": null
       },
       {
-        "output": "0x7d88c1856cc95352",
+        "output": "0x",
+        "revertReason": "0x7d88c1856cc95352",
         "stateDiff": {
           "0x0000000000000000000000000000000000000000": {
             "balance": {

diff --git a/...rpc/trace/specs/replay-trace-transaction/vm-trace/trace_replayBlockTransactions_0x12.json b/...rpc/trace/specs/replay-trace-transaction/vm-trace/trace_replayBlockTransactions_0x12.json
@@ -134,7 +134,8 @@
         }
       },
       {
-        "output": "0x7d88c1856cc95352",
+        "output": "0x",
+        "revertReason": "0x7d88c1856cc95352",
         "stateDiff": null,
         "trace": [],
         "transactionHash": "0xc388baa0e55e6b73e850b22dc7e9853700f6b995fd55d95dd6ccd5a13d63c566",

diff --git a/...s/org/hyperledger/besu/ethereum/api/jsonrpc/trace/specs/trace-block/trace_block_0x12.json b/...s/org/hyperledger/besu/ethereum/api/jsonrpc/trace/specs/trace-block/trace_block_0x12.json
@@ -43,6 +43,7 @@
         "blockHash": "0x220bc13dc4f1ed38dcca927a5be15eca16497d279f4c40d7b8fe9704eadf1464",
         "blockNumber": 18,
         "error": "Reverted",
+        "revertReason": "0x7d88c1856cc95352",
         "subtraces": 0,
         "traceAddress": [],
         "transactionHash": "0xc388baa0e55e6b73e850b22dc7e9853700f6b995fd55d95dd6ccd5a13d63c566",

diff --git a/...edger/besu/ethereum/api/jsonrpc/trace/specs/trace-transaction/trace_transaction_0x12.json b/...edger/besu/ethereum/api/jsonrpc/trace/specs/trace-transaction/trace_transaction_0x12.json
@@ -60,6 +60,7 @@
           "blockHash": "0x220bc13dc4f1ed38dcca927a5be15eca16497d279f4c40d7b8fe9704eadf1464",
           "blockNumber": 18,
           "error": "Reverted",
+          "revertReason": "0x7d88c1856cc95352",
           "subtraces": 0,
           "traceAddress": [],
           "transactionHash": "0xc388baa0e55e6b73e850b22dc7e9853700f6b995fd55d95dd6ccd5a13d63c566",