Skip to content

feat(spec, spec-specs): EIP-7997#2802

Open
kclowes wants to merge 5 commits into
ethereum:eips/amsterdam/eip-7997from
kclowes:eips/amsterdam/eip-7997
Open

feat(spec, spec-specs): EIP-7997#2802
kclowes wants to merge 5 commits into
ethereum:eips/amsterdam/eip-7997from
kclowes:eips/amsterdam/eip-7997

Conversation

@kclowes

@kclowes kclowes commented May 4, 2026

Copy link
Copy Markdown
Contributor

🗒️ Description

EIP-7997 implementation and tests

🔗 Related Issues or PRs

#1988

✅ Checklist

  • All: Ran fast static checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:
    just static
  • All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
  • All: Considered updating the online docs in the ./docs/ directory.
  • All: Set appropriate labels for the changes (only maintainers can apply labels).
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.
  • [ ] Tests: For PRs implementing a missed test case, update the post-mortem document to add an entry the list.
  • [ ] Ported Tests: All converted JSON/YML tests from ethereum/tests or tests/static have been assigned @ported_from marker.

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@kclowes kclowes changed the title Eips/amsterdam/eip 7997 feat(spec, spec-specs): EIP-7997 May 4, 2026
@kclowes kclowes added A-spec-specs Area: Specification—The Ethereum specification itself (eg. `src/ethereum/*`) C-feat Category: an improvement or new feature A-test-specs Area: execution_testing.specs labels May 4, 2026
@codecov

codecov Bot commented May 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.50%. Comparing base (6137bb3) to head (9f9e080).

Files with missing lines Patch % Lines
src/ethereum/forks/amsterdam/fork.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@                     Coverage Diff                     @@
##           eips/amsterdam/eip-7997    #2802      +/-   ##
===========================================================
- Coverage                    90.50%   90.50%   -0.01%     
===========================================================
  Files                          535      535              
  Lines                        32407    32411       +4     
  Branches                      3011     3011              
===========================================================
+ Hits                         29331    29333       +2     
- Misses                        2559     2561       +2     
  Partials                       517      517              
Flag Coverage Δ
unittests 90.50% <50.00%> (-0.01%) ⬇️

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

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kclowes kclowes force-pushed the eips/amsterdam/eip-7997 branch from 4eb98bd to 2daf7a9 Compare May 6, 2026 20:22
@LouisTsai-Csie LouisTsai-Csie self-requested a review May 7, 2026 06:10
@petertdavies

Copy link
Copy Markdown
Contributor

Deploying a system contract in the precompile range has the potential to cause weird bugs in clients that have longstanding hardcoded assumptions that the 0x01-0xff range is only precompiles. This isn't a precompile, it's just a regular contract. The level of testing required is higher than might superficially appear from the EIP.

The included tests look pretty good, but I could think of few more:

  • Test the interaction between 0x12 and access list prewarming. The EIP is silent on prewarming, as written this PR does not prewarm.
  • Try CALLCODEing it.
  • Give it a balance using SELFDESTRUCT.

@kclowes

kclowes commented May 7, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @petertdavies! I will add those! It is very strange that this is in the precompile range, but is not a precompile.

@marioevz marioevz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great so far, a few comments. Thanks!

ref:ethereum.forks.amsterdam.fork.DETERMINISTIC_FACTORY_ADDRESS

"""
code_hash = store_code(old.state, DETERMINISTIC_FACTORY_CODE)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting, I did not know we had this functionality!

I'm not sure though how do we signal from the tests that a block is the first block in the fork?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@marioevz I'm not sure what you mean by:

I'm not sure though how do we signal from the tests that a block is the first block in the fork?

Can you clarify? AFAIK, the tests don't call apply_fork, and AIUI, we'd have to test with a pre_alloc mutable as discussed below, for inherently not a great test. This is also highlighted in the codecov report since there is not a great way to test this method. Let me know if you see another way though! I'm happy to update.

REFERENCE_SPEC_GIT_PATH = ref_spec_7997.git_path
REFERENCE_SPEC_VERSION = ref_spec_7997.version

pytestmark = pytest.mark.valid_from("Amsterdam")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
pytestmark = pytest.mark.valid_from("Amsterdam")
pytestmark = pytest.mark.valid_from("EIP7997")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should test the transition to the fork where this EIP is enabled, but that circles back to my question on the first comment since I'm not really sure how we signal this in the t8n. Maybe @gurukamath has more insight.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think that the best we can do is do a pre_alloc_mutable and do something like: pre[0x12] - Account(code=b"", nonce=0, balance=0, and assert that you can call the contract, and it returns success for the call, but it doesn't return anything else. It looks like we do a similar thing here. I'm not sure how much value that test really adds though.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I dont fully understand here, why couldnt we test this state transition like other eip? Using block 14,999 -> 15,000 -> 15,001. We could just do EXTCODEHASH, EXTCODESIZE, EXTCODECOPY to the contract and compare the result across these three blocks?

@kclowes kclowes marked this pull request as ready for review May 11, 2026 17:40
@kclowes

kclowes commented May 11, 2026

Copy link
Copy Markdown
Contributor Author

@marioevz @petertdavies @LouisTsai-Csie - this is ready for another look when you get a chance. I added the tests @petertdavies suggested.

I'm not sure how to raise the question of whether this contract should be prewarmed - is that something that we'd generally put into the tests and see which clients fail or is it something we'd bring up in a sync call or comment on the EIP?

Edit to add: I don't think there is a way around the apply_fork coverage fail, but very happy to hear suggestions!

@LouisTsai-Csie

LouisTsai-Csie commented May 19, 2026

Copy link
Copy Markdown
Collaborator

Deploying a system contract in the precompile range has the potential to cause weird bugs in clients that have longstanding hardcoded assumptions that the 0x01-0xff range is only precompiles.

I agree with Peter, deploying a contract in the precompile range is confusing. Although i understand design choice, what about using 0xde ("deploy") instead of 0x12? If we add more precompiles later, we'd have a cleaner range without a system contract in the middle.

Could we PR to the eip?

@LouisTsai-Csie LouisTsai-Csie left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice work, some additional testing idea:

  • Account delegate to the 0x12
  • Constructor revert with large data (16KB+)
  • Oversized initcode revert
  • 0xEF-prefix deployed code revert

Since this EIP isn't SFI, should we create eips/amsterdam/eip-7997 from forks/amsterdam and merge into that branch instead of directly into forks/amsterdam?

main
 └── forks/amsterdam              ← Integration branch for Amsterdam fork
      ├── eips/amsterdam/eip-7997 ← Sub-integration branch for individual EIP
      │    ├── feature-A          ← Your PR
      │    └── feature-B          ← Someone else's PR
      ├── eips/amsterdam/eip-XXXX
      └── ...

One more thing: should we update eth_config to include this system contract?

Comment thread tests/amsterdam/eip7997_deterministic_factory_predeploy/test_factory.py Outdated
tx=Transaction(
sender=pre.fund_eoa(),
to=caller,
gas_limit=200_000,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would suggest we add metadata to the caller.

Op.SSTORE(0, Op.EXTCODESIZE(FACTORY), original_value=0, new_value=1) + Op.STOP

So that this hardcoded gas limit would not be affected by other repricing eip (e.g., eip-8038).

There are more instance below, what do you think? Maybe this is too much to change.

Comment thread tests/amsterdam/eip7997_deterministic_factory_predeploy/test_factory.py Outdated
Comment thread tests/amsterdam/eip7997_deterministic_factory_predeploy/test_factory.py Outdated
expected_address = compute_create2_address(FACTORY, salt, initcode)
pre_balance = 0xBA1

pre[expected_address] = Account(balance=pre_balance)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we could fund it via CALL, this way we make the test compatible with execute mode.

"""EIP-7997 class."""

@classmethod
def deterministic_factory_predeploy_address(cls) -> Address | None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This could be a problem for benchmarking tests. We've already deployed a large number of contracts using the original factory. After the fork activates in benchmark tests, our address calculations would change, leading to mismatch. But i have no good suggestion for now, let me think about it.

@kclowes kclowes changed the base branch from forks/amsterdam to eips/amsterdam/eip-7997 June 4, 2026 19:54
@kclowes kclowes force-pushed the eips/amsterdam/eip-7997 branch from 97a92d1 to d36c5ff Compare June 4, 2026 20:25
@jochem-brouwer

Copy link
Copy Markdown
Member

Cross-linking: #1988

@spencer-tb

Copy link
Copy Markdown
Contributor

Cc-ing EIP update: ethereum/EIPs#11783

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-spec-specs Area: Specification—The Ethereum specification itself (eg. `src/ethereum/*`) A-test-specs Area: execution_testing.specs C-feat Category: an improvement or new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants