Skip to content

Conversation

@michaelsproul
Copy link
Member

@michaelsproul michaelsproul commented Sep 28, 2025

Issue Addressed

Follow-up to the bug fixed in:

This fixes the root cause of that bug, which was introduced by me in:

Jimmy and Lion both identified the issue here:

Proposed Changes

In the methods that compute the proposer shuffling decision root, ensure we don't use lookahead for the Fulu fork epoch itself. This is accomplished by checking if Fulu is enabled at epoch - 1, i.e. if epoch > fulu_fork_epoch.

I haven't updated the methods that compute shufflings to use these new corrected bounds (e.g. BeaconState::compute_proposer_indices), although we could make this change in future. The get_beacon_proposer_indices method already gracefully handles the Fulu boundary case by using the proposer_lookahead field (if initialised).

Additional Info

  • Unit test added for proposer_shuffling_decision_slot at the boundary
  • Updated the consistency tests to check cases around the fork boundary

@michaelsproul michaelsproul added ready-for-review The code is ready for review consensus An issue/PR that touches consensus code, such as state_processing or block verification. fulu Required for the upcoming Fulu hard fork v8.0.0-rc.0 Q3 2025 release for Fusaka on Holesky labels Sep 28, 2025
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tests!

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Sep 29, 2025
@mergify mergify bot added the queued label Sep 29, 2025
@jimmygchen jimmygchen removed the ready-for-merge This PR is ready to merge. label Sep 29, 2025
@mergify
Copy link

mergify bot commented Sep 29, 2025

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #8128 has been dequeued. The pull request rule doesn't match anymore. The following conditions don't match anymore:

  • label=ready-for-merge.

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@mergify mergify bot removed the queued label Sep 29, 2025
@jimmygchen jimmygchen added the ready-for-merge This PR is ready to merge. label Sep 29, 2025
@mergify mergify bot added the queued label Sep 29, 2025
@mergify mergify bot merged commit 38fdaf7 into sigp:unstable Sep 29, 2025
45 of 47 checks passed
@mergify mergify bot removed the queued label Sep 29, 2025
lmnzx pushed a commit to lmnzx/lighthouse that referenced this pull request Nov 4, 2025
Follow-up to the bug fixed in:

- sigp#8121

This fixes the root cause of that bug, which was introduced by me in:

- sigp#8101

Lion identified the issue here:

- sigp#8101 (comment)


  In the methods that compute the proposer shuffling decision root, ensure we don't use lookahead for the Fulu fork epoch itself. This is accomplished by checking if Fulu is enabled at `epoch - 1`, i.e. if `epoch > fulu_fork_epoch`.

I haven't updated the methods that _compute_ shufflings to use these new corrected bounds (e.g. `BeaconState::compute_proposer_indices`), although we could make this change in future. The `get_beacon_proposer_indices` method already gracefully handles the Fulu boundary case by using the `proposer_lookahead` field (if initialised).


Co-Authored-By: Michael Sproul <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

consensus An issue/PR that touches consensus code, such as state_processing or block verification. fulu Required for the upcoming Fulu hard fork ready-for-merge This PR is ready to merge. v8.0.0-rc.0 Q3 2025 release for Fusaka on Holesky

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants