Do not check parent weight on early FCU #13683
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a late block arrives and the beacon is proposing the next block, we perform several checks to allow for the next block to reorg the incoming late block. Among those checks, we check that the parent block has been heavily attested (currently 160% of the committee size). We perform this check in these circumstances: - When the late block arrives - At 10 seconds into the slot - At 0 seconds into the next slot (at proposing time) The problem is that for blocks that arrive between 4 seconds and 10 seconds, the parent block will not have yet this expected weight since attestations from the current committee were not imported yet, and thus Prysm will send an FCU with payload attributes anyway at this time. What happens is that Prysm keeps the EL building different blocks based on different parents at the same time, when later in the next slot it calls to propose, it will reorg the late block anyway and the EL would have been computing a second payload uselessly. This PR enables this check only when calling `ShouldOverrideFCU` after 10 seconds into the slot which we do only after having imported the current attestations. We may want to actually remove this check entirely from `ShouldOverrideFCU` and only keep it in `ProposerHead`. Shout out to Anthithesis for reporting an issue that led to this discoverly.
nisdas
approved these changes
Mar 5, 2024
prestonvanloon
approved these changes
Mar 5, 2024
prestonvanloon
pushed a commit
that referenced
this pull request
Mar 5, 2024
When a late block arrives and the beacon is proposing the next block, we perform several checks to allow for the next block to reorg the incoming late block. Among those checks, we check that the parent block has been heavily attested (currently 160% of the committee size). We perform this check in these circumstances: - When the late block arrives - At 10 seconds into the slot - At 0 seconds into the next slot (at proposing time) The problem is that for blocks that arrive between 4 seconds and 10 seconds, the parent block will not have yet this expected weight since attestations from the current committee were not imported yet, and thus Prysm will send an FCU with payload attributes anyway at this time. What happens is that Prysm keeps the EL building different blocks based on different parents at the same time, when later in the next slot it calls to propose, it will reorg the late block anyway and the EL would have been computing a second payload uselessly. This PR enables this check only when calling `ShouldOverrideFCU` after 10 seconds into the slot which we do only after having imported the current attestations. We may want to actually remove this check entirely from `ShouldOverrideFCU` and only keep it in `ProposerHead`. Shout out to Anthithesis for reporting an issue that led to this discoverly. (cherry picked from commit b6ce6c2)
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a late block arrives and the beacon is proposing the next block, we perform several checks to allow for the next block to reorg the incoming late block.
Among those checks, we check that the parent block has been heavily attested (currently 160% of the committee size).
We perform this check in these circumstances:
The problem is that for blocks that arrive between 4 seconds and 10 seconds, the parent block will not have yet this expected weight since attestations from the current committee were not imported yet, and thus Prysm will send an FCU with payload attributes anyway at this time.
What happens is that Prysm keeps the EL building different blocks based on different parents at the same time, when later in the next slot it calls to propose, it will reorg the late block anyway and the EL would have been computing a second payload uselessly.
This PR enables this check only when calling
ShouldOverrideFCUafter 10 seconds into the slot which we do only after having imported the current attestations. We may want to actually remove this check entirely fromShouldOverrideFCUand only keep it inProposerHead.Shout out to Anthithesis for reporting an issue that led to this discoverly.