Skip to content
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.

Sign up for GitHub

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

Remember a fast block proposal: it remains locked even if re-proposed. #3140

Merged
merged 5 commits into from
Jan 16, 2025
Merged

Remember a fast block proposal: it remains locked even if re-proposed. #3140

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
75c8f16
Verify that the fast block remains locked.
afck Jan 15, 2025
935166a
Fix the consensus bug.
afck Jan 15, 2025
28fbfa5
Improve LockedBlock comment.
afck Jan 16, 2025
3876f9a
Always check round number, even if locked block is fast.
afck Jan 16, 2025
7bb5420
Add comment, round number check.
afck Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
246 changes: 149 additions & 97 deletions linera-chain/src/manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//! 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
Expand Down Expand Up @@ -108,6 +108,35 @@ pub enum Outcome {

pub type ValidatedOrConfirmedVote<'a> = Either<&'a Vote<ValidatedBlock>, &'a Vote<ConfirmedBlock>>;

/// The current locked block: Validators are allowed to sign a different block (from the locked
/// block) iff they see a `ValidatedBlockCertificate` for it 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)]
Expand All @@ -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<ValidatedBlockCertificate>>,
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.
Expand Down Expand Up @@ -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_round = proposal.content.round;
let new_block = &proposal.content.block;
let owner = &proposal.owner;

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
);
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() {
if old_proposal.content == proposal.content {
return Ok(Outcome::Skip); // We already voted for this proposal; nothing to do.
}
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)
);
// Any proposal in the fast round is considered locked, because validators vote to
// confirm it immediately.
if old_proposal.content.round.is_fast()
&& proposal.validated_block_certificate.is_none()
{
// 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!(
old_proposal.content.block == *new_block,
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)
);
}
}
// If we have a locked block, the proposal must contain a certificate that validates the
// proposed block and is no older than the locked block.
if let Some(locked) = self.locked.get() {
ensure!(
proposal
.validated_block_certificate
.as_ref()
.is_some_and(|cert| locked.round <= cert.round),
ChainError::HasLockedBlock(locked.executed_block().block.height, locked.round)
)
}
Ok(Outcome::Accept)
}
Expand Down Expand Up @@ -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(locked) = self.locked.get() {
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);
}
Expand All @@ -424,51 +424,60 @@ where
key_pair: Option<&KeyPair>,
local_time: Timestamp,
blobs: BTreeMap<BlobId, Blob>,
) -> Result<Option<ValidatedOrConfirmedVote>, ViewError> {
) -> 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)
.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(certificate, blobs)?;
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)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, that's fine. Just maybe leave that part in addition to the comment above (in the else { ... } clause).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this (and line below) to line 452?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we can't update the current round before checking against the current round.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this was conditional on the acceptance of the previous comment.

self.update_current_round(local_time);

if let Some(key_pair) = key_pair {
// 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,
),
))))
}
// 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 {
Ok(None)
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,
),
))))
}
}

Expand All @@ -487,7 +496,7 @@ where
return Ok(());
}
let confirmed_block = ConfirmedBlock::new(validated.inner().executed_block().clone());
self.set_locked(validated, blobs)?;
self.set_locked(LockedBlock::Regular(validated), blobs)?;
self.update_current_round(local_time);
if let Some(key_pair) = key_pair {
// Vote to confirm.
Expand Down Expand Up @@ -523,12 +532,7 @@ where
.next_round(certificate.round)
.unwrap_or(Round::Validator(u32::MAX))
})
.chain(
self.locked
.get()
.iter()
.map(|certificate| certificate.round),
)
.chain(self.locked.get().as_ref().map(LockedBlock::round))
.chain(
self.proposed
.get()
Expand Down Expand Up @@ -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,
certificate: ValidatedBlockCertificate,
locked: LockedBlock,
blobs: BTreeMap<BlobId, Blob>,
) -> Result<(), ViewError> {
self.locked.set(Some(certificate));
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.
Expand All @@ -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<ValidatedBlockCertificate>>,
pub requested_locked: Option<Box<LockedBlock>>,
/// Latest timeout certificate we have seen.
#[debug(skip_if = Option::is_none)]
pub timeout: Option<Box<TimeoutCertificate>>,
Expand Down Expand Up @@ -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()
.is_some_and(|certificate| certificate.round == self.current_round)
self.requested_locked.as_ref().map(|locked| locked.round()) == Some(self.current_round)
}
}
4 changes: 2 additions & 2 deletions linera-core/src/chain_worker/state/attempted_changes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ where
check_block_epoch(epoch, block)?;
certificate.check(committee)?;
let mut actions = NetworkActions::default();
let already_validated_block = self
let already_committed_block = self
.state
.chain
.tip_state
Expand All @@ -196,7 +196,7 @@ where
.check_validated_block(&certificate)
.map(|outcome| outcome == manager::Outcome::Skip)
};
if already_validated_block || should_skip_validated_block()? {
if already_committed_block || should_skip_validated_block()? {
// If we just processed the same pending block, return the chain info unchanged.
return Ok((
ChainInfoResponse::new(&self.state.chain, self.state.config.key_pair()),
Expand Down
Loading
Loading