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: Change the failure to OOG #6249

Merged
merged 2 commits into from
Jan 9, 2023

Conversation

chfast
Copy link
Member

@chfast chfast commented Jan 2, 2023

Change the failure in case of exceeding the initcode size limit for CREATE instructions to be OOG (same as the other check and many others).

See https://ethereum-magicians.org/t/eip-3860-limit-and-meter-initcode/7018/15.

Change the failure in case of exceeding the initcode size limit for
CREATE instructions to be OOG (same as the other check and many others).
@github-actions github-actions bot added c-update Modifies an existing proposal s-review This EIP is in Review t-core labels Jan 2, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Jan 2, 2023

A critical exception has occurred:
Message: pr 6249 is already merged; quitting
(cc @alita-moore, @mryalamanchi)

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

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

lgtm

holiman
holiman previously approved these changes Jan 2, 2023
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

yperbasis
yperbasis previously approved these changes Jan 2, 2023
jochem-brouwer
jochem-brouwer previously approved these changes Jan 2, 2023
EIPS/eip-3860.md Outdated
@@ -94,7 +94,7 @@ The initcode cost for create transaction data (0.0625 gas per byte) is negligibl

### How to report initcode limit violation?

We specified that initcode size limit violation for `CREATE`/`CREATE2` results in `0` on stack. Most checks in these instructions are specified this way, except for 3 checks not specific to creation instructions (stack underflow, out of gas, static call violation). In these three cases the entire execution is exceptionally aborted. However, we decided to be consistent with the majority of the possible error conditions in order to keep the specification and implementations simple.
We specified that initcode size limit violation for `CREATE`/`CREATE2` results in exceptional abort of the execution. This place it in the group of early out-of-gas checks, including: stack underflow, memory expansion, static call violation, initcode hashing cost, and initcode cost introduced by this EIP. They precede the later "light" checks: call depth and balance. The choice gives consistency to the order of checks and lowers implementation complexity (out-of-gas checks can be performed in any order).
Copy link
Member

Choose a reason for hiding this comment

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

It also pushes the burden of length check to the caller side for factory contracts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any examples in the wild?

@chfast
Copy link
Member Author

chfast commented Jan 4, 2023

diff --git a/lib/evmone/instructions_calls.cpp b/lib/evmone/instructions_calls.cpp
index e9a6ba910..830c38129 100644
--- a/lib/evmone/instructions_calls.cpp
+++ b/lib/evmone/instructions_calls.cpp
@@ -138,7 +138,7 @@ evmc_status_code create_impl(StackTop stack, ExecutionState& state) noexcept
     const auto init_code_size = static_cast<size_t>(init_code_size_u256);

     if (state.rev >= EVMC_SHANGHAI && init_code_size > 0xC000)
-        return EVMC_SUCCESS;  // "Light" failure.
+        return EVMC_OUT_OF_GAS;

     const auto init_code_words = num_words(init_code_size);
     if constexpr (Op == OP_CREATE2)

After some refactoring the difference between two implementation variants in evmone is not very big.

jflo
jflo previously approved these changes Jan 5, 2023
@chfast
Copy link
Member Author

chfast commented Jan 5, 2023

Let's give it 24 hours before merging.

EIPS/eip-3860.md Outdated Show resolved Hide resolved
Co-authored-by: Alex Stokes <[email protected]>
@chfast chfast marked this pull request as ready for review January 9, 2023 12:18
@chfast chfast requested a review from eth-bot as a code owner January 9, 2023 12:18
@chfast chfast marked this pull request as draft January 9, 2023 12:33
@chfast chfast marked this pull request as ready for review January 9, 2023 12:34
@eth-bot eth-bot enabled auto-merge (squash) January 9, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-update Modifies an existing proposal s-review This EIP is in Review t-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants