EOF Reference Test Fixes (hyperledger#7306)

Fix a number of issues found in reference tests and evmone tests.

- Be tolerant of more nulls in json
- Support ContainerKind in reference tests
- re-order EXTCALL oeprands
- correct return value for REVERT in EXT*CALL
- re-order EOFCREATE code validation

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

ethereum/referencetests/src/main/java/org/hyperledger/besu/ethereum/referencetests/GeneralStateTestCaseSpec.java

@@ -56,10 +56,12 @@ private Map<String, List<GeneralStateTestCaseEipSpec>> generate(
       final ReferenceTestWorldState initialWorldState,
       final Map<String, List<PostSection>> postSections,
       final StateTestVersionedTransaction versionedTransaction) {
-
+    if (initialWorldState == null) {
+      return Map.of();
+    }
     initialWorldState.persist(null);
     final Map<String, List<GeneralStateTestCaseEipSpec>> res =
-        new LinkedHashMap<>(postSections.size());
+        LinkedHashMap.newLinkedHashMap(postSections.size());
     for (final Map.Entry<String, List<PostSection>> entry : postSections.entrySet()) {
       final String eip = entry.getKey();
       final List<PostSection> post = entry.getValue();

ethereum/referencetests/src/main/java/org/hyperledger/besu/ethereum/referencetests/ReferenceTestProtocolSchedules.java

@@ -91,6 +91,11 @@ public static ReferenceTestProtocolSchedules create(final StubGenesisConfigOptio
         "CancunToPragueAtTime15k",
         createSchedule(genesisStub.clone().cancunTime(0).pragueTime(15000)));
     builder.put("Prague", createSchedule(genesisStub.clone().pragueEOFTime(0)));
+    builder.put("Osaka", createSchedule(genesisStub.clone().futureEipsTime(0)));
+    builder.put("Amsterdam", createSchedule(genesisStub.clone().futureEipsTime(0)));
+    builder.put("Bogota", createSchedule(genesisStub.clone().futureEipsTime(0)));
+    builder.put("Polis", createSchedule(genesisStub.clone().futureEipsTime(0)));
+    builder.put("Bangkok", createSchedule(genesisStub.clone().futureEipsTime(0)));
     builder.put("Future_EIPs", createSchedule(genesisStub.clone().futureEipsTime(0)));
     builder.put("Experimental_EIPs", createSchedule(genesisStub.clone().experimentalEipsTime(0)));
     return new ReferenceTestProtocolSchedules(builder.build());

ethereum/referencetests/src/reference-test/java/org/hyperledger/besu/ethereum/eof/EOFReferenceTestTools.java

@@ -21,13 +21,19 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
+
 import org.apache.tuweni.bytes.Bytes;
+
 import org.hyperledger.besu.ethereum.referencetests.EOFTestCaseSpec;
+import org.hyperledger.besu.ethereum.referencetests.EOFTestCaseSpec.TestResult;
 import org.hyperledger.besu.ethereum.referencetests.ReferenceTestProtocolSchedules;
 import org.hyperledger.besu.evm.Code;
 import org.hyperledger.besu.evm.EVM;
 import org.hyperledger.besu.evm.code.CodeInvalid;
+import org.hyperledger.besu.evm.code.CodeV1;
 import org.hyperledger.besu.evm.code.EOFLayout;
+import org.hyperledger.besu.evm.code.EOFLayout.EOFContainerMode;
 import org.hyperledger.besu.testutil.JsonTestParameters;
 
 public class EOFReferenceTestTools {
@@ -43,20 +49,26 @@ public class EOFReferenceTestTools {
       JsonTestParameters.create(EOFTestCaseSpec.class, EOFTestCaseSpec.TestResult.class)
           .generator(
               (testName, fullPath, eofSpec, collector) -> {
+                if (eofSpec.getVector() == null) {
+                  return;
+                }
                 final Path path = Path.of(fullPath).getParent().getFileName();
                 final String prefix = path + "/" + testName + "-";
                 for (final Map.Entry<String, EOFTestCaseSpec.TestVector> entry :
                     eofSpec.getVector().entrySet()) {
                   final String name = entry.getKey();
                   final Bytes code = Bytes.fromHexString(entry.getValue().code());
-                  for (final var result : entry.getValue().results().entrySet()) {
+                  final String containerKind = entry.getValue().containerKind();
+                  for (final Entry<String, TestResult> result :
+                      entry.getValue().results().entrySet()) {
                     final String eip = result.getKey();
                     final boolean runTest = EIPS_TO_RUN.contains(eip);
                     collector.add(
                         prefix + eip + '[' + name + ']',
                         fullPath,
                         eip,
                         code,
+                        containerKind,
                         result.getValue(),
                         runTest);
                   }
@@ -72,7 +84,7 @@ public class EOFReferenceTestTools {
     params.ignore("EOF1_undefined_opcodes_186");
 
     // embedded containers rules changed
-      params.ignore("efValidation/EOF1_embedded_container-Prague\\[EOF1_embedded_container_\\d+\\]");
+    params.ignore("efValidation/EOF1_embedded_container-Prague\\[EOF1_embedded_container_\\d+\\]");
 
     // truncated data is only allowed in embedded containers
     params.ignore("ori/validInvalid-Prague\\[validInvalid_48\\]");
@@ -101,7 +113,10 @@ public static Collection<Object[]> generateTestParametersForConfig(final String[
 
   @SuppressWarnings("java:S5960") // This is not production code, this is testing code.
   public static void executeTest(
-      final String fork, final Bytes code, final EOFTestCaseSpec.TestResult expected) {
+      final String fork,
+      final Bytes code,
+      final String containerKind,
+      final EOFTestCaseSpec.TestResult expected) {
     EVM evm = ReferenceTestProtocolSchedules.create().geSpecByName(fork).getEvm();
     assertThat(evm).isNotNull();
 
@@ -112,23 +127,40 @@ public static void executeTest(
       EOFLayout layout = EOFLayout.parseEOF(code);
 
       if (layout.isValid()) {
-        Code parsedCode = evm.getCodeUncached(code);
-        assertThat(parsedCode.isValid())
-            .withFailMessage(
-                () ->
-                    EOFLayout.parseEOF(code).prettyPrint()
-                        + "\nExpected exception :"
-                        + expected.exception()
-                        + " actual exception :"
-                        + (parsedCode.isValid()
-                            ? null
-                            : ((CodeInvalid) parsedCode).getInvalidReason()))
-            .isEqualTo(expected.result());
-
-        if (expected.result()) {
-          assertThat(code)
-              .withFailMessage("Container round trip failed")
-              .isEqualTo(layout.writeContainer(null));
+        Code parsedCode;
+        if ("INITCODE".equals(containerKind)) {
+          parsedCode = evm.getCodeForCreation(code);
+        } else {
+          parsedCode = evm.getCodeUncached(code);
+        }
+        if ("EOF_IncompatibleContainerKind".equals(expected.exception()) && parsedCode.isValid()) {
+          EOFContainerMode expectedMode =
+              EOFContainerMode.valueOf(containerKind == null ? "RUNTIME" : containerKind);
+          EOFContainerMode containerMode =
+              ((CodeV1) parsedCode).getEofLayout().containerMode().get();
+          EOFContainerMode actualMode =
+              containerMode == null ? EOFContainerMode.RUNTIME : containerMode;
+          assertThat(actualMode)
+              .withFailMessage("Code did not parse to valid containerKind of " + expectedMode)
+              .isNotEqualTo(expectedMode);
+        } else {
+          assertThat(parsedCode.isValid())
+              .withFailMessage(
+                  () ->
+                      EOFLayout.parseEOF(code).prettyPrint()
+                          + "\nExpected exception :"
+                          + expected.exception()
+                          + " actual exception :"
+                          + (parsedCode.isValid()
+                              ? null
+                              : ((CodeInvalid) parsedCode).getInvalidReason()))
+              .isEqualTo(expected.result());
+
+          if (expected.result()) {
+            assertThat(code)
+                .withFailMessage("Container round trip failed")
+                .isEqualTo(layout.writeContainer(null));
+          }
         }
       } else {
         assertThat(layout.isValid())

ethereum/referencetests/src/reference-test/templates/EOFReferenceTest.java.template

@@ -34,9 +34,10 @@ public class %%TESTS_NAME%% {
       final String name,
       final String fork,
       final Bytes code,
+      final String containerKind,
       final EOFTestCaseSpec.TestResult results,
       final boolean runTest) {
     assumeTrue(runTest, "Test " + name + " was ignored");
-    executeTest(fork, code, results);
+    executeTest(fork, code, containerKind, results);
   }
 }

evm/src/main/java/org/hyperledger/besu/evm/code/CodeFactory.java

@@ -15,6 +15,7 @@
 package org.hyperledger.besu.evm.code;
 
 import org.hyperledger.besu.evm.Code;
+import org.hyperledger.besu.evm.code.EOFLayout.EOFContainerMode;
 
 import javax.annotation.Nonnull;
 
@@ -94,6 +95,9 @@ public Code createCode(final Bytes bytes, final boolean createTransaction) {
       }
 
       final EOFLayout layout = EOFLayout.parseEOF(bytes, !createTransaction);
+      if (createTransaction) {
+        layout.containerMode().set(EOFContainerMode.INITCODE);
+      }
       return createCode(layout);
     } else {
       return new CodeV0(bytes);

evm/src/main/java/org/hyperledger/besu/evm/code/CodeV1.java

@@ -111,7 +111,7 @@ public Optional<Code> getSubContainer(final int index, final Bytes auxData, fina
       codeToLoad = subcontainerLayout.container();
     }
 
-    Code subContainerCode = evm.getCodeForCreation(codeToLoad);
+    Code subContainerCode = evm.getCodeUncached(codeToLoad);
 
     return subContainerCode.isValid() && subContainerCode.getEofVersion() > 0
         ? Optional.of(subContainerCode)

evm/src/main/java/org/hyperledger/besu/evm/code/EOFLayout.java

@@ -57,9 +57,16 @@ public record EOFLayout(
     String invalidReason,
     AtomicReference<EOFContainerMode> containerMode) {
 
-  enum EOFContainerMode {
+  /**
+   * Enum tracking the useage mode of an EOF container. Detected either by opcode usage or
+   * determined by the source.
+   */
+  public enum EOFContainerMode {
+    /** Usage mode is unknown */
     UNKNOWN,
+    /** Usage mode is as init code */
     INITCODE,
+    /** Usage mode is as deployed or runtime code */
     RUNTIME
   }
 
@@ -324,6 +331,9 @@ public static EOFLayout parseEOF(final Bytes container, final boolean strictSize
       Bytes subcontainer = container.slice(pos, subcontianerSize);
       pos += subcontianerSize;
       EOFLayout subLayout = EOFLayout.parseEOF(subcontainer, false);
+      if (subLayout.container.size() < subcontainer.size()) {
+        return invalidLayout(container, version, "excess data in subcontainer");
+      }
       if (!subLayout.isValid()) {
         String invalidSubReason = subLayout.invalidReason;
         return invalidLayout(

evm/src/main/java/org/hyperledger/besu/evm/operation/AbstractCreateOperation.java

@@ -214,32 +214,32 @@ protected Bytes getInputData(final MessageFrame frame) {
   private void complete(final MessageFrame frame, final MessageFrame childFrame, final EVM evm) {
     frame.setState(MessageFrame.State.CODE_EXECUTING);
 
-    Code outputCode =
-        (childFrame.getCreatedCode() != null)
-            ? childFrame.getCreatedCode()
-            : evm.getCodeForCreation(childFrame.getOutputData());
+    frame.incrementRemainingGas(childFrame.getRemainingGas());
+    frame.addLogs(childFrame.getLogs());
+    frame.addSelfDestructs(childFrame.getSelfDestructs());
+    frame.addCreates(childFrame.getCreates());
     frame.popStackItems(getStackItemsConsumed());
 
-    if (outputCode.isValid()) {
-      frame.incrementRemainingGas(childFrame.getRemainingGas());
-      frame.addLogs(childFrame.getLogs());
-      frame.addSelfDestructs(childFrame.getSelfDestructs());
-      frame.addCreates(childFrame.getCreates());
-
-      if (childFrame.getState() == MessageFrame.State.COMPLETED_SUCCESS) {
+    if (childFrame.getState() == MessageFrame.State.COMPLETED_SUCCESS) {
+      Code outputCode =
+          (childFrame.getCreatedCode() != null)
+              ? childFrame.getCreatedCode()
+              : evm.getCodeForCreation(childFrame.getOutputData());
+      if (outputCode.isValid()) {
         Address createdAddress = childFrame.getContractAddress();
         frame.pushStackItem(Words.fromAddress(createdAddress));
+        frame.setReturnData(Bytes.EMPTY);
         onSuccess(frame, createdAddress);
       } else {
+        frame.getWorldUpdater().deleteAccount(childFrame.getRecipientAddress());
         frame.setReturnData(childFrame.getOutputData());
         frame.pushStackItem(LEGACY_FAILURE_STACK_ITEM);
-        onFailure(frame, childFrame.getExceptionalHaltReason());
+        onInvalid(frame, (CodeInvalid) outputCode);
       }
     } else {
-      frame.getWorldUpdater().deleteAccount(childFrame.getRecipientAddress());
       frame.setReturnData(childFrame.getOutputData());
       frame.pushStackItem(LEGACY_FAILURE_STACK_ITEM);
-      onInvalid(frame, (CodeInvalid) outputCode);
+      onFailure(frame, childFrame.getExceptionalHaltReason());
     }
 
     final int currentPC = frame.getPC();

evm/src/main/java/org/hyperledger/besu/evm/operation/AbstractExtCallOperation.java

@@ -38,6 +38,8 @@
 public abstract class AbstractExtCallOperation extends AbstractCallOperation {
 
   static final int STACK_TO = 0;
+  static final int STACK_INPUT_OFFSET = 1;
+  static final int STACK_INPUT_LENGTH = 2;
 
   /** EXT*CALL response indicating success */
   public static final Bytes EOF1_SUCCESS_STACK_ITEM = Bytes.EMPTY;
@@ -195,6 +197,10 @@ Bytes getCallResultStackItem(final MessageFrame childFrame) {
     return switch (childFrame.getState()) {
       case COMPLETED_SUCCESS -> EOF1_SUCCESS_STACK_ITEM;
       case EXCEPTIONAL_HALT -> EOF1_EXCEPTION_STACK_ITEM;
+      case COMPLETED_FAILED ->
+          childFrame.getExceptionalHaltReason().isPresent()
+              ? EOF1_FAILURE_STACK_ITEM
+              : EOF1_EXCEPTION_STACK_ITEM;
       default -> EOF1_FAILURE_STACK_ITEM;
     };
   }

evm/src/main/java/org/hyperledger/besu/evm/operation/ExtCallOperation.java

@@ -24,9 +24,7 @@
 /** The Call operation. */
 public class ExtCallOperation extends AbstractExtCallOperation {
 
-  static final int STACK_VALUE = 1;
-  static final int STACK_INPUT_OFFSET = 2;
-  static final int STACK_INPUT_LENGTH = 3;
+  static final int STACK_VALUE = 3;
 
   /**
    * Instantiates a new Call operation.

evm/src/main/java/org/hyperledger/besu/evm/operation/ExtDelegateCallOperation.java

@@ -24,9 +24,6 @@
 /** The Delegate call operation. */
 public class ExtDelegateCallOperation extends AbstractExtCallOperation {
 
-  static final int STACK_INPUT_OFFSET = 1;
-  static final int STACK_INPUT_LENGTH = 2;
-
   /**
    * Instantiates a new Delegate call operation.
    *

evm/src/main/java/org/hyperledger/besu/evm/operation/ExtStaticCallOperation.java

@@ -24,9 +24,6 @@
 /** The Static call operation. */
 public class ExtStaticCallOperation extends AbstractExtCallOperation {
 
-  static final int STACK_INPUT_OFFSET = 1;
-  static final int STACK_INPUT_LENGTH = 2;
-
   /**
    * Instantiates a new Static call operation.
    *

evm/src/test/java/org/hyperledger/besu/evm/operations/ExtCallOperationTest.java

@@ -211,9 +211,9 @@ void callWithValueTest(
         new TestMessageFrameBuilder()
             .initialGas(parentGas)
             .pushStackItem(CONTRACT_ADDRESS) // canary for non-returning
+            .pushStackItem(valueSent)
             .pushStackItem(Bytes.EMPTY)
             .pushStackItem(Bytes.EMPTY)
-            .pushStackItem(valueSent)
             .pushStackItem(CONTRACT_ADDRESS)
             .worldUpdater(worldUpdater)
             .isStatic(isStatic)

testutil/src/main/java/org/hyperledger/besu/testutil/JsonTestParameters.java

@@ -96,6 +96,7 @@ public void add(
      * @param fullPath the full path of the test
      * @param fork the fork to be tested
      * @param code the code to be tested
+     * @param containerKind the containerKind, if specified
      * @param value the value
      * @param runTest the run test
      */
@@ -104,10 +105,13 @@ public void add(
         final String fullPath,
         final String fork,
         final Bytes code,
+        final String containerKind,
         final S value,
         final boolean runTest) {
       testParameters.add(
-          new Object[] {name, fork, code, value, runTest && includes(name) && includes(fullPath)});
+          new Object[] {
+            name, fork, code, containerKind, value, runTest && includes(name) && includes(fullPath)
+          });
     }
 
     private boolean includes(final String name) {
@@ -304,7 +308,7 @@ private Collection<File> getFilteredFiles(final String[] paths) {
     final List<File> files = new ArrayList<>();
     for (final String path : paths) {
       final URL url = classLoader.getResource(path);
-      checkState(url != null, "Cannot find test directory " + path);
+      checkState(url != null, "Cannot find test directory %s", path);
       final Path dir;
       try {
         dir = Paths.get(url.toURI());