From 75c8f168c559ce596c35959bb31825590814a01d Mon Sep 17 00:00:00 2001 From: Andreas Fackler Date: Wed, 15 Jan 2025 11:40:23 +0100 Subject: [PATCH 1/5] Verify that the fast block remains locked. --- linera-core/src/unit_tests/worker_tests.rs | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/linera-core/src/unit_tests/worker_tests.rs b/linera-core/src/unit_tests/worker_tests.rs index 4b1b5fbe409..9f31ebb610c 100644 --- a/linera-core/src/unit_tests/worker_tests.rs +++ b/linera-core/src/unit_tests/worker_tests.rs @@ -3561,6 +3561,7 @@ where let value1 = Hashed::new(ConfirmedBlock::new(executed_block1)); let (response, _) = worker.handle_block_proposal(proposal1).await?; let vote = response.info.manager.pending.as_ref().unwrap(); + assert_eq!(vote.round, Round::Fast); assert_eq!(vote.value.value_hash, value1.hash()); // Set the clock to the end of the round. @@ -3576,20 +3577,29 @@ where assert_eq!(response.info.manager.current_round, Round::MultiLeader(0)); assert_eq!(response.info.manager.leader, None); - // Now any owner can propose a block. But block1 is locked. + // Now any owner can propose a block. But block1 is locked. Re-proposing it is allowed. + let proposal1b = block1 + .clone() + .into_proposal_with_round(&key_pairs[1], Round::MultiLeader(0)); + let (response, _) = worker.handle_block_proposal(proposal1b).await?; + let vote = response.info.manager.pending.as_ref().unwrap(); + assert_eq!(vote.round, Round::MultiLeader(0)); + assert_eq!(vote.value.value_hash, value1.hash()); + + // Proposing a different block is not. let block2 = make_child_block(&value0) .with_simple_transfer(ChainId::root(1), Amount::ONE) .with_authenticated_signer(Some(pub_key1.into())); let proposal2 = block2 .clone() - .into_proposal_with_round(&key_pairs[1], Round::MultiLeader(0)); + .into_proposal_with_round(&key_pairs[1], Round::MultiLeader(1)); let result = worker.handle_block_proposal(proposal2).await; assert_matches!(result, Err(WorkerError::ChainError(err)) if matches!(*err, ChainError::HasLockedBlock(_, Round::Fast)) ); let proposal3 = block1 .clone() - .into_proposal_with_round(&key_pairs[1], Round::MultiLeader(0)); + .into_proposal_with_round(&key_pairs[1], Round::MultiLeader(2)); worker.handle_block_proposal(proposal3).await?; // A validated block certificate from a later round can override the locked fast block. @@ -3598,7 +3608,7 @@ where let certificate2 = make_certificate_with_round(&committee, &worker, value2.clone(), Round::MultiLeader(0)); let proposal = BlockProposal::new_retry( - Round::MultiLeader(1), + Round::MultiLeader(3), certificate2.clone(), &key_pairs[1], Vec::new(), @@ -3613,7 +3623,7 @@ where ); let vote = response.info.manager.pending.as_ref().unwrap(); assert_eq!(vote.value, lite_value2); - assert_eq!(vote.round, Round::MultiLeader(1)); + assert_eq!(vote.round, Round::MultiLeader(3)); Ok(()) } From 935166a50a073e52940664cdafb70231c06de13f Mon Sep 17 00:00:00 2001 From: Andreas Fackler Date: Wed, 15 Jan 2025 13:47:36 +0100 Subject: [PATCH 2/5] Fix the consensus bug. --- linera-chain/src/manager.rs | 246 +++++++++++------- .../chain_worker/state/attempted_changes.rs | 4 +- linera-core/src/client/mod.rs | 96 ++++--- linera-core/src/remote_node.rs | 3 +- linera-core/src/unit_tests/client_tests.rs | 52 ++-- linera-core/src/unit_tests/worker_tests.rs | 9 +- linera-rpc/tests/format.rs | 3 +- .../tests/snapshots/format__format.yaml.snap | 12 +- 8 files changed, 268 insertions(+), 157 deletions(-) diff --git a/linera-chain/src/manager.rs b/linera-chain/src/manager.rs index 0c60343bfdd..069d93bf0ed 100644 --- a/linera-chain/src/manager.rs +++ b/linera-chain/src/manager.rs @@ -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 @@ -108,6 +108,35 @@ pub enum Outcome { pub type ValidatedOrConfirmedVote<'a> = Either<&'a Vote, &'a Vote>; +/// 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>, + pub locked: RegisterView>, /// These are blobs published or read by the locked block. pub locked_blobs: MapView, /// 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 { - 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) } @@ -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() { if locked.hash() == certificate.hash() && locked.round == certificate.round { return Ok(Outcome::Skip); } @@ -424,8 +424,11 @@ where key_pair: Option<&KeyPair>, local_time: Timestamp, blobs: BTreeMap, - ) -> Result, ViewError> { + ) -> Result, 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 { @@ -433,42 +436,48 @@ where .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() { + // 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)?; // Record the proposed block, so it can be supplied to clients that request it. - self.proposed.set(Some(proposal)); + self.set_proposed(proposal); 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, + ), + )))) } } @@ -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. @@ -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() @@ -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, ) -> 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. @@ -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>, + pub requested_locked: Option>, /// Latest timeout certificate we have seen. #[debug(skip_if = Option::is_none)] pub timeout: Option>, @@ -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) } } diff --git a/linera-core/src/chain_worker/state/attempted_changes.rs b/linera-core/src/chain_worker/state/attempted_changes.rs index 98a1bb7feea..e674a2f697a 100644 --- a/linera-core/src/chain_worker/state/attempted_changes.rs +++ b/linera-core/src/chain_worker/state/attempted_changes.rs @@ -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 @@ -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()), diff --git a/linera-core/src/client/mod.rs b/linera-core/src/client/mod.rs index ec400e64376..d4881a21695 100644 --- a/linera-core/src/client/mod.rs +++ b/linera-core/src/client/mod.rs @@ -44,6 +44,7 @@ use linera_chain::{ Block, BlockProposal, ChainAndHeight, ExecutedBlock, IncomingBundle, LiteVote, MessageAction, }, + manager::LockedBlock, types::{ CertificateKind, CertificateValue, ConfirmedBlock, ConfirmedBlockCertificate, GenericCertificate, LiteCertificate, Timeout, TimeoutCertificate, ValidatedBlock, @@ -1764,13 +1765,38 @@ where break; } } - if let Some(cert) = info.manager.requested_locked { - let hash = cert.hash(); - if let Err(err) = self.try_process_locked_block_from(remote_node, cert).await { - warn!( - "Skipping certificate {hash} from validator {}: {err}", - remote_node.name - ); + if let Some(locked) = info.manager.requested_locked { + match *locked { + LockedBlock::Fast(proposal) => { + let owner = proposal.owner; + while let Err(original_err) = self + .client + .local_node + .handle_block_proposal(proposal.clone()) + .await + { + if let LocalNodeError::BlobsNotFound(blob_ids) = &original_err { + self.update_local_node_with_blobs_from(blob_ids.clone(), remote_node) + .await?; + continue; // We found the missing blobs: retry. + } + + warn!( + "Skipping proposal from {} and validator {}: {}", + owner, remote_node.name, original_err + ); + break; + } + } + LockedBlock::Regular(cert) => { + let hash = cert.hash(); + if let Err(err) = self.try_process_locked_block_from(remote_node, cert).await { + warn!( + "Skipping certificate {hash} from validator {}: {err}", + remote_node.name + ); + } + } } } Ok(()) @@ -1779,10 +1805,10 @@ where async fn try_process_locked_block_from( &self, remote_node: &RemoteNode, - certificate: Box>, + certificate: GenericCertificate, ) -> Result<(), ChainClientError> { let chain_id = certificate.inner().chain_id(); - match self.process_certificate(*certificate.clone(), vec![]).await { + match self.process_certificate(certificate.clone(), vec![]).await { Err(LocalNodeError::BlobsNotFound(blob_ids)) => { let mut blobs = Vec::new(); for blob_id in blob_ids { @@ -1792,7 +1818,7 @@ where .await?; blobs.push(Blob::new(blob_content)); } - self.process_certificate(*certificate, blobs).await?; + self.process_certificate(certificate, blobs).await?; Ok(()) } Err(err) => Err(err.into()), @@ -2398,28 +2424,26 @@ where let info = self.request_leader_timeout_if_needed().await?; // If there is a validated block in the current round, finalize it. - if info.manager.has_locked_block_in_current_round() { + if info.manager.has_locked_block_in_current_round() && !info.manager.current_round.is_fast() + { return self.finalize_locked_block(info).await; } // Otherwise we have to re-propose the highest validated block, if there is one. - let executed_block = if let Some(certificate) = &info.manager.requested_locked { - certificate.executed_block().clone() - } else { - let block = if let Some(proposal) = info - .manager - .requested_proposed - .as_ref() - .filter(|proposal| proposal.content.round.is_fast()) - { - // The fast block counts as "locked", too. If there is one, re-propose that. - proposal.content.block.clone() - } else if let Some(block) = self.state().pending_block() { - block.clone() // Otherwise we are free to propose our own pending block. - } else { - return Ok(ClientOutcome::Committed(None)); // Nothing to do. - }; + let pending: Option = self.state().pending_block().clone(); + let executed_block = if let Some(locked) = &info.manager.requested_locked { + match &**locked { + LockedBlock::Regular(certificate) => certificate.executed_block().clone(), + LockedBlock::Fast(proposal) => { + let block = proposal.content.block.clone(); + self.stage_block_execution(block).await?.0 + } + } + } else if let Some(block) = pending { + // Otherwise we are free to propose our own pending block. self.stage_block_execution(block).await?.0 + } else { + return Ok(ClientOutcome::Committed(None)); // Nothing to do. }; let identity = self.identity().await?; @@ -2434,8 +2458,15 @@ where let already_handled_locally = info.manager.already_handled_proposal(round, block); let key_pair = self.key_pair().await?; // Create the final block proposal. - let proposal = if let Some(cert) = info.manager.requested_locked { - Box::new(BlockProposal::new_retry(round, *cert, &key_pair, blobs)) + let proposal = if let Some(locked) = info.manager.requested_locked { + Box::new(match *locked { + LockedBlock::Regular(cert) => { + BlockProposal::new_retry(round, cert, &key_pair, blobs) + } + LockedBlock::Fast(proposal) => { + BlockProposal::new_initial(round, proposal.content.block, &key_pair, blobs) + } + }) } else { let block = executed_block.block.clone(); Box::new(BlockProposal::new_initial(round, block, &key_pair, blobs)) @@ -2488,12 +2519,15 @@ where &self, info: Box, ) -> Result>, ChainClientError> { - let certificate = info + let locked = info .manager .requested_locked .expect("Should have a locked block"); + let LockedBlock::Regular(certificate) = *locked else { + panic!("Should have a locked validated block"); + }; let committee = self.local_committee().await?; - match self.finalize_block(&committee, *certificate.clone()).await { + match self.finalize_block(&committee, certificate.clone()).await { Ok(certificate) => Ok(ClientOutcome::Committed(Some(certificate))), Err(ChainClientError::CommunicationError(error)) => { // Communication errors in this case often mean that someone else already diff --git a/linera-core/src/remote_node.rs b/linera-core/src/remote_node.rs index dadffd441b8..882d055fea8 100644 --- a/linera-core/src/remote_node.rs +++ b/linera-core/src/remote_node.rs @@ -161,8 +161,7 @@ impl RemoteNode { let locked = manager.requested_locked.as_ref(); ensure!( proposed.map_or(true, |proposal| proposal.content.block.chain_id == chain_id) - && locked.map_or(true, |cert| cert.executed_block().block.chain_id - == chain_id) + && locked.map_or(true, |locked| locked.chain_id() == chain_id) && response.check(&self.name).is_ok(), NodeError::InvalidChainInfoResponse ); diff --git a/linera-core/src/unit_tests/client_tests.rs b/linera-core/src/unit_tests/client_tests.rs index 639bd35af93..87fbf3dd298 100644 --- a/linera-core/src/unit_tests/client_tests.rs +++ b/linera-core/src/unit_tests/client_tests.rs @@ -15,6 +15,7 @@ use linera_base::{ }; use linera_chain::{ data_types::{IncomingBundle, Medium, MessageBundle, Origin, PostedMessage}, + manager::LockedBlock, types::Timeout, ChainError, ChainExecutionContext, }; @@ -1374,10 +1375,13 @@ where // We make validator 3 (who does not have the block proposal) process the validated block. let info2_a = client2_a.chain_info_with_manager_values().await?; let locked = *info2_a.manager.requested_locked.unwrap(); + let LockedBlock::Regular(validated) = locked else { + panic!("Unexpected locked fast block."); + }; let blobs = vec![blob1]; let response = builder .node(3) - .handle_validated_certificate(locked.clone(), blobs) + .handle_validated_certificate(validated.clone(), blobs) .await?; assert_eq!( response.info.manager.pending.unwrap().round, @@ -1387,7 +1391,10 @@ where // Client 2B should be able to synchronize the locked block and the blobs from validator 3. client2_b.synchronize_from_validators().await.unwrap(); let info2_b = client2_b.chain_info_with_manager_values().await?; - assert_eq!(locked, *info2_b.manager.requested_locked.unwrap()); + assert_eq!( + LockedBlock::Regular(validated), + *info2_b.manager.requested_locked.unwrap() + ); let bt_certificate = client2_b .burn(None, Amount::from_tokens(1)) .await @@ -1665,7 +1672,10 @@ where // Validator 2 may or may not have processed the validated block before the update was // canceled due to the errors from the faulty validators. Submit it again to make sure // it's there, so that client 2 can download and re-propose it later. - let validated_block_certificate = *manager.requested_locked.unwrap(); + let locked = *manager.requested_locked.unwrap(); + let LockedBlock::Regular(validated_block_certificate) = locked else { + panic!("Unexpected locked fast block."); + }; let resubmission_result = builder .node(2) .handle_validated_certificate(validated_block_certificate, vec![blob1]) @@ -1690,13 +1700,12 @@ where ); if i == 2 { + let locked = *validator_manager.requested_locked.unwrap(); + let LockedBlock::Regular(validated) = locked else { + panic!("Unexpected locked fast block."); + }; assert_eq!( - validator_manager - .requested_locked - .unwrap() - .executed_block() - .block - .operations, + validated.executed_block().block.operations, blob_0_1_operations, ); } else { @@ -1733,7 +1742,10 @@ where // Validator 3 may or may not have processed the validated block before the update was // canceled due to the errors from the faulty validators. Submit it again to make sure // it's there, so that client 2 can download and re-propose it later. - let validated_block_certificate = *manager.requested_locked.unwrap(); + let locked = *manager.requested_locked.unwrap(); + let LockedBlock::Regular(validated_block_certificate) = locked else { + panic!("Unexpected locked fast block."); + }; let resubmission_result = builder .node(3) .handle_validated_certificate(validated_block_certificate, vec![blob3]) @@ -1755,13 +1767,12 @@ where .operations, blob_2_3_operations, ); + let locked = *validator_manager.requested_locked.unwrap(); + let LockedBlock::Regular(validated) = locked else { + panic!("Unexpected locked fast block."); + }; assert_eq!( - validator_manager - .requested_locked - .unwrap() - .executed_block() - .block - .operations, + validated.executed_block().block.operations, blob_2_3_operations, ); @@ -1996,7 +2007,7 @@ where .unwrap() .manager; assert_eq!( - manager.requested_locked.unwrap().round, + manager.requested_locked.unwrap().round(), Round::MultiLeader(1) ); assert!(client0.pending_block().is_some()); @@ -2112,7 +2123,10 @@ where // Validator 0 may or may not have processed the validated block before the update was // canceled due to the errors from the faulty validators. Submit it again to make sure // it's there, so that client 1 can download and re-propose it later. - let validated_block_certificate = *manager.requested_locked.unwrap(); + let locked = *manager.requested_locked.unwrap(); + let LockedBlock::Regular(validated_block_certificate) = locked else { + panic!("Unexpected locked fast block."); + }; builder .node(0) .handle_validated_certificate(validated_block_certificate, Vec::new()) @@ -2149,7 +2163,7 @@ where .unwrap() .manager; assert_eq!( - manager.requested_locked.unwrap().round, + manager.requested_locked.unwrap().round(), Round::MultiLeader(0) ); assert_eq!(manager.current_round, Round::MultiLeader(1)); diff --git a/linera-core/src/unit_tests/worker_tests.rs b/linera-core/src/unit_tests/worker_tests.rs index 9f31ebb610c..fddb7d7ccb3 100644 --- a/linera-core/src/unit_tests/worker_tests.rs +++ b/linera-core/src/unit_tests/worker_tests.rs @@ -32,6 +32,7 @@ use linera_chain::{ IncomingBundle, LiteValue, LiteVote, Medium, MessageAction, MessageBundle, Origin, OutgoingMessage, PostedMessage, SignatureAggregator, }, + manager::LockedBlock, test::{make_child_block, make_first_block, BlockTestExt, MessageTestExt, VoteTestExt}, types::{ CertificateValue, ConfirmedBlock, ConfirmedBlockCertificate, GenericCertificate, Timeout, @@ -3349,7 +3350,7 @@ where let (response, _) = worker.handle_chain_info_query(query_values.clone()).await?; assert_eq!( response.info.manager.requested_locked, - Some(Box::new(certificate1)) + Some(Box::new(LockedBlock::Regular(certificate1))) ); // Proposing block2 now would fail. @@ -3376,7 +3377,7 @@ where let (response, _) = worker.handle_chain_info_query(query_values.clone()).await?; assert_eq!( response.info.manager.requested_locked, - Some(Box::new(certificate2)) + Some(Box::new(LockedBlock::Regular(certificate2))) ); let vote = response.info.manager.pending.as_ref().unwrap(); assert_eq!(vote.value, lite_value2); @@ -3421,7 +3422,7 @@ where let (response, _) = worker.handle_chain_info_query(query_values).await?; assert_eq!( response.info.manager.requested_locked, - Some(Box::new(certificate)) + Some(Box::new(LockedBlock::Regular(certificate))) ); Ok(()) } @@ -3619,7 +3620,7 @@ where let (response, _) = worker.handle_chain_info_query(query_values).await?; assert_eq!( response.info.manager.requested_locked, - Some(Box::new(certificate2)) + Some(Box::new(LockedBlock::Regular(certificate2))) ); let vote = response.info.manager.pending.as_ref().unwrap(); assert_eq!(vote.value, lite_value2); diff --git a/linera-rpc/tests/format.rs b/linera-rpc/tests/format.rs index 93a28a6effc..29a432339cb 100644 --- a/linera-rpc/tests/format.rs +++ b/linera-rpc/tests/format.rs @@ -10,7 +10,7 @@ use linera_base::{ }; use linera_chain::{ data_types::{Medium, MessageAction}, - manager::ChainManagerInfo, + manager::{ChainManagerInfo, LockedBlock}, types::{Certificate, CertificateKind, ConfirmedBlock, Timeout, ValidatedBlock}, }; use linera_core::{data_types::CrossChainRequest, node::NodeError}; @@ -52,6 +52,7 @@ fn get_registry() -> Result { tracer.trace_type::(&samples)?; tracer.trace_type::(&samples)?; tracer.trace_type::(&samples)?; + tracer.trace_type::(&samples)?; tracer.trace_type::(&samples)?; tracer.trace_type::(&samples)?; tracer.trace_type::(&samples)?; diff --git a/linera-rpc/tests/snapshots/format__format.yaml.snap b/linera-rpc/tests/snapshots/format__format.yaml.snap index 74289dea678..f28efb7a007 100644 --- a/linera-rpc/tests/snapshots/format__format.yaml.snap +++ b/linera-rpc/tests/snapshots/format__format.yaml.snap @@ -258,7 +258,7 @@ ChainManagerInfo: TYPENAME: BlockProposal - requested_locked: OPTION: - TYPENAME: ValidatedBlockCertificate + TYPENAME: LockedBlock - timeout: OPTION: TYPENAME: TimeoutCertificate @@ -468,6 +468,16 @@ LiteVote: TYPENAME: ValidatorName - signature: TYPENAME: Signature +LockedBlock: + ENUM: + 0: + Fast: + NEWTYPE: + TYPENAME: BlockProposal + 1: + Regular: + NEWTYPE: + TYPENAME: ValidatedBlockCertificate Medium: ENUM: 0: From 28fbfa5c6ccd1d5711b85b9a3656c4bd46aa70d2 Mon Sep 17 00:00:00 2001 From: Andreas Fackler Date: Thu, 16 Jan 2025 11:22:38 +0100 Subject: [PATCH 3/5] Improve LockedBlock comment. --- linera-chain/src/manager.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/linera-chain/src/manager.rs b/linera-chain/src/manager.rs index 069d93bf0ed..e605dc2a956 100644 --- a/linera-chain/src/manager.rs +++ b/linera-chain/src/manager.rs @@ -108,8 +108,8 @@ pub enum Outcome { pub type ValidatedOrConfirmedVote<'a> = Either<&'a Vote, &'a Vote>; -/// 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. +/// 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 { From 3876f9ad5dd59e4c6e4121e8b9078590fa627c81 Mon Sep 17 00:00:00 2001 From: Andreas Fackler Date: Thu, 16 Jan 2025 12:30:11 +0100 Subject: [PATCH 4/5] Always check round number, even if locked block is fast. --- linera-chain/src/manager.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/linera-chain/src/manager.rs b/linera-chain/src/manager.rs index e605dc2a956..0ef61feb49c 100644 --- a/linera-chain/src/manager.rs +++ b/linera-chain/src/manager.rs @@ -404,13 +404,15 @@ 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() { - if locked.hash() == certificate.hash() && locked.round == certificate.round { - return Ok(Outcome::Skip); + if let Some(locked) = self.locked.get() { + if let LockedBlock::Regular(locked_cert) = locked { + if locked_cert.hash() == certificate.hash() && locked.round() == certificate.round { + return Ok(Outcome::Skip); // We already handled this certificate. + } } ensure!( - new_round > locked.round, - ChainError::InsufficientRoundStrict(locked.round) + new_round > locked.round(), + ChainError::InsufficientRoundStrict(locked.round()) ); } Ok(Outcome::Accept) @@ -443,7 +445,7 @@ where self.set_locked(LockedBlock::Regular(certificate), blobs)?; } } - } else if proposal.content.round.is_fast() { + } else if round.is_fast() { // The fast block also counts as locked. self.set_locked(LockedBlock::Fast(proposal.clone()), blobs)?; } From 7bb542040710adb1d479eefe327dc413b021966a Mon Sep 17 00:00:00 2001 From: Andreas Fackler Date: Thu, 16 Jan 2025 13:42:35 +0100 Subject: [PATCH 5/5] Add comment, round number check. --- linera-chain/src/manager.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/linera-chain/src/manager.rs b/linera-chain/src/manager.rs index 0ef61feb49c..e5c315cbf06 100644 --- a/linera-chain/src/manager.rs +++ b/linera-chain/src/manager.rs @@ -450,13 +450,17 @@ where self.set_locked(LockedBlock::Fast(proposal.clone()), blobs)?; } + // If we are a client, we record the proposed block, in case it affects the current + // round number. That way, we don't make another proposal in the same round. 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); + if proposal.content.round < Round::SingleLeader(0) { + self.set_proposed(proposal); + self.update_current_round(local_time); + } return Ok(None); }; + // Otherwise we are a validator: self.check_proposal_round(&proposal)?; // Record the proposed block, so it can be supplied to clients that request it. self.set_proposed(proposal);