Allow EOF creation transactions #878

pdobacz · 2024-04-30T10:06:52Z

First approach to allowing EOF creation transactions, thereby making the creator contract not necessary to deploy first EOF.

Follows the design from ipsilon/eof#78 / ethereum/EIPs#8498

However the validation problem has been resolved differently - it seemed easier to remove the validation rule to never have stray bytes in a container from the main validation_eof and to put it in the spots where it applies (TXCREATE initcontainer and subcontainers). This way this rule doesn't apply to creation transaction containers.

The validation state_tests are broken and should be reworked similar to the validation tests we have here, in light of the above change to validation.

The commit history reflects some interim steps, will be squashed before merging.

codecov · 2024-04-30T10:13:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.29%. Comparing base (a1e64f7) to head (abafe4f).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #878      +/-   ##
==========================================
+ Coverage   98.26%   98.29%   +0.02%     
==========================================
  Files         131      131              
  Lines       15701    15968     +267     
==========================================
+ Hits        15429    15696     +267     
  Misses        272      272

Flag	Coverage Δ
ethereum-tests	`27.40% <11.82%> (-0.47%)`	⬇️
ethereum-tests-silkpre	`19.31% <3.04%> (-0.33%)`	⬇️
execution-spec-tests	`18.70% <3.26%> (-0.31%)`	⬇️
unittests	`94.24% <100.00%> (+0.10%)`	⬆️

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

Files	Coverage Δ
lib/evmone/eof.cpp	`87.33% <100.00%> (ø)`
lib/evmone/eof.hpp	`100.00% <100.00%> (ø)`
test/state/errors.hpp	`50.00% <ø> (-1.93%)`	⬇️
test/state/host.cpp	`98.31% <100.00%> (+0.12%)`	⬆️
test/state/host.hpp	`100.00% <ø> (ø)`
test/state/state.cpp	`91.45% <100.00%> (ø)`
test/unittests/eof_example_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/eof_validation_test.cpp	`100.00% <100.00%> (ø)`
...est/unittests/state_transition_eof_create_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/state_tx_test.cpp	`100.00% <100.00%> (ø)`

test/state/host.cpp

gumb0 · 2024-05-02T16:56:38Z

So this kinda changes the definition of validation (at least for top-level container), which I'm not very sure is acceptable. (It would mean every client has to implement it in this way)

My other idea was something like:

move stray data bytes check from validate_section_headers() to a level above to validate_header()
make validation_section_headers() public
call validation_section_headers() in Host::prepare_message() and sum up all section sizes to find the full container size and split to container/calldata
validate container normally

pdobacz · 2024-05-06T11:08:00Z

validate container normally

Hm, you mean we redo all the previous steps (from validation_section_headers) in this last step?

gumb0 · 2024-05-06T13:10:03Z

validate container normally

Hm, you mean we redo all the previous steps (from validation_section_headers) in this last step?

Yes, I would say it's fine. Or make another variant of validate_eof() which accepts already parsed section headers.

pdobacz · 2024-05-07T06:51:16Z

The other approach is in 903dae2. I like it, but it broadens eof.hpp significantly, not sure if this is a problem. I'll give it some more thought later. Or we can just proceed and plan a rewrite of these APIs later?

pdobacz · 2024-05-07T13:12:12Z

One thing I've noticed is this. validate_header performs duties which are beyond validating the header:

validate_types
the new condition check for no stray bytes

If we could move these two outside of validate_header, we could use validate_header to parse the EOF1Header struct and not have to call again into read_valid_eof1_header in host.cpp, This is the only thing I came up with so far, but I'm not sure it's worth doing such logistics at this point. WDYT? @gumb0

Otherwise this should be good to review.

gumb0 · 2024-05-08T13:34:43Z

test/state/host.cpp

+                    if (holds_alternative<EOFValidationError>(section_headers_or_error))
+                        return {};  // Light early exception.
+
+                    const auto header = read_valid_eof1_header(input);


it feels hacky to me, we don't read to re-parse the header at this point, we already have all section sizes returned from validate_section_headers() and can sum them up to find the split.

I wanted to avoid the summing up, which we already handle in two places. BTW, did you see the comment #878 (comment), would this work in your opinion?

In general I would also prefer to take a safe and easy path now and possibly make a more thorough redesign of these validation routines, once we finalize the spec of the validations? There's still a lot of moving parts.

test/state/host.cpp

gumb0 · 2024-05-08T13:40:53Z

lib/evmone/eof.hpp

 #include <vector>

 namespace evmone
 {
+constexpr uint8_t MAGIC[] = {0xef, 0x00};


I didn't realize it will drag along all the constants into the header... What if instead of making validation_section_headers() public as is, we make a public wrapper of it that returns only full container size that we need? maybe like std::variant<size_t, EOFValidationError> validate_eof_section_headers(bytes_view)

Also MAGIC doesn't have to be in the header in any case, does it?

re MAGIC - yup, I grabbed too much. Re the fist comment, see my other comment and the idea to use validate_header instead.

pdobacz · 2024-05-15T17:01:29Z

Okay, another version of. I figured that a new public wrapper of validate_section_headers would still not fully cut it, because we like to use the header.can_init in the creation transaction pipeline. But there was yet another possibility - to use validate_header, just moving the stray bytes check one level up (to validate eof1). It seems like a valid move, because the stray bytes check doesn't logically belong with validating the header... I suppose. This allows us to use that as the header parser in host.cpp. WDYT @gumb0 ?

This version needs to rely on #891 so that needs to merge first and then I force push the cherry-picked fix away.

(cherry picked from commit 963c7f0)

While working on #878 I noticed the offset calculation in validate_header can overflow the uint16, if the _declared_ sizes of container sections are large. Currently there is no upper limit on container size in validation (there's only the indirect limit imposed by max_initcode_size), so there can be a large container, which has a subcontainer of size 0xffff, and will have it's `data_offset` not fit the uint16. We have 2 tests which happened to run into this, but the problem was masked (the data_offset was just really tiny, despite the container had a huge subcontainer, but it was never used, and the check for stray data wasn't affected by the overflow). I adjust them so (IMO) they still test what they're supposed to test, but not overflow.

test/state/host.cpp

test/state/state.cpp

test/unittests/state_transition_eof_create_test.cpp

gumb0 · 2024-05-29T10:26:35Z

Looks good now, but there's weird clang-tidy warning now, not sure why.

gumb0 · 2024-05-29T12:58:57Z

test/state/host.cpp

@@ -256,7 +256,7 @@ std::optional<evmc_message> Host::prepare_message(evmc_message msg)
                    if (holds_alternative<EOFValidationError>(header_or_error))
                        return {};  // Light early exception.

-                    const auto& header = std::get<EOF1Header>(header_or_error);
+                    const auto header = std::get<EOF1Header>(header_or_error);


So std::get is actually throwing, I guess that's why the warning.

The reference shouldn't affect this I think, so would be good to report to clang-tidy if it's possible to reproduce on some minimal example

To work around we can use get_if

const auto* header = std::get_if<EOF1Header>(&header_or_error); if (!header) return {}; // Light early exception.

I think it makes sense to add noexcept to prepare_message()

But in validate_eof1 a similar pattern is OK, which is what throws me off completely. Thanks for the suggestions, I'll poke around this

gumb0

Looks good, some squashing before merge would be nice

pdobacz commented Apr 30, 2024

View reviewed changes

test/state/host.cpp Show resolved Hide resolved

pdobacz force-pushed the isc-less-eof branch from abcf469 to 903dae2 Compare May 7, 2024 06:47

pdobacz requested a review from gumb0 May 7, 2024 13:12

pdobacz marked this pull request as ready for review May 8, 2024 10:27

gumb0 reviewed May 8, 2024

View reviewed changes

test/state/host.cpp Show resolved Hide resolved

gumb0 reviewed May 8, 2024

View reviewed changes

This was referenced May 8, 2024

Use can_init for EOFCREATE validation #887

Closed

Refactor subcontainer validation #888

Merged

chfast added the EOF label May 13, 2024

pdobacz force-pushed the isc-less-eof branch from dd5078b to a980d0c Compare May 15, 2024 13:46

pdobacz mentioned this pull request May 15, 2024

EOF: Assert against overflow in header.data_offset #891

Merged

pdobacz force-pushed the isc-less-eof branch 2 times, most recently from bf8a791 to 4b3dd12 Compare May 15, 2024 16:57

pdobacz force-pushed the isc-less-eof branch from 4b3dd12 to 4c94dca Compare May 15, 2024 17:03

pdobacz added 3 commits May 16, 2024 11:41

Legacy EOF create (squashed)

0624106

Assert against overflow in header.data_offset

0cea63f

(cherry picked from commit 963c7f0)

Avoid expansion of eof.hpp and reparsing the header

11e70a1

pdobacz force-pushed the isc-less-eof branch from 4c94dca to 11e70a1 Compare May 16, 2024 09:41

pdobacz requested a review from gumb0 May 20, 2024 10:51

pdobacz added 2 commits May 27, 2024 11:02

Merge branch 'master' into isc-less-eof

e7d5a71

Update tests and examples

0c61309

pdobacz force-pushed the isc-less-eof branch from 6d06bb1 to 0c61309 Compare May 27, 2024 09:34

Merge remote-tracking branch 'origin/master' into isc-less-eof

6a4cd80

gumb0 reviewed May 28, 2024

View reviewed changes

test/state/host.cpp Outdated Show resolved Hide resolved

gumb0 reviewed May 28, 2024

View reviewed changes

test/state/state.cpp Outdated Show resolved Hide resolved

gumb0 reviewed May 28, 2024

View reviewed changes

test/unittests/state_transition_eof_create_test.cpp Show resolved Hide resolved

Merge remote-tracking branch 'origin/master' into isc-less-eof

adb39d6

pdobacz requested a review from gumb0 May 29, 2024 09:51

gumb0 reviewed May 29, 2024

View reviewed changes

Address PR comments

9e0314e

pdobacz force-pushed the isc-less-eof branch from b2fcf18 to 9e0314e Compare May 29, 2024 16:56

gumb0 approved these changes May 30, 2024

View reviewed changes

Merge branch 'master' into isc-less-eof

abafe4f

pdobacz merged commit 3417de7 into master May 31, 2024
25 checks passed

pdobacz deleted the isc-less-eof branch May 31, 2024 13:24

pdobacz linked an issue Jun 3, 2024 that may be closed by this pull request

EOF - implement EOF creation tx #859

Closed

pdobacz mentioned this pull request Jun 3, 2024

EOF - implement EOF creation tx #859

Closed

pdobacz mentioned this pull request Oct 15, 2024

eof: Move type validation outside of header validation #1052

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow EOF creation transactions #878

Allow EOF creation transactions #878

pdobacz commented Apr 30, 2024 •

edited

Loading

codecov bot commented Apr 30, 2024 •

edited

Loading

gumb0 commented May 2, 2024 •

edited

Loading

pdobacz commented May 6, 2024 •

edited

Loading

gumb0 commented May 6, 2024

pdobacz commented May 7, 2024

pdobacz commented May 7, 2024 •

edited

Loading

gumb0 May 8, 2024

pdobacz May 8, 2024

gumb0 May 8, 2024

gumb0 May 8, 2024

pdobacz May 8, 2024

pdobacz commented May 15, 2024

gumb0 commented May 29, 2024

gumb0 May 29, 2024 •

edited

Loading

pdobacz May 29, 2024

gumb0 left a comment

Allow EOF creation transactions #878

Allow EOF creation transactions #878

Conversation

pdobacz commented Apr 30, 2024 • edited Loading

codecov bot commented Apr 30, 2024 • edited Loading

Codecov Report

gumb0 commented May 2, 2024 • edited Loading

pdobacz commented May 6, 2024 • edited Loading

gumb0 commented May 6, 2024

pdobacz commented May 7, 2024

pdobacz commented May 7, 2024 • edited Loading

gumb0 May 8, 2024

Choose a reason for hiding this comment

pdobacz May 8, 2024

Choose a reason for hiding this comment

gumb0 May 8, 2024

Choose a reason for hiding this comment

gumb0 May 8, 2024

Choose a reason for hiding this comment

pdobacz May 8, 2024

Choose a reason for hiding this comment

pdobacz commented May 15, 2024

gumb0 commented May 29, 2024

gumb0 May 29, 2024 • edited Loading

Choose a reason for hiding this comment

pdobacz May 29, 2024

Choose a reason for hiding this comment

gumb0 left a comment

Choose a reason for hiding this comment

pdobacz commented Apr 30, 2024 •

edited

Loading

codecov bot commented Apr 30, 2024 •

edited

Loading

gumb0 commented May 2, 2024 •

edited

Loading

pdobacz commented May 6, 2024 •

edited

Loading

pdobacz commented May 7, 2024 •

edited

Loading

gumb0 May 29, 2024 •

edited

Loading