-
Notifications
You must be signed in to change notification settings - Fork 975
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
Deneb fork choice tests - take 2 #3463
Conversation
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.
very nice!
I'm comfortable getting this into the release if no opposition from clients (as seemingly on the call)
tests/formats/fork_choice/README.md
Outdated
If it's `false`, this execution step is expected to be invalid. | ||
block: string -- the name of the `block_<32-byte-root>.ssz_snappy` file. | ||
To execute `on_block(store, block)` with the given attestation. | ||
blobs: string or `null` -- optional, the name of the `blobs_<32-byte-root>.ssz_snappy` file. |
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.
Why null
instead of just an empty list in the file? seems like that would reduce the exceptional cases here.
blobs: string or `null` -- optional, the name of the `blobs_<32-byte-root>.ssz_snappy` file. | ||
The blobs file content is a `List[Blob, MAX_BLOBS_PER_BLOCK]` SSZ object. | ||
If it's `null`, `blobs` is an empty list. | ||
proofs: array of byte48 hex string -- optional, the proofs of blob commitments. |
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.
What if one of blobs
or proofs
exists but the other is not included? Is that an invalid test? or does one just consider the missing key as empty list?
I think it makes sense that either both keys should be there or both should be absent. And if so, maybe a note below about the relationship between the option keys
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.
And must these two lists have same length? or should they be able to handle if you have more of one than other?
If the latter, should add a test of mismatched lengths
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 the feedback. I changed it back to allow empty blobs
SSZ file.
I think it makes sense that either both keys should be there or both should be absent.
Agreed. This is also the is_blob_data_test
variable indicated in fork_choice.py::add_block
.
And must these two lists have same length?
So the verify_blob_kzg_proof_batch
API in is_data_available
would check the length consistency and throw invalid.
If the latter, should add a test of mismatched lengths
👍 added these tests.
state_transition_and_sign_block, | ||
) | ||
from eth2spec.test.helpers.sharding import ( | ||
get_sample_opaque_tx |
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.
get_sample_opaque_tx | |
get_sample_opaque_tx, |
block = build_empty_block_for_next_slot(spec, state) | ||
opaque_tx, blobs, blob_kzg_commitments, blob_kzg_proofs = get_sample_opaque_tx(spec, blob_count=1, rng=rng) | ||
block.body.execution_payload.transactions = [opaque_tx] | ||
# block.body.execution_payload.block_hash = compute_el_block_hash(spec, block.body.execution_payload) |
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.
Is this intentionally left here?
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.
No, it should have been uncommented although it's unrelated here.
|
||
def with_blob_data(spec, blob_data, func): | ||
""" | ||
This helper runs the given ``func`` with monkeypatched ``retrieve_blobs_and_proofs`` |
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.
fancy.
signed_block = state_transition_and_sign_block(spec, state, block) | ||
blob_data = BlobData(blobs, blob_kzg_proofs) | ||
|
||
def run_func_1(): |
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.
maybe run_tick_and_add_block_with_data_1
-- a little long... just a bit hesitant on run_func_1
generic name not explaining what's going on
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.
Or maybe we should hide with_blob_data
usage with a helper -- tick_and_add_block_with_data
-- that does the meta-programming so you don't have to explicitly do it each time. Seems like the exact same usage is used over and over in this file so I think helper to hide makes sense
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.
good suggestion 👍
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.
looks good!
Preparation for processing new tests from: - ethereum/consensus-specs#3463
signed_block = state_transition_and_sign_block(spec, state, block) | ||
blob_data = BlobData(blobs, blob_kzg_proofs) | ||
|
||
yield from tick_and_add_block_with_data(spec, store, signed_block, test_steps, blob_data) |
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.
there is no {tick}
entry in the resulting steps.yaml
, fork choice is still at time 12 when processing the block.
assert spec.get_head(store) == signed_block.message.hash_tree_root() | ||
|
||
# On receiving a block of next epoch | ||
store.time = current_time + spec.config.SECONDS_PER_SLOT * spec.SLOTS_PER_EPOCH |
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.
shouldn't there be a on_tick_and_append_step
to run tick processing in fork choice?
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.
Without the on_tick_and_append_step
, the followup checks
in steps.yaml
will fail as the time doesn't match with the test runner fork choice. test runner fork choice needs to be told manually to advance to the target time, via steps.yaml
assert spec.get_head(store) == signed_block.message.hash_tree_root() | ||
|
||
# On receiving a block of next epoch | ||
store.time = current_time + spec.config.SECONDS_PER_SLOT * spec.SLOTS_PER_EPOCH |
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.
Ahh nice catch! I think this line should be removed because tick_and_add_block_with_data
would call on_tick
properly.
Preparation for processing new tests from: - ethereum/consensus-specs#3463
Replace #3402
on_block
step fields:blobs
andproofs
are new fields from Deneb EIP-4844. These are the expected values fromretrieve_blobs_and_proofs()
helper insideis_data_available()
helper.is_data_available
helper: remove the "# For testing,retrieve_blobs_and_proofs
returns ("TEST", "TEST")." stub. Theretrieve_blobs_and_proofs
helper returns empty lists[] ,[]
by default, but we can usewith_blob_data
testing helper to inject the return values now.test_simple_blob_data
: one simple blobtest_invalid_incorrect_proof
: one blob commitment with incorrect prooftest_invalid_data_unavailable
: one blob commitment, butretrieve_blobs_and_proofs
returns empty lists.