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.

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

EIP-3860: Limit and meter initcode #525

Closed
wants to merge 3 commits into from

Conversation

yperbasis
Copy link
Member

@yperbasis yperbasis commented Nov 8, 2022

EIP-3860 is included into Shanghai. This PR implements Rules 3 & 4 of the specification. Rules 1 & 2 should be implemented on the host side. See erigontech/silkworm#808 for an example of host implementation.

@yperbasis yperbasis requested a review from chfast November 8, 2022 16:05
@codecov
Copy link

codecov bot commented Nov 8, 2022

Codecov Report

Merging #525 (0298d8a) into instr_create (4ea026b) will decrease coverage by 0.05%.
The diff coverage is 95.77%.

Additional details and impacted files
@@               Coverage Diff                @@
##           instr_create     #525      +/-   ##
================================================
- Coverage         97.45%   97.40%   -0.06%     
================================================
  Files                62       63       +1     
  Lines              5980     6047      +67     
================================================
+ Hits               5828     5890      +62     
- Misses              152      157       +5     
Flag Coverage Δ
blockchaintests 76.80% <60.00%> (-0.12%) ⬇️
statetests 72.64% <75.00%> (-0.05%) ⬇️
unittests 89.82% <87.32%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
test/state/host.hpp 40.00% <ø> (ø)
test/state/state.cpp 77.64% <62.50%> (-2.10%) ⬇️
lib/evmone/instructions_calls.cpp 100.00% <100.00%> (ø)
test/state/host.cpp 80.54% <100.00%> (ø)
test/unittests/evm_eip3860_initcode_test.cpp 100.00% <100.00%> (ø)
test/utils/bytecode.hpp 96.37% <100.00%> (+0.03%) ⬆️
lib/evmone/advanced_analysis.hpp 97.05% <0.00%> (-2.95%) ⬇️
lib/evmone/instructions.hpp 99.79% <0.00%> (-0.21%) ⬇️

@yperbasis yperbasis marked this pull request as draft November 23, 2022 08:06
@yperbasis yperbasis force-pushed the eip-3860 branch 2 times, most recently from 8381247 to bdc62f1 Compare November 23, 2022 14:09
@yperbasis yperbasis marked this pull request as ready for review November 23, 2022 14:21
@chfast chfast requested a review from gumb0 November 23, 2022 14:22
lib/evmone/instructions_calls.cpp Outdated Show resolved Hide resolved
lib/evmone/instructions_calls.cpp Outdated Show resolved Hide resolved
lib/evmone/instructions_calls.cpp Show resolved Hide resolved
Copy link
Member

@chfast chfast left a comment

Choose a reason for hiding this comment

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

I'd add rule 3 here as well (before rule 4). I like to have "light" checks on the EVM side. This also helps against multiplication overflow. Any counter-arguments?

@yperbasis
Copy link
Member Author

I'd add rule 3 here as well (before rule 4). I like to have "light" checks on the EVM side. This also helps against multiplication overflow. Any counter-arguments?

Initially I implemented rule 3 in evmone, but that deviates from geth's implementation ethereum/go-ethereum#23847 (see core/vm/evm.go). Specifically, that leads to an incorrect nonce in the createInitCodeSizeLimit_d1g0v0_Merge+3860 consensus test.

@yperbasis yperbasis marked this pull request as draft November 24, 2022 13:53
@yperbasis yperbasis force-pushed the eip-3860 branch 2 times, most recently from f2b318b to 110bff5 Compare November 30, 2022 09:50
@yperbasis
Copy link
Member Author

I've updated this PR according to ethereum/EIPs#6040.

@yperbasis yperbasis marked this pull request as ready for review November 30, 2022 09:56
@yperbasis yperbasis marked this pull request as draft November 30, 2022 10:32
@yperbasis yperbasis marked this pull request as ready for review November 30, 2022 10:43
gumb0
gumb0 previously approved these changes Nov 30, 2022
@yperbasis yperbasis force-pushed the eip-3860 branch 2 times, most recently from 725b28b to ed8a85d Compare December 7, 2022 12:24
@chfast chfast requested a review from gumb0 December 12, 2022 11:18
@chfast chfast dismissed gumb0’s stale review December 12, 2022 11:19

The order of the checks in CREATE2 are going to change in the spec.

@chfast chfast changed the base branch from master to instr_create December 28, 2022 15:03
@chfast chfast requested a review from rodiazet December 28, 2022 15:52
@chfast chfast deleted the branch ethereum:instr_create December 29, 2022 08:22
@chfast chfast closed this Dec 29, 2022
@chfast
Copy link
Member

chfast commented Jan 4, 2023

I messed up with GitHub here. Can you re-create the PR?

@yperbasis
Copy link
Member Author

I messed up with GitHub here. Can you re-create the PR?

Sure. Re-created as PR #545.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants