Remember a fast block proposal: it remains locked even if re-proposed. #3140
Conversation
linera-chain/src/manager.rs
Outdated
| /// The current locked block: Validators are only allowed to sign any proposal with a different | ||
| /// block if they see a `ValidatedBlock` certificate with a higher round. |
There was a problem hiding this comment.
Do you mean:
Validators are allowed to sign a different block (from the locked block) iff they see a ValidatedBlockCertificate for it with a higher round
if so, consider replacing the comment as my version reads better (IMHO ofc).
linera-chain/src/manager.rs
Outdated
| self.set_proposed(proposal); | ||
| self.update_current_round(local_time); | ||
| return Ok(None); | ||
| }; | ||
|
|
||
| self.check_proposal_round(&proposal)?; |
There was a problem hiding this comment.
So we check the proposal's vailidity w.r.t. to the round only when we're actively validating? i.e. shouldn't this check be before the let Some(key_pair) = key_pair else { ... } which will return early?
There was a problem hiding this comment.
No, because even if the proposal isn't from the current round, on the client side we still want to add it to the proposed field so that in multi leader rounds we know we shouldn't make a proposal in the same round ourselves. (This partially addresses #2971.)
There was a problem hiding this comment.
That's confusing TBH. That the logic here is makes these distinction b/c of some hidden requirements (i.e. being invoked on client vs server side).
There was a problem hiding this comment.
I agree. As I mentioned on Slack, I think we should make that distinction more explicit. But I think that will be a bigger change, and I'm not sure if it wouldn't make this PR even harder to review. Happy to give it a try, though, if you prefer.
There was a problem hiding this comment.
No, that's fine. Just maybe leave that part in addition to the comment above (in the else { ... } clause).
There was a problem hiding this comment.
Actually, I'll also add another check so that we only add the proposal if it's before the single leader rounds. For those we don't need it, and we also shouldn't accept it before the round has begun. Sorry for the confusion! I hope we can separate some of the client vs. validator logic soon, and clarify all of this.
| // Record the proposed block, so it can be supplied to clients that request it. | ||
| self.proposed.set(Some(proposal)); | ||
| self.set_proposed(proposal); |
There was a problem hiding this comment.
Can we move this (and line below) to line 452?
There was a problem hiding this comment.
No, we can't update the current round before checking against the current round.
There was a problem hiding this comment.
Yes, this was conditional on the acceptance of the previous comment.
Motivation
In the fast round, the validators sign to confirm right away, so the block proposal must count as locked.
However, if the same block is re-proposed in a later round, the
proposedfield is overwritten and, due to the different round, is now not considered locked anymore.Proposal
Keep track of the fast round proposal as part of the
lockedfield, too.Test Plan
The problem was reproduced by extending
test_fast_proposal_is_locked.Release Plan
Links