Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Disable EOF↔Legacy cross-creation #825

Merged
merged 5 commits into from
Mar 13, 2024
Merged

Disable EOF↔Legacy cross-creation #825

merged 5 commits into from
Mar 13, 2024

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented Feb 27, 2024

Pulled out of #553

@gumb0 gumb0 added the EOF label Feb 27, 2024
@codecov Codecov
Copy link

codecov bot commented Feb 27, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 99.09091% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 98.12%. Comparing base (75e4ac5) to head (34f7b2b).

Additional details and impacted files 
@@           Coverage Diff           @@
##           master     #825   +/-   ##
=======================================
  Coverage   98.11%   98.12%           
=======================================
  Files         119      118    -1     
  Lines       11752    11842   +90     
=======================================
+ Hits        11531    11620   +89     
- Misses        221      222    +1
Flag Coverage Δ
blockchaintests 59.73% <50.00%> (-0.02%) ⬇️
statetests 61.93% <55.55%> (-0.21%) ⬇️
statetests-silkpre 23.33% <5.10%> (-0.16%) ⬇️
unittests 96.66% <98.18%> (+<0.01%) ⬆️

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

Files Coverage Δ
lib/evmone/baseline_instruction_table.cpp 100.00% <ø> (ø)
lib/evmone/instructions_calls.cpp 100.00% <100.00%> (ø)
test/state/errors.hpp 58.69% <100.00%> (+1.87%) ⬆️
test/state/state.cpp 100.00% <100.00%> (ø)
test/unittests/eof_validation_test.cpp 100.00% <100.00%> (ø)
test/unittests/state_transition_create_test.cpp 100.00% <100.00%> (ø)
test/unittests/state_tx_test.cpp 100.00% <100.00%> (ø)
test/state/host.cpp 95.96% <80.00%> (-0.80%) ⬇️

... and 1 file with indirect coverage changes

gumb0
gumb0 commented Feb 27, 2024
View reviewed changes
circle.yml Outdated
@@ -535,7 +535,7 @@ jobs:
~/tests/EIPTests/BlockchainTests/
- download_execution_tests:
repo: ipsilon/tests
rev: eof-relaxed-stack-validation-20240221
rev: create3-not-supported
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the branch form ethereum/tests#1337 and rolled back some changes there for supporting truncated data in containers. In this PR truncated data is not supported yet.

gumb0
gumb0 commented Feb 27, 2024
View reviewed changes
test/state/host.cpp Outdated
{
// Only CREATE3/4 is allowed to deploy code starting with EF.
// It must be valid EOF, which was validated before execution.
if (msg.kind == EVMC_CREATE || msg.kind == EVMC_CREATE2)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be always true on current master, but anticipates support for CREATE3/4

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use assert for now.

@gumb0 gumb0 requested review from chfast and pdobacz February 27, 2024 19:29
@chfast chfast changed the title Disable EOF<->Legacy cross-creation Disable EOF↔Legacy cross-creation Feb 28, 2024
chfast
chfast reviewed Feb 28, 2024
View reviewed changes
test/state/host.cpp Outdated
{
// Only CREATE3/4 is allowed to deploy code starting with EF.
// It must be valid EOF, which was validated before execution.
if (msg.kind == EVMC_CREATE || msg.kind == EVMC_CREATE2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use assert for now.

test/state/host.cpp Outdated Show resolved Hide resolved
test/unittests/state_transition_create_test.cpp Outdated Show resolved Hide resolved
lib/evmone/instructions_calls.cpp
@@ -182,6 +182,10 @@ Result create_impl(StackTop stack, int64_t gas_left, ExecutionState& state) noex
// init_code_offset may be garbage if init_code_size == 0.
msg.input_data = &state.memory[init_code_offset];
msg.input_size = init_code_size;

// EOF initcode is not allowed for legacy creation
if (is_eof_container({msg.input_data, msg.input_size}))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this unnecessarily modifies current behavior. If the initcode starts with EF it fails during execution.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to change the spec? I think motivation for light failure was better UX and being aligned with legacy creation transaction being invalid with EOF.
ipsilon/eof#53
cc @pdobacz

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, if we skip this, this is a change to the spec we produced (in both aspects as Andrei noted: CREATE and create tx, as they should remain aligned). I don't have a preference except that we settle the spec soon.

@gumb0 gumb0 force-pushed the eof/undefine-legacy-create branch 2 times, most recently from 2e00a7a to 68e7291 Compare March 11, 2024 16:35
@gumb0 gumb0 requested a review from chfast March 12, 2024 11:27
@gumb0 gumb0 force-pushed the eof/undefine-legacy-create branch from 68e7291 to ba4e37a Compare March 13, 2024 11:54
chfast
chfast reviewed Mar 13, 2024
View reviewed changes
test/state/host.cpp Outdated Show resolved Hide resolved
@gumb0 gumb0 force-pushed the eof/undefine-legacy-create branch from ba4e37a to f3133ba Compare March 13, 2024 13:33
@gumb0 gumb0 force-pushed the eof/undefine-legacy-create branch from f3133ba to 75365fb Compare March 13, 2024 13:37
@gumb0 gumb0 force-pushed the eof/undefine-legacy-create branch from 75365fb to 3bf96fe Compare March 13, 2024 13:44
@gumb0 gumb0 force-pushed the eof/undefine-legacy-create branch from 3bf96fe to 34f7b2b Compare March 13, 2024 14:21
@gumb0 gumb0 merged commit 808cc18 into master Mar 13, 2024
25 checks passed
@gumb0 gumb0 deleted the eof/undefine-legacy-create branch March 13, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chfast chfast chfast approved these changes

@pdobacz pdobacz Awaiting requested review from pdobacz

Assignees

@gumb0 gumb0

Labels
EOF
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gumb0 @chfast @pdobacz