Improve error message for Invalid operation (#8141)

Signed-off-by: Karim Taam <[email protected]>
hyperledger · Jan 21, 2025 · 6d05f83 · 6d05f83
1 parent 5933a2a
commit 6d05f83
Show file tree

Hide file tree

Showing 7 changed files with 56 additions and 11 deletions.
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
@@ -492,7 +492,19 @@ private static FlatTrace.Context handleHalt(
       traceFrameBuilder = currentContext.getBuilder();
     }
     traceFrameBuilder.error(
-        traceFrame.getExceptionalHaltReason().map(ExceptionalHaltReason::getDescription));
+        traceFrame
+            .getExceptionalHaltReason()
+            .map(
+                exceptionalHaltReason -> {
+                  if (exceptionalHaltReason
+                      .name()
+                      .equals(ExceptionalHaltReason.INVALID_OPERATION.name())) {
+                    return ExceptionalHaltReason.INVALID_OPERATION.getDescription();
+                  } else {
+                    return exceptionalHaltReason.getDescription();
+                  }
+                }));
+
     if (currentContext != null) {
       final Action.Builder actionBuilder = traceFrameBuilder.getActionBuilder();
       actionBuilder.value(Quantity.create(traceFrame.getValue()));

diff --git a/ethereum/evmtool/src/test/resources/org/hyperledger/besu/evmtool/state-test/create-eof.json b/ethereum/evmtool/src/test/resources/org/hyperledger/besu/evmtool/state-test/create-eof.json
@@ -80,7 +80,7 @@
     {"pc":6,"section":0,"op":95,"gas":"0x793d6f","gasCost":"0x2","memSize":0,"stack":["0x0"],"depth":1,"refund":0,"opName":"PUSH0"},
     {"pc":7,"section":0,"op":238,"immediate":"0x00","gas":"0x793d6d","gasCost":"0x0","memSize":0,"stack":["0x0","0x0"],"depth":1,"refund":0,"opName":"RETURNCONTRACT"},
     {"output":"","gasUsed":"0xe433","test":"create-eof","fork":"Osaka","d":0,"g":0,"v":0,"postHash":"0x1a8642a04dae90535f00f53d3a30284c4db051d508a653db89eb100ba9aecbf3","postLogsHash":"0xf48b954a6a6f4ce6b28e4950b7027413f4bdc8f459df6003b6e8d7a1567c8940","pass":true},
-    {"pc":0,"op":239,"gas":"0x794068","gasCost":"0x0","memSize":0,"stack":[],"depth":1,"refund":0,"opName":"INVALID","error":"Bad instruction"},
+    {"pc":0,"op":239,"gas":"0x794068","gasCost":"0x0","memSize":0,"stack":[],"depth":1,"refund":0,"opName":"INVALID","error":"Invalid opcode: 0xef"},
     {"output":"","gasUsed":"0x7a1200","test":"create-eof","fork":"Cancun","d":0,"g":0,"v":0,"postHash":"0xaa80d89bc89f58da8de41d3894bd1a241896ff91f7a5964edaefb39e8e3a4a98","postLogsHash":"0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347","pass":true,"error":"INVALID_OPERATION"}
   ]
 }
diff --git a/...vmtool/src/test/resources/org/hyperledger/besu/evmtool/state-test/create-invalid-eof.json b/...vmtool/src/test/resources/org/hyperledger/besu/evmtool/state-test/create-invalid-eof.json
@@ -72,7 +72,7 @@
   },
   "stdout": [
     {"output":"","gasUsed":"0xd198","test":"create-eof","fork":"Osaka","d":0,"g":0,"v":0,"postHash":"0x2a9c58298ba5d4ec86ca682b9fcc9ff67c3fc44dbd39f85a2f9b74bfe4e5178e","postLogsHash":"0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347","pass":false,"error":"Invalid EOF Layout: unexpected_header_kind expected 1 actual 17"},
-    {"pc":0,"op":239,"gas":"0x794068","gasCost":"0x0","memSize":0,"stack":[],"depth":1,"refund":0,"opName":"INVALID","error":"Bad instruction"},
+    {"pc":0,"op":239,"gas":"0x794068","gasCost":"0x0","memSize":0,"stack":[],"depth":1,"refund":0,"opName":"INVALID","error":"Invalid opcode: 0xef"},
     {"output":"","gasUsed":"0x7a1200","test":"create-eof","fork":"Cancun","d":0,"g":0,"v":0,"postHash":"0xaa80d89bc89f58da8de41d3894bd1a241896ff91f7a5964edaefb39e8e3a4a98","postLogsHash":"0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347","pass":true,"error":"INVALID_OPERATION"}
   ]
 }
diff --git a/ethereum/evmtool/src/test/resources/org/hyperledger/besu/evmtool/trace/badcode.json b/ethereum/evmtool/src/test/resources/org/hyperledger/besu/evmtool/trace/badcode.json
@@ -8,7 +8,7 @@
   "stdin": "",
   "stdout": [
     {"pc":0,"op":96,"gas":"0x2540be400","gasCost":"0x3","memSize":0,"stack":[],"depth":1,"refund":0,"opName":"PUSH1"},
-    {"pc":2,"op":239,"gas":"0x2540be3fd","gasCost":"0x0","memSize":0,"stack":["0x0"],"depth":1,"refund":0,"opName":"INVALID","error":"Bad instruction"},
+    {"pc":2,"op":239,"gas":"0x2540be3fd","gasCost":"0x0","memSize":0,"stack":["0x0"],"depth":1,"refund":0,"opName":"INVALID","error":"Invalid opcode: 0xef"},
     {"stateRoot":"0xfc9dc1be50c1b0a497afa545d770cc7064f0d71efbc4338f002dc2e086965d98","output":"0x","gasUsed":"0x2540be400","pass":false,"fork":"Cancun"}
   ]
 }
diff --git a/evm/src/main/java/org/hyperledger/besu/evm/EVM.java b/evm/src/main/java/org/hyperledger/besu/evm/EVM.java
@@ -241,7 +241,7 @@ public void runToHalt(final MessageFrame frame, final OperationTracer tracing) {
               case 0x09 -> MulModOperation.staticOperation(frame);
               case 0x0a -> ExpOperation.staticOperation(frame, gasCalculator);
               case 0x0b -> SignExtendOperation.staticOperation(frame);
-              case 0x0c, 0x0d, 0x0e, 0x0f -> InvalidOperation.INVALID_RESULT;
+              case 0x0c, 0x0d, 0x0e, 0x0f -> InvalidOperation.invalidOperationResult(opcode);
               case 0x10 -> LtOperation.staticOperation(frame);
               case 0x11 -> GtOperation.staticOperation(frame);
               case 0x12 -> SLtOperation.staticOperation(frame);
@@ -259,7 +259,7 @@ public void runToHalt(final MessageFrame frame, final OperationTracer tracing) {
               case 0x5f ->
                   enableShanghai
                       ? Push0Operation.staticOperation(frame)
-                      : InvalidOperation.INVALID_RESULT;
+                      : InvalidOperation.invalidOperationResult(opcode);
               case 0x60, // PUSH1-32
                       0x61,
                       0x62,

diff --git a/evm/src/main/java/org/hyperledger/besu/evm/frame/ExceptionalHaltReason.java b/evm/src/main/java/org/hyperledger/besu/evm/frame/ExceptionalHaltReason.java
@@ -86,6 +86,33 @@ public interface ExceptionalHaltReason {
    */
   String getDescription();
 
+  /**
+   * Creates an ExceptionalHaltReason for an invalid operation .
+   *
+   * @see org.hyperledger.besu.evm.operation.InvalidOperation
+   *     <p>Includes the problematic opcode in the description.
+   * @param opcode The invalid opcode.
+   * @return An ExceptionalHaltReason instance with the opcode details.
+   */
+  static ExceptionalHaltReason newInvalidOperation(final long opcode) {
+    return new ExceptionalHaltReason() {
+      @Override
+      public String name() {
+        return INVALID_OPERATION.name();
+      }
+
+      @Override
+      public String getDescription() {
+        return "Invalid opcode: 0x%02x".formatted(opcode);
+      }
+
+      @Override
+      public String toString() {
+        return name();
+      }
+    };
+  }
+
   /** The enum Default exceptional halt reason. */
   enum DefaultExceptionalHaltReason implements ExceptionalHaltReason {
     /** None default exceptional halt reason. */

diff --git a/evm/src/main/java/org/hyperledger/besu/evm/operation/InvalidOperation.java b/evm/src/main/java/org/hyperledger/besu/evm/operation/InvalidOperation.java
@@ -29,9 +29,6 @@ public class InvalidOperation extends AbstractOperation {
   public static final OperationResult INVALID_RESULT =
       new OperationResult(0, ExceptionalHaltReason.INVALID_OPERATION);
 
-  /** The Invalid operation result. */
-  protected final OperationResult invalidResult;
-
   /**
    * Instantiates a new Invalid operation.
    *
@@ -49,11 +46,20 @@ public InvalidOperation(final GasCalculator gasCalculator) {
    */
   public InvalidOperation(final int opcode, final GasCalculator gasCalculator) {
     super(opcode, "INVALID", -1, -1, gasCalculator);
-    invalidResult = new OperationResult(0L, ExceptionalHaltReason.INVALID_OPERATION);
   }
 
   @Override
   public OperationResult execute(final MessageFrame frame, final EVM evm) {
-    return invalidResult;
+    return invalidOperationResult(getOpcode());
+  }
+
+  /**
+   * Creates an {@link OperationResult} for an invalid opcode.
+   *
+   * @param opcode the invalid opcode encountered
+   * @return an {@link OperationResult} with zero gas cost and a description of the invalid opcode
+   */
+  public static OperationResult invalidOperationResult(final int opcode) {
+    return new OperationResult(0, ExceptionalHaltReason.newInvalidOperation(opcode));
   }
 }