diff --git a/node/libs/roles/src/validator/messages/genesis.rs b/node/libs/roles/src/validator/messages/genesis.rs index 9af95411..9f126409 100644 --- a/node/libs/roles/src/validator/messages/genesis.rs +++ b/node/libs/roles/src/validator/messages/genesis.rs @@ -59,7 +59,7 @@ impl fmt::Debug for GenesisHash { /// Genesis with cached hash. #[derive(Clone)] -pub struct Genesis(GenesisRaw, GenesisHash); +pub struct Genesis(pub(crate) GenesisRaw, pub(crate) GenesisHash); impl std::ops::Deref for Genesis { type Target = GenesisRaw; diff --git a/node/libs/roles/src/validator/messages/replica_commit.rs b/node/libs/roles/src/validator/messages/replica_commit.rs index 76f86606..13563a84 100644 --- a/node/libs/roles/src/validator/messages/replica_commit.rs +++ b/node/libs/roles/src/validator/messages/replica_commit.rs @@ -17,10 +17,6 @@ impl ReplicaCommit { .verify(genesis) .map_err(ReplicaCommitVerifyError::BadView)?; - if self.proposal.number < genesis.first_block { - return Err(ReplicaCommitVerifyError::BadBlockNumber); - } - Ok(()) } } @@ -31,9 +27,6 @@ pub enum ReplicaCommitVerifyError { /// Invalid view. #[error("view: {0:#}")] BadView(anyhow::Error), - /// Bad block number. - #[error("block number < first block")] - BadBlockNumber, } /// A Commit Quorum Certificate. It is an aggregate of signed ReplicaCommit messages. @@ -124,7 +117,7 @@ impl CommitQC { let weight = genesis.validators.weight(&self.signers); let threshold = genesis.validators.quorum_threshold(); if weight < threshold { - return Err(CommitQCVerifyError::NotEnoughSigners { + return Err(CommitQCVerifyError::NotEnoughWeight { got: weight, want: threshold, }); @@ -181,7 +174,7 @@ pub enum CommitQCVerifyError { BadSignersSet, /// Weight not reached. #[error("Signers have not reached threshold weight: got {got}, want {want}")] - NotEnoughSigners { + NotEnoughWeight { /// Got weight. got: u64, /// Want weight. diff --git a/node/libs/roles/src/validator/messages/replica_timeout.rs b/node/libs/roles/src/validator/messages/replica_timeout.rs index 02f4b448..5851a0c0 100644 --- a/node/libs/roles/src/validator/messages/replica_timeout.rs +++ b/node/libs/roles/src/validator/messages/replica_timeout.rs @@ -21,22 +21,16 @@ impl ReplicaTimeout { pub fn verify(&self, genesis: &Genesis) -> Result<(), ReplicaTimeoutVerifyError> { self.view .verify(genesis) - .map_err(ReplicaTimeoutVerifyError::View)?; + .map_err(ReplicaTimeoutVerifyError::BadView)?; if let Some(v) = &self.high_vote { - if self.view.number <= v.view.number { - return Err(ReplicaTimeoutVerifyError::HighVoteFutureView); - } v.verify(genesis) - .map_err(ReplicaTimeoutVerifyError::HighVote)?; + .map_err(ReplicaTimeoutVerifyError::InvalidHighVote)?; } if let Some(qc) = &self.high_qc { - if self.view.number <= qc.view().number { - return Err(ReplicaTimeoutVerifyError::HighQCFutureView); - } qc.verify(genesis) - .map_err(ReplicaTimeoutVerifyError::HighQC)?; + .map_err(ReplicaTimeoutVerifyError::InvalidHighQC)?; } Ok(()) @@ -48,19 +42,13 @@ impl ReplicaTimeout { pub enum ReplicaTimeoutVerifyError { /// View. #[error("view: {0:#}")] - View(anyhow::Error), - /// FutureHighVoteView. - #[error("high vote from the future")] - HighVoteFutureView, - /// FutureHighQCView. - #[error("high qc from the future")] - HighQCFutureView, - /// HighVote. - #[error("high_vote: {0:#}")] - HighVote(ReplicaCommitVerifyError), - /// HighQC. - #[error("high_qc: {0:#}")] - HighQC(CommitQCVerifyError), + BadView(anyhow::Error), + /// Invalid High Vote. + #[error("invalid high_vote: {0:#}")] + InvalidHighVote(ReplicaCommitVerifyError), + /// Invalid High QC. + #[error("invalid high_qc: {0:#}")] + InvalidHighQC(CommitQCVerifyError), } /// A quorum certificate of ReplicaTimeout messages. Since not all ReplicaTimeout messages are @@ -129,7 +117,7 @@ impl TimeoutQC { }); }; - // Check if already have a message from the same signer. + // Check if we already have a message from the same signer. if self.map.values().any(|s| s.0[i]) { return Err(TimeoutQCAddError::DuplicateSigner { signer: Box::new(msg.key.clone()), @@ -164,29 +152,23 @@ impl TimeoutQC { pub fn verify(&self, genesis: &Genesis) -> Result<(), TimeoutQCVerifyError> { self.view .verify(genesis) - .map_err(TimeoutQCVerifyError::View)?; + .map_err(TimeoutQCVerifyError::BadView)?; let mut sum = Signers::new(genesis.validators.len()); // Check the ReplicaTimeout messages. for (i, (msg, signers)) in self.map.iter().enumerate() { if msg.view != self.view { - return Err(TimeoutQCVerifyError::InconsistentViews); + return Err(TimeoutQCVerifyError::InconsistentView(i)); } if signers.len() != sum.len() { - return Err(TimeoutQCVerifyError::BadFormat(anyhow::format_err!( - "msg[{i}].signers has wrong length" - ))); + return Err(TimeoutQCVerifyError::WrongSignersLength(i)); } if signers.is_empty() { - return Err(TimeoutQCVerifyError::BadFormat(anyhow::format_err!( - "msg[{i}] has no signers assigned" - ))); + return Err(TimeoutQCVerifyError::NoSignersAssigned(i)); } if !(&sum & signers).is_empty() { - return Err(TimeoutQCVerifyError::BadFormat(anyhow::format_err!( - "overlapping signature sets for different messages" - ))); + return Err(TimeoutQCVerifyError::OverlappingSignatureSet(i)); } msg.verify(genesis) .map_err(|err| TimeoutQCVerifyError::InvalidMessage(i, err))?; @@ -198,7 +180,7 @@ impl TimeoutQC { let weight = genesis.validators.weight(&sum); let threshold = genesis.validators.quorum_threshold(); if weight < threshold { - return Err(TimeoutQCVerifyError::NotEnoughSigners { + return Err(TimeoutQCVerifyError::NotEnoughWeight { got: weight, want: threshold, }); @@ -261,19 +243,25 @@ pub enum TimeoutQCAddError { pub enum TimeoutQCVerifyError { /// Bad view. #[error("Bad view: {0:#}")] - View(anyhow::Error), + BadView(anyhow::Error), /// Inconsistent views. - #[error("Inconsistent views of signed messages")] - InconsistentViews, + #[error("Message with inconsistent view: number [{0}]")] + InconsistentView(usize), /// Invalid message. #[error("Invalid message: number [{0}], {1:#}")] InvalidMessage(usize, ReplicaTimeoutVerifyError), - /// Bad message format. - #[error(transparent)] - BadFormat(anyhow::Error), + /// Wrong signers length. + #[error("Message with wrong signers length: number [{0}]")] + WrongSignersLength(usize), + /// No signers assigned. + #[error("Message with no signers assigned: number [{0}]")] + NoSignersAssigned(usize), + /// Overlapping signature sets. + #[error("Message with overlapping signature set: number [{0}]")] + OverlappingSignatureSet(usize), /// Weight not reached. #[error("Signers have not reached threshold weight: got {got}, want {want}")] - NotEnoughSigners { + NotEnoughWeight { /// Got weight. got: u64, /// Want weight. diff --git a/node/libs/roles/src/validator/messages/tests/leader_proposal.rs b/node/libs/roles/src/validator/messages/tests/leader_proposal.rs index 3c1fdac3..11b66e63 100644 --- a/node/libs/roles/src/validator/messages/tests/leader_proposal.rs +++ b/node/libs/roles/src/validator/messages/tests/leader_proposal.rs @@ -95,3 +95,50 @@ fn test_leader_proposal_verify() { Err(LeaderProposalVerifyError::NewProposalWhenPreviousNotFinalized) ); } + +#[test] +fn test_justification_get_implied_block() { + let ctx = ctx::test_root(&ctx::RealClock); + let rng = &mut ctx.rng(); + let mut setup = Setup::new(rng, 6); + setup.push_blocks(rng, 3); + + let payload: Payload = rng.gen(); + let block_header = BlockHeader { + number: setup.next(), + payload: payload.hash(), + }; + + // Justification with a commit QC + let commit_qc = match setup.blocks.last().unwrap() { + Block::Final(block) => block.justification.clone(), + _ => unreachable!(), + }; + let justification = ProposalJustification::Commit(commit_qc); + let proposal = LeaderProposal { + proposal: block_header, + proposal_payload: Some(payload.clone()), + justification, + }; + + let (implied_block_number, implied_payload) = + proposal.justification.get_implied_block(&setup.genesis); + + assert_eq!(implied_block_number, setup.next()); + assert!(implied_payload.is_none()); + + // Justification with a timeout QC + let timeout_qc = setup.make_timeout_qc(rng, ViewNumber(7), Some(&payload)); + let justification = ProposalJustification::Timeout(timeout_qc); + let proposal = LeaderProposal { + proposal: block_header, + proposal_payload: None, + justification, + }; + + let (implied_block_number, implied_payload) = + proposal.justification.get_implied_block(&setup.genesis); + + assert_eq!(implied_block_number, setup.next()); + assert_eq!(implied_payload, Some(payload.hash())); +} diff --git a/node/libs/roles/src/validator/messages/tests/replica_commit.rs b/node/libs/roles/src/validator/messages/tests/replica_commit.rs index 06516ba5..d7b9d349 100644 --- a/node/libs/roles/src/validator/messages/tests/replica_commit.rs +++ b/node/libs/roles/src/validator/messages/tests/replica_commit.rs @@ -3,88 +3,86 @@ use assert_matches::assert_matches; use zksync_concurrency::ctx; #[test] -fn test_commit_qc() { - let ctx = ctx::test_root(&ctx::RealClock); - let rng = &mut ctx.rng(); +fn test_replica_commit_verify() { + let genesis = genesis_empty_attesters(); + let commit = replica_commit(); + assert!(commit.verify(&genesis).is_ok()); - // This will create equally weighted validators - let setup1 = Setup::new(rng, 6); - let setup2 = Setup::new(rng, 6); - let mut genesis3 = (*setup1.genesis).clone(); - genesis3.validators = - Committee::new(setup1.genesis.validators.iter().take(3).cloned()).unwrap(); - let genesis3 = genesis3.with_hash(); - - for i in 0..setup1.validator_keys.len() + 1 { - let view = rng.gen(); - let mut qc = CommitQC::new(setup1.make_replica_commit(rng, view), &setup1.genesis); - for key in &setup1.validator_keys[0..i] { - qc.add(&key.sign_msg(qc.message.clone()), &setup1.genesis) - .unwrap(); - } - let expected_weight: u64 = setup1 - .genesis - .validators - .iter() - .take(i) - .map(|w| w.weight) - .sum(); - if expected_weight >= setup1.genesis.validators.quorum_threshold() { - qc.verify(&setup1.genesis).unwrap(); - } else { - assert_matches!( - qc.verify(&setup1.genesis), - Err(CommitQCVerifyError::NotEnoughSigners { .. }) - ); - } - - // Mismatching validator sets. - assert!(qc.verify(&setup2.genesis).is_err()); - assert!(qc.verify(&genesis3).is_err()); - } + // Wrong view + let wrong_genesis = genesis_with_attesters().clone(); + assert_matches!( + commit.verify(&wrong_genesis), + Err(ReplicaCommitVerifyError::BadView(_)) + ); } #[test] -fn test_commit_qc_add_errors() { +fn test_commit_qc_add() { let ctx = ctx::test_root(&ctx::RealClock); let rng = &mut ctx.rng(); let setup = Setup::new(rng, 2); let view = rng.gen(); let mut qc = CommitQC::new(setup.make_replica_commit(rng, view), &setup.genesis); let msg = qc.message.clone(); - // Add the message + + // Add the first signature + assert_eq!(qc.signers.count(), 0); + assert!(qc + .add( + &setup.validator_keys[0].sign_msg(msg.clone()), + &setup.genesis + ) + .is_ok()); + assert_eq!(qc.signers.count(), 1); + + // Try to add a signature from a signer not in committee assert_matches!( qc.add( - &setup.validator_keys[0].sign_msg(msg.clone()), + &rng.gen::().sign_msg(msg.clone()), &setup.genesis ), - Ok(()) + Err(CommitQCAddError::SignerNotInCommittee { .. }) ); - // Try to add a message for a different view - let mut msg1 = msg.clone(); - msg1.view.number = view.next(); + // Try to add a signature from the same validator assert_matches!( - qc.add(&setup.validator_keys[0].sign_msg(msg1), &setup.genesis), - Err(CommitQCAddError::InconsistentMessages { .. }) + qc.add( + &setup.validator_keys[0].sign_msg(msg.clone()), + &setup.genesis + ), + Err(CommitQCAddError::DuplicateSigner { .. }) ); - // Try to add a message from a signer not in committee + // Try to add an invalid signature assert_matches!( qc.add( - &rng.gen::().sign_msg(msg.clone()), + &Signed { + msg: msg.clone(), + key: setup.validator_keys[1].public(), + sig: rng.gen() + }, &setup.genesis ), - Err(CommitQCAddError::SignerNotInCommittee { .. }) + Err(CommitQCAddError::BadSignature(_)) + ); + + // Try to add a signature for a different message + let mut msg1 = msg.clone(); + msg1.view.number = view.next(); + assert_matches!( + qc.add(&setup.validator_keys[1].sign_msg(msg1), &setup.genesis), + Err(CommitQCAddError::InconsistentMessages) ); - // Try to add the same message already added by same validator + // Try to add an invalid message + let mut wrong_genesis = setup.genesis.clone().0; + wrong_genesis.chain_id = rng.gen(); assert_matches!( qc.add( - &setup.validator_keys[0].sign_msg(msg.clone()), - &setup.genesis + &setup.validator_keys[1].sign_msg(msg.clone()), + &wrong_genesis.with_hash() ), - Err(CommitQCAddError::DuplicateSigner { .. }) + Err(CommitQCAddError::InvalidMessage(_)) ); // Add same message signed by another validator. @@ -92,4 +90,50 @@ fn test_commit_qc_add_errors() { qc.add(&setup.validator_keys[1].sign_msg(msg), &setup.genesis), Ok(()) ); + assert_eq!(qc.signers.count(), 2); +} + +#[test] +fn test_commit_qc_verify() { + let ctx = ctx::test_root(&ctx::RealClock); + let rng = &mut ctx.rng(); + let setup = Setup::new(rng, 6); + let view = rng.gen(); + let qc = setup.make_commit_qc(rng, view); + + // Verify the QC + assert!(qc.verify(&setup.genesis).is_ok()); + + // QC with bad message + let mut qc1 = qc.clone(); + qc1.message.view.genesis = rng.gen(); + assert_matches!( + qc1.verify(&setup.genesis), + Err(CommitQCVerifyError::InvalidMessage(_)) + ); + + // QC with too many signers + let mut qc2 = qc.clone(); + qc2.signers = Signers::new(setup.genesis.validators.len() + 1); + assert_matches!( + qc2.verify(&setup.genesis), + Err(CommitQCVerifyError::BadSignersSet) + ); + + // QC with not enough weight + let mut qc3 = qc.clone(); + qc3.signers.0.set(0, false); + qc3.signers.0.set(4, false); + assert_matches!( + qc3.verify(&setup.genesis), + Err(CommitQCVerifyError::NotEnoughWeight { .. }) + ); + + // QC with bad signature + let mut qc4 = qc.clone(); + qc4.signature = rng.gen(); + assert_matches!( + qc4.verify(&setup.genesis), + Err(CommitQCVerifyError::BadSignature(_)) + ); } diff --git a/node/libs/roles/src/validator/messages/tests/replica_timeout.rs b/node/libs/roles/src/validator/messages/tests/replica_timeout.rs index f69b08a6..e07221cc 100644 --- a/node/libs/roles/src/validator/messages/tests/replica_timeout.rs +++ b/node/libs/roles/src/validator/messages/tests/replica_timeout.rs @@ -1,55 +1,35 @@ use super::*; use assert_matches::assert_matches; -use rand::seq::SliceRandom as _; use zksync_concurrency::ctx; #[test] -fn test_timeout_qc() { - let ctx = ctx::test_root(&ctx::RealClock); - let rng = &mut ctx.rng(); +fn test_replica_timeout_verify() { + let genesis = genesis_empty_attesters(); + let timeout = replica_timeout(); + assert!(timeout.verify(&genesis).is_ok()); - // This will create equally weighted validators - let setup1 = Setup::new(rng, 6); - let setup2 = Setup::new(rng, 6); - let mut genesis3 = (*setup1.genesis).clone(); - genesis3.validators = - Committee::new(setup1.genesis.validators.iter().take(3).cloned()).unwrap(); - let genesis3 = genesis3.with_hash(); - - let view: ViewNumber = rng.gen(); - let msgs: Vec<_> = (0..3) - .map(|_| setup1.make_replica_timeout(rng, view)) - .collect(); - - for n in 0..setup1.validator_keys.len() + 1 { - let mut qc = TimeoutQC::new(msgs[0].view.clone()); - for key in &setup1.validator_keys[0..n] { - qc.add( - &key.sign_msg(msgs.choose(rng).unwrap().clone()), - &setup1.genesis, - ) - .unwrap(); - } - let expected_weight: u64 = setup1 - .genesis - .validators - .iter() - .take(n) - .map(|w| w.weight) - .sum(); - if expected_weight >= setup1.genesis.validators.quorum_threshold() { - qc.verify(&setup1.genesis).unwrap(); - } else { - assert_matches!( - qc.verify(&setup1.genesis), - Err(TimeoutQCVerifyError::NotEnoughSigners { .. }) - ); - } - - // Mismatching validator sets. - assert!(qc.verify(&setup2.genesis).is_err()); - assert!(qc.verify(&genesis3).is_err()); - } + // Wrong view + let wrong_genesis = genesis_with_attesters().clone(); + assert_matches!( + timeout.verify(&wrong_genesis), + Err(ReplicaTimeoutVerifyError::BadView(_)) + ); + + // Invalid high vote + let mut timeout = replica_timeout(); + timeout.high_vote.as_mut().unwrap().view.genesis = genesis_with_attesters().hash(); + assert_matches!( + timeout.verify(&genesis), + Err(ReplicaTimeoutVerifyError::InvalidHighVote(_)) + ); + + // Invalid high QC + let mut timeout = replica_timeout(); + timeout.high_qc.as_mut().unwrap().message.view.genesis = genesis_with_attesters().hash(); + assert_matches!( + timeout.verify(&genesis), + Err(ReplicaTimeoutVerifyError::InvalidHighQC(_)) + ); } #[test] @@ -112,31 +92,57 @@ fn test_timeout_qc_high_vote() { } #[test] -fn test_timeout_qc_add_errors() { +fn test_timeout_qc_high_qc() { + let ctx = ctx::test_root(&ctx::RealClock); + let rng = &mut ctx.rng(); + let setup = Setup::new(rng, 3); + let view = View { + genesis: setup.genesis.hash(), + number: ViewNumber(100), + }; + let mut qc = TimeoutQC::new(view); + + // No high QC + assert!(qc.high_qc().is_none()); + + // Add signatures with different high QC views + for i in 0..3 { + let high_vote_view = view.number; + let high_qc_view = ViewNumber(view.number.0 - i as u64); + let msg = ReplicaTimeout { + view: setup.make_view(view.number), + high_vote: Some(setup.make_replica_commit(rng, high_vote_view)), + high_qc: Some(setup.make_commit_qc(rng, high_qc_view)), + }; + qc.add( + &setup.validator_keys[i].sign_msg(msg.clone()), + &setup.genesis, + ) + .unwrap(); + } + + assert_eq!(qc.high_qc().unwrap().message.view.number, view.number); +} + +#[test] +fn test_timeout_qc_add() { let ctx = ctx::test_root(&ctx::RealClock); let rng = &mut ctx.rng(); - let setup = Setup::new(rng, 2); + let setup = Setup::new(rng, 3); let view = rng.gen(); let msg = setup.make_replica_timeout(rng, view); let mut qc = TimeoutQC::new(msg.view.clone()); - let msg = setup.make_replica_timeout(rng, view); - // Add the message - assert_matches!( - qc.add( + // Add the first signature + assert!(qc.map.is_empty()); + assert!(qc + .add( &setup.validator_keys[0].sign_msg(msg.clone()), &setup.genesis - ), - Ok(()) - ); - - // Try to add a message for a different view - let mut msg1 = msg.clone(); - msg1.view.number = view.next(); - assert_matches!( - qc.add(&setup.validator_keys[0].sign_msg(msg1), &setup.genesis), - Err(TimeoutQCAddError::InconsistentViews { .. }) - ); + ) + .is_ok()); + assert_eq!(qc.map.len(), 1); + assert_eq!(qc.map.values().next().unwrap().count(), 1); // Try to add a message from a signer not in committee assert_matches!( @@ -156,19 +162,171 @@ fn test_timeout_qc_add_errors() { Err(TimeoutQCAddError::DuplicateSigner { .. }) ); - // Try to add a message for a validator that already added another message - let msg2 = setup.make_replica_timeout(rng, view); + // Try to add an invalid signature assert_matches!( - qc.add(&setup.validator_keys[0].sign_msg(msg2), &setup.genesis), - Err(TimeoutQCAddError::DuplicateSigner { .. }) + qc.add( + &Signed { + msg: msg.clone(), + key: setup.validator_keys[1].public(), + sig: rng.gen() + }, + &setup.genesis + ), + Err(TimeoutQCAddError::BadSignature(_)) ); - // Add same message signed by another validator. + // Try to add a message with a different view + let mut msg1 = msg.clone(); + msg1.view.number = view.next(); + assert_matches!( + qc.add(&setup.validator_keys[1].sign_msg(msg1), &setup.genesis), + Err(TimeoutQCAddError::InconsistentViews) + ); + + // Try to add an invalid message + let mut wrong_genesis = setup.genesis.clone().0; + wrong_genesis.chain_id = rng.gen(); assert_matches!( qc.add( &setup.validator_keys[1].sign_msg(msg.clone()), - &setup.genesis + &wrong_genesis.with_hash() ), - Ok(()) + Err(TimeoutQCAddError::InvalidMessage(_)) + ); + + // Add same message signed by another validator. + assert!(qc + .add( + &setup.validator_keys[1].sign_msg(msg.clone()), + &setup.genesis + ) + .is_ok()); + assert_eq!(qc.map.len(), 1); + assert_eq!(qc.map.values().next().unwrap().count(), 2); + + // Add a different message signed by another validator. + let msg2 = setup.make_replica_timeout(rng, view); + assert!(qc + .add( + &setup.validator_keys[2].sign_msg(msg2.clone()), + &setup.genesis + ) + .is_ok()); + assert_eq!(qc.map.len(), 2); + assert_eq!(qc.map.values().next().unwrap().count(), 2); + assert_eq!(qc.map.values().last().unwrap().count(), 1); +} + +#[test] +fn test_timeout_qc_verify() { + let ctx = ctx::test_root(&ctx::RealClock); + let rng = &mut ctx.rng(); + let mut setup = Setup::new(rng, 6); + setup.push_blocks(rng, 2); + let view = rng.gen(); + let qc = setup.make_timeout_qc(rng, view, None); + + // Verify the QC + assert!(qc.verify(&setup.genesis).is_ok()); + + // QC with bad view + let mut qc1 = qc.clone(); + qc1.view = rng.gen(); + assert_matches!( + qc1.verify(&setup.genesis), + Err(TimeoutQCVerifyError::BadView(_)) + ); + + // QC with message with inconsistent view + let mut qc2 = qc.clone(); + qc2.map.insert( + ReplicaTimeout { + view: qc2.view.clone().next(), + high_vote: None, + high_qc: None, + }, + Signers::new(setup.genesis.validators.len()), + ); + assert_matches!( + qc2.verify(&setup.genesis), + Err(TimeoutQCVerifyError::InconsistentView(_)) + ); + + // QC with message with wrong signer length + let mut qc3 = qc.clone(); + qc3.map.insert( + ReplicaTimeout { + view: qc3.view.clone(), + high_vote: None, + high_qc: None, + }, + Signers::new(setup.genesis.validators.len() + 1), + ); + assert_matches!( + qc3.verify(&setup.genesis), + Err(TimeoutQCVerifyError::WrongSignersLength(_)) + ); + + // QC with message with no signers + let mut qc4 = qc.clone(); + qc4.map.insert( + ReplicaTimeout { + view: qc4.view.clone(), + high_vote: None, + high_qc: None, + }, + Signers::new(setup.genesis.validators.len()), + ); + assert_matches!( + qc4.verify(&setup.genesis), + Err(TimeoutQCVerifyError::NoSignersAssigned(_)) + ); + + // QC with overlapping signers + let mut qc5 = qc.clone(); + let mut signers = Signers::new(setup.genesis.validators.len()); + signers + .0 + .set(rng.gen_range(0..setup.genesis.validators.len()), true); + qc5.map.insert( + ReplicaTimeout { + view: qc5.view.clone(), + high_vote: None, + high_qc: None, + }, + signers, + ); + assert_matches!( + qc5.verify(&setup.genesis), + Err(TimeoutQCVerifyError::OverlappingSignatureSet(_)) + ); + + // QC with invalid message + let mut qc6 = qc.clone(); + let (mut timeout, signers) = qc6.map.pop_first().unwrap(); + timeout.high_qc = Some(rng.gen()); + qc6.map.insert(timeout, signers); + assert_matches!( + qc6.verify(&setup.genesis), + Err(TimeoutQCVerifyError::InvalidMessage(_, _)) + ); + + // QC with not enough weight + let mut qc7 = qc.clone(); + let (timeout, mut signers) = qc7.map.pop_first().unwrap(); + signers.0.set(0, false); + signers.0.set(4, false); + qc7.map.insert(timeout, signers); + assert_matches!( + qc7.verify(&setup.genesis), + Err(TimeoutQCVerifyError::NotEnoughWeight { .. }) + ); + + // QC with bad signature + let mut qc8 = qc.clone(); + qc8.signature = rng.gen(); + assert_matches!( + qc8.verify(&setup.genesis), + Err(TimeoutQCVerifyError::BadSignature(_)) ); } diff --git a/node/libs/roles/src/validator/testonly.rs b/node/libs/roles/src/validator/testonly.rs index b88768d9..eff17eda 100644 --- a/node/libs/roles/src/validator/testonly.rs +++ b/node/libs/roles/src/validator/testonly.rs @@ -249,7 +249,7 @@ impl Setup { /// Creates a ReplicaTimeout with a random payload. pub fn make_replica_timeout(&self, rng: &mut impl Rng, view: ViewNumber) -> ReplicaTimeout { - let high_vote_view = ViewNumber(rng.gen_range(0..view.0)); + let high_vote_view = ViewNumber(rng.gen_range(0..=view.0)); let high_qc_view = ViewNumber(rng.gen_range(0..high_vote_view.0)); ReplicaTimeout { view: self.make_view(view), @@ -277,16 +277,16 @@ impl Setup { }; let mut qc = TimeoutQC::new(self.make_view(view)); + if payload_opt.is_none() { + vote.proposal.payload = rng.gen(); + } + let msg = ReplicaTimeout { + view: self.make_view(view), + high_vote: Some(vote.clone()), + high_qc: Some(commit_qc.clone()), + }; for key in &self.validator_keys { - if payload_opt.is_none() { - vote.proposal.payload = rng.gen(); - } - let msg = ReplicaTimeout { - view: self.make_view(view), - high_vote: Some(vote.clone()), - high_qc: Some(commit_qc.clone()), - }; - qc.add(&key.sign_msg(msg), &self.genesis).unwrap(); + qc.add(&key.sign_msg(msg.clone()), &self.genesis).unwrap(); } qc diff --git a/spec/informal-spec/types.rs b/spec/informal-spec/types.rs index f5789cff..a2a517c6 100644 --- a/spec/informal-spec/types.rs +++ b/spec/informal-spec/types.rs @@ -183,8 +183,10 @@ impl SignedTimeoutVote { } fn verify(self) -> bool { - // If we wish, there are two invariants that are easy to check but aren't required for correctness: - // self.view() >= self.high_vote.view() && self.high_vote.view() >= self.high_commit_qc_view + // If we wish, there are three invariants that are easy to check but don't need to be stricly enforced for correctness: + // 1. self.view() >= self.high_vote.view() + // 2. self.high_vote.view() >= self.high_commit_qc_view + // 3. self.view() > self.high_commit_qc_view self.vote.high_commit_qc_view == self.high_commit_qc.map(|x| x.view()) && self.verify_sig() && self.high_commit_qc.map(|qc| qc.verify()) }