Remember a fast block proposal: it remains locked even if re-proposed. #3140
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,7 +36,7 @@ | |
| //! `ValidatedBlock` certificate (with a quorum of validator signatures) for **A** in some round | ||
| //! between **s** and **r** included in the block proposal. | ||
| //! * Validators only vote for a `ConfirmedBlock` if there is a `ValidatedBlock` certificate for the | ||
| //! same block in the same round. (Or, in the `Fast` round, if there is a valid proposal.) | ||
| //! | ||
| //! This guarantees that once a quorum votes for some `ConfirmedBlock`, there can never be a | ||
| //! `ValidatedBlock` certificate (and thus also no `ConfirmedBlock` certificate) for a different | ||
|
|
@@ -108,6 +108,35 @@ pub enum Outcome { | |
|
|
||
| pub type ValidatedOrConfirmedVote<'a> = Either<&'a Vote<ValidatedBlock>, &'a Vote<ConfirmedBlock>>; | ||
|
|
||
| /// 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. | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| #[cfg_attr(with_testing, derive(Eq, PartialEq))] | ||
| pub enum LockedBlock { | ||
| /// A proposal in the `Fast` round. | ||
| Fast(BlockProposal), | ||
| /// A `ValidatedBlock` certificate in a round other than `Fast`. | ||
| Regular(ValidatedBlockCertificate), | ||
| } | ||
|
|
||
| impl LockedBlock { | ||
| /// Returns the locked block's round. To propose a different block, a `ValidatedBlock` | ||
| /// certificate from a higher round is needed. | ||
| pub fn round(&self) -> Round { | ||
| match self { | ||
| Self::Fast(_) => Round::Fast, | ||
| Self::Regular(certificate) => certificate.round, | ||
| } | ||
| } | ||
|
|
||
| pub fn chain_id(&self) -> ChainId { | ||
| match self { | ||
| Self::Fast(proposal) => proposal.content.block.chain_id, | ||
| Self::Regular(certificate) => certificate.value().inner().chain_id(), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// The state of the certification process for a chain's next block. | ||
| #[derive(Debug, View, ClonableView, SimpleObject)] | ||
| #[graphql(complex)] | ||
|
|
@@ -132,7 +161,7 @@ where | |
| /// Latest validated proposal that we have voted to confirm (or would have, if we are not a | ||
| /// validator). | ||
| #[graphql(skip)] | ||
| pub locked: RegisterView<C, Option<LockedBlock>>, | ||
| /// These are blobs published or read by the locked block. | ||
| pub locked_blobs: MapView<C, BlobId, Blob>, | ||
| /// Latest leader timeout certificate we have received. | ||
|
|
@@ -263,69 +292,40 @@ where | |
|
|
||
| /// Verifies the safety of a proposed block with respect to voting rules. | ||
| pub fn check_proposed_block(&self, proposal: &BlockProposal) -> Result<Outcome, ChainError> { | ||
| let new_block = &proposal.content.block; | ||
| if let Some(old_proposal) = self.proposed.get() { | ||
| if old_proposal.content == proposal.content { | ||
| return Ok(Outcome::Skip); // We already voted for this proposal; nothing to do. | ||
| } | ||
| } | ||
| // When a block is certified, incrementing its height must succeed. | ||
| ensure!( | ||
| new_block.height < BlockHeight::MAX, | ||
| ChainError::InvalidBlockHeight | ||
| ); | ||
| // TODO(#2971): The client still needs to make a note of proposals that fail here: | ||
| match self.locked.get().as_ref() { | ||
| None => {} // No locked block; any proposal is allowed. | ||
| Some(LockedBlock::Fast(old_proposal)) => { | ||
| // We have a locked block in the `Fast` round. Either the proposal contains a | ||
| // validated block certificate, or it must propose the same block. | ||
| ensure!( | ||
| proposal.validated_block_certificate.is_some() | ||
| || proposal.content.block == old_proposal.content.block, | ||
| ChainError::HasLockedBlock(new_block.height, Round::Fast) | ||
| ); | ||
| } | ||
| Some(LockedBlock::Regular(validated)) => { | ||
| // We have a locked block in a round other than `Fast`. The proposal must contain | ||
| // a certificate that is no older than the locked block. | ||
| ensure!( | ||
| proposal | ||
| .validated_block_certificate | ||
| .as_ref() | ||
| .is_some_and(|cert| validated.round <= cert.round), | ||
| ChainError::HasLockedBlock(new_block.height, validated.round) | ||
| ); | ||
| } | ||
| } | ||
| Ok(Outcome::Accept) | ||
| } | ||
|
|
@@ -404,7 +404,7 @@ where | |
| // We don't compare to `current_round` here: Non-validators must update their locked block | ||
| // even if it is older than the current round. Validators will only sign in the current | ||
| // round, though. (See `create_final_vote` below.) | ||
| if let Some(LockedBlock::Regular(locked)) = self.locked.get() { | ||
deuszx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if locked.hash() == certificate.hash() && locked.round == certificate.round { | ||
| return Ok(Outcome::Skip); | ||
| } | ||
|
|
@@ -424,51 +424,60 @@ where | |
| key_pair: Option<&KeyPair>, | ||
| local_time: Timestamp, | ||
| blobs: BTreeMap<BlobId, Blob>, | ||
| ) -> Result<Option<ValidatedOrConfirmedVote>, ChainError> { | ||
| let round = proposal.content.round; | ||
| if key_pair.is_some() && round < self.current_round() { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| // If the validated block certificate is more recent, update our locked block. | ||
| if let Some(lite_cert) = &proposal.validated_block_certificate { | ||
| if self | ||
| .locked | ||
| .get() | ||
| .as_ref() | ||
| .map_or(true, |locked| locked.round() < lite_cert.round) | ||
| { | ||
| let value = Hashed::new(ValidatedBlock::new(executed_block.clone())); | ||
| if let Some(certificate) = lite_cert.clone().with_value(value) { | ||
| self.set_locked(LockedBlock::Regular(certificate), blobs)?; | ||
| } | ||
| } | ||
| } else if proposal.content.round.is_fast() { | ||
deuszx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // The fast block also counts as locked. | ||
| self.set_locked(LockedBlock::Fast(proposal.clone()), blobs)?; | ||
| } | ||
|
|
||
| let Some(key_pair) = key_pair else { | ||
| // Record the proposed block, in case it affects the current round number. | ||
| 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 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 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.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. |
||
| self.update_current_round(local_time); | ||
| // If this is a fast block, vote to confirm. Otherwise vote to validate. | ||
| if round.is_fast() { | ||
| self.validated_vote.set(None); | ||
| Ok(Some(Either::Right(self.confirmed_vote.get_mut().insert( | ||
| Vote::new( | ||
| Hashed::new(ConfirmedBlock::new(executed_block)), | ||
| round, | ||
| key_pair, | ||
| ), | ||
| )))) | ||
| } else { | ||
| self.confirmed_vote.set(None); | ||
| Ok(Some(Either::Left(&*self.validated_vote.get_mut().insert( | ||
| Vote::new( | ||
| Hashed::new(ValidatedBlock::new(executed_block)), | ||
| round, | ||
| key_pair, | ||
| ), | ||
| )))) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -487,7 +496,7 @@ where | |
| return Ok(()); | ||
| } | ||
| let confirmed_block = ConfirmedBlock::new(validated.inner().executed_block().clone()); | ||
| self.set_locked(LockedBlock::Regular(validated), blobs)?; | ||
| self.update_current_round(local_time); | ||
| if let Some(key_pair) = key_pair { | ||
| // Vote to confirm. | ||
|
|
@@ -523,12 +532,7 @@ where | |
| .next_round(certificate.round) | ||
| .unwrap_or(Round::Validator(u32::MAX)) | ||
| }) | ||
| .chain(self.locked.get().as_ref().map(LockedBlock::round)) | ||
| .chain( | ||
| self.proposed | ||
| .get() | ||
|
|
@@ -635,19 +639,69 @@ where | |
| self.ownership.get().super_owners.contains_key(owner) | ||
| } | ||
|
|
||
| fn set_proposed(&mut self, proposal: BlockProposal) { | ||
| if self | ||
| .proposed | ||
| .get() | ||
| .as_ref() | ||
| .is_some_and(|old_proposal| old_proposal.content.round >= proposal.content.round) | ||
| { | ||
| return; | ||
| } | ||
| self.proposed.set(Some(proposal)); | ||
| } | ||
|
|
||
| /// Sets the locked block and the associated blobs. | ||
| fn set_locked( | ||
| &mut self, | ||
| locked: LockedBlock, | ||
| blobs: BTreeMap<BlobId, Blob>, | ||
| ) -> Result<(), ViewError> { | ||
| self.locked.set(Some(locked)); | ||
| self.locked_blobs.clear(); | ||
| for (blob_id, blob) in blobs { | ||
| self.locked_blobs.insert(&blob_id, blob)?; | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Checks that the block proposal is in a round where we can sign it. | ||
| fn check_proposal_round(&self, proposal: &BlockProposal) -> Result<(), ChainError> { | ||
| let new_round = proposal.content.round; | ||
| let owner = &proposal.owner; | ||
| let expected_round = match &proposal.validated_block_certificate { | ||
| None => self.current_round(), | ||
| Some(cert) => self | ||
| .ownership | ||
| .get() | ||
| .next_round(cert.round) | ||
| .ok_or_else(|| ChainError::ArithmeticError(ArithmeticError::Overflow))? | ||
| .max(self.current_round()), | ||
| }; | ||
| // In leader rotation mode, the round must equal the expected one exactly. | ||
| // Only the first single-leader round can be entered at any time. | ||
| if self.is_super(owner) | ||
| || (new_round <= Round::SingleLeader(0) && !expected_round.is_fast()) | ||
| { | ||
| ensure!( | ||
| expected_round <= new_round, | ||
| ChainError::InsufficientRound(expected_round) | ||
| ); | ||
| } else { | ||
| ensure!( | ||
| expected_round == new_round, | ||
| ChainError::WrongRound(expected_round) | ||
| ); | ||
| } | ||
| if let Some(old_proposal) = self.proposed.get() { | ||
| ensure!( | ||
| new_round > old_proposal.content.round, | ||
| // We already accepted a proposal in this round or in a higher round. | ||
| ChainError::InsufficientRoundStrict(old_proposal.content.round) | ||
| ); | ||
| } | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| /// Chain manager information that is included in `ChainInfo` sent to clients. | ||
|
|
@@ -662,7 +716,7 @@ pub struct ChainManagerInfo { | |
| /// Latest validated proposal that we have voted to confirm (or would have, if we are not a | ||
| /// validator). | ||
| #[debug(skip_if = Option::is_none)] | ||
| pub requested_locked: Option<Box<LockedBlock>>, | ||
| /// Latest timeout certificate we have seen. | ||
| #[debug(skip_if = Option::is_none)] | ||
| pub timeout: Option<Box<TimeoutCertificate>>, | ||
|
|
@@ -767,8 +821,6 @@ impl ChainManagerInfo { | |
|
|
||
| /// Returns whether there is a locked block in the current round. | ||
| pub fn has_locked_block_in_current_round(&self) -> bool { | ||
| self.requested_locked.as_ref().map(|locked| locked.round()) == Some(self.current_round) | ||
| } | ||
| } | ||
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
Do you mean:
Validators are allowed to sign a different block (from the locked block) iff they see a
ValidatedBlockCertificatefor it with a higher roundif so, consider replacing the comment as my version reads better (IMHO ofc).