new(tests): add more test cases for request types #1340

felix314159 · 2025-03-21T17:09:47Z

🗒️ Description

WIP

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

…valid_deposit_withdrawal_consolidation_requests

marioevz

Some comments on this preliminary review 🙌

Let me know if any of them needs more info.

marioevz · 2025-03-21T17:25:10Z

tests/prague/eip7685_general_purpose_el_requests/test_deposits_withdrawals_consolidations.py

        pytest.param(
            [],
            [
                bytes([i]) for i in range(fork.max_request_type() + 1)
            ],  # Using empty requests, calculate the hash using an invalid calculation method:
-            # sha256(sha256(b"\0") ++ sha256(b"\1") ++ sha256(b"\2") ++ ...)
+            # sha256(sha256(b'0x00') ++ sha256(b'0x01') ++ sha256(b'0x02') ++ ...)


This is actually slightly different to what the test is actually doing:

b'\0'.hex() is equal to 00

b'0x00'.hex() is equal to 30783030, the ascii representation of string "0x00"

You are right, I think I wanted to do sth like bytes([0x00]) but we can just leave it as it was

Could also do b"\x00" because it's more common and b"\0" is a bit obscure.

marioevz · 2025-03-21T17:27:18Z

tests/prague/eip7685_general_purpose_el_requests/test_deposits_withdrawals_consolidations.py

+    #         BlockException.INVALID_REQUESTS,
+    #         id="more_than_16_withdrawals_per_block",
+    #     ),
+    # )


Copy-pasting from discord:
The limit is included in the system contract, not anywhere else, I don't think it's even included in the consensus layer clients.

So since it's not enforced by the execution clients other than by smart contract logic, it's theoretically possible to modify the system contract in a testnet to manually overload the limit, and see what happens.

Given the issue we had already with Sepolia, where the smart contract was different in the testnet, I think it would be interesting to inject a modified contract to force 17 withdrawals and at least verify that the ELs behave the same.

Either:

all EL clients mark the block as invalid

all EL clients successfully process the block

I think the latter is the one that's going to happen.

See this PR to see how to inject a different contract in the place of the normal system contract: #1280

marioevz · 2025-03-21T17:29:46Z

tests/prague/eip7685_general_purpose_el_requests/test_deposits_withdrawals_consolidations.py

@@ -5,10 +5,11 @@
 """  # noqa: E501

 from itertools import permutations
-from typing import Any, Dict, Generator, List, Tuple
+from typing import Dict, Generator, List, Tuple


I think we should take the opportunity to rename this file to something more generic like test_multi_type_requests.py, so it can be updated in the future if a new request type is added.

marioevz · 2025-03-21T17:34:21Z

tests/prague/eip7685_general_purpose_el_requests/test_deposits_withdrawals_consolidations.py

+def get_permutations(
+    n: int = 3,
+) -> Generator[
+    ParameterSet,


This is more of an union:

DepositRequest | WithdrawalRequest | ConsolidationRequest

ParameterSet only applies if the pytest.param is called.

Thanks, I tried that actually but then was shown a mypy type mismatch warning. I have silenced this warning now with # type: ignore and the filling still works without issues when using DepositRequest | WithdrawalRequest | ConsolidationRequest instead of ParameterSet

Oh really? It might be then that there's another type that is actually being returned, because mypy is usually correct.

marioevz · 2025-03-21T17:45:10Z

tests/prague/eip7685_general_purpose_el_requests/test_deposits_withdrawals_consolidations.py

+                # single_consolidation_from_contract(2),  # i=2 is not allowed cuz
+                # only 2 MAX CONSOLIDATIONS PER BLOCK (EIP-7251), but why does the error message
+                # not inform about this? only reports hash mismatch


That is because the limit not actually enforced by restricting the number of calls that can be made to the consolidation contract, rather, the consolidations executed after the limit is reached will queue up and only will be dequeued on the next block.

In general, the functions in this file that generate calls to the system contracts cannot know if their requests are going to be included in the block or not.

That's the reason behind the requests hash mismatch: the tx that generates the call is added to the block, it's executed, we add it to the list of expected requests (by our tests) in the header, but it will not match because of the queuing mechanism in the contract.

Hope I explained myself ok, but if not let me know 😄

marioevz · 2025-03-21T17:50:09Z

tests/prague/eip7685_general_purpose_el_requests/test_deposits_withdrawals_consolidations.py

+                    )  # when using larger numbers: it fails around 1000 due to:
+                    # UnixHTTPConnectionPool(host='localhost', port=None): Read timed out.
+                    # (read timeout=20), workaround: in transition_tool.py use
+                    #  SLOW_REQUEST_TIMEOUT (60 sec), so slow=True


We can add a mark to this test to avoid this and give the transition tool more time to respond:

marks=pytest.mark.slow(),

…_requests.py

… added mypy silencer for type warning

…lows 18 withdrawals per block

marioevz

I mainly reviewed test_modified_withdrawal_contract.py here, but some thoughts:

I would use --traces when running uv run fill to make sure that the transactions are indeed correctly calling the system contract and are not failing or running out of gas.
My main concern with the modified contract is that it will be a bit difficult to debug if the system call is not working correctly because currently the t8n does not provide traces for the system call (although that would be a nice feature!).

If you have the modified sys-asm contract to take a look at we could also help out reviewing that 👍

marioevz · 2025-03-25T12:56:43Z

tests/prague/eip7685_general_purpose_el_requests/test_modified_withdrawal_contract.py

@@ -0,0 +1,97 @@
+"""


Just a nitpick but I think this fie should be in tests/prague/eip7002_el_triggerable_withdrawals.

marioevz · 2025-03-25T13:01:23Z

tests/prague/eip7685_general_purpose_el_requests/test_modified_withdrawal_contract.py

+    [
+        # create 18 withdrawal contract requests (usually 16 is max, but modified_code dequeues 18)
+        [
+            single_withdrawal_from_contract(i)


I think you can simplify this a bit by using the convention in here:

execution-spec-tests/tests/prague/eip7002_el_triggerable_withdrawals/test_withdrawal_requests.py

Lines 48 to 56 in 8713172

WithdrawalRequestTransaction(

requests=[

WithdrawalRequest(

validator_pubkey=0x01,

amount=0,

fee=Spec.get_fee(0),

)

],

),

WithdrawalRequestTransaction and WithdrawalRequestContract have methods that allow you to automatically generate transactions and call to contracts to generate the withdrawal requests.

They both have these methods:

execution-spec-tests/tests/prague/eip7002_el_triggerable_withdrawals/helpers.py

Lines 76 to 82 in 8713172

def transactions(self) -> List[Transaction]:

"""Return a transaction for the withdrawal request."""

raise NotImplementedError

def update_pre(self, pre: Alloc):

"""Return the pre-state of the account."""

raise NotImplementedError

and basically you call them with the test's pre and you should obtain the transactions that need to be put in the block to trigger the withdrawals.

marioevz · 2025-03-25T14:36:26Z

tests/prague/eip7685_general_purpose_el_requests/test_modified_withdrawal_contract.py

+    )
+
+    sender: EOA = pre.fund_eoa()
+    tx: Transaction = Transaction(


See my previous comment, transactions to trigger the withdrawals can be obtained using the transactions method mentioned in there.

marioevz · 2025-03-25T14:43:54Z

tests/prague/eip7685_general_purpose_el_requests/test_modified_withdrawal_contract.py

+            Block(
+                txs=[tx],
+                # header_verify=Header(
+                # requests_hash=Requests(


Taking the previous comments into account, I think the modifications should be:

Create test parameter requests_list, that you can parametrize with values from this class:

execution-spec-tests/tests/prague/eip7002_el_triggerable_withdrawals/helpers.py

Line 15 in 8713172

class WithdrawalRequest(WithdrawalRequestBase):

The requests_list is passed to either WithdrawalRequestTransaction or WithdrawalRequestContract, and then from the instance created you can call update_pre to automatically prepare the senders of the withdrawals, and then transactions to obtain the transactions to put in the block to trigger the withdrawals.

Finally requests_list is used in this line too to obtain the expected requests hash:

Suggested change

# requests_hash=Requests(

requests_hash=Requests(requests_list),

Ideally, given the scenario here, requests_list should contain the 18 elements you want to introduce for this test case.

In my latest commit I tried to do these things however there are three issues:

The first issue is that the parameterized version (which I hoped would do the same as the code actually being used in my commit) does not work. If you uncomment it out and fill with uv run fill -k "modified_withdrawal_contract" tests/prague/ -m blockchain_test --fork=Prague -vv you can see what I mean

The second issue is that with my current code I can not pass Requests(requests_list)

The third issue is that I can not add --traces to the fill command because I get odd errors like

with open(src, 'rb') as fsrc: E FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmpuxnl0ouu/trace-0-0x14cfc1c6268f1e8f70866357f10ad719591f03bdc1b62cfb03330195a70d5b54.jsonl'

The code of my latest commit fills without errors [on my machine] (and is consumed without errors) but does not do what I want.

marioevz

Hey, I addressed your questions in each comment, it works for me locally although I didn't check for 18 withdrawals only two.

marioevz · 2025-03-25T17:38:21Z

tests/prague/eip7002_el_triggerable_withdrawals/helpers.py

@@ -176,7 +176,7 @@ def contract_code(self) -> Bytecode:
        return code + self.extra_code

    def transactions(self) -> List[Transaction]:
-        """Return a transaction for the deposit request."""
+        """Return a transaction for the withdrawal request."""


Thanks! 🙏

marioevz · 2025-03-25T17:39:31Z

tests/prague/eip7002_el_triggerable_withdrawals/test_modified_withdrawal_contract.py

+)
+from ethereum_test_types import Requests
+
+from ..eip7002_el_triggerable_withdrawals.helpers import (


Suggested change

from ..eip7002_el_triggerable_withdrawals.helpers import (

from .helpers import (

marioevz · 2025-03-25T17:39:40Z

tests/prague/eip7002_el_triggerable_withdrawals/test_modified_withdrawal_contract.py

+    # WithdrawalRequestContract,
+    WithdrawalRequestTransaction,
+)
+from ..eip7002_el_triggerable_withdrawals.spec import Spec as Spec_EIP7002


Suggested change

from ..eip7002_el_triggerable_withdrawals.spec import Spec as Spec_EIP7002

from .spec import Spec as Spec_EIP7002

marioevz · 2025-03-25T17:44:33Z

tests/prague/eip7002_el_triggerable_withdrawals/test_modified_withdrawal_contract.py

+# @pytest.mark.parametrize(
+#     "requests_list",
+#     [
+#         WithdrawalRequest(
+#             validator_pubkey=0x01,
+#             amount=0,
+#             fee=Spec_EIP7002.get_fee(0),
+#         ),
+#         WithdrawalRequest(
+#             validator_pubkey=0x01,
+#             amount=0,
+#             fee=Spec_EIP7002.get_fee(0),
+#         ),
+#     ],
+# )


Suggested change

# @pytest.mark.parametrize(

# "requests_list",

# [

# WithdrawalRequest(

# validator_pubkey=0x01,

# amount=0,

# fee=Spec_EIP7002.get_fee(0),

# ),

# WithdrawalRequest(

# validator_pubkey=0x01,

# amount=0,

# fee=Spec_EIP7002.get_fee(0),

# ),

# ],

# )

@pytest.mark.parametrize(

"requests_list",

[

pytest.param(

[

WithdrawalRequest(

validator_pubkey=0x01,

amount=0,

fee=Spec_EIP7002.get_fee(0),

),

WithdrawalRequest(

validator_pubkey=0x01,

amount=0,

fee=Spec_EIP7002.get_fee(0),

),

],

id="two_requests", # 18 requests should be very similar but with more requests, you could use range

)

],

)

Regarding comment line 59, reason is that pytest.mark.parametrize is running once with each different request, but what we want is for it to run once with a list of 18 requests, so we should pass a list of lists:

the first list being the list of parametrized cases

the second list being the list of requests in a single test

I added pytest.param because it makes this a bit clearer, but please note that this case only has 2 requests, so there's modifications left to be done to this code still.

marioevz · 2025-03-25T17:48:07Z

tests/prague/eip7002_el_triggerable_withdrawals/test_modified_withdrawal_contract.py

+                # TODO: why do i have to put the first element here to avoid an error? there is more than 1 request per block  # noqa: E501
+                requests_hash=Requests(requests_list[0]),


Suggested change

# TODO: why do i have to put the first element here to avoid an error? there is more than 1 request per block # noqa: E501

requests_hash=Requests(requests_list[0]),

requests_hash=Requests(*requests_list),

Sorry about that, my original suggestion was incorrect as Requests expects one positional parameter per request, so we expand the list here by using the* operator.

marioevz · 2025-03-25T17:49:42Z

tests/prague/eip7002_el_triggerable_withdrawals/test_modified_withdrawal_contract.py

+    # TODO: when i replace the following list with the parameterized version nothing works, why?
+    requests_list: List[WithdrawalRequest] = [
+        WithdrawalRequest(
+            validator_pubkey=0x01,
+            amount=0,
+            fee=Spec_EIP7002.get_fee(0),
+        ),
+        WithdrawalRequest(
+            validator_pubkey=0x01,
+            amount=0,
+            fee=Spec_EIP7002.get_fee(0),
+        ),
+    ]
+


Suggested change

# TODO: when i replace the following list with the parameterized version nothing works, why?

requests_list: List[WithdrawalRequest] = [

WithdrawalRequest(

validator_pubkey=0x01,

amount=0,

fee=Spec_EIP7002.get_fee(0),

),

WithdrawalRequest(

validator_pubkey=0x01,

amount=0,

fee=Spec_EIP7002.get_fee(0),

),

]

Can be removed after previous comment is applied.

new(tests): added more cases (two contract req, one EOA req) in test_…

a7d548e

…valid_deposit_withdrawal_consolidation_requests

marioevz reviewed Mar 21, 2025

View reviewed changes

felix314159 added 4 commits March 24, 2025 09:55

fixed comments

f67fbb2

rename test_deposits_withdrawals_consolidations.py to test_multi_type…

e22fe91

…_requests.py

changed type from ParameterSet to a union of the three request types,…

4310e6c

… added mypy silencer for type warning

added draft test of trying out a modified withdrawal contract that al…

fb157d6

…lows 18 withdrawals per block

marioevz reviewed Mar 25, 2025

View reviewed changes

felix314159 added 3 commits March 25, 2025 15:40

fixed docstrings

4c48495

tried to implement marios feedback

c0f885d

moved test to a better suited folder (eip7002)

b61fe3e

felix314159 force-pushed the eip-7685-testcases-added branch from 4f602a2 to b61fe3e Compare March 25, 2025 16:37

marioevz reviewed Mar 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new(tests): add more test cases for request types #1340

new(tests): add more test cases for request types #1340

felix314159 commented Mar 21, 2025

marioevz left a comment

marioevz Mar 21, 2025

felix314159 Mar 24, 2025

marioevz Mar 24, 2025

marioevz Mar 21, 2025

marioevz Mar 21, 2025

marioevz Mar 21, 2025

felix314159 Mar 24, 2025

marioevz Mar 24, 2025

marioevz Mar 21, 2025

marioevz Mar 21, 2025

marioevz left a comment

marioevz Mar 25, 2025

marioevz Mar 25, 2025

marioevz Mar 25, 2025

marioevz Mar 25, 2025

felix314159 Mar 25, 2025 •

edited

Loading

marioevz left a comment

marioevz Mar 25, 2025

marioevz Mar 25, 2025

marioevz Mar 25, 2025

marioevz Mar 25, 2025

marioevz Mar 25, 2025

marioevz Mar 25, 2025

	WithdrawalRequestTransaction(
	requests=[
	WithdrawalRequest(
	validator_pubkey=0x01,
	amount=0,
	fee=Spec.get_fee(0),
	)
	],
	),

	def transactions(self) -> List[Transaction]:
	"""Return a transaction for the withdrawal request."""
	raise NotImplementedError

	def update_pre(self, pre: Alloc):
	"""Return the pre-state of the account."""
	raise NotImplementedError

	# requests_hash=Requests(
	requests_hash=Requests(requests_list),

	from ..eip7002_el_triggerable_withdrawals.helpers import (
	from .helpers import (

	from ..eip7002_el_triggerable_withdrawals.spec import Spec as Spec_EIP7002
	from .spec import Spec as Spec_EIP7002

		# TODO: why do i have to put the first element here to avoid an error? there is more than 1 request per block # noqa: E501
		requests_hash=Requests(requests_list[0]),

	# TODO: why do i have to put the first element here to avoid an error? there is more than 1 request per block # noqa: E501
	requests_hash=Requests(requests_list[0]),
	requests_hash=Requests(*requests_list),

new(tests): add more test cases for request types #1340

Are you sure you want to change the base?

new(tests): add more test cases for request types #1340

Conversation

felix314159 commented Mar 21, 2025

🗒️ Description

🔗 Related Issues

✅ Checklist

marioevz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marioevz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felix314159 Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

marioevz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felix314159 Mar 25, 2025 •

edited

Loading