-
Notifications
You must be signed in to change notification settings - Fork 168
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?
Changes from 6 commits
4172364
0723798
fc4b64e
9fcf163
3c19014
d79a022
ecd4047
1ad04f0
c990f17
02c43c2
64a3a99
8b1275b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. I feel this belongs to the
If we put it in execution-spec-tests/src/ethereum_test_specs/blockchain.py Lines 522 to 533 in 485ff27
and then even verify it during the test by using header_verify as in this example here.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @marioevz I see this differently. The test supplies the following to the client as part of a block:
In the PR (1) and (2) is part of a test. (3) is computed by the framework from (2). State transitionThe client's state transition function takes all three to produce a block. In addition to executing the transactions, the client:
The Client and NOT test verifies hashThe client MUST reject the block if computed hash does not match the provided one. ref: spec If hash is not supplied as an input to the client it will not be able to perform this check. Hence its inclusion in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me know if I misunderstood anything in this flow. We are trying to fill tests here to generate a test fixture with a built block to then test against clients. So we are essentially testing the EL's ability to locally build a valid block when we're filling tests. This block is then sent as a payload to a client via the test fixture. So I feel like we could actually perform the check at the
I think this comes into play when we're executing the test payload against a client. They will now have a filled block in the test fixture, with the hash that we validated is correct, and they should indeed correctly raise on an invalid hash according to the spec. Am I missing anything @raxhvl? Does this go along with what you were thinking @marioevz? |
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
@computed_field # type: ignore[misc] | ||||||||||||||||||||||||||
@cached_property | ||||||||||||||||||||||||||
def parent_hash(self) -> Hash | None: | ||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,14 +1,23 @@ | ||||||
"""Tests for validating EIP-7928: Block-level Access Lists (BAL).""" | ||||||
raxhvl marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
import pytest | ||||||
from pokebal.bal.types import BlockAccessList | ||||||
fselmo marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
from ethereum_test_tools import ( | ||||||
Account, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is unused, do we plan to use it? Gives lint errors atm. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For simplicity, I have broken down implementation into 2 parts.
To avoid confusion, I have cleared the contents of test file from this branch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, thank you. That context helps. |
||||||
Alloc, | ||||||
Block, | ||||||
BlockchainTestFiller, | ||||||
Transaction, | ||||||
) | ||||||
|
||||||
from .spec import ACTIVATION_FORK_NAME, ref_spec_7928 | ||||||
|
||||||
@pytest.mark.valid_from("Amsterdam") | ||||||
fselmo marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
REFERENCE_SPEC_GIT_PATH = ref_spec_7928.git_path | ||||||
REFERENCE_SPEC_VERSION = ref_spec_7928.version | ||||||
|
||||||
|
||||||
@pytest.mark.valid_from(ACTIVATION_FORK_NAME) | ||||||
class TestBALValidity: | ||||||
"""Test BAL validity and data structure integrity.""" | ||||||
|
||||||
|
@@ -18,8 +27,40 @@ def test_bal_hash_basic_transaction( | |||||
blockchain_test: BlockchainTestFiller, | ||||||
): | ||||||
"""Test BAL hash generation for basic ETH transfer.""" | ||||||
# TODO: Implement BAL hash validation for basic ETH transfer | ||||||
pass | ||||||
# Setup accounts for basic ETH transfer | ||||||
|
||||||
# TODO: Populate BAL. | ||||||
bal = BlockAccessList() | ||||||
|
||||||
transfer_amount = 1000 | ||||||
sender = pre.fund_eoa() | ||||||
recipient = pre.fund_eoa(amount=0) | ||||||
|
||||||
# Create a basic ETH transfer transaction | ||||||
tx = Transaction( | ||||||
sender=sender, | ||||||
to=recipient, | ||||||
value=1000, | ||||||
) | ||||||
|
||||||
# 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 commentThe 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 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:
Suggested change
Assuming There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Continuing the discussion, client simply rejects a block with invalid hash. For the test, client behaves like a black box which takes all three inputs to either accept or reject the block. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just getting to this comment now but this agrees with the way I'm thinking about it as well as I described above. The client will build its own BAL and compute its own hash. We do so internally in the test and will need validate that it's correct after filling. So what we need is a built BAL that can be hashed in the appropriate way (this can be |
||||||
|
||||||
# Execute the blockchain test | ||||||
blockchain_test( | ||||||
pre=pre, | ||||||
blocks=[block], | ||||||
post={ | ||||||
sender: Account( | ||||||
nonce=1, | ||||||
), | ||||||
recipient: Account(balance=transfer_amount), | ||||||
}, | ||||||
) | ||||||
|
||||||
# Note: In the generated fixture, the block header will include: | ||||||
# - bal_hash: the computed hash of the Block Access List | ||||||
# - bal_data: the SSZ-encoded Block Access List data (when framework supports it) | ||||||
|
||||||
def test_bal_hash_storage_operations( | ||||||
self, | ||||||
|
@@ -103,7 +144,7 @@ def test_bal_failed_transaction_inclusion( | |||||
pass | ||||||
|
||||||
|
||||||
@pytest.mark.valid_from("Amsterdam") | ||||||
@pytest.mark.valid_from(ACTIVATION_FORK_NAME) | ||||||
class TestBALEncoding: | ||||||
"""Test SSZ encoding/decoding of BAL data structures.""" | ||||||
|
||||||
|
@@ -153,7 +194,7 @@ def test_ssz_encoding_full_bal( | |||||
pass | ||||||
|
||||||
|
||||||
@pytest.mark.valid_from("Amsterdam") | ||||||
@pytest.mark.valid_from(ACTIVATION_FORK_NAME) | ||||||
class TestBALEdgeCases: | ||||||
"""Test edge cases and error conditions for BAL.""" | ||||||
|
||||||
|
@@ -203,7 +244,7 @@ def test_bal_maximum_block_size( | |||||
pass | ||||||
|
||||||
|
||||||
@pytest.mark.valid_from("Amsterdam") | ||||||
@pytest.mark.valid_from(ACTIVATION_FORK_NAME) | ||||||
class TestBALValidationFailures: | ||||||
"""Test validation failure scenarios for malformed or incorrect BALs.""" | ||||||
|
||||||
|
@@ -244,7 +285,7 @@ def test_malformed_ssz_rejection( | |||||
pass | ||||||
|
||||||
|
||||||
@pytest.mark.valid_from("Amsterdam") | ||||||
@pytest.mark.valid_from(ACTIVATION_FORK_NAME) | ||||||
class TestBALLimits: | ||||||
"""Test EIP-7928 specification limits and boundaries.""" | ||||||
|
||||||
|
@@ -357,7 +398,7 @@ def test_hash_size_validation( | |||||
pass | ||||||
|
||||||
|
||||||
@pytest.mark.valid_from("Amsterdam") | ||||||
@pytest.mark.valid_from(ACTIVATION_FORK_NAME) | ||||||
class TestBALBoundaryConditions: | ||||||
"""Test boundary conditions and edge cases for EIP-7928 limits.""" | ||||||
|
||||||
|
Uh oh!
There was an error while loading. Please reload this page.