From 965e757d81072f31d2a44bb5757ff46f7d102e36 Mon Sep 17 00:00:00 2001 From: Danno Ferrin Date: Fri, 12 Jul 2024 08:43:44 -0600 Subject: [PATCH] EOF Reference Test Fixes (#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 --- .../GeneralStateTestCaseSpec.java | 6 +- .../ReferenceTestProtocolSchedules.java | 5 ++ .../ethereum/eof/EOFReferenceTestTools.java | 72 +++++++++++++------ .../templates/EOFReferenceTest.java.template | 3 +- .../besu/evm/code/CodeFactory.java | 4 ++ .../org/hyperledger/besu/evm/code/CodeV1.java | 2 +- .../hyperledger/besu/evm/code/EOFLayout.java | 12 +++- .../operation/AbstractCreateOperation.java | 28 ++++---- .../operation/AbstractExtCallOperation.java | 6 ++ .../besu/evm/operation/ExtCallOperation.java | 4 +- .../operation/ExtDelegateCallOperation.java | 3 - .../evm/operation/ExtStaticCallOperation.java | 3 - .../evm/operations/ExtCallOperationTest.java | 2 +- .../besu/testutil/JsonTestParameters.java | 8 ++- 14 files changed, 107 insertions(+), 51 deletions(-) diff --git a/ethereum/referencetests/src/main/java/org/hyperledger/besu/ethereum/referencetests/GeneralStateTestCaseSpec.java b/ethereum/referencetests/src/main/java/org/hyperledger/besu/ethereum/referencetests/GeneralStateTestCaseSpec.java index 73735fdb0e0..affc6e46010 100644 --- a/ethereum/referencetests/src/main/java/org/hyperledger/besu/ethereum/referencetests/GeneralStateTestCaseSpec.java +++ b/ethereum/referencetests/src/main/java/org/hyperledger/besu/ethereum/referencetests/GeneralStateTestCaseSpec.java @@ -56,10 +56,12 @@ private Map> generate( final ReferenceTestWorldState initialWorldState, final Map> postSections, final StateTestVersionedTransaction versionedTransaction) { - + if (initialWorldState == null) { + return Map.of(); + } initialWorldState.persist(null); final Map> res = - new LinkedHashMap<>(postSections.size()); + LinkedHashMap.newLinkedHashMap(postSections.size()); for (final Map.Entry> entry : postSections.entrySet()) { final String eip = entry.getKey(); final List post = entry.getValue(); diff --git a/ethereum/referencetests/src/main/java/org/hyperledger/besu/ethereum/referencetests/ReferenceTestProtocolSchedules.java b/ethereum/referencetests/src/main/java/org/hyperledger/besu/ethereum/referencetests/ReferenceTestProtocolSchedules.java index 4d62c314ae3..8ac419f7a68 100644 --- a/ethereum/referencetests/src/main/java/org/hyperledger/besu/ethereum/referencetests/ReferenceTestProtocolSchedules.java +++ b/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()); diff --git a/ethereum/referencetests/src/reference-test/java/org/hyperledger/besu/ethereum/eof/EOFReferenceTestTools.java b/ethereum/referencetests/src/reference-test/java/org/hyperledger/besu/ethereum/eof/EOFReferenceTestTools.java index 58f8c025297..a15bcb24189 100644 --- a/ethereum/referencetests/src/reference-test/java/org/hyperledger/besu/ethereum/eof/EOFReferenceTestTools.java +++ b/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,13 +49,18 @@ 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 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 result : + entry.getValue().results().entrySet()) { final String eip = result.getKey(); final boolean runTest = EIPS_TO_RUN.contains(eip); collector.add( @@ -57,6 +68,7 @@ public class EOFReferenceTestTools { 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 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()) diff --git a/ethereum/referencetests/src/reference-test/templates/EOFReferenceTest.java.template b/ethereum/referencetests/src/reference-test/templates/EOFReferenceTest.java.template index 5f5cafd6760..d9e52403d78 100644 --- a/ethereum/referencetests/src/reference-test/templates/EOFReferenceTest.java.template +++ b/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); } } diff --git a/evm/src/main/java/org/hyperledger/besu/evm/code/CodeFactory.java b/evm/src/main/java/org/hyperledger/besu/evm/code/CodeFactory.java index a0bba703718..18561001205 100644 --- a/evm/src/main/java/org/hyperledger/besu/evm/code/CodeFactory.java +++ b/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); diff --git a/evm/src/main/java/org/hyperledger/besu/evm/code/CodeV1.java b/evm/src/main/java/org/hyperledger/besu/evm/code/CodeV1.java index 50ffdd41061..a1517d1d7f4 100644 --- a/evm/src/main/java/org/hyperledger/besu/evm/code/CodeV1.java +++ b/evm/src/main/java/org/hyperledger/besu/evm/code/CodeV1.java @@ -111,7 +111,7 @@ public Optional 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) diff --git a/evm/src/main/java/org/hyperledger/besu/evm/code/EOFLayout.java b/evm/src/main/java/org/hyperledger/besu/evm/code/EOFLayout.java index 8b41a979231..401308715af 100644 --- a/evm/src/main/java/org/hyperledger/besu/evm/code/EOFLayout.java +++ b/evm/src/main/java/org/hyperledger/besu/evm/code/EOFLayout.java @@ -57,9 +57,16 @@ public record EOFLayout( String invalidReason, AtomicReference 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( diff --git a/evm/src/main/java/org/hyperledger/besu/evm/operation/AbstractCreateOperation.java b/evm/src/main/java/org/hyperledger/besu/evm/operation/AbstractCreateOperation.java index f026fd7110b..f20c2914a09 100644 --- a/evm/src/main/java/org/hyperledger/besu/evm/operation/AbstractCreateOperation.java +++ b/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(); diff --git a/evm/src/main/java/org/hyperledger/besu/evm/operation/AbstractExtCallOperation.java b/evm/src/main/java/org/hyperledger/besu/evm/operation/AbstractExtCallOperation.java index 2c0535b197b..43d7e343ff1 100644 --- a/evm/src/main/java/org/hyperledger/besu/evm/operation/AbstractExtCallOperation.java +++ b/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; }; } diff --git a/evm/src/main/java/org/hyperledger/besu/evm/operation/ExtCallOperation.java b/evm/src/main/java/org/hyperledger/besu/evm/operation/ExtCallOperation.java index bf986a572e5..5914f0e966a 100644 --- a/evm/src/main/java/org/hyperledger/besu/evm/operation/ExtCallOperation.java +++ b/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. diff --git a/evm/src/main/java/org/hyperledger/besu/evm/operation/ExtDelegateCallOperation.java b/evm/src/main/java/org/hyperledger/besu/evm/operation/ExtDelegateCallOperation.java index bddb337c73b..db851ee5a0b 100644 --- a/evm/src/main/java/org/hyperledger/besu/evm/operation/ExtDelegateCallOperation.java +++ b/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. * diff --git a/evm/src/main/java/org/hyperledger/besu/evm/operation/ExtStaticCallOperation.java b/evm/src/main/java/org/hyperledger/besu/evm/operation/ExtStaticCallOperation.java index 3a202f46e71..ef3dc042d9c 100644 --- a/evm/src/main/java/org/hyperledger/besu/evm/operation/ExtStaticCallOperation.java +++ b/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. * diff --git a/evm/src/test/java/org/hyperledger/besu/evm/operations/ExtCallOperationTest.java b/evm/src/test/java/org/hyperledger/besu/evm/operations/ExtCallOperationTest.java index 984ce28027f..946dd0b52eb 100644 --- a/evm/src/test/java/org/hyperledger/besu/evm/operations/ExtCallOperationTest.java +++ b/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) diff --git a/testutil/src/main/java/org/hyperledger/besu/testutil/JsonTestParameters.java b/testutil/src/main/java/org/hyperledger/besu/testutil/JsonTestParameters.java index 062e14cfc7f..f95ac9348ea 100644 --- a/testutil/src/main/java/org/hyperledger/besu/testutil/JsonTestParameters.java +++ b/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 getFilteredFiles(final String[] paths) { final List 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());