EOF validation of subcontainer kinds #876

gumb0 · 2024-04-29T12:41:31Z

Implementing ipsilon/eof#86

Depends on #888

TODO:

"outside-in" refactoring - done, can be squashed with Check that container is initcontainer in creation transaction/TXCREATE
Remove ContainerKind::runtime_initcode mode - after outside-in refactoring this requires* Disallow unreferenced subcontainers and subcontainers referenced by both EOFCREATE and RETURNCONTRACT #916 so will be done there.
*if subcontainer is unreferenced, you cannot call its validation requiring neither runtime nor initcode mode.

codecov · 2024-04-29T12:45:45Z

Codecov Report

Attention: Patch coverage is 95.23810% with 9 lines in your changes missing coverage. Please review.

Project coverage is 95.11%. Comparing base (85a89e5) to head (37ed902).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #876      +/-   ##
==========================================
- Coverage   95.15%   95.11%   -0.04%     
==========================================
  Files         140      140              
  Lines       15805    15881      +76     
==========================================
+ Hits        15039    15106      +67     
- Misses        766      775       +9

Flag	Coverage Δ
eof_execution_spec_tests	`16.37% <14.81%> (-0.01%)`	⬇️
ethereum_tests	`26.87% <13.22%> (-0.06%)`	⬇️
ethereum_tests_silkpre	`18.36% <0.59%> (-0.10%)`	⬇️
execution_spec_tests	`17.83% <0.52%> (-0.11%)`	⬇️
unittests	`90.26% <95.23%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
lib/evmone/baseline.cpp	`100.00% <100.00%> (ø)`
lib/evmone/eof.hpp	`100.00% <ø> (ø)`
lib/evmone/instructions.hpp	`100.00% <100.00%> (ø)`
lib/evmone/instructions_calls.cpp	`98.66% <100.00%> (ø)`
test/eofparse/eofparse.cpp	`91.42% <100.00%> (ø)`
test/state/host.cpp	`100.00% <100.00%> (ø)`
test/statetest/statetest_loader.cpp	`96.87% <100.00%> (ø)`
test/unittests/eof_test.cpp	`93.02% <100.00%> (+0.16%)`	⬆️
test/unittests/eof_validation.hpp	`71.42% <100.00%> (+21.42%)`	⬆️
test/unittests/eof_validation_test.cpp	`99.32% <100.00%> (+0.13%)`	⬆️
... and 7 more

lib/evmone/eof.hpp

Refactoring pulled out of #876 Main idea: 1. Instruction validation returns the list of references to subcontainers (instruction, subcontainer_idx) 1.1. (Not directly related, but I moved returning accessed section list into returned `InstructionValidationResult` struct instead of non-const reference argument) 3. The check for `EOFCREATE` with truncated data is moved out of instruction validation into being done once for a referenced container, after it is already validated. 4. This way we don't need to split subcontainer header validation and instructions validation and the loop over containers gets simpler. Then it gets handy for additional container kind validation in #876.

gumb0 · 2024-05-14T15:20:43Z

lib/evmone/eof.cpp

+            if (referenced_by_eofcreate)
+                return EOFValidationError::incompatible_container_kind;
+        }
+


Validation whether subcontainer is referenced by anything can be added here, or below in "enqueue subcontainers" loop, better in a separate PR.

gumb0 · 2024-06-04T12:55:04Z

test/eoftest/eoftest_runner.cpp

+        return ContainerKind::runtime;
+    else if (s == "initcode")
+        return ContainerKind::initcode;
+    else if (s == "initcode_runtime")


Not sure it makes much sense to support this mode in the tests. If we don't then probably validate_eof() also shouldn't support it as input (should fail with special error?)

It is proposed to remove this from the spec and #916 has a commit that implements the removal.

gumb0 · 2024-06-06T16:18:54Z

Required ethereum/tests update:

It requires the change in validation tests format (adding optional kind field) - I currently have done this manually in ipsilon/eof-initcode-tests branch, but proper support would require change in retesteth.
Alternatively we remove the failing cases from ethereum/tests and port them to EEST.

cc @hugo-dc

I'm also fine merging this PR with my "temporary" manual fix of thereum/tests.

gumb0 · 2024-06-21T14:34:17Z

Alternatively we remove the failing cases from ethereum/tests and port them to EEST.

I plan to do this.

chfast · 2024-06-23T09:17:05Z

test/eoftest/eoftest_runner.cpp

 void from_json(const json::json& j, EOFValidationTest::Case& o)
 {
    const auto op_code = evmc::from_hex(j.at("code").get<std::string>());
    if (!op_code)
        throw std::invalid_argument{"code is invalid hex string"};
    o.code = *op_code;

+    if (const auto it_kind = j.find("kind"); it_kind != j.end())


Is this standardized?

No, this is not supported anywhere, but I plan to try to add this to EEST.

chfast · 2024-06-23T09:23:03Z

test/unittests/state_transition_eof_create_test.cpp

@@ -1162,7 +1062,7 @@ TEST_F(state_transition, txcreate_initcontainer_return)
    tx.to = To;
    pre.insert(*tx.to, {.nonce = 1, .code = factory_container});

-    expect.post[*tx.to].nonce = pre.get(*tx.to).nonce + 1;
+    expect.post[*tx.to].nonce = pre.get(*tx.to).nonce;
    expect.post[*tx.to].storage[0x00_bytes32] = 0x00_bytes32;


It is not exactly void, the check is only done if a value is assigned to "expect". There are std::optionals.

chfast · 2024-06-23T09:24:05Z

test/unittests/state_transition_eof_create_test.cpp

@@ -2089,7 +1989,7 @@ TEST_F(state_transition, txcreate_failure_after_txcreate_success)
        sstore(0, txcreate().initcode(keccak256(init_container)).salt(Salt)) +
        sstore(1, txcreate().initcode(keccak256(init_container)).salt(Salt)) +  // address collision
        sstore(2, returndatasize()) + sstore(3, 1) + OP_STOP;
-    const auto factory_container = eof_bytecode(factory_code, 5).container(init_container);


What is this change about?

It was a bug in original test, so it was failing for the wrong reason I guess.

chfast · 2024-06-23T09:30:10Z

lib/evmone/eof.cpp

+                else if (opcode == OP_RETURNCONTRACT)
+                    subcontainer_referenced_by_returncontract[index] = true;
+                else
+                    assert(false);


use intx::unreachable().

Refactor validation API to include a parameter for required container kind of the top-level container.

Make container kind configurable in validation tests.

gumb0 added the EOF label Apr 29, 2024

gumb0 commented May 3, 2024

View reviewed changes

lib/evmone/eof.hpp Outdated Show resolved Hide resolved

gumb0 force-pushed the validate-subcontainer-kinds branch 10 times, most recently from e51ba64 to 7c4ee1e Compare May 8, 2024 11:03

gumb0 mentioned this pull request May 8, 2024

Use can_init for EOFCREATE validation #887

Closed

gumb0 force-pushed the validate-subcontainer-kinds branch 4 times, most recently from 9f3f46e to e26fdad Compare May 10, 2024 12:33

gumb0 mentioned this pull request May 10, 2024

Refactor subcontainer validation #888

Merged

gumb0 force-pushed the validate-subcontainer-kinds branch from e26fdad to face250 Compare May 13, 2024 12:22

gumb0 changed the base branch from master to validate-instr-refactor May 13, 2024 12:22

gumb0 force-pushed the validate-subcontainer-kinds branch from face250 to b6abd3e Compare May 13, 2024 12:55

gumb0 force-pushed the validate-instr-refactor branch from 0e48680 to e406e19 Compare May 13, 2024 12:56

gumb0 force-pushed the validate-subcontainer-kinds branch 2 times, most recently from ba1ced0 to 1380198 Compare May 14, 2024 08:55

gumb0 force-pushed the validate-instr-refactor branch from e406e19 to dc1d74c Compare May 14, 2024 08:55

Base automatically changed from validate-instr-refactor to master May 14, 2024 15:16

gumb0 force-pushed the validate-subcontainer-kinds branch from 1380198 to 9e8b3e1 Compare May 14, 2024 15:18

gumb0 commented May 14, 2024

View reviewed changes

gumb0 force-pushed the validate-subcontainer-kinds branch 5 times, most recently from 92e80be to 0a3b1e0 Compare June 4, 2024 12:51

gumb0 commented Jun 4, 2024

View reviewed changes

gumb0 force-pushed the validate-subcontainer-kinds branch 2 times, most recently from e5f5c92 to 783757c Compare June 6, 2024 16:07

gumb0 mentioned this pull request Jun 18, 2024

Disallow unreferenced subcontainers and subcontainers referenced by both EOFCREATE and RETURNCONTRACT #916

Merged

gumb0 force-pushed the validate-subcontainer-kinds branch 3 times, most recently from a601529 to 71afb81 Compare June 21, 2024 14:31

chfast reviewed Jun 23, 2024

View reviewed changes

gumb0 added 7 commits June 24, 2024 12:39

Add validation of subcontainer kinds

7464c55

Remove initcode mode runtime checks from RETURNCONTRACT, RETURN, STOP

1a6c7ac

Fix validation tests

c3df715

Check that container is initcontainer in creation transaction/TXCREATE

baf2db0

Refactor validation API to include a parameter for required container kind of the top-level container.

Fix tests that use initcode in toplevel container

b294d38

Make container kind configurable in validation tests.

Container kind validation tests

96700b5

Support container kind field in json EOF validation tests

843878f

gumb0 force-pushed the validate-subcontainer-kinds branch from 71afb81 to d91990a Compare June 24, 2024 10:39

chfast approved these changes Jun 24, 2024

View reviewed changes

Update tests

37ed902

gumb0 force-pushed the validate-subcontainer-kinds branch from d91990a to 37ed902 Compare June 24, 2024 11:07

pdobacz approved these changes Jun 24, 2024

View reviewed changes

gumb0 merged commit b790bc7 into master Jun 24, 2024
26 checks passed

gumb0 deleted the validate-subcontainer-kinds branch June 24, 2024 12:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EOF validation of subcontainer kinds #876

EOF validation of subcontainer kinds #876

gumb0 commented Apr 29, 2024 •

edited

Loading

codecov bot commented Apr 29, 2024 •

edited

Loading

gumb0 May 14, 2024

chfast May 15, 2024

gumb0 Jun 3, 2024

gumb0 Jun 4, 2024

gumb0 Jun 6, 2024

gumb0 commented Jun 6, 2024

gumb0 commented Jun 21, 2024

chfast Jun 23, 2024

gumb0 Jun 24, 2024

chfast Jun 23, 2024

chfast Jun 23, 2024

gumb0 Jun 24, 2024

chfast Jun 23, 2024

gumb0 Jun 24, 2024

EOF validation of subcontainer kinds #876

EOF validation of subcontainer kinds #876

Conversation

gumb0 commented Apr 29, 2024 • edited Loading

codecov bot commented Apr 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Jun 6, 2024

gumb0 commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Apr 29, 2024 •

edited

Loading

codecov bot commented Apr 29, 2024 •

edited

Loading