Skip to content

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Sep 11, 2025

🗒️ Description

To be clear, the current tests are not broken because the spec is implemented correctly. However, we were not enforcing certain validation rules that are required by EIP-7928. These changes introduce an initial up-front check on the entire order of the BAL coming from t8n as defined in EIP-7928:

Ordering and Determinism

The following ordering rules MUST apply:

- Addresses: lexicographic (bytewise).
- Storage keys: lexicographic within each account.
- Block access indices: ascending within each change list.

BlockAccessList (t8n model)

We fully validate all ordering upfront, based on the above rules, for the entire BAL coming from t8n. This is crucial that it meets all the requirements for the EIP implementation.

BlockAccessListExpectation (test expectation definition class)

Address Ordering

We define the expectation as a dict with address: BalAccountChanges. We don't need to check that our ordering is correct here because we will validate that the BAL coming from t8n is already in the correct order. So if the addresses we define are present at all, they will be in the correct order already, and we are free to define this as an unordered dict in the expectation test class. This is especially "nice" because utilities like pre.deploy_contract() and pre.fund_eoa() will generate addresses for us and it would be very difficult to put the burden on the test writer to place these within an OrderedDict, for example.

Transaction Index and Slot Ordering

We DO validate that the transaction indexes and slots appear in the correct order, but only as a sub-sequence of the full list - there’s no need to define every element. Since these are defined within a Python list, we do impose on the test writer that the sub-sequence they define here should be in order to maintain readability and reduce confusion for a reader of the test. I don't think this is too much to ask nor too big of a burden to place.

✅ 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).

@fselmo fselmo force-pushed the fix/bal-address-order-check branch 2 times, most recently from 69b6c8b to 81c22eb Compare September 11, 2025 21:53
The specs are currently correctly written, but we were not validating
all of the ordering according to EIP-7928:
  - Addresses: lexicographic (bytewise).
  - Storage keys: lexicographic within each account.
  - Block access indices: ascending within each change list.

This change validates the order of the BAL before we even begin to
compare against our expectation. We also now validate that the
expectations we define are subsequences within the BAL (expected order).

- refactor: Explicit check for the fields we care about up front for `model_fields_set`

- refactor: awkward comparison method should just be a validation method (_validate_change_lists)
@fselmo fselmo force-pushed the fix/bal-address-order-check branch 2 times, most recently from cddf6f9 to 4d695c0 Compare September 15, 2025 17:15
- This becomes an issue when JSON-serializing the BAL object and then
  re-filling from the fixture. We should use `HexNumber` for any Number
  fields as this correctly serializes to JSON as hex representation.
@fselmo fselmo force-pushed the fix/bal-address-order-check branch from 4d695c0 to 303b9bf Compare September 15, 2025 17:15
@fselmo
Copy link
Collaborator Author

fselmo commented Sep 15, 2025

The last commit here attempts to fix issues relate to consume direct with geth which we agreed, for now (and for consistency I think it's also good), on a chat would be hex-encoded integers.

Unrelated but maybe relevant, the current state of the tests on geth branch jwasinger/bal-execution (the latest BAL branch):

  • Pass all consume rlp tests
  • Fail for consume engine. But after debugging issues with the current engine flow in geth, pass all tests except for invalid tests (which is just an exception mismatch issue and can be mitigated via exception mapper) if / when changes similar to what are in this PR are applied.
  • Fail consume direct which geth expects as uint16 currently but will be expecting hex-encoded integers which this PR fixes. I tested the current implementation changing those values to int and all consume direct tests pass so if we agree both on the EEST side and geth side, they should end up passing once the agreement is met (hex-encoded).

^ This comment is mostly meant as a sanity check on the current state of the tests in case it helps to review with this context.

@fselmo fselmo requested a review from spencer-tb September 16, 2025 03:32
Copy link
Contributor

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

Some small comments for now! The validation functions look good to me. Will review the tests tmo :D

@fselmo
Copy link
Collaborator Author

fselmo commented Sep 16, 2025

Some small comments for now!

Sounds good, ty! I will address these today.

Will review the tests tmo

The tests here are mostly sanity checking unit tests (no actual BAL tests added here) to make sure things don't get moved around and break these expectations in the future. But yes it would be good to get a sanity check on the sanity check! 😄


edit: Suggestions applied in latest commit 🙏🏼

Copy link
Contributor

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

LGTM! Thanks :)

@spencer-tb spencer-tb merged commit 562fde1 into ethereum:main Sep 17, 2025
15 checks passed
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