Skip to content

feat(tests): add increased nonce with invalid nonce tests #1441

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

Merged
merged 6 commits into from
Apr 15, 2025

Conversation

danceratopz
Copy link
Member

@danceratopz danceratopz commented Apr 11, 2025

🗒️ Description

This PR attempts to add some missing test cases based purely on a coverage analysis of the test source itself. In particular, L269 in test_gas was not hit:

case SignerType.MULTIPLE_SIGNERS:
if authorization_invalidity_type == AuthorizationInvalidityType.REPEATED_NONCE:
# Reuse the first two authorities for the repeated nonce case
authority_iterator = cycle([next(authority_iterator), next(authority_iterator)])
for i in range(authorizations_count):
# Get the nonce of this authorization
authority_with_properties = next(authority_iterator)
increased_nonce = (
self_sponsored and i == 0
) or authority_with_properties.address_type == AddressType.EOA_WITH_SET_CODE
if increased_nonce:
if authorization_invalidity_type == AuthorizationInvalidityType.INVALID_NONCE:
nonce = 0
else:
nonce = 1
else:
if authorization_invalidity_type == AuthorizationInvalidityType.INVALID_NONCE:
nonce = 1
else:
nonce = 0

Either of these two test cases fill the line coverage gap, but I am out of my depth with 7702 and would appreciate feedback on the test cases and whether they're reasonable. In particular, the second case fails to fill for all but one parameter combo - it need further restriction on the conditions it runs (ran out of time on this one).

  1. The multiple_invalid_nonce_authorizations_self_sponsored_multiple_signers test cases fill and add coverage. All clients pass with consume-engine.
  2. The single_invalid_authorization_eoa_authority_multiple_signers test cases only fill successfully for one parameter combo - this is WIP, needs further debugging.

Perhaps this already helps you understand the fails from 2.:

FAILED tests/prague/eip7702_set_code_tx/test_gas.py::test_gas_cost[fork_Prague-state_test-single_invalid_authorization_eoa_authority_multiple_signers] - ethereum_test_base_types.composite_types.Storage.KeyValueMismatchError: incorrect value in address 0x0000000000000000000000000000000000001000 (test_code_address) for key 0x0000000000000000000000000000000000000000000000000000000000000000: want 0x35f81 (dec:221057), got 0x0 (dec:0)
FAILED tests/prague/eip7702_set_code_tx/test_gas.py::test_account_warming[fork_Prague-state_test-single_invalid_authorization_eoa_authority_multiple_signers-check_delegated_account_first_True] - ethereum_test_base_types.composite_types.Storage.KeyValueMismatchError: incorrect value in address 0x0000000000000000000000000000000000001000 (callee_address) for key 0x0000000000000000000000000000000000000000000000000000000000000001: want 0xa28 (dec:2600), got 0xa8c (dec:2700)
FAILED tests/prague/eip7702_set_code_tx/test_gas.py::test_account_warming[fork_Prague-state_test-single_invalid_authorization_eoa_authority_multiple_signers-check_delegated_account_first_False] - ethereum_test_base_types.composite_types.Storage.KeyValueMismatchError: incorrect value in address 0x0000000000000000000000000000000000001000 (callee_address) for key 0x0000000000000000000000000000000000000000000000000000000000000000: want 0xa28 (dec:2600), got 0x1450 (dec:5200)
FAILED tests/prague/eip7702_set_code_tx/test_gas.py::test_intrinsic_gas_cost[fork_Prague-state_test-valid_True-single_invalid_authorization_eoa_authority_multiple_signers] - ethereum_test_specs.helpers.UnexpectedExecutionFailError: Unexpected fail for Transaction ({'index': 0, 'nonce': 0}):

Generate coverage for this module with:

uv sync --all-extras
fill --until Prague -m state_test
 tests/prague/eip7702_set_code_tx/test_gas.py  --cov tests --cov-report=html:7702-coverage

🔗 Related Issues

None

✅ Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md.

@danceratopz danceratopz added scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature fork:prague Prague hardfork labels Apr 11, 2025
@danceratopz danceratopz requested review from fselmo and marioevz April 11, 2025 12:09
@marioevz
Copy link
Member

Created PR #1444 to fix the logic of the test expectation for the two added tests, and added a new case.

Basically, because the delegation had an invalid nonce, the test expected that there was no delegation at the authority address, but that's not true because the type of the authority was EOA_WITH_SET_CODE, so the delegation was there even though the authorization failed.

Sorry about the confusing logic to this test, at this point we are close to re-implement the EVM in the tests.

I added some more comments to help understand the test a bit better.

@danceratopz danceratopz force-pushed the 7702-add-increased-nonce-with-invalid-nonce-test branch from 0fe233f to 0a9472e Compare April 15, 2025 09:26
@danceratopz
Copy link
Member Author

This fills with the 7702-to-precompile EELS branch at commit ethereum/execution-specs@bbc4697 from:

I diffed the fixtures against those from main as a sanity check and it LGTM. Contract code and storage has changed, as expected due to the use of addresses as storage keys, but the transactions remain unchanged 👍.

@marioevz I updated eels_resolver.json as this EELS branch is the currently the best branch to fill Prague (forks/prague doesn't fill due to a rename of a t8n field, which has also been addressed in ethereum/execution-specs#1191). We should update to forks/prague as soon as this PR been merged, but execution-specs are having some CI issues at the moment.

Feel free to revert the latest commit, if you'd rather leave the old eels branch.

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, I think the eels_resolutions.json change is ok, and I have this other PR #1454 to update to forks/prague once ethereum/execution-specs#1191 is merged, but this temporary workaround should be ok.

Thanks!

@marioevz marioevz merged commit d2f5237 into main Apr 15, 2025
21 checks passed
@marioevz marioevz deleted the 7702-add-increased-nonce-with-invalid-nonce-test branch April 15, 2025 22:08
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