Initialize anchor PTC vote arrays in get_forkchoice_store - #5545
Conversation
| if block.slot > slot: | ||
| # [Modified in Gloas:EIP7732] | ||
| if block.parent_root not in store.blocks: | ||
| return node | ||
| parent = ForkChoiceNode( |
There was a problem hiding this comment.
Sorry, I don't think I like this fix. Seems like a hack. Also the annotation (modified in gloas) is over the wrong thing now. I believe @jihoonsong is looking at this. Let's see what he thinks.
There was a problem hiding this comment.
The issue is related to Heze genesis, not here. I'll make a commit, which will exclude this change.
There was a problem hiding this comment.
After looking into this, it was not about Heze genesis either. See the other comment.
|
After reviewing this PR, I've concluded that the proposed spec change is correct. #5135 removed wrongly seeding the PTC arrays with True. Later, #5180 introduced None votes and it seems this spec change should've included there. However, the new test case is unreachable. I think there are two cases relevant to this change: genesis block and checkpoint sync. The genesis block is always EMPTY post-Gloas (see #5172 for more context) and during checkpoint syncing, the anchor block cannot be from the previous slot. Hence, the test is removed in 745433f. While I think it's unreachable that the PTC arrays being accessed by anchor root, I believe having this code makes the invariant hold without an exception as the author described. |
nflaig
left a comment
There was a problem hiding this comment.
LGTM, this matches what lodestar does, but this code should not be reachable as @jihoonsong pointed out as we only evaluate ptc votes near the tip, ie. for payload status of a block from current_slot - 1 but the finalized anchor during checkpoint sync is at least 2 epochs behind the current epoch
Description
get_forkchoice_storeinitializes the store's PTC vote arrays (payload_timeliness_voteandpayload_data_availability_vote) for every block added viaon_block, but leaves the anchor root out. Sincepayload_timeliness,payload_data_availability, andon_payload_attestation_messagerequire every known block root to be present in these arrays, the anchor breaks the invariant:get_headfails with anAssertionErroronce the anchor is a previous-slot payload decision with its payload delivered, becauseshould_extend_payloadconsultspayload_timelinesson the anchor root.Initialize both arrays for the anchor root with no votes cast (
[None] * PTC_SIZE), matchingon_block.test_genesispreviously codified the missing arrays as expected behavior; it now asserts that the anchor's vote arrays are initialized with no votes recorded, and a regression test coversget_headwith the anchor's payload delivered.Checklist
test_get_head_with_anchor_payload_deliveredand updatedtest_genesismake lintto check formattingmake testto check testsRelations
None