-
Notifications
You must be signed in to change notification settings - Fork 153
feat(tools,forks): Extend EEST to support EIP-7928 payload #1866
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
base: feat/block-access-list
Are you sure you want to change the base?
Conversation
Unsure what this error is about:
|
I haven't modified the engine API payload yet because there are no specifications for it. I can add it if it would help clients prototype more effectively. cc: @nerolation - We need to update the Engine API specs once the EIP is SFI'd. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing this.
I left a couple of comments that should help clarify how these kinds of test should be written IMO.
@@ -114,6 +114,9 @@ class Environment(EnvironmentGeneric[ZeroPaddedHexNumber]): | |||
withdrawals: List[Withdrawal] | None = Field(None) | |||
extra_data: Bytes = Field(Bytes(b"\x00"), exclude=True) | |||
|
|||
# EIP-7928: Block-level access lists | |||
bal_hash: Hash | None = Field(None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this belongs to theResult
returned from the transition tool after executing the state transition.
Environment
is more for things that affect the EVM execution and/or are observable in the EVM context.
If we put it in Result
, we can catch it in here:
execution-spec-tests/src/ethereum_test_specs/blockchain.py
Lines 522 to 533 in 485ff27
header = FixtureHeader( | |
**( | |
transition_tool_output.result.model_dump( | |
exclude_none=True, exclude={"blob_gas_used", "transactions_trie"} | |
) | |
| env.model_dump(exclude_none=True, exclude={"blob_gas_used"}) | |
), | |
blob_gas_used=blob_gas_used, | |
transactions_trie=Transaction.list_root(txs), | |
extra_data=block.extra_data if block.extra_data is not None else b"", | |
fork=fork, | |
) |
and then even verify it during the test by using
header_verify
as in this example here.
) | ||
|
||
# Create block with custom header that includes BAL hash | ||
block = Block(txs=[tx], block_access_lists=bal.serialize()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be more of a verification that the BAL-specific tests do, rather than something you have to pass on every test.
There's two ways of handling BALs in tests IMO:
- The test is unconcerned with BALs, so we take the value that was returned from the transition tool for granted and just put it in the result.
- The test is meant to verify BALs, so we add the
header_verify
field when constructing the block for the result from the transition tool to be cross-checked with the one that the test expects.
The BAL will be either way be verified at client level when the test is being executed either in hive or by the client consumer, and the test should fail if the BAL hash does not match what the filled test says.
Taking into account, I think this specific example should be rewritten as something like:
block = Block(txs=[tx], block_access_lists=bal.serialize()) | |
block = Block(txs=[tx], header_verify=Header(bal_hash=bal.serialize().hash())) |
Assuming bal.serialize().hash()
returns the hash that should be placed in the header.
🗒️ Description
A sub-PR of #205 that introduces following changes to the framework:
BlockAccessLists
bal_hash
to block headerblock_access_lists
to block bodybal_hash
andblock_access_lists
to t8n environment, and test fixtures.pokebal
python package as dependency, which handles creation and serialization of BAL.🔗 Related Issues or PRs
closes #1836
✅ Checklist
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
type(scope):
.