Skip to content

Conversation

@danceratopz
Copy link
Member

Flagged by the reth team.

🗒️ Description

This PR additionally raises EngineAPIError.InvalidParams for the following 2 test cases in the [email protected] release:

tests/prague/eip7685_general_purpose_el_requests/test_request_types.py::test_invalid_request_type[fork_Prague-blockchain_test_engine--no_data-include_valid_requests_True]
tests/prague/eip7685_general_purpose_el_requests/test_request_types.py::test_invalid_request_type[fork_Prague-blockchain_test_engine--no_data-include_valid_requests_False]

These tests have a request of type 0x03 which does not contain data. As such, they are 1 byte in length and according to the API spec:

If any element is out of order, has a length of 1-byte or shorter, or more than one element has the same type byte, client software MUST return -32602: Invalid params error.

the Engine API call should raise EngineAPIError.InvalidParams (they currently do correctly raise a BlockException.INVALID_REQUESTS).

🔗 Related Issues

#1097

✅ Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md. skipped But should be added to release notes.

@danceratopz danceratopz added type:bug Something isn't working scope:tests Scope: Changes EL client test cases in `./tests` labels Jan 28, 2025
@danceratopz danceratopz requested a review from marioevz January 28, 2025 12:58
Copy link
Member Author

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

I tested client behavior locally with:

  1. [email protected]:
  • Reth, Geth fail.
  • Nethermind pass.
  1. This branch:
  • Reth, Geth, Nethermind pass.

The fact that Nethermind passed with and without the changes in this PR led to #1139:
3. This branch and #1139:

  • Reth, Geth pass
  • Nethermind fails.

@marioevz
Copy link
Member

I think we should:

  • Remove tests/prague/eip7685_general_purpose_el_requests/test_request_types.py altogether, since tests/prague/eip7685_general_purpose_el_requests/test_deposits_withdrawals_consolidations.py::test_invalid_deposit_withdrawal_consolidation_requests already covers most of the cases from that file, and even does it dynamically.
  • Add the only extra case from this file to invalid_requests_block_combinations, which is extra invalid type request with data.
  • Perhaps rename tests/prague/eip7685_general_purpose_el_requests/test_deposits_withdrawals_consolidations.py to something more generic, since in the future this file will contain tests for newer request types too, but this can be done in a future PR.

I'll open a PR to address these.

@danceratopz danceratopz mentioned this pull request Jan 29, 2025
* refactor(tests): EIP-7685: Add invalid cases

* refactor(tests): EIP-7685: Remove redundant `test_request_types.py`, remove unused fixtures

* small comment

* Update tests/prague/eip7685_general_purpose_el_requests/test_deposits_withdrawals_consolidations.py

Co-authored-by: danceratopz <[email protected]>

---------

Co-authored-by: danceratopz <[email protected]>
@marioevz marioevz merged commit 9af76b3 into main Jan 29, 2025
21 checks passed
@marioevz marioevz deleted the 7685-no-data branch January 29, 2025 17:37
marioevz added a commit to marioevz/execution-spec-tests that referenced this pull request Jan 30, 2025
…n 1 byte (ethereum#1138)

* fix(tests): raise `InvalidParams` for requests shorter than 1 byte

* Refactor to ethereum#1138 (ethereum#1141)

* refactor(tests): EIP-7685: Add invalid cases

* refactor(tests): EIP-7685: Remove redundant `test_request_types.py`, remove unused fixtures

* small comment

* Update tests/prague/eip7685_general_purpose_el_requests/test_deposits_withdrawals_consolidations.py

Co-authored-by: danceratopz <[email protected]>

---------

Co-authored-by: danceratopz <[email protected]>

---------

Co-authored-by: Mario Vega <[email protected]>
kclowes pushed a commit to kclowes/execution-spec-tests that referenced this pull request Oct 20, 2025
…n 1 byte (ethereum#1138)

* fix(tests): raise `InvalidParams` for requests shorter than 1 byte

* Refactor to ethereum#1138 (ethereum#1141)

* refactor(tests): EIP-7685: Add invalid cases

* refactor(tests): EIP-7685: Remove redundant `test_request_types.py`, remove unused fixtures

* small comment

* Update tests/prague/eip7685_general_purpose_el_requests/test_deposits_withdrawals_consolidations.py

Co-authored-by: danceratopz <[email protected]>

---------

Co-authored-by: danceratopz <[email protected]>

---------

Co-authored-by: Mario Vega <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope:tests Scope: Changes EL client test cases in `./tests` type:bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants