-
-
Notifications
You must be signed in to change notification settings - Fork 479
feat: implement non-eip-7688 changes in alpha.12 #9606
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
Changes from all commits
236ff2d
c923cd8
9471f2f
e9f7925
764665b
5f20a23
155a433
4f17768
c7663ac
68cd17b
a241c05
c00a00b
754ccf5
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 |
|---|---|---|
|
|
@@ -84,8 +84,6 @@ export const defaultSkipOpts: SkipOpts = { | |
| // New test suite added in v1.7.0-alpha.8 (consensus-specs #5206); gloas PTC fork choice | ||
| // handling is not yet implemented in Lodestar. | ||
| /^gloas\/fork_choice\/on_payload_attestation_message\/.*$/, | ||
| // TODO GLOAS: Unskip in #9606 | ||
| /^gloas\/operations\/builder_deposit_request\/.*$/, | ||
| // TODO GLOAS: enable this after gloas fork choice is ready | ||
| /^gloas\/fork_choice_compliance\/.*/, | ||
| ], | ||
|
|
@@ -96,23 +94,6 @@ export const defaultSkipOpts: SkipOpts = { | |
| // boost wrongly denied. Fails identically on every pre-gloas fork. | ||
| // Enable this after https://github.com/ChainSafe/lodestar/issues/9666 is resolved | ||
| /fork_choice_compliance\/block_tree_test\/pyspec_tests\/block_tree_test_16_201284350_1$/, | ||
| // TODO GLOAS: Unskip in #9606 | ||
|
Member
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. it's great to see we removed a lot of skipped tests in this PR
Contributor
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. Thanks 🙏 — most of the gloas handlers that were stubbed through the earlier alphas are implemented now, so the skips could go. A few remain with TODOs for follow-up (the |
||
| /^gloas\/operations\/builder_deposit_request\/.*$/, | ||
| /\/fork_builder_deposit_followed_by_non_builder_credentials$/, | ||
| /\/fork_builder_deposit_uses_deposit_slot_epoch$/, | ||
| /\/fork_builder_deposit_version$/, | ||
| /\/fork_invalid_builder_deposit_followed_by_valid_builder_deposit$/, | ||
| /\/fork_invalid_validator_deposit_followed_by_builder_credentials$/, | ||
| /\/fork_mixed_pending_deposits$/, | ||
| /\/fork_multiple_builder_deposits$/, | ||
| /\/fork_multiple_deposits_same_builder$/, | ||
| /\/fork_single_builder_deposit$/, | ||
| /\/fork_valid_builder_deposit_followed_by_invalid_builder_deposit$/, | ||
| /\/deposit_requests_greater_than_electra_max$/, | ||
| /\/process_parent_execution_payload__new_builder_does_not_reuse_topped_up_builder_slot$/, | ||
| /\/process_builder_exit_request__success$/, | ||
| /\/process_parent_execution_payload__builder_exit_request$/, | ||
| /\/switch_to_compounding_with_pending_consolidations_at_limit$/, | ||
| ], | ||
| // TODO GLOAS: Investigate why networking tests are failing since alpha.5 | ||
| skippedRunners: ["networking"], | ||
|
|
||
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.
This updates the unaggregated attestation path to require
forkChoice.hasPayloadHexUnsafe, but the aggregate path still useschain.seenPayloadEnvelopeinpackages/beacon-node/src/chain/validation/aggregateAndProof.ts:99. In Gloas, anindex == 1aggregate submitted throughpublishAggregateAndProofsV2can therefore be accepted and published while the envelope is only inseenPayloadEnvelopeInputCacheand not fully imported with data, even though the alpha.12 gossip rule requiresis_payload_verifiedforbeacon_aggregate_and_prooftoo. Please apply the same fork-choice payload check there.Useful? React with 👍 / 👎.
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.
yes this is valid
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.
Addressed as of
c00a00bc8c("fix: require imported payload for index==1 aggregate gossip"): theindex == 1aggregate path inaggregateAndProof.tsnow gates on!chain.forkChoice.hasPayloadHexUnsafe(toRootHex(attData.beaconBlockRoot))→ IGNOREEXECUTION_PAYLOAD_NOT_SEEN, structurally identical to the unaggregated path inattestation.ts:330. NoseenPayloadEnvelopereference remains in the file, sopublishAggregateAndProofsV2now enforces the sameis_payload_verifiedgossip rule. Good catch by the bot.