Skip to content

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Aug 21, 2025

🗒️ Description

This builds upon #1823 and #1866. I worked on this side by side with ethereum/execution-specs#1381 to get BAL support and tests working with each other.

🔗 Related Issues or PRs

#ethereum/execution-specs#1381

✅ Checklist

  • All: Ran fast tox checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:
    uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint
  • All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
  • All: Considered adding an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • All: Set appropriate labels for the changes (only maintainers can apply labels).
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.
  • Tests: For PRs implementing a missed test case, update the post-mortem document to add an entry the list.
  • Ported Tests: All converted JSON/YML tests from ethereum/tests or tests/static have been assigned @ported_from marker.

@fselmo fselmo added scope:forks Scope: Changes ethereum_test_forks package scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature fork: amsterdam Amsterdam hard fork labels Aug 21, 2025
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.

Some initial comments!

@@ -230,6 +233,8 @@ def genesis(cls, fork: Fork, env: Environment, state_root: Hash) -> "FixtureHead
extras = {
"state_root": state_root,
"requests_hash": Requests() if fork.header_requests_required(0, 0) else None,
# TODO: How should we handle the genesis block access list? Is `Hash(0)` fine?
Copy link
Member

Choose a reason for hiding this comment

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

This should be defined in the EIP, potentially the same hash as a block without any transactions and no system contract block level executions?

Comment on lines 153 to 167
account_changes: List[BalAccountChange] = Field(
default_factory=list, description="List of account changes in the block"
)

rlp_fields: ClassVar[List[str]] = ["account_changes"]

def to_list(self, signing: bool = False) -> List[Any]:
"""
Override to_list to return the account changes list directly.

The BlockAccessList IS the list of account changes, not a container
that contains a list, per EIP-7928.
"""
# Return the list of accounts directly, not wrapped in another list
return to_serializable_element(self.account_changes)
Copy link
Member

Choose a reason for hiding this comment

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

I think what we need here is then a EthereumTestRootModel:

class BlockAccessList(EthereumTestRootModel, RLPSerializable):
    ...
    root: List[BalAccountChange] = Field(
        default_factory=list, description="List of account changes in the block"
    )

It might be that RLPSerializable root types, but it's something we could fix in that class perhaps.

Copy link
Collaborator Author

@fselmo fselmo Aug 22, 2025

Choose a reason for hiding this comment

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

Ah, yeah! I think this is exactly the route. I will explore making RLPSerializable (or an equivalent class) work with a root type. That would be ideal and we may run into this again in the future.

Copy link
Collaborator Author

@fselmo fselmo Aug 22, 2025

Choose a reason for hiding this comment

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

@marioevz Hmm actually, now that I'm looking at the code more and played around with a RLPSerializableRootModel and a 2-model approach, I think the current approach may be best but with slight modifications. Hear me out and then let's compare it against what you're thinking because I may have missed something.

The current BlockAccessList works well in that it validates JSON from t8n... and since only its elements need to be RLPSerializable (not the container itself), we can add very basic functionality via rlp() and to_list() methods to handle the proper serialization. The verify_against() already uses model_fields_set for partial validation as you mentioned.

Making this a RootModel would sacrifice the JSON validation part and add complexity imo. A 2-model approach (RootModel + CamelModel) introduces maintenance overhead + devex takes a hit in knowing which to use and where + doesn't seem to provide very clear benefits.

Am I missing any advantages to the RootModel approach beyond the semantics of "BlockAccessList IS a list so it should be one"? (which I agree makes a whole lot of sense but seems to add complexity everywhere else). Happy to reconsider if I'm missing something!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I pushed some minor changes to simplify it, rather than trying to override RLPSerializable methods directly.

Copy link
Member

@marioevz marioevz Aug 22, 2025

Choose a reason for hiding this comment

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

Am I missing any advantages to the RootModel approach beyond the semantics of "BlockAccessList IS a list so it should be one"?

So the main advantage is pydantic compatibility, if you add this type as a field to an object, it depends on how are you trying to parse it.

I currently see that it's in Result object, so it depends if we receive this as:

{
    ...
    "blockAccessList": [ ... ]
    ...
}

Or:

{
    ...
    "blockAccessList": {
        "accountChanges": [ ... ],
    }
    ...
}

The first one is a root pydantic model, while the second one is a base pydantic model.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good. I'll go that route and I'll make the t8n BAL a root model. It'll still need the very basic rlp() instructions to its elements as that doesn't work with RLPSerializable either since the latter has properties other than root defined. Unless you have another idea there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

allows us to better describe what we are trying to verify in the actual BlockAccessList object.

Curious what this looks like. The current flow specifically only compares what is in the model_fields_set between one and the other. Is there anything else we should take into account?

Copy link
Member

Choose a reason for hiding this comment

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

So the two classes in my mind should accomplish these two different purposes:
BlockAccessList:

  • As close to the EIP description as possible
  • Should be easily transformed to RLP because it adheres to the RLP description in the EIP
  • It's used for communication in the t8n interface.
  • Should be also what ends up in fixtures (as JSON or as RLP)

BlockAccessListCheck (Name tbd):

  • Allows easier writing of expectation in tests
  • Allows to easily describe the absence-of expectation (e.g. we are testing that an address is not present in the block level access list, and the test should fail if it's present in the t8n response).
  • Contains the verify_bal method which is applied to the response from the t8n tool
  • Can be skipped by a test if the test is not BAL specific, and in this case no verification is done to the t8n response and that is passed verbatim to the fixture (we trust t8n)

Copy link
Member

@marioevz marioevz Aug 22, 2025

Choose a reason for hiding this comment

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

Open question (can be left for a later time):

  • What happens when we want to produce an invalid-block-test, e.g. where the BAL is almost entirely correct except for one missing or extra value/item? The test writer should be able to describe a mutation to the correct BAL received from the t8n tool, then this mutation is added to the BlockAccessList and it's what ends up in the test fixture/rlp.

Copy link
Collaborator Author

@fselmo fselmo Aug 22, 2025

Choose a reason for hiding this comment

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

Yeah the 2-class approach makes sense here for this separation of concerns. I'll keep these questions in mind when building out the BlockAccessListCheck class.

@fselmo fselmo force-pushed the feat/amsterdam-and-block-access-lists branch 3 times, most recently from 0422c6d to 6999fa3 Compare August 22, 2025 15:22
@fselmo fselmo changed the title feat(fork,tests): Adds amsterdam fork and block access list tests feat(tests): Support for EIP-7928 - Block-Level Access Lists Aug 22, 2025
@fselmo fselmo force-pushed the feat/amsterdam-and-block-access-lists branch from fa36237 to d6368b6 Compare August 22, 2025 21:25
@fselmo fselmo force-pushed the feat/amsterdam-and-block-access-lists branch 3 times, most recently from 2cd87e6 to 02a740d Compare August 27, 2025 21:30
fselmo added 12 commits August 30, 2025 13:25
- Use the full bal object in the t8n. We can use this to model_validate()
  into our pydantic model.
- Also, pass the bal_hash via t8n so that once we do build the BAL, validate
  the explicitly defined values, rlp encode it, then hash it, we can compare
  the full BAL against the hash and make sure that even though we only validated
  the part that was important for our test, the full BAL is also always valid.

Bonus:

- Added a new type for ``StorageKey`` because these were not being rlp encoded
  properly without the left padding. This was tricky to find and I think this
  can be a common issue. New type here seemed appropriate.
- chore(cleanup): Remove reference to told BlockAccessLists fork
- refactor(fork): ``Amsterdam`` above old EOF fork.
- Update some docstrings to remove AI excessive verbosity and add
  CodeChanges case to example.
- Use ``Number`` instead of ``int`` where appropriate
- Use ``default_factory=list`` without ``Optional`` and ``None`` where
  appropriate
- Use a root model class for the t8n BAL model. This lets us stick
  closer to the EIP definition where the BAL itself is a list.
- The class doesn't need to be `RLPSerializable`, just have
  simple instructions to serialize its elements (`rlp()`).
- Create a `BlockAccessListExpectation` class to represent the
  expected BAL for a given block. This class can describe different
  expectations for the test cases, such as defining mutations to
  be used for invalid tests.
@fselmo fselmo force-pushed the feat/amsterdam-and-block-access-lists branch from 02a740d to 0eabfef Compare August 30, 2025 19:29
@fselmo fselmo marked this pull request as ready for review August 30, 2025 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fork: amsterdam Amsterdam hard fork scope:forks Scope: Changes ethereum_test_forks package 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