Skip to content

Conversation

marioevz
Copy link
Member

@marioevz marioevz commented Apr 3, 2025

🗒️ Description

Requires #1454.

Addresses following EIP changes:

I have a branch to fill these tests but it's currently failing, this is because we don't have a way of t8n interface to respond to this scenario, all our invalid block tests invalidate the block after a valid response from the t8n, but in this case we rely of t8n returning the exception along with the executed block somehow.

We need this issue addressed before merging these tests.

From the list of checks added to the EIP this PR checks only one at the moment:

  • ✅(This PR) The call has a dedicated gas limit of 30_000_000: 30_000_000 == valid block, 30_000_001 == invalid block.
  • ✅(Existing test) Gas consumed by this call does not count against the block’s overall gas usage.
  • ✅(Existing test)Both the gas limit assigned to the call and the gas consumed is excluded from any checks against the block’s gas limit.
  • ✅(Existing test) The call does not follow EIP-1559 fee burn semantics — no value should be transferred as part of this call.
  • ✅(This PR) If there is no code at WITHDRAWAL_REQUEST_PREDEPLOY_ADDRESS, the corresponding block MUST be marked invalid.
  • ✅(This PR) If the call to the contract fails or returns an error, the block MUST be invalidated.

🔗 Related Issues

✅ Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • Tests: All converted JSON/YML tests from ethereum/tests have been added to converted-ethereum-tests.txt.
  • Tests: A PR with removal of converted JSON/YML tests from ethereum/tests have been opened.
  • Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

@winsvega
Copy link
Contributor

winsvega commented Apr 7, 2025

here you hit the case where t8n must do some validation logic. I many times ask them to do so. but many blockchain logics are out of t8n scope. and asking t8n to validate something leads to people implementing additional t8n test logic in their t8n. so instead of checking the client we checking t8n's test logic.

I would suggest we invalidate this logic/blocks on our side and don't necessarily force t8ns to do so. as t8n is used only to generate the test. the end test verification happens on hive or consume direct test runners.

@spencer-tb spencer-tb added fork:prague Prague hardfork scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature labels Apr 14, 2025
@marioevz marioevz force-pushed the eip-7002-7251-no-code-tests branch 2 times, most recently from de7f16b to c829a0c Compare April 15, 2025 19:52
Copy link
Contributor

@spencer-tb spencer-tb left a comment

Choose a reason for hiding this comment

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

Looks good from my side so far. Spent a bit of time today looking into this.

@marioevz marioevz force-pushed the eip-7002-7251-no-code-tests branch from c829a0c to 2fbcb78 Compare April 16, 2025 23:07
@marioevz marioevz marked this pull request as ready for review April 16, 2025 23:07
@marioevz
Copy link
Member Author

here you hit the case where t8n must do some validation logic. I many times ask them to do so. but many blockchain logics are out of t8n scope. and asking t8n to validate something leads to people implementing additional t8n test logic in their t8n. so instead of checking the client we checking t8n's test logic.

I would suggest we invalidate this logic/blocks on our side and don't necessarily force t8ns to do so. as t8n is used only to generate the test. the end test verification happens on hive or consume direct test runners.

It's interesting that this is the first time we hit an issue after all txs have been executed in the t8n, and it was just a particular problem that in EELS t8n implementation, if something failed out of the scope of tx execution, the t8n would error out and not return anything, so there was no way of separate a t8n error on formulating the response from an error during block execution which was an actual protocol check.

@marioevz
Copy link
Member Author

PR should be ready for review @spencer-tb, I added a couple more things, including an updated eels_resolutions.json which points to this branch: https://github.com/marioevz/execution-specs/tree/forks/prague
Which is basically https://github.com/ethereum/execution-specs/tree/forks/prague, with ethereum/execution-specs#1191, ethereum/execution-specs#1183 and ethereum/execution-specs#1182 merged on top of it.

I could create a separate PR, or we could create a merge commit to preserve the commits in the PR, which I think are cleanly separated as of now 👍

Copy link
Contributor

@spencer-tb spencer-tb left a comment

Choose a reason for hiding this comment

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

LGTM!

Amazing - we just need the changelog! We should add that eels resolutions changed in this PR too within the changelog.

@marioevz marioevz force-pushed the eip-7002-7251-no-code-tests branch from 2fbcb78 to 23f0ae2 Compare April 17, 2025 15:00
@marioevz marioevz merged commit c8ec6e6 into main Apr 17, 2025
21 checks passed
@marioevz marioevz deleted the eip-7002-7251-no-code-tests branch April 17, 2025 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fork:prague Prague hardfork scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants