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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow honest validators to reorg late blocks #3034
Allow honest validators to reorg late blocks #3034
Changes from 3 commits
45a3615
651db2f
0f61819
24b4d46
22215b8
3f8d8fe
35d444b
3f1bc20
9ce8e3d
b9285b8
d8440f8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is phrased in a way that it is agnostic to whether we are calling it on the slot we are about to propose or during the previous slot. However, what worries me is the situation where
current_slot = N
, we are supposed to propose atN+1
,head_slot = N-1
and we call during slotN
. In this case, suppose the blockN
is based onN-2
, forking offN-1
, it is a timely block, and we haven't yet counted attestations because we are calling this function duringN
. What will happen is that our head will still beN-1
and every check here will pass. It may well be that later, at the time we need to propose, during N+1, this timely block at N would become our head, but this function here may return true.Now this is not a problem per-se in this specification since later on, when calling
get_proposer_head
we will get a misprediction because we will not have passedsingle_slot_reorg
. Anyway, I feel it's better to change this condition here from checking that the attestations for the head block to have been counted to "changing that the attestations for the current slot have been counted if we are calling late in the slot". I do recognize that this is difficult to specify, but perhaps it's better to not be agnostic as to the timing in which this function is being called.As a side in this long comment: if we are in the situation above, and we call this function during N, then the
assert store.proposer_boost_root == Root()
will fail in this function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the head is at
N - 1
and we are proposing atN + 1
then this function won't return true. This function infers theproposal_slot
from the head slot, so it will check whether we are proposing at slot N:consensus-specs/specs/bellatrix/fork-choice.md
Line 111 in 56f20f7
i.e. if we are actually proposing at
N+1
and some other block forN
already exists, theproposing_reorg_slot
check will beFalse
.Therefore I think the spec already minimises the chance of a misprediction. It's a really nice test case though, I'll make sure it gets a spec test when we add them (I'll also add a test to Lighthouse before then).
I'm open to making this change, but would like to clarify something about how you envisage Prysm's implementation applying attestations at 10 or 11 secs into the slot will work: are you going to avoid updating the fork choice store's current slot "early"? I.e. if you are 10s into slot
N
will you apply the attestations for slotN
and leaveget_current_slot(store) == N
? The way everything is written at the moment maintains the invariant that only attestations fromget_current_slot(store) - 1
or earlier are applied, supporting Lighthouse's implementation which advances the fork choice slot early at 11.5s into slotN
and pre-emptively increments the fork choice slot toN + 1
.Nice catch on the
assert
! I think we need to change it so we returnFalse
rather than erroring, or assert that the proposer boost is for a block other thanhead_block
:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed I mixed the previous check, but now I'm worried about something else:
validator_is_connected
can return true for a large number of validators in the case of a pool. In the above scenario, a pool proposing at N and N+1 will actually pass with the wrong validator the check forproposing_reorg_slot
.Yeah that's my plan, I'm open to change this to make it closer to LightHouse, but it seems very hard on our end cause we track the wallclock on forkchoice and tricking it to change the slot involves bad hacks like changing the genesis time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@potuz I don't understand what you mean here, can you spell it out in a bit more detail? I thought we already covered this case by demonstrating that the fcU for
N - 1
won't be suppressed, and if the head switches toN
upon counting attestations towards the end of slotN
, then the fcU forN
also won't be suppressed because it will fail theparent_slot_ok
check (N
's parent isN - 2
). Hence we'll still build onN
via the normal process (no re-org attempts).