Gloas fork choice redux#9025
Conversation
…to gloas-fc-proto
…to gloas-fc-proto
Co-authored-by: Co-author hopinheimer <knmanas7@gmail.com> Co-authored-by: Co-author brech1 <11075677+brech1@users.noreply.github.com>
michaelsproul
added
ready-for-review
| spec: &ChainSpec, | ||
| time_into_slot: Duration, |
There was a problem hiding this comment.
Typical order is spec last
| // direction). If this child is on the FULL path from the parent, | ||
| // all weight supports the parent's FULL virtual node, and vice versa. | ||
| if let Ok(child_v29) = node.as_v29() { | ||
| if child_v29.parent_payload_status == PayloadStatus::Full { |
There was a problem hiding this comment.
this should be a match parent_payload_status, should we handle parent_payload_status == Pending?
There was a problem hiding this comment.
Should be unreachable, right? Maybe we could consider using a more restrictive enum for the parent_payload_status (just Full or Empty).
|
the attestation due deadline changes based on fork, fixed on dapplion@f45e420cf which also adds the exact fractions to the chainspec |
dapplion
left a comment
dapplion
left a comment
There was a problem hiding this comment.
Aside 2 minor details, good to go!
I checked the spec parity of all new functions in fork-choice and correctness of the new code. This PR regresses in performance introducing O(n^2) but we can fix easily later
…nSpec Spec: `get_attestation_due_ms(epoch)` uses ATTESTATION_DUE_BPS_GLOAS (2500) for Gloas epochs vs ATTESTATION_DUE_BPS (3333) pre-Gloas. `get_payload_attestation_due_ms` uses PAYLOAD_ATTESTATION_DUE_BPS (7500). - Add both BPS fields to ChainSpec with derived Duration values - Add `get_attestation_due::<E>(slot)` that returns epoch-appropriate threshold matching the spec - Add `get_payload_attestation_due()` matching the spec - Use them in proto_array record_block_timeliness instead of hardcoded values
|
Some required checks have failed. Could you please take a look @michaelsproul? 🙏 |
mergify
Bot
added
waiting-on-author
michaelsproul
added
ready-for-review
eserilev
left a comment
eserilev
left a comment
There was a problem hiding this comment.
LGTM, going to press the big red button and merge this thing. This unlocks tons of ePBS stuff, thanks for all the hard work everyone <3
Merge Queue Status
This pull request spent 27 minutes 14 seconds in the queue, including 25 minutes 40 seconds running CI. Required conditions to merge
|
6 participants
Summary
Refined version of Gloas fork choice, removing some optimisations to yield a more readable implementation that more obviously aligns with the spec. The plan is to later introduce optimisations in a way that preserves spec similarity.
Differences from previous fork choice
best_childandbest_descendantcaching is removed.ProtoNode.weightjust includes attestation weight, to make calculating proposer boost simpler.Schema migration
V29 schema migration encompasses several changes:
justified_checkpointandfinalized_checkpointfromSszContainerprevious_proposer_boostfromSszContainer(proto array no longer needs it, it is only tracked on the fork choice store).VoteTrackerV28(epoch-based) toVoteTracker(slot-based)Vec<ProtoNodeV17>toVec<ProtoNode>(add enum discriminant)queued_attestations: we do not need to persist these between restarts. New attestations will quickly replace any attestations that are one slot old.V17(pre-Gloas) andV29(post-Gloas) nodes are allowed to coexist in theproto_nodesarray. After the Gloas fork finalizes, we will be able to delete support forV17nodes if we are happy with removing pre-Gloas fork choice.