-
Notifications
You must be signed in to change notification settings - Fork 971
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
Add optimistic sync tests #2982
Conversation
4ed2249
to
f8d92c3
Compare
el_block_hash = current_block.body.execution_payload.block_hash | ||
|
||
if payload_status.status != PayloadStatusV1Status.VALID: | ||
valid = False |
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.
Should the block be invalid (valid = False
) only when the status does belong to INVALIDATED
subset? I believe current logic will produce valid: False
output in case when the status is e.g. SYNCING
.
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 point! So we may have to set it manually since SYNCING
block's validity that we output here will be determined later in optimistic nodes.
I updated it in ac717b1, now it is set by parameter valid
from the test script.
Awesome work! Looks good to me except validity status of optimistic blocks, and the following duplication in the output: - {block: block_0xa1e8cd339e0ac129a0892197507f7d49d731d1a3db08d32036f926ac1dbaf335,
valid: false}
- {block: block_0xa1e8cd339e0ac129a0892197507f7d49d731d1a3db08d32036f926ac1dbaf335} It looks like we have to add an |
Fixed in ac717b1. Thanks! - {tick: 7728}
- payload_status: {status: PayloadStatusV1Status.VALID, latest_valid_hash: '0xb9a07089b65517ade3874fc891cc6206dfe5e753ac3bfb8dbf2cf0fe514b2a98',
validation_error: 'null'}
- {block: block_0x3d1241fb9fa6d442d45e1f2153abdb1158cfe90f273978b6212b80d15053d1d8}
- checks:
head: {slot: 9, root: '0x30b495d00fb542d2c513a464bba858295b884ab2451953e5876804a049afc2ef'}
- payload_status: {status: PayloadStatusV1Status.VALID, latest_valid_hash: '0xcfa86982c4ca3a0c429d223ee3000f64305e7bbb6ed57f12748a3ca1bb73e2ff',
validation_error: 'null'}
- {block: block_0x2a06b3082961d053d0b59c1fd232b298acc0353cdd58480c0d2dab16403f85fb}
- checks:
head: {slot: 10, root: '0x1486942d4953830f9bbf43141838db41686edc43ca870d5b22c500e5e0c0898b'}
- payload_status: {status: PayloadStatusV1Status.VALID, latest_valid_hash: '0xb051302363f1bdb6ef94a54cee5958539aedda44229baaf1dae3e8db08baa5de',
validation_error: 'null'}
- {block: block_0xeb2300a9028f0dc91cd026d74010f7a0122a48984d2b207071e7015698cf7887}
- checks:
head: {slot: 11, root: '0xfadf15c33151200546709d8bdeed7f00de88e15f93d9ffed48d8c1673f257c52'}
- payload_status: {status: PayloadStatusV1Status.VALID, latest_valid_hash: '0x6b5e520e48f6736070911079162157dd87cc473da78b328a5a77d18069d223b2',
validation_error: 'null'}
- {block: block_0x28cfd6113e88cfdd3903c576bada1f79aa7f3955e2ef30604e63091680a8b625}
- checks:
head: {slot: 12, root: '0x44155297210f07a144d61d57e239dc1908d08ae42edcee79a9e3d8b803ea0877'}
- payload_status: {status: PayloadStatusV1Status.SYNCING, latest_valid_hash: 'null',
validation_error: 'null'}
- {block: block_0xa1e8cd339e0ac129a0892197507f7d49d731d1a3db08d32036f926ac1dbaf335,
valid: false}
- checks:
head: {slot: 10, root: '0x3f6acbc5be90c353052aad42117cac2c9577c103adea76d1775fcec45b4c0c72'}
- payload_status: {status: PayloadStatusV1Status.SYNCING, latest_valid_hash: 'null',
validation_error: 'null'}
- {block: block_0x3c131a495076f7af107cb3a02f84f7c8fff22ccd5d9204e50e95f4c1ed640269,
valid: false}
- checks:
head: {slot: 11, root: '0x55dd6957eea9d4af18165761974233976808c88b64cbcd9f53328e514f1c5cca'}
- payload_status: {status: PayloadStatusV1Status.SYNCING, latest_valid_hash: 'null',
validation_error: 'null'}
- {block: block_0x23aa3b0a1ca807c7de4724944aa46f19bb0530198bf5ac77196f2c877337c65e,
valid: false}
- checks:
head: {slot: 12, root: '0xe43498e53d60a08019406571637ae1b63122e3b9e2a7e97a2c7378dcacef6c55'}
- payload_status: {status: PayloadStatusV1Status.INVALID, latest_valid_hash: '0xb9a07089b65517ade3874fc891cc6206dfe5e753ac3bfb8dbf2cf0fe514b2a98',
validation_error: invalid}
- {block: block_0x0fd4c324301e908366d2e53a0b9f601ffb4c33f5726939a975a9d4d5fa73a293,
valid: false}
- checks:
head: {slot: 12, root: '0x44155297210f07a144d61d57e239dc1908d08ae42edcee79a9e3d8b803ea0877'}
Right, that was why I said "In these tests, all the beacon blocks are valid except for the payload which is determined by the payload_status value". With this assumption, the I'm fine with either way. |
Sync test vectors updates
|
Got it! Let's keep it simple 👍 So, CL clients who do support this format should ignore |
This comment was marked as spam.
This comment was marked as spam.
Since this is a draft I will have a request to change the semanthics of |
Some preliminary feedback on the test format:
(haven't actually got them running yet, will post again once I have) |
b9cb5a5
to
0f8b5ae
Compare
@potuz good call. we can address it in the test format.
You're right. These bugs are fixed in 0f8b5ae 🙏
Agreed 👍 It will be as same as the engine API response. I updated it in 0f8b5ae Updated example: - {tick: 7728}
- block_hash: '0xb9a07089b65517ade3874fc891cc6206dfe5e753ac3bfb8dbf2cf0fe514b2a98'
payload_status: {status: VALID, latest_valid_hash: '0xb9a07089b65517ade3874fc891cc6206dfe5e753ac3bfb8dbf2cf0fe514b2a98',
validation_error: null}
- {block: block_0x3d1241fb9fa6d442d45e1f2153abdb1158cfe90f273978b6212b80d15053d1d8}
- checks:
head: {slot: 9, root: '0x30b495d00fb542d2c513a464bba858295b884ab2451953e5876804a049afc2ef'}
- block_hash: '0xcfa86982c4ca3a0c429d223ee3000f64305e7bbb6ed57f12748a3ca1bb73e2ff'
payload_status: {status: VALID, latest_valid_hash: '0xcfa86982c4ca3a0c429d223ee3000f64305e7bbb6ed57f12748a3ca1bb73e2ff',
validation_error: null}
- {block: block_0x2a06b3082961d053d0b59c1fd232b298acc0353cdd58480c0d2dab16403f85fb}
- checks:
head: {slot: 10, root: '0x1486942d4953830f9bbf43141838db41686edc43ca870d5b22c500e5e0c0898b'}
- block_hash: '0xb051302363f1bdb6ef94a54cee5958539aedda44229baaf1dae3e8db08baa5de'
payload_status: {status: VALID, latest_valid_hash: '0xb051302363f1bdb6ef94a54cee5958539aedda44229baaf1dae3e8db08baa5de',
validation_error: null}
- {block: block_0xeb2300a9028f0dc91cd026d74010f7a0122a48984d2b207071e7015698cf7887}
- checks:
head: {slot: 11, root: '0xfadf15c33151200546709d8bdeed7f00de88e15f93d9ffed48d8c1673f257c52'}
- block_hash: '0x6b5e520e48f6736070911079162157dd87cc473da78b328a5a77d18069d223b2'
payload_status: {status: VALID, latest_valid_hash: '0x6b5e520e48f6736070911079162157dd87cc473da78b328a5a77d18069d223b2',
validation_error: null}
- {block: block_0x28cfd6113e88cfdd3903c576bada1f79aa7f3955e2ef30604e63091680a8b625}
- checks:
head: {slot: 12, root: '0x44155297210f07a144d61d57e239dc1908d08ae42edcee79a9e3d8b803ea0877'}
- block_hash: '0x13d0f34b2c411f286db9f8164333a97e20793ac64313a5f68fb20a7decc1487d'
payload_status: {status: SYNCING, latest_valid_hash: null, validation_error: null}
- {block: block_0xa1e8cd339e0ac129a0892197507f7d49d731d1a3db08d32036f926ac1dbaf335,
valid: false}
- checks:
head: {slot: 10, root: '0x3f6acbc5be90c353052aad42117cac2c9577c103adea76d1775fcec45b4c0c72'}
- block_hash: '0xfa8e3d85ee065a9350cd480ac76de90f6d8238d1c93f1f5fb7339d05e280d832'
payload_status: {status: SYNCING, latest_valid_hash: null, validation_error: null}
- {block: block_0x3c131a495076f7af107cb3a02f84f7c8fff22ccd5d9204e50e95f4c1ed640269,
valid: false}
- checks:
head: {slot: 11, root: '0x55dd6957eea9d4af18165761974233976808c88b64cbcd9f53328e514f1c5cca'}
- block_hash: '0x3b99e4259065cc75f1c8d658d3c37a4d63cb712a263fb410b93771b109aeb47f'
payload_status: {status: SYNCING, latest_valid_hash: null, validation_error: null}
- {block: block_0x23aa3b0a1ca807c7de4724944aa46f19bb0530198bf5ac77196f2c877337c65e,
valid: false}
- checks:
head: {slot: 12, root: '0xe43498e53d60a08019406571637ae1b63122e3b9e2a7e97a2c7378dcacef6c55'}
- block_hash: '0xe874f491ac490606d848657cc80c48f92975820c35d81e9ece632fc3702ac378'
payload_status: {status: INVALID, latest_valid_hash: '0xb9a07089b65517ade3874fc891cc6206dfe5e753ac3bfb8dbf2cf0fe514b2a98',
validation_error: invalid}
- {block: block_0x0fd4c324301e908366d2e53a0b9f601ffb4c33f5726939a975a9d4d5fa73a293,
valid: false}
- checks:
head: {slot: 12, root: '0x44155297210f07a144d61d57e239dc1908d08ae42edcee79a9e3d8b803ea0877'} |
I think we should remove |
yeah, I would rather see for these blocks |
I would make |
I think this is exactly the same as my request isn't it? in any case I'm pleased if this interpretation is taken. I manually changed the steps file and we pass with those conditions, without any code changes. |
My suggestion is to have |
I've just got the test to pass in Lighthouse by changing all |
I can live with EDIT: Also notice that from the spec perspective, a block that fails validation of the EE is in fact invalid on the CL side, so it makes really little sense to have |
After clarification in Discord, my suggestion for
Justification: |
Thank you for the feedback! 2e73091 implmeneted #2982 (comment) suggestions Sync test vectors updates
|
Thanks for the new format, confirming Prysm passes those vectors without any changes to the runner |
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 to me! 🚀
## Issue Addressed Implements new optimistic sync test format from ethereum/consensus-specs#2982. ## Proposed Changes - Add parsing and runner support for the new test format. - Extend the mock EL with a set of canned responses keyed by block hash. Although this doubles up on some of the existing functionality I think it's really nice to use compared to the `preloaded_responses` or static responses. I think we could write novel new opt sync tests using these primtives much more easily than the previous ones. Forks are natively supported, and different responses to `forkchoiceUpdated` and `newPayload` are also straight-forward. ## Additional Info Blocked on merge of the spec PR and release of new test vectors.
## Issue Addressed Implements new optimistic sync test format from ethereum/consensus-specs#2982. ## Proposed Changes - Add parsing and runner support for the new test format. - Extend the mock EL with a set of canned responses keyed by block hash. Although this doubles up on some of the existing functionality I think it's really nice to use compared to the `preloaded_responses` or static responses. I think we could write novel new opt sync tests using these primtives much more easily than the previous ones. Forks are natively supported, and different responses to `forkchoiceUpdated` and `newPayload` are also straight-forward. ## Additional Info Blocked on merge of the spec PR and release of new test vectors.
Implements new optimistic sync test format from ethereum/consensus-specs#2982. - Add parsing and runner support for the new test format. - Extend the mock EL with a set of canned responses keyed by block hash. Although this doubles up on some of the existing functionality I think it's really nice to use compared to the `preloaded_responses` or static responses. I think we could write novel new opt sync tests using these primtives much more easily than the previous ones. Forks are natively supported, and different responses to `forkchoiceUpdated` and `newPayload` are also straight-forward. Blocked on merge of the spec PR and release of new test vectors.
Issue
Address ethereum/consensus-spec-tests#28
Description
Test format: #2965
Example
test_from_syncing_to_invalid
is the basic test from ethereum/consensus-spec-tests#28 issue description:minimal/bellatrix/sync/optimistic/pyspec_tests/from_syncing_to_invalid/steps.yaml
(20220908 fixed)Testing tool implementation
MegaStore
: combine fork choice'sStore
and optimistic sync'sOptimisticStore
during the test scripts.ExecutionEngine
.payload_status
value. i.e., no other state transition exceptions except fornotify_new_payload
.Sync test vectors:
TODOs: