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

new(tests): EIP-7251: Increase the MAX_EFFECTIVE_BALANCE (EL Consolidations) #642

Merged
merged 4 commits into from
Jul 23, 2024

Conversation

marioevz
Copy link
Member

@marioevz marioevz commented Jun 20, 2024

πŸ—’οΈ Description

Implements tests for EIP-7251

πŸ”— Related Issues

#606

βœ… 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.

@marioevz marioevz marked this pull request as ready for review June 24, 2024 20:07
@marioevz marioevz force-pushed the eip-7251 branch 2 times, most recently from 3a41f67 to f7c1325 Compare June 25, 2024 21:19
Copy link
Collaborator

@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.

Tests are looking good from my side. Helped my understanding of the EIP. These align really nicely with the 6110, 7002 & 7685 tests.

Some additional coverage ideas:

  • Should we add cases where the source and target public keys are in an invalid format? Assuming t8n rejects filling these cases then I assume we'd want to go the "overwriting tx rlp" route. Maybe we could do this for the other request eips.
  • I'm not certain what would happen when we have a duplicate consolidation request. My assumption is that both would be valid on the EL side where the CL would reject one of the requests when adding it to the queue. Nonetheless we could add a duplicate consolidation request test.
  • Maybe a test where the target and source keys are the same.
  • These might exist here already but could we do recursive consolidation requests? My guess is where we use the same target public key.

tests/prague/eip7251_consolidations/spec.py Outdated Show resolved Hide resolved
tests/prague/eip7251_consolidations/spec.py Outdated Show resolved Hide resolved
@marioevz
Copy link
Member Author

* These might exist here already but could we do recursive consolidation requests? My guess is where we use the same target public key.

I don't understand how could they be recursive since the contracts cannot do calls themselves to other contracts, do you mean on a loop?

@marioevz
Copy link
Member Author

Added a modification that ensures the test keeps producing blocks until the consolidation/withdrawal requests queues are exhausted, in order to properly validate that the invalid requests are not included in a future block. This is done to all test cases.

@marioevz marioevz requested a review from spencer-tb July 17, 2024 19:15
@marioevz marioevz changed the title [WIP] new(tests): EIP-7251: Increase the MAX_EFFECTIVE_BALANCE (EL Consolidations) new(tests): EIP-7251: Increase the MAX_EFFECTIVE_BALANCE (EL Consolidations) Jul 17, 2024
@spencer-tb
Copy link
Collaborator

* These might exist here already but could we do recursive consolidation requests? My guess is where we use the same target public key.

I don't understand how could they be recursive since the contracts cannot do calls themselves to other contracts, do you mean on a loop?

Yeah you are right, they can't be recursive!

Copy link
Collaborator

@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.

Amazing! LGTM

marioevz and others added 3 commits July 23, 2024 16:02
fix(evm_transition_tool): result type fix
Update src/ethereum_test_tools/common/conversions.py

Co-authored-by: spencer <spencer.taylor-brown@ethereum.org>
fix(tests): EIP-7251

new(tests): Add deposit+withdrawal+consolidation tests

new(tests): Add different requests in same tx

new(tests): Add invalid-order requests tests with consolidations

Update tests/prague/eip7251_consolidations/helpers.py

Co-authored-by: spencer <spencer.taylor-brown@ethereum.org>

Update tests/prague/eip7251_consolidations/spec.py

Co-authored-by: spencer <spencer.taylor-brown@ethereum.org>

Update tests/prague/eip7251_consolidations/spec.py

Co-authored-by: spencer <spencer.taylor-brown@ethereum.org>

tests: add consolidation test with same pubkey

fix(tests): EIP-7002,EIP-7251: keep producing empty blocks to exhaust queues

fix(tests): tox

fix(tests): tox fix
@marioevz marioevz merged commit a3408b3 into main Jul 23, 2024
7 checks passed
@marioevz marioevz deleted the eip-7251 branch July 23, 2024 16:44
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.

2 participants