Fix payload availability lookup using the parent block's slot (alpha.13) - #9761
Conversation
|
Some required checks have failed. Could you please take a look @eserilev? 🙏 |
e37f83c to
ed908df
Compare
ed908df to
4696330
Compare
4696330 to
341f939
Compare
341f939 to
b6976c0
Compare
b6976c0 to
be02cff
Compare
chong-he
left a comment
There was a problem hiding this comment.
Looks great. The main change in the consensus-specs PR is about adding the parent_slot in the functions get_attestation_participation_flag_indices and process_attestations. They are also added here now.
| } | ||
|
|
||
| fn compute_beacon_block_attestation_reward_altair_deneb< | ||
| fn compute_beacon_block_attestation_reward_altair_and_later< |
There was a problem hiding this comment.
Just noting here that this is a nice rename of the function
| let data = attestation.data(); | ||
| let inclusion_delay = state.slot().safe_sub(data.slot)?.as_u64(); | ||
|
|
||
| // [Modified in Deneb:EIP7045] |
There was a problem hiding this comment.
I am not sure do we always have this kind of comments like the consensus spec? But this comment is stale now that the function get_attestation_participation_flag_indices is modified in Gloas.
Maybe we can delete the comment or update it? Maybe deleting is cleaner.
25c6ffb to
e150bbf
Compare
e150bbf to
b94b981
Compare
pawanjay176
left a comment
There was a problem hiding this comment.
Codex flagged some issues here that I don't think I can fully validate.
@dapplion would be really good to have your review on this one
| let mut previous_epoch_participation = | ||
| state.previous_epoch_participation()?.to_owned_list(); | ||
|
|
||
| let parent_slot = state |
There was a problem hiding this comment.
Codex tells me that for gloas, we need to run process_parent_execution_payload first before this is run because it modifies state.execution_payload_availability which is used below in get_attestation_participation_flag_indices
There was a problem hiding this comment.
yeah i think we probably should have made a change here when we deferred payload processing to the next slot (ethereum/consensus-specs#5094) in alpha spec 5?
Anyways good catch, i opened a separate PR with these changes
|
|
||
| let data = unaggregated_attestation.data(); | ||
|
|
||
| let parent_slot = state |
There was a problem hiding this comment.
Potentially same issue here? we might end up reporting the wrong thing here.
There was a problem hiding this comment.
yeah similar thing here, fix is in this PR #9828
b94b981 to
f11c285
Compare
f11c285 to
2409d25
Compare
Merge Queue Status
This pull request spent 3 hours 24 minutes 3 seconds in the queue, including 3 hours 22 minutes 15 seconds running CI. Waiting for
All conditions
ReasonThe merge conditions cannot be satisfied due to failing checks
HintYou may have to fix your CI before adding the pull request to the queue again. Requeued — the merge queue status continues in this comment ↓. |
4aeb5eb to
894bb39
Compare
c304832 to
86a1c14
Compare
Merge Queue Status
This pull request spent 12 minutes 12 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonPull request #9761 has been dequeued by @pawanjay176 with a Requeued — the merge queue status continues in this comment ↓. |
|
@mergify dequeue |
Merge Queue Status
This pull request spent 40 minutes 43 seconds in the queue, including 27 minutes 47 seconds running CI. Required conditions to merge
|
Issue Addressed
ethereum/consensus-specs#5473