Skip to content

Conversation

@danceratopz
Copy link
Member

🗒️ Description

Running coverage on the test cases themselves revealed that although helpers.py handled the case for higher call depths in update_pre:

def update_pre(self, pre: Alloc):
"""Return the pre-state of the account."""
self.sender_account = pre.fund_eoa(self.sender_balance)
self.contract_address = pre.deploy_contract(
code=self.contract_code, balance=self.contract_balance
)
self.entry_address = self.contract_address
if self.call_depth > 2:
for _ in range(1, self.call_depth - 1):
self.entry_address = pre.deploy_contract(
code=Op.CALLDATACOPY(0, 0, Op.CALLDATASIZE)
+ Op.POP(
Op.CALL(
Op.GAS,
self.entry_address,
0,
0,
Op.CALLDATASIZE,
0,
0,
)
)
)

the logic was never hit in L197:L212.

This PR adds some tests which trigger requests with a call_depth > 2.

TODO

  • Understand why this new test fails to fill: tests/prague/eip7251_consolidations/test_consolidations.py::test_consolidation_requests[fork_Prague-blockchain_test-single_block_multiple_consolidation_requests_from_contract_call_depth_high]

🔗 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.
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

@danceratopz danceratopz added scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature labels Apr 8, 2025
@marioevz
Copy link
Member

marioevz commented Apr 9, 2025

I debugged a bit but I also could not find the culprit for the failing tests. My gut says is either:

  • Not enough gas
  • Too much gas above the block limit
  • Stack too deep

@danceratopz danceratopz force-pushed the feat/add-more-call-depth-request-tests branch from 6e8b8fa to 527e335 Compare April 11, 2025 09:01
@spencer-tb spencer-tb added the fork:prague Prague hardfork label Apr 14, 2025
@danceratopz
Copy link
Member Author

These tests now fill with EELS, all clients pass via consume engine. I would like to better understand the limitations on the call depth, as it doesn't match what I'd expect with gas consumption, but I think they're good enough for now.

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

LGTM, all tests passing. Created issue #1469 of an idea that spawned from this review.

@marioevz marioevz merged commit 2b5f461 into main Apr 16, 2025
21 checks passed
@marioevz marioevz deleted the feat/add-more-call-depth-request-tests branch April 16, 2025 17:46
pacrob pushed a commit to pacrob/execution-spec-tests that referenced this pull request May 5, 2025
…h>2` (ethereum#1415)

* feat(tests): add consolidations & withdrawal requests with `call_depth>2`

* fix(tests): increase `gas_limit`; reduce depth

* fix(tests): fix tx_gas_limit, reduce depth, make test id consistent
felix314159 pushed a commit to felix314159/execution-spec-tests that referenced this pull request May 16, 2025
…h>2` (ethereum#1415)

* feat(tests): add consolidations & withdrawal requests with `call_depth>2`

* fix(tests): increase `gas_limit`; reduce depth

* fix(tests): fix tx_gas_limit, reduce depth, make test id consistent
kclowes pushed a commit to kclowes/execution-spec-tests that referenced this pull request Oct 20, 2025
…h>2` (ethereum#1415)

* feat(tests): add consolidations & withdrawal requests with `call_depth>2`

* fix(tests): increase `gas_limit`; reduce depth

* fix(tests): fix tx_gas_limit, reduce depth, make test id consistent
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.

4 participants