Append ABI-decoded revert reason to message field in error responses (#5706)

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

CHANGELOG.md

@@ -4,6 +4,8 @@
 
 ### Breaking Changes
 
+- Add ABI-decoded revert reason to `eth_call` and `eth_estimateGas` responses [#5705](https://github.com/hyperledger/besu/issues/5705)
+
 ### Additions and Improvements
 
 ### Bug Fixes

acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/privacy/PrivCallAcceptanceTest.java

@@ -139,7 +139,7 @@ public class PrivCallAcceptanceTest extends ParameterizedEnclaveTestBase {
         privCall(privacyGroupId, revertReasonContract, false, false, true);
 
     EthCall resp = priv_call.send();
-    assertThat(resp.getRevertReason()).isEqualTo("Execution reverted");
+    assertThat(resp.getRevertReason()).isEqualTo("Execution reverted: RevertReason");
 
     byte[] bytes = Hex.decode(resp.getError().getData().substring(3, 203));
     String revertMessage =

ethereum/api/build.gradle

@@ -72,6 +72,12 @@ dependencies {
   implementation 'org.bouncycastle:bcprov-jdk18on'
   implementation 'org.springframework.security:spring-security-crypto'
   implementation 'org.xerial.snappy:snappy-java'
+  implementation 'com.google.guava:guava'
+  implementation 'io.vertx:vertx-core'
+  implementation 'com.fasterxml.jackson.core:jackson-databind'
+  implementation 'io.tmio:tuweni-bytes'
+  implementation 'io.tmio:tuweni-units'
+  implementation 'org.web3j:abi'
 
   annotationProcessor "org.immutables:value"
   implementation "org.immutables:value-annotations"

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/response/JsonRpcError.java

@@ -21,6 +21,7 @@ import com.fasterxml.jackson.annotation.JsonFormat;
 import com.fasterxml.jackson.annotation.JsonGetter;
 import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.tuweni.bytes.Bytes;
 
 @JsonInclude(value = JsonInclude.Include.NON_NULL)
 @JsonFormat(shape = JsonFormat.Shape.OBJECT)
@@ -28,6 +29,7 @@ public class JsonRpcError {
   private final int code;
   private final String message;
   private final String data;
+  private String reason;
 
   @JsonCreator
   public JsonRpcError(
@@ -41,6 +43,15 @@ public class JsonRpcError {
 
   public JsonRpcError(final RpcErrorType errorType, final String data) {
     this(errorType.getCode(), errorType.getMessage(), data);
+
+    // For execution reverted errors decode the data (if present)
+    if (errorType == RpcErrorType.REVERT_ERROR && data != null) {
+      JsonRpcErrorResponse.decodeRevertReason(Bytes.fromHexString(data))
+          .ifPresent(
+              (decodedReason) -> {
+                this.reason = decodedReason;
+              });
+    }
   }
 
   public JsonRpcError(final RpcErrorType errorType) {
@@ -54,7 +65,7 @@ public class JsonRpcError {
 
   @JsonGetter("message")
   public String getMessage() {
-    return message;
+    return (reason == null ? message : message + ": " + reason);
   }
 
   @JsonGetter("data")
@@ -72,12 +83,12 @@ public class JsonRpcError {
     }
     final JsonRpcError that = (JsonRpcError) o;
     return code == that.code
-        && Objects.equals(message, that.message)
+        && Objects.equals(message.split(":", -1)[0], that.message.split(":", -1)[0])
         && Objects.equals(data, that.data);
   }
 
   @Override
   public int hashCode() {
-    return Objects.hash(code, message, data);
+    return Objects.hash(code, message.split(":", -1)[0], data);
   }
 }

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/response/JsonRpcErrorResponse.java

@@ -15,12 +15,21 @@
 package org.hyperledger.besu.ethereum.api.jsonrpc.internal.response;
 
 import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
 import java.util.Objects;
+import java.util.Optional;
 
 import com.fasterxml.jackson.annotation.JsonGetter;
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonPropertyOrder;
 import com.google.common.base.MoreObjects;
+import org.apache.tuweni.bytes.Bytes;
+import org.web3j.abi.FunctionReturnDecoder;
+import org.web3j.abi.TypeReference;
+import org.web3j.abi.datatypes.AbiTypes;
+import org.web3j.abi.datatypes.Type;
+import org.web3j.abi.datatypes.Utf8String;
 
 @JsonPropertyOrder({"jsonrpc", "id", "error"})
 public class JsonRpcErrorResponse implements JsonRpcResponse {
@@ -29,6 +38,9 @@ public class JsonRpcErrorResponse implements JsonRpcResponse {
   private final JsonRpcError error;
   @JsonIgnore private final RpcErrorType errorType;
 
+  // Encoding of "Error(string)" to check for at the start of the revert reason
+  static final String errorMethodABI = "0x08c379a0";
+
   public JsonRpcErrorResponse(final Object id, final JsonRpcError error) {
     this.id = id;
     this.error = error;
@@ -84,8 +96,33 @@ public class JsonRpcErrorResponse implements JsonRpcResponse {
 
   private RpcErrorType findErrorType(final int code, final String message) {
     return Arrays.stream(RpcErrorType.values())
-        .filter(e -> e.getCode() == code && e.getMessage().equals(message))
+        .filter(e -> e.getCode() == code && message.startsWith(e.getMessage()))
         .findFirst()
         .get();
   }
+
+  @SuppressWarnings({"unchecked", "rawtypes"})
+  public static Optional<String> decodeRevertReason(final Bytes revertReason) {
+    if (revertReason.toHexString().startsWith(errorMethodABI)) {
+      // Remove the "Error(string)" prefix
+      final String encodedReasonText =
+          revertReason.toHexString().substring(errorMethodABI.length());
+
+      try {
+        List<TypeReference<Type>> revertReasonTypes =
+            Collections.singletonList(
+                TypeReference.create((Class<Type>) AbiTypes.getType("string")));
+        List<Type> decoded = FunctionReturnDecoder.decode(encodedReasonText, revertReasonTypes);
+
+        // Expect a single decoded string
+        if (decoded.size() == 1 && (decoded.get(0) instanceof Utf8String)) {
+          Utf8String decodedRevertReason = (Utf8String) decoded.get(0);
+          return Optional.of(decodedRevertReason.getValue());
+        }
+      } catch (StringIndexOutOfBoundsException exception) {
+        return Optional.of("ABI decode error");
+      }
+    }
+    return Optional.empty();
+  }
 }

ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthCallTest.java

@@ -17,6 +17,7 @@ package org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.RpcErrorType.BLOCK_NOT_FOUND;
 import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.RpcErrorType.INTERNAL_ERROR;
+import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.RpcErrorType.REVERT_ERROR;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.ArgumentMatchers.eq;
@@ -32,6 +33,7 @@ import org.hyperledger.besu.datatypes.Wei;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequest;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequestContext;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.parameters.JsonCallParameter;
+import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcError;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcErrorResponse;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcResponse;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcSuccessResponse;
@@ -44,6 +46,7 @@ import org.hyperledger.besu.ethereum.core.MutableWorldState;
 import org.hyperledger.besu.ethereum.mainnet.ImmutableTransactionValidationParams;
 import org.hyperledger.besu.ethereum.mainnet.TransactionValidationParams;
 import org.hyperledger.besu.ethereum.mainnet.ValidationResult;
+import org.hyperledger.besu.ethereum.processing.TransactionProcessingResult;
 import org.hyperledger.besu.ethereum.transaction.CallParameter;
 import org.hyperledger.besu.ethereum.transaction.PreCloseStateHandler;
 import org.hyperledger.besu.ethereum.transaction.TransactionSimulator;
@@ -164,6 +167,128 @@ public class EthCallTest {
     verifyNoMoreInteractions(blockchainQueries);
   }
 
+  @Test
+  public void shouldReturnBasicExecutionRevertErrorWithoutReason() {
+    final JsonRpcRequestContext request = ethCallRequest(callParameter(), "latest");
+
+    // Expect a revert error with no decoded reason (error doesn't begin "Error(string)" so ignored)
+    final String abiHexString = "0x1234";
+    final JsonRpcError expectedError = new JsonRpcError(REVERT_ERROR, abiHexString);
+    final JsonRpcResponse expectedResponse = new JsonRpcErrorResponse(null, expectedError);
+
+    assertThat(((JsonRpcErrorResponse) expectedResponse).getError().getMessage())
+        .isEqualTo("Execution reverted");
+
+    mockTransactionProcessorSuccessResult(expectedResponse);
+    when(blockchainQueries.getBlockchain()).thenReturn(blockchain);
+    when(blockchain.getChainHead()).thenReturn(chainHead);
+
+    final BlockHeader blockHeader = mock(BlockHeader.class);
+    when(blockHeader.getBaseFee()).thenReturn(Optional.of(Wei.ZERO));
+    when(chainHead.getBlockHeader()).thenReturn(blockHeader);
+
+    final JsonRpcResponse response = method.response(request);
+
+    final TransactionProcessingResult processingResult =
+        new TransactionProcessingResult(
+            null, null, 0, 0, null, null, Optional.of(Bytes.fromHexString(abiHexString)));
+
+    final TransactionSimulatorResult result = mock(TransactionSimulatorResult.class);
+    when(result.isSuccessful()).thenReturn(false);
+    when(result.getValidationResult()).thenReturn(ValidationResult.valid());
+    when(result.getResult()).thenReturn(processingResult);
+    verify(transactionSimulator).process(any(), any(), any(), mapperCaptor.capture(), any());
+    assertThat(mapperCaptor.getValue().apply(mock(MutableWorldState.class), Optional.of(result)))
+        .isEqualTo(Optional.of(expectedResponse));
+
+    assertThat(response).usingRecursiveComparison().isEqualTo(expectedResponse);
+    assertThat(((JsonRpcErrorResponse) response).getError().getMessage())
+        .isEqualTo("Execution reverted");
+  }
+
+  @Test
+  public void shouldReturnExecutionRevertErrorWithABIParseError() {
+    final JsonRpcRequestContext request = ethCallRequest(callParameter(), "latest");
+
+    // Expect a revert error with no decoded reason (error begins with "Error(string)" but trailing
+    // bytes are invalid ABI)
+    final String abiHexString = "0x08c379a002d36d";
+    final JsonRpcError expectedError = new JsonRpcError(REVERT_ERROR, abiHexString);
+    final JsonRpcResponse expectedResponse = new JsonRpcErrorResponse(null, expectedError);
+
+    assertThat(((JsonRpcErrorResponse) expectedResponse).getError().getMessage())
+        .isEqualTo("Execution reverted: ABI decode error");
+
+    mockTransactionProcessorSuccessResult(expectedResponse);
+    when(blockchainQueries.getBlockchain()).thenReturn(blockchain);
+    when(blockchain.getChainHead()).thenReturn(chainHead);
+
+    final BlockHeader blockHeader = mock(BlockHeader.class);
+    when(blockHeader.getBaseFee()).thenReturn(Optional.of(Wei.ZERO));
+    when(chainHead.getBlockHeader()).thenReturn(blockHeader);
+
+    final JsonRpcResponse response = method.response(request);
+    final TransactionProcessingResult processingResult =
+        new TransactionProcessingResult(
+            null, null, 0, 0, null, null, Optional.of(Bytes.fromHexString(abiHexString)));
+
+    final TransactionSimulatorResult result = mock(TransactionSimulatorResult.class);
+    when(result.isSuccessful()).thenReturn(false);
+    when(result.getValidationResult()).thenReturn(ValidationResult.valid());
+    when(result.getResult()).thenReturn(processingResult);
+    verify(transactionSimulator).process(any(), any(), any(), mapperCaptor.capture(), any());
+    assertThat(mapperCaptor.getValue().apply(mock(MutableWorldState.class), Optional.of(result)))
+        .isEqualTo(Optional.of(expectedResponse));
+
+    assertThat(response).usingRecursiveComparison().isEqualTo(expectedResponse);
+    assertThat(((JsonRpcErrorResponse) response).getError().getMessage())
+        .isEqualTo("Execution reverted: ABI decode error");
+  }
+
+  @Test
+  public void shouldReturnExecutionRevertErrorWithParsedABI() {
+    final JsonRpcRequestContext request = ethCallRequest(callParameter(), "latest");
+
+    // Expect a revert error with decoded reason (error begins with "Error(string)", trailing bytes
+    // = "ERC20: transfer from the zero address")
+    final String abiHexString =
+        "0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002545524332303a207472616e736665722066726f6d20746865207a65726f2061646472657373000000000000000000000000000000000000000000000000000000";
+    final JsonRpcError expectedError = new JsonRpcError(REVERT_ERROR, abiHexString);
+    final JsonRpcResponse expectedResponse = new JsonRpcErrorResponse(null, expectedError);
+
+    assertThat(((JsonRpcErrorResponse) expectedResponse).getError().getMessage())
+        .isEqualTo("Execution reverted: ERC20: transfer from the zero address");
+
+    mockTransactionProcessorSuccessResult(expectedResponse);
+    when(blockchainQueries.getBlockchain()).thenReturn(blockchain);
+    when(blockchain.getChainHead()).thenReturn(chainHead);
+
+    final BlockHeader blockHeader = mock(BlockHeader.class);
+    when(blockHeader.getBaseFee()).thenReturn(Optional.of(Wei.ZERO));
+    when(chainHead.getBlockHeader()).thenReturn(blockHeader);
+
+    final JsonRpcResponse response = method.response(request);
+
+    final TransactionProcessingResult processingResult =
+        new TransactionProcessingResult(
+            null, null, 0, 0, null, null, Optional.of(Bytes.fromHexString(abiHexString)));
+
+    final TransactionSimulatorResult result = mock(TransactionSimulatorResult.class);
+    when(result.isSuccessful()).thenReturn(false);
+    when(result.getValidationResult()).thenReturn(ValidationResult.valid());
+    when(result.getResult()).thenReturn(processingResult);
+
+    verify(transactionSimulator).process(any(), any(), any(), mapperCaptor.capture(), any());
+    System.out.println(result);
+    System.out.println(expectedResponse);
+    assertThat(mapperCaptor.getValue().apply(mock(MutableWorldState.class), Optional.of(result)))
+        .isEqualTo(Optional.of(expectedResponse));
+
+    assertThat(response).usingRecursiveComparison().isEqualTo(expectedResponse);
+    assertThat(((JsonRpcErrorResponse) response).getError().getMessage())
+        .isEqualTo("Execution reverted: ERC20: transfer from the zero address");
+  }
+
   @Test
   public void shouldUseCorrectBlockNumberWhenLatest() {
     final JsonRpcRequestContext request = ethCallRequest(callParameter(), "latest");

ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthEstimateGasTest.java

@@ -144,7 +144,7 @@ public class EthEstimateGasTest {
     final Wei gasPrice = Wei.of(1000);
     final JsonRpcRequestContext request =
         ethEstimateGasRequest(defaultLegacyTransactionCallParameter(gasPrice));
-    mockTransientProcessorResultGasEstimate(1L, true, false, gasPrice);
+    mockTransientProcessorResultGasEstimate(1L, true, gasPrice, Optional.empty());
 
     final JsonRpcResponse expectedResponse = new JsonRpcSuccessResponse(null, Quantity.create(1L));
 
@@ -257,9 +257,71 @@ public class EthEstimateGasTest {
     final JsonRpcResponse expectedResponse =
         new JsonRpcErrorResponse(null, new JsonRpcError(RpcErrorType.REVERT_ERROR, "0x00"));
 
-    Assertions.assertThat(method.response(request))
-        .usingRecursiveComparison()
-        .isEqualTo(expectedResponse);
+    assertThat(((JsonRpcErrorResponse) expectedResponse).getError().getMessage())
+        .isEqualTo("Execution reverted");
+
+    final JsonRpcResponse actualResponse = method.response(request);
+
+    Assertions.assertThat(actualResponse).usingRecursiveComparison().isEqualTo(expectedResponse);
+
+    assertThat(((JsonRpcErrorResponse) actualResponse).getError().getMessage())
+        .isEqualTo("Execution reverted");
+  }
+
+  @Test
+  public void shouldReturnErrorReasonWhenTransactionReverted() {
+    final JsonRpcRequestContext request =
+        ethEstimateGasRequest(defaultLegacyTransactionCallParameter(Wei.ZERO));
+
+    // ABI encoding of EVM "Error(string)" prefix + "ERC20: transfer from the zero address"
+    final String executionRevertedReason =
+        "0x08c379a000000000000000000000000000000000000000000000000000000000"
+            + "000000200000000000000000000000000000000000000000000000000000000000"
+            + "00002545524332303a207472616e736665722066726f6d20746865207a65726f20"
+            + "61646472657373000000000000000000000000000000000000000000000000000000";
+
+    mockTransientProcessorTxReverted(1L, false, Bytes.fromHexString(executionRevertedReason));
+
+    final JsonRpcResponse expectedResponse =
+        new JsonRpcErrorResponse(
+            null, new JsonRpcError(RpcErrorType.REVERT_ERROR, executionRevertedReason));
+
+    assertThat(((JsonRpcErrorResponse) expectedResponse).getError().getMessage())
+        .isEqualTo("Execution reverted: ERC20: transfer from the zero address");
+
+    final JsonRpcResponse actualResponse = method.response(request);
+
+    Assertions.assertThat(actualResponse).usingRecursiveComparison().isEqualTo(expectedResponse);
+
+    assertThat(((JsonRpcErrorResponse) actualResponse).getError().getMessage())
+        .isEqualTo("Execution reverted: ERC20: transfer from the zero address");
+  }
+
+  @Test
+  public void shouldReturnABIDecodeErrorReasonWhenInvalidRevertReason() {
+    final JsonRpcRequestContext request =
+        ethEstimateGasRequest(defaultLegacyTransactionCallParameter(Wei.ZERO));
+
+    // Invalid ABI bytes
+    final String invalidRevertReason =
+        "0x08c379a000000000000000000000000000000000000000000000000000000000"
+            + "123451234512345123451234512345123451234512345123451234512345123451";
+
+    mockTransientProcessorTxReverted(1L, false, Bytes.fromHexString(invalidRevertReason));
+
+    final JsonRpcResponse expectedResponse =
+        new JsonRpcErrorResponse(
+            null, new JsonRpcError(RpcErrorType.REVERT_ERROR, invalidRevertReason));
+
+    assertThat(((JsonRpcErrorResponse) expectedResponse).getError().getMessage())
+        .isEqualTo("Execution reverted: ABI decode error");
+
+    final JsonRpcResponse actualResponse = method.response(request);
+
+    Assertions.assertThat(actualResponse).usingRecursiveComparison().isEqualTo(expectedResponse);
+
+    assertThat(((JsonRpcErrorResponse) actualResponse).getError().getMessage())
+        .isEqualTo("Execution reverted: ABI decode error");
   }
 
   @Test
@@ -304,28 +366,38 @@ public class EthEstimateGasTest {
 
   private void mockTransientProcessorResultTxInvalidReason(final TransactionInvalidReason reason) {
     final TransactionSimulatorResult mockTxSimResult =
-        getMockTransactionSimulatorResult(false, false, 0, Wei.ZERO);
+        getMockTransactionSimulatorResult(false, 0, Wei.ZERO, Optional.empty());
     when(mockTxSimResult.getValidationResult()).thenReturn(ValidationResult.invalid(reason));
   }
 
+  private void mockTransientProcessorTxReverted(
+      final long estimateGas, final boolean isSuccessful, final Bytes revertReason) {
+    mockTransientProcessorResultGasEstimate(
+        estimateGas, isSuccessful, Wei.ZERO, Optional.of(revertReason));
+  }
+
   private void mockTransientProcessorResultGasEstimate(
       final long estimateGas, final boolean isSuccessful, final boolean isReverted) {
-    mockTransientProcessorResultGasEstimate(estimateGas, isSuccessful, isReverted, Wei.ZERO);
+    mockTransientProcessorResultGasEstimate(
+        estimateGas,
+        isSuccessful,
+        Wei.ZERO,
+        isReverted ? Optional.of(Bytes.of(0)) : Optional.empty());
   }
 
   private void mockTransientProcessorResultGasEstimate(
       final long estimateGas,
       final boolean isSuccessful,
-      final boolean isReverted,
-      final Wei gasPrice) {
-    getMockTransactionSimulatorResult(isSuccessful, isReverted, estimateGas, gasPrice);
+      final Wei gasPrice,
+      final Optional<Bytes> revertReason) {
+    getMockTransactionSimulatorResult(isSuccessful, estimateGas, gasPrice, revertReason);
   }
 
   private TransactionSimulatorResult getMockTransactionSimulatorResult(
       final boolean isSuccessful,
-      final boolean isReverted,
       final long estimateGas,
-      final Wei gasPrice) {
+      final Wei gasPrice,
+      final Optional<Bytes> revertReason) {
     final TransactionSimulatorResult mockTxSimResult = mock(TransactionSimulatorResult.class);
     when(transactionSimulator.process(
             eq(modifiedLegacyTransactionCallParameter(gasPrice)),
@@ -339,10 +411,11 @@ public class EthEstimateGasTest {
             any(OperationTracer.class),
             eq(1L)))
         .thenReturn(Optional.of(mockTxSimResult));
+
     final TransactionProcessingResult mockResult = mock(TransactionProcessingResult.class);
     when(mockResult.getEstimateGasUsedByTransaction()).thenReturn(estimateGas);
-    when(mockResult.getRevertReason())
-        .thenReturn(isReverted ? Optional.of(Bytes.of(0)) : Optional.empty());
+    when(mockResult.getRevertReason()).thenReturn(revertReason);
+
     when(mockTxSimResult.getResult()).thenReturn(mockResult);
     when(mockTxSimResult.isSuccessful()).thenReturn(isSuccessful);
     return mockTxSimResult;