From be233f17242be2a9a8703e84a6ebd81bec2404aa Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Fri, 12 Apr 2024 12:29:19 +0200 Subject: [PATCH 01/11] light-client-verifier: refactor VotingPowerTally by introducing helper methods --- .../src/operations/voting_power.rs | 67 +++++++++++-------- 1 file changed, 38 insertions(+), 29 deletions(-) diff --git a/light-client-verifier/src/operations/voting_power.rs b/light-client-verifier/src/operations/voting_power.rs index e0f98e26c..2acf83f48 100644 --- a/light-client-verifier/src/operations/voting_power.rs +++ b/light-client-verifier/src/operations/voting_power.rs @@ -30,6 +30,34 @@ pub struct VotingPowerTally { pub trust_threshold: TrustThreshold, } +impl VotingPowerTally { + fn new(total: u64, trust_threshold: TrustThreshold) -> Self { + Self { + total, + tallied: 0, + trust_threshold, + } + } + + /// Adds given amount of power to tallied voting power amount. + fn tally(&mut self, power: u64) { + self.tallied += power; + debug_assert!(self.tallied <= self.total); + } + + /// Checks whether tallied amount meets trust threshold. + fn check(&self) -> Result<(), Self> { + if self + .trust_threshold + .is_enough_power(self.tallied, self.total) + { + Ok(()) + } else { + Err(*self) + } + } +} + impl fmt::Display for VotingPowerTally { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!( @@ -60,14 +88,9 @@ pub trait VotingPowerCalculator: Send + Sync { trusted_validators: &ValidatorSet, trust_threshold: TrustThreshold, ) -> Result<(), VerificationError> { - let voting_power = - self.voting_power_in(untrusted_header, trusted_validators, trust_threshold)?; - - if trust_threshold.is_enough_power(voting_power.tallied, voting_power.total) { - Ok(()) - } else { - Err(VerificationError::not_enough_trust(voting_power)) - } + self.voting_power_in(untrusted_header, trusted_validators, trust_threshold)? + .check() + .map_err(VerificationError::not_enough_trust) } /// Check if there is 2/3rd overlap between an untrusted header and untrusted validator set @@ -77,16 +100,9 @@ pub trait VotingPowerCalculator: Send + Sync { untrusted_validators: &ValidatorSet, ) -> Result<(), VerificationError> { let trust_threshold = TrustThreshold::TWO_THIRDS; - let voting_power = - self.voting_power_in(untrusted_header, untrusted_validators, trust_threshold)?; - - if trust_threshold.is_enough_power(voting_power.tallied, voting_power.total) { - Ok(()) - } else { - Err(VerificationError::insufficient_signers_overlap( - voting_power, - )) - } + self.voting_power_in(untrusted_header, untrusted_validators, trust_threshold)? + .check() + .map_err(VerificationError::insufficient_signers_overlap) } /// Compute the voting power in a header and its commit against a validator set. @@ -185,9 +201,8 @@ impl VotingPowerCalculator for ProvidedVotingPowerCalcul trust_threshold: TrustThreshold, ) -> Result { let signatures = &signed_header.commit.signatures; - let total_voting_power = self.total_power_of(validator_set); - - let mut tallied_voting_power = 0_u64; + let mut voting_power = + VotingPowerTally::new(self.total_power_of(validator_set), trust_threshold); // Get non-absent votes from the signatures let non_absent_votes = signatures.iter().enumerate().flat_map(|(idx, signature)| { @@ -237,24 +252,18 @@ impl VotingPowerCalculator for ProvidedVotingPowerCalcul // If the vote is neither absent nor nil, tally its power if signature.is_commit() { - tallied_voting_power += validator.power(); + voting_power.tally(validator.power()); } else { // It's OK. We include stray signatures (~votes for nil) // to measure validator availability. } // Break out of the loop when we have enough voting power. - if trust_threshold.is_enough_power(tallied_voting_power, total_voting_power) { + if voting_power.check().is_ok() { break; } } - let voting_power = VotingPowerTally { - total: total_voting_power, - tallied: tallied_voting_power, - trust_threshold, - }; - Ok(voting_power) } } From c7a33197cf81ca16dfa59b791cb8dc2bb5811ca6 Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Fri, 12 Apr 2024 09:37:17 +0200 Subject: [PATCH 02/11] light-client-verifier: avoid checking signatures multiple times Introduce VotingPowerCalculator::voting_power_in_sets method. --- .../src/operations/voting_power.rs | 345 ++++++++++-------- light-client-verifier/src/predicates.rs | 31 +- light-client-verifier/src/verifier.rs | 54 ++- 3 files changed, 246 insertions(+), 184 deletions(-) diff --git a/light-client-verifier/src/operations/voting_power.rs b/light-client-verifier/src/operations/voting_power.rs index 2acf83f48..df46dff80 100644 --- a/light-client-verifier/src/operations/voting_power.rs +++ b/light-client-verifier/src/operations/voting_power.rs @@ -7,6 +7,7 @@ use serde::{Deserialize, Serialize}; use tendermint::{ account, block::CommitSig, + chain, crypto::signature, trust_threshold::TrustThreshold as _, validator, @@ -80,17 +81,28 @@ pub trait VotingPowerCalculator: Send + Sync { .fold(0u64, |total, val_info| total + val_info.power.value()) } - /// Check against the given threshold that there is enough trust - /// between an untrusted header and a trusted validator set - fn check_enough_trust( + /// Check a) against the given threshold that there is enough trust between + /// an untrusted header and a trusted validator set and b) if there is 2/3rd + /// overlap between an untrusted header and untrusted validator set. + fn check_enough_trust_and_signers( &self, untrusted_header: &SignedHeader, trusted_validators: &ValidatorSet, trust_threshold: TrustThreshold, + untrusted_validators: &ValidatorSet, ) -> Result<(), VerificationError> { - self.voting_power_in(untrusted_header, trusted_validators, trust_threshold)? + let (trusted_power, untrusted_power) = self.voting_power_in_sets( + untrusted_header, + (trusted_validators, trust_threshold), + (untrusted_validators, TrustThreshold::TWO_THIRDS), + )?; + trusted_power + .check() + .map_err(VerificationError::not_enough_trust)?; + untrusted_power .check() - .map_err(VerificationError::not_enough_trust) + .map_err(VerificationError::insufficient_signers_overlap)?; + Ok(()) } /// Check if there is 2/3rd overlap between an untrusted header and untrusted validator set @@ -105,16 +117,32 @@ pub trait VotingPowerCalculator: Send + Sync { .map_err(VerificationError::insufficient_signers_overlap) } - /// Compute the voting power in a header and its commit against a validator set. + /// Compute the voting power in a header and its commit against a validator + /// set. + /// + /// Note that the returned tally may be lower than actual tally so long as + /// it meets the `trust_threshold`. /// - /// The `trust_threshold` is currently not used, but might be in the future - /// for optimization purposes. + /// If you have two separate sets of validators and need to check voting + /// power for both of them, prefer [`Self::voting_power_in_sets`] method. fn voting_power_in( &self, signed_header: &SignedHeader, validator_set: &ValidatorSet, trust_threshold: TrustThreshold, ) -> Result; + + /// Compute the voting power in a header and its commit against two separate + /// validator sets. + /// + /// This is equivalent to calling [`Self::voting_power_in`] on each set + /// separately but may be more optimised. + fn voting_power_in_sets( + &self, + signed_header: &SignedHeader, + first_set: (&ValidatorSet, TrustThreshold), + second_set: (&ValidatorSet, TrustThreshold), + ) -> Result<(VotingPowerTally, VotingPowerTally), VerificationError>; } /// Default implementation of a `VotingPowerCalculator`, parameterized with @@ -136,55 +164,135 @@ impl Default for ProvidedVotingPowerCalculator { } } -/// Dictionary of validators sorted by address. -/// -/// The map stores reference to [`validator::Info`] object (typically held by -/// a `ValidatorSet`) and a boolean flag indicating whether the validator has -/// already been seen. The validators are sorted by their address such that -/// lookup by address is a logarithmic operation. -struct ValidatorMap<'a> { - validators: Vec<(&'a validator::Info, bool)>, +/// A signed non-nill vote. +struct NonAbsentCommitVote { + signed_vote: SignedVote, + /// Flag indicating whether the signature has already been verified. + verified: bool, } -/// Error during validator lookup. -enum LookupError { - NotFound, - AlreadySeen, +impl NonAbsentCommitVote { + /// Returns a signed non-nil vote for given commit. + /// + /// If the CimmtSig represents a missing vote or a vote for nil returns + /// `None`. Otherwise, if the vote is missing a signature returns + /// `Some(Err)`. Otherwise, returns a `SignedVote` corresponding to given + /// `CommitSig`. + pub fn new( + commit_sig: &CommitSig, + validator_index: ValidatorIndex, + commit: &Commit, + chain_id: &chain::Id, + ) -> Option> { + let (validator_address, timestamp, signature) = match commit_sig { + CommitSig::BlockIdFlagAbsent { .. } => return None, + CommitSig::BlockIdFlagCommit { + validator_address, + timestamp, + signature, + } => (*validator_address, *timestamp, signature), + CommitSig::BlockIdFlagNil { .. } => return None, + }; + + let vote = Vote { + vote_type: tendermint::vote::Type::Precommit, + height: commit.height, + round: commit.round, + block_id: Some(commit.block_id), + timestamp: Some(timestamp), + validator_address, + validator_index, + signature: signature.clone(), + extension: Default::default(), + extension_signature: None, + }; + Some( + SignedVote::from_vote(vote, chain_id.clone()) + .ok_or_else(VerificationError::missing_signature) + .map(|signed_vote| Self { + signed_vote, + verified: false, + }), + ) + } + + /// Returns address of the validator making the vote. + pub fn validator_id(&self) -> account::Id { + self.signed_vote.validator_id() + } } -impl<'a> ValidatorMap<'a> { - /// Constructs a new map from given list of validators. - /// - /// Sorts the validators by address which makes the operation’s time - /// complexity `O(N lng N)`. - /// - /// Produces unspecified result if two objects with the same address are - /// found. Unspecified in that it’s not guaranteed which entry will be - /// subsequently returned by [`Self::find_mut`] however it will always be - /// consistently the same entry. - pub fn new(validators: &'a [validator::Info]) -> Self { - let mut validators = validators.iter().map(|v| (v, false)).collect::>(); - validators.sort_unstable_by_key(|item| &item.0.address); - Self { validators } +/// Collection of non-absent commit votes. +struct NonAbsentCommitVotes { + /// Votes sorted by validator address. + votes: Vec, +} + +impl NonAbsentCommitVotes { + pub fn new(signed_header: &SignedHeader) -> Result { + let mut votes = signed_header + .commit + .signatures + .iter() + .enumerate() + .flat_map(|(idx, signature)| { + let idx = ValidatorIndex::try_from(idx).unwrap(); + NonAbsentCommitVote::new( + signature, + idx, + &signed_header.commit, + &signed_header.header.chain_id, + ) + }) + .collect::, VerificationError>>()?; + votes.sort_unstable_by_key(NonAbsentCommitVote::validator_id); + + // Check if there are duplicate signatures. If at least one duplicate + // is found, report it as an error. + let duplicate = votes + .windows(2) + .find(|pair| pair[0].validator_id() == pair[1].validator_id()); + if let Some(pair) = duplicate { + Err(VerificationError::duplicate_validator( + pair[0].validator_id(), + )) + } else { + Ok(Self { votes }) + } } - /// Finds entry for validator with given address; returns error if validator - /// has been returned already by previous call to `find`. + /// Looks up a vote cast by given validator. /// - /// Uses binary search resulting in logarithmic lookup time. - pub fn find(&mut self, address: &account::Id) -> Result<&'a validator::Info, LookupError> { - let index = self - .validators - .binary_search_by_key(&address, |item| &item.0.address) - .map_err(|_| LookupError::NotFound)?; - - let (validator, seen) = &mut self.validators[index]; - if *seen { - Err(LookupError::AlreadySeen) - } else { - *seen = true; - Ok(validator) + /// If the validator didn’t cast a vote or voted for `nil`, returns + /// `Ok(false)`. Otherwise, if the vote had valid signature, returns + /// `Ok(true)`. If the vote had invalid signature, returns `Err`. + pub fn has_voted( + &mut self, + validator: &validator::Info, + ) -> Result { + let idx = self + .votes + .binary_search_by_key(&validator.address, NonAbsentCommitVote::validator_id); + let vote = match idx { + Ok(idx) => &mut self.votes[idx], + Err(_) => return Ok(false), + }; + + if !vote.verified { + let sign_bytes = vote.signed_vote.sign_bytes(); + validator + .verify_signature::(&sign_bytes, vote.signed_vote.signature()) + .map_err(|_| { + VerificationError::invalid_signature( + vote.signed_vote.signature().as_bytes().to_vec(), + Box::new(validator.clone()), + sign_bytes, + ) + })?; } + + vote.verified = true; + Ok(true) } } @@ -200,110 +308,55 @@ impl VotingPowerCalculator for ProvidedVotingPowerCalcul validator_set: &ValidatorSet, trust_threshold: TrustThreshold, ) -> Result { - let signatures = &signed_header.commit.signatures; - let mut voting_power = - VotingPowerTally::new(self.total_power_of(validator_set), trust_threshold); - - // Get non-absent votes from the signatures - let non_absent_votes = signatures.iter().enumerate().flat_map(|(idx, signature)| { - non_absent_vote( - signature, - ValidatorIndex::try_from(idx).unwrap(), - &signed_header.commit, - ) - .map(|vote| (signature, vote)) - }); - - // Create index of validators sorted by address. - let mut validators = ValidatorMap::new(validator_set.validators()); - - for (signature, vote) in non_absent_votes { - // Find the validator by address. - let validator = match validators.find(&vote.validator_address) { - Ok(validator) => validator, - Err(LookupError::NotFound) => { - // Cannot find matching validator, so we skip the vote - continue; - }, - Err(LookupError::AlreadySeen) => { - // Ensure we only count a validator's power once - return Err(VerificationError::duplicate_validator( - vote.validator_address, - )); - }, - }; - - let signed_vote = - SignedVote::from_vote(vote.clone(), signed_header.header.chain_id.clone()) - .ok_or_else(VerificationError::missing_signature)?; - - // Check vote is valid - let sign_bytes = signed_vote.sign_bytes(); - if validator - .verify_signature::(&sign_bytes, signed_vote.signature()) - .is_err() - { - return Err(VerificationError::invalid_signature( - signed_vote.signature().as_bytes().to_vec(), - Box::new(validator.clone()), - sign_bytes, - )); - } + let mut votes = NonAbsentCommitVotes::new(signed_header)?; + voting_power_in_impl::( + &mut votes, + validator_set, + trust_threshold, + self.total_power_of(validator_set), + ) + } - // If the vote is neither absent nor nil, tally its power - if signature.is_commit() { - voting_power.tally(validator.power()); - } else { - // It's OK. We include stray signatures (~votes for nil) - // to measure validator availability. - } + fn voting_power_in_sets( + &self, + signed_header: &SignedHeader, + first_set: (&ValidatorSet, TrustThreshold), + second_set: (&ValidatorSet, TrustThreshold), + ) -> Result<(VotingPowerTally, VotingPowerTally), VerificationError> { + let mut votes = NonAbsentCommitVotes::new(signed_header)?; + let first_tally = voting_power_in_impl::( + &mut votes, + first_set.0, + first_set.1, + self.total_power_of(first_set.0), + )?; + let second_tally = voting_power_in_impl::( + &mut votes, + second_set.0, + second_set.1, + self.total_power_of(second_set.0), + )?; + Ok((first_tally, second_tally)) + } +} +fn voting_power_in_impl( + votes: &mut NonAbsentCommitVotes, + validator_set: &ValidatorSet, + trust_threshold: TrustThreshold, + total_voting_power: u64, +) -> Result { + let mut power = VotingPowerTally::new(total_voting_power, trust_threshold); + for validator in validator_set.validators() { + if votes.has_voted::(validator)? { + power.tally(validator.power()); // Break out of the loop when we have enough voting power. - if voting_power.check().is_ok() { + if power.check().is_ok() { break; } } - - Ok(voting_power) } -} - -fn non_absent_vote( - commit_sig: &CommitSig, - validator_index: ValidatorIndex, - commit: &Commit, -) -> Option { - let (validator_address, timestamp, signature, block_id) = match commit_sig { - CommitSig::BlockIdFlagAbsent { .. } => return None, - CommitSig::BlockIdFlagCommit { - validator_address, - timestamp, - signature, - } => ( - *validator_address, - *timestamp, - signature, - Some(commit.block_id), - ), - CommitSig::BlockIdFlagNil { - validator_address, - timestamp, - signature, - } => (*validator_address, *timestamp, signature, None), - }; - - Some(Vote { - vote_type: tendermint::vote::Type::Precommit, - height: commit.height, - round: commit.round, - block_id, - timestamp: Some(timestamp), - validator_address, - validator_index, - signature: signature.clone(), - extension: Default::default(), - extension_signature: None, - }) + Ok(power) } // The below unit tests replaces the static voting power test files diff --git a/light-client-verifier/src/predicates.rs b/light-client-verifier/src/predicates.rs index c0e70b518..b0f96c206 100644 --- a/light-client-verifier/src/predicates.rs +++ b/light-client-verifier/src/predicates.rs @@ -184,28 +184,36 @@ pub trait VerificationPredicates: Send + Sync { } } - /// Check that there is enough validators overlap between the trusted validator set - /// and the untrusted signed header. - fn has_sufficient_validators_overlap( + /// Check that there is enough signers overlap between the given, untrusted + /// validator set and the untrusted signed header. + fn has_sufficient_signers_overlap( &self, untrusted_sh: &SignedHeader, - trusted_validators: &ValidatorSet, - trust_threshold: &TrustThreshold, + untrusted_validators: &ValidatorSet, calculator: &dyn VotingPowerCalculator, ) -> Result<(), VerificationError> { - calculator.check_enough_trust(untrusted_sh, trusted_validators, *trust_threshold)?; + calculator.check_signers_overlap(untrusted_sh, untrusted_validators)?; Ok(()) } - /// Check that there is enough signers overlap between the given, untrusted validator set - /// and the untrusted signed header. - fn has_sufficient_signers_overlap( + /// Check that there is enough a) validators overlap between the trusted + /// validator set and the untrusted signed header and b) signers overlap + /// between the given, untrusted validator set and the untrusted signed + /// header. + fn has_sufficient_signers_and_validators_overlap( &self, untrusted_sh: &SignedHeader, + trusted_validators: &ValidatorSet, + trust_threshold: &TrustThreshold, untrusted_validators: &ValidatorSet, calculator: &dyn VotingPowerCalculator, ) -> Result<(), VerificationError> { - calculator.check_signers_overlap(untrusted_sh, untrusted_validators)?; + calculator.check_enough_trust_and_signers( + untrusted_sh, + trusted_validators, + *trust_threshold, + untrusted_validators, + )?; Ok(()) } @@ -621,6 +629,8 @@ mod tests { } } + // TODO: Fix the test. DO NOT SUBMIT + /* #[test] fn test_has_sufficient_validators_overlap() { let light_block: LightBlock = TestgenLightBlock::new_default(1).generate().unwrap().into(); @@ -675,6 +685,7 @@ mod tests { _ => panic!("expected NotEnoughTrust error"), } } + */ #[test] fn test_has_sufficient_signers_overlap() { diff --git a/light-client-verifier/src/verifier.rs b/light-client-verifier/src/verifier.rs index ba8e65146..1cee4a229 100644 --- a/light-client-verifier/src/verifier.rs +++ b/light-client-verifier/src/verifier.rs @@ -147,17 +147,6 @@ where Verdict::Success } - /// Verify that more than 2/3 of the validators correctly committed the block. - pub fn verify_commit(&self, untrusted: &UntrustedBlockState<'_>) -> Verdict { - verdict!(self.predicates.has_sufficient_signers_overlap( - untrusted.signed_header, - untrusted.validators, - &self.voting_power_calculator, - )); - - Verdict::Success - } - /// Validate an `UntrustedBlockState` coming from a client update, /// based on the given `TrustedBlockState`, `Options` and current time. pub fn validate_against_trusted( @@ -220,27 +209,38 @@ where Verdict::Success } - /// Check there is enough overlap between the validator sets of the trusted and untrusted - /// blocks. - pub fn verify_commit_against_trusted( + /// Verify that a) there is enough overlap between the validator sets of the + /// trusted and untrusted blocks and b) more than 2/3 of the validators + /// correctly committed the block. + pub fn verify_commit( &self, untrusted: &UntrustedBlockState<'_>, trusted: &TrustedBlockState<'_>, options: &Options, ) -> Verdict { + // If the trusted validator set has changed we need to check if there’s + // overlap between the old trusted set and the new untrested header in + // addition to checking if the new set correctly signed the header. let trusted_next_height = trusted.height.increment(); - - if untrusted.height() != trusted_next_height { - // Check there is enough overlap between the validator sets of - // the trusted and untrusted blocks. - verdict!(self.predicates.has_sufficient_validators_overlap( + let need_both = untrusted.height() != trusted_next_height; + + let result = if need_both { + self.predicates + .has_sufficient_signers_and_validators_overlap( + untrusted.signed_header, + trusted.next_validators, + &options.trust_threshold, + untrusted.validators, + &self.voting_power_calculator, + ) + } else { + self.predicates.has_sufficient_signers_overlap( untrusted.signed_header, - trusted.next_validators, - &options.trust_threshold, + untrusted.validators, &self.voting_power_calculator, - )); - } - + ) + }; + verdict!(result); Verdict::Success } } @@ -288,8 +288,7 @@ where ensure_verdict_success!(self.verify_validator_sets(&untrusted)); ensure_verdict_success!(self.validate_against_trusted(&untrusted, &trusted, options, now)); ensure_verdict_success!(self.check_header_is_from_past(&untrusted, options, now)); - ensure_verdict_success!(self.verify_commit_against_trusted(&untrusted, &trusted, options)); - ensure_verdict_success!(self.verify_commit(&untrusted)); + ensure_verdict_success!(self.verify_commit(&untrusted, &trusted, options)); Verdict::Success } @@ -306,8 +305,7 @@ where ) -> Verdict { ensure_verdict_success!(self.verify_validator_sets(&untrusted)); ensure_verdict_success!(self.validate_against_trusted(&untrusted, &trusted, options, now)); - ensure_verdict_success!(self.verify_commit_against_trusted(&untrusted, &trusted, options)); - ensure_verdict_success!(self.verify_commit(&untrusted)); + ensure_verdict_success!(self.verify_commit(&untrusted, &trusted, options)); Verdict::Success } } From e5f2330b064f89354754ea1661c37829c5264117 Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Wed, 17 Apr 2024 12:47:26 +0200 Subject: [PATCH 03/11] Update light-client-verifier/src/operations/voting_power.rs Co-authored-by: Romain Ruetschi --- light-client-verifier/src/operations/voting_power.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/light-client-verifier/src/operations/voting_power.rs b/light-client-verifier/src/operations/voting_power.rs index df46dff80..c6cea3bc7 100644 --- a/light-client-verifier/src/operations/voting_power.rs +++ b/light-client-verifier/src/operations/voting_power.rs @@ -164,7 +164,7 @@ impl Default for ProvidedVotingPowerCalculator { } } -/// A signed non-nill vote. +/// A signed non-nil vote. struct NonAbsentCommitVote { signed_vote: SignedVote, /// Flag indicating whether the signature has already been verified. From e440bd9faf8c6968c53138d63c9a4525755b944f Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Wed, 17 Apr 2024 12:47:32 +0200 Subject: [PATCH 04/11] Update light-client-verifier/src/operations/voting_power.rs Co-authored-by: Romain Ruetschi --- light-client-verifier/src/operations/voting_power.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/light-client-verifier/src/operations/voting_power.rs b/light-client-verifier/src/operations/voting_power.rs index c6cea3bc7..eff599d91 100644 --- a/light-client-verifier/src/operations/voting_power.rs +++ b/light-client-verifier/src/operations/voting_power.rs @@ -174,7 +174,7 @@ struct NonAbsentCommitVote { impl NonAbsentCommitVote { /// Returns a signed non-nil vote for given commit. /// - /// If the CimmtSig represents a missing vote or a vote for nil returns + /// If the CommitSig represents a missing vote or a vote for nil returns /// `None`. Otherwise, if the vote is missing a signature returns /// `Some(Err)`. Otherwise, returns a `SignedVote` corresponding to given /// `CommitSig`. From a2bbe838558cae991525e517c9c30ac832ba2b17 Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Wed, 17 Apr 2024 13:03:57 +0200 Subject: [PATCH 05/11] comment --- light-client-verifier/src/operations/voting_power.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/light-client-verifier/src/operations/voting_power.rs b/light-client-verifier/src/operations/voting_power.rs index eff599d91..cb369e907 100644 --- a/light-client-verifier/src/operations/voting_power.rs +++ b/light-client-verifier/src/operations/voting_power.rs @@ -236,6 +236,8 @@ impl NonAbsentCommitVotes { .iter() .enumerate() .flat_map(|(idx, signature)| { + // We never have more than 2³¹ signatures so this always + // succeeds. let idx = ValidatorIndex::try_from(idx).unwrap(); NonAbsentCommitVote::new( signature, From 92ff74654dcd63f159a0cf70c957037c260cfe0c Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Wed, 17 Apr 2024 16:29:56 +0200 Subject: [PATCH 06/11] remove vote_power_in and vote_power_in_sets methods --- .../src/operations/voting_power.rs | 120 +++++------------- 1 file changed, 30 insertions(+), 90 deletions(-) diff --git a/light-client-verifier/src/operations/voting_power.rs b/light-client-verifier/src/operations/voting_power.rs index cb369e907..0af19550d 100644 --- a/light-client-verifier/src/operations/voting_power.rs +++ b/light-client-verifier/src/operations/voting_power.rs @@ -32,9 +32,9 @@ pub struct VotingPowerTally { } impl VotingPowerTally { - fn new(total: u64, trust_threshold: TrustThreshold) -> Self { + fn new(validators: &[validator::Info], trust_threshold: TrustThreshold) -> Self { Self { - total, + total: validators.iter().map(|info| info.power.value()).sum(), tallied: 0, trust_threshold, } @@ -73,14 +73,6 @@ impl fmt::Display for VotingPowerTally { /// /// This trait provides default implementation of some helper functions. pub trait VotingPowerCalculator: Send + Sync { - /// Compute the total voting power in a validator set - fn total_power_of(&self, validator_set: &ValidatorSet) -> u64 { - validator_set - .validators() - .iter() - .fold(0u64, |total, val_info| total + val_info.power.value()) - } - /// Check a) against the given threshold that there is enough trust between /// an untrusted header and a trusted validator set and b) if there is 2/3rd /// overlap between an untrusted header and untrusted validator set. @@ -90,59 +82,14 @@ pub trait VotingPowerCalculator: Send + Sync { trusted_validators: &ValidatorSet, trust_threshold: TrustThreshold, untrusted_validators: &ValidatorSet, - ) -> Result<(), VerificationError> { - let (trusted_power, untrusted_power) = self.voting_power_in_sets( - untrusted_header, - (trusted_validators, trust_threshold), - (untrusted_validators, TrustThreshold::TWO_THIRDS), - )?; - trusted_power - .check() - .map_err(VerificationError::not_enough_trust)?; - untrusted_power - .check() - .map_err(VerificationError::insufficient_signers_overlap)?; - Ok(()) - } + ) -> Result<(), VerificationError>; /// Check if there is 2/3rd overlap between an untrusted header and untrusted validator set fn check_signers_overlap( &self, untrusted_header: &SignedHeader, untrusted_validators: &ValidatorSet, - ) -> Result<(), VerificationError> { - let trust_threshold = TrustThreshold::TWO_THIRDS; - self.voting_power_in(untrusted_header, untrusted_validators, trust_threshold)? - .check() - .map_err(VerificationError::insufficient_signers_overlap) - } - - /// Compute the voting power in a header and its commit against a validator - /// set. - /// - /// Note that the returned tally may be lower than actual tally so long as - /// it meets the `trust_threshold`. - /// - /// If you have two separate sets of validators and need to check voting - /// power for both of them, prefer [`Self::voting_power_in_sets`] method. - fn voting_power_in( - &self, - signed_header: &SignedHeader, - validator_set: &ValidatorSet, - trust_threshold: TrustThreshold, - ) -> Result; - - /// Compute the voting power in a header and its commit against two separate - /// validator sets. - /// - /// This is equivalent to calling [`Self::voting_power_in`] on each set - /// separately but may be more optimised. - fn voting_power_in_sets( - &self, - signed_header: &SignedHeader, - first_set: (&ValidatorSet, TrustThreshold), - second_set: (&ValidatorSet, TrustThreshold), - ) -> Result<(VotingPowerTally, VotingPowerTally), VerificationError>; + ) -> Result<(), VerificationError>; } /// Default implementation of a `VotingPowerCalculator`, parameterized with @@ -304,51 +251,42 @@ pub type ProdVotingPowerCalculator = ProvidedVotingPowerCalculator; impl VotingPowerCalculator for ProvidedVotingPowerCalculator { - fn voting_power_in( + fn check_enough_trust_and_signers( &self, - signed_header: &SignedHeader, - validator_set: &ValidatorSet, + untrusted_header: &SignedHeader, + trusted_validators: &ValidatorSet, trust_threshold: TrustThreshold, - ) -> Result { - let mut votes = NonAbsentCommitVotes::new(signed_header)?; - voting_power_in_impl::( - &mut votes, - validator_set, - trust_threshold, - self.total_power_of(validator_set), - ) + untrusted_validators: &ValidatorSet, + ) -> Result<(), VerificationError> { + let mut votes = NonAbsentCommitVotes::new(untrusted_header)?; + voting_power_in::(&mut votes, trusted_validators, trust_threshold)? + .check() + .map_err(VerificationError::not_enough_trust)?; + voting_power_in::(&mut votes, untrusted_validators, TrustThreshold::TWO_THIRDS)? + .check() + .map_err(VerificationError::insufficient_signers_overlap)?; + Ok(()) } - fn voting_power_in_sets( + /// Check if there is 2/3rd overlap between an untrusted header and untrusted validator set + fn check_signers_overlap( &self, - signed_header: &SignedHeader, - first_set: (&ValidatorSet, TrustThreshold), - second_set: (&ValidatorSet, TrustThreshold), - ) -> Result<(VotingPowerTally, VotingPowerTally), VerificationError> { - let mut votes = NonAbsentCommitVotes::new(signed_header)?; - let first_tally = voting_power_in_impl::( - &mut votes, - first_set.0, - first_set.1, - self.total_power_of(first_set.0), - )?; - let second_tally = voting_power_in_impl::( - &mut votes, - second_set.0, - second_set.1, - self.total_power_of(second_set.0), - )?; - Ok((first_tally, second_tally)) + untrusted_header: &SignedHeader, + untrusted_validators: &ValidatorSet, + ) -> Result<(), VerificationError> { + let mut votes = NonAbsentCommitVotes::new(untrusted_header)?; + voting_power_in::(&mut votes, untrusted_validators, TrustThreshold::TWO_THIRDS)? + .check() + .map_err(VerificationError::insufficient_signers_overlap) } } -fn voting_power_in_impl( +fn voting_power_in( votes: &mut NonAbsentCommitVotes, validator_set: &ValidatorSet, trust_threshold: TrustThreshold, - total_voting_power: u64, ) -> Result { - let mut power = VotingPowerTally::new(total_voting_power, trust_threshold); + let mut power = VotingPowerTally::new(validator_set.validators(), trust_threshold); for validator in validator_set.validators() { if votes.has_voted::(validator)? { power.tally(validator.power()); @@ -361,6 +299,7 @@ fn voting_power_in_impl( Ok(power) } +/* // The below unit tests replaces the static voting power test files // see https://github.com/informalsystems/tendermint-rs/pull/383 // This is essentially to remove the heavy dependency on MBT @@ -520,3 +459,4 @@ mod tests { assert_eq!(result_ok.unwrap(), EXPECTED_RESULT); } } +*/ From f7a6026e41565c92531fc6680ba39432ba060377 Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Thu, 18 Apr 2024 10:30:53 +0200 Subject: [PATCH 07/11] Revert "remove vote_power_in and vote_power_in_sets methods" This reverts commit 92ff74654dcd63f159a0cf70c957037c260cfe0c. --- .../src/operations/voting_power.rs | 120 +++++++++++++----- 1 file changed, 90 insertions(+), 30 deletions(-) diff --git a/light-client-verifier/src/operations/voting_power.rs b/light-client-verifier/src/operations/voting_power.rs index 0af19550d..cb369e907 100644 --- a/light-client-verifier/src/operations/voting_power.rs +++ b/light-client-verifier/src/operations/voting_power.rs @@ -32,9 +32,9 @@ pub struct VotingPowerTally { } impl VotingPowerTally { - fn new(validators: &[validator::Info], trust_threshold: TrustThreshold) -> Self { + fn new(total: u64, trust_threshold: TrustThreshold) -> Self { Self { - total: validators.iter().map(|info| info.power.value()).sum(), + total, tallied: 0, trust_threshold, } @@ -73,6 +73,14 @@ impl fmt::Display for VotingPowerTally { /// /// This trait provides default implementation of some helper functions. pub trait VotingPowerCalculator: Send + Sync { + /// Compute the total voting power in a validator set + fn total_power_of(&self, validator_set: &ValidatorSet) -> u64 { + validator_set + .validators() + .iter() + .fold(0u64, |total, val_info| total + val_info.power.value()) + } + /// Check a) against the given threshold that there is enough trust between /// an untrusted header and a trusted validator set and b) if there is 2/3rd /// overlap between an untrusted header and untrusted validator set. @@ -82,14 +90,59 @@ pub trait VotingPowerCalculator: Send + Sync { trusted_validators: &ValidatorSet, trust_threshold: TrustThreshold, untrusted_validators: &ValidatorSet, - ) -> Result<(), VerificationError>; + ) -> Result<(), VerificationError> { + let (trusted_power, untrusted_power) = self.voting_power_in_sets( + untrusted_header, + (trusted_validators, trust_threshold), + (untrusted_validators, TrustThreshold::TWO_THIRDS), + )?; + trusted_power + .check() + .map_err(VerificationError::not_enough_trust)?; + untrusted_power + .check() + .map_err(VerificationError::insufficient_signers_overlap)?; + Ok(()) + } /// Check if there is 2/3rd overlap between an untrusted header and untrusted validator set fn check_signers_overlap( &self, untrusted_header: &SignedHeader, untrusted_validators: &ValidatorSet, - ) -> Result<(), VerificationError>; + ) -> Result<(), VerificationError> { + let trust_threshold = TrustThreshold::TWO_THIRDS; + self.voting_power_in(untrusted_header, untrusted_validators, trust_threshold)? + .check() + .map_err(VerificationError::insufficient_signers_overlap) + } + + /// Compute the voting power in a header and its commit against a validator + /// set. + /// + /// Note that the returned tally may be lower than actual tally so long as + /// it meets the `trust_threshold`. + /// + /// If you have two separate sets of validators and need to check voting + /// power for both of them, prefer [`Self::voting_power_in_sets`] method. + fn voting_power_in( + &self, + signed_header: &SignedHeader, + validator_set: &ValidatorSet, + trust_threshold: TrustThreshold, + ) -> Result; + + /// Compute the voting power in a header and its commit against two separate + /// validator sets. + /// + /// This is equivalent to calling [`Self::voting_power_in`] on each set + /// separately but may be more optimised. + fn voting_power_in_sets( + &self, + signed_header: &SignedHeader, + first_set: (&ValidatorSet, TrustThreshold), + second_set: (&ValidatorSet, TrustThreshold), + ) -> Result<(VotingPowerTally, VotingPowerTally), VerificationError>; } /// Default implementation of a `VotingPowerCalculator`, parameterized with @@ -251,42 +304,51 @@ pub type ProdVotingPowerCalculator = ProvidedVotingPowerCalculator; impl VotingPowerCalculator for ProvidedVotingPowerCalculator { - fn check_enough_trust_and_signers( + fn voting_power_in( &self, - untrusted_header: &SignedHeader, - trusted_validators: &ValidatorSet, + signed_header: &SignedHeader, + validator_set: &ValidatorSet, trust_threshold: TrustThreshold, - untrusted_validators: &ValidatorSet, - ) -> Result<(), VerificationError> { - let mut votes = NonAbsentCommitVotes::new(untrusted_header)?; - voting_power_in::(&mut votes, trusted_validators, trust_threshold)? - .check() - .map_err(VerificationError::not_enough_trust)?; - voting_power_in::(&mut votes, untrusted_validators, TrustThreshold::TWO_THIRDS)? - .check() - .map_err(VerificationError::insufficient_signers_overlap)?; - Ok(()) + ) -> Result { + let mut votes = NonAbsentCommitVotes::new(signed_header)?; + voting_power_in_impl::( + &mut votes, + validator_set, + trust_threshold, + self.total_power_of(validator_set), + ) } - /// Check if there is 2/3rd overlap between an untrusted header and untrusted validator set - fn check_signers_overlap( + fn voting_power_in_sets( &self, - untrusted_header: &SignedHeader, - untrusted_validators: &ValidatorSet, - ) -> Result<(), VerificationError> { - let mut votes = NonAbsentCommitVotes::new(untrusted_header)?; - voting_power_in::(&mut votes, untrusted_validators, TrustThreshold::TWO_THIRDS)? - .check() - .map_err(VerificationError::insufficient_signers_overlap) + signed_header: &SignedHeader, + first_set: (&ValidatorSet, TrustThreshold), + second_set: (&ValidatorSet, TrustThreshold), + ) -> Result<(VotingPowerTally, VotingPowerTally), VerificationError> { + let mut votes = NonAbsentCommitVotes::new(signed_header)?; + let first_tally = voting_power_in_impl::( + &mut votes, + first_set.0, + first_set.1, + self.total_power_of(first_set.0), + )?; + let second_tally = voting_power_in_impl::( + &mut votes, + second_set.0, + second_set.1, + self.total_power_of(second_set.0), + )?; + Ok((first_tally, second_tally)) } } -fn voting_power_in( +fn voting_power_in_impl( votes: &mut NonAbsentCommitVotes, validator_set: &ValidatorSet, trust_threshold: TrustThreshold, + total_voting_power: u64, ) -> Result { - let mut power = VotingPowerTally::new(validator_set.validators(), trust_threshold); + let mut power = VotingPowerTally::new(total_voting_power, trust_threshold); for validator in validator_set.validators() { if votes.has_voted::(validator)? { power.tally(validator.power()); @@ -299,7 +361,6 @@ fn voting_power_in( Ok(power) } -/* // The below unit tests replaces the static voting power test files // see https://github.com/informalsystems/tendermint-rs/pull/383 // This is essentially to remove the heavy dependency on MBT @@ -459,4 +520,3 @@ mod tests { assert_eq!(result_ok.unwrap(), EXPECTED_RESULT); } } -*/ From d750e1ae91e2bf180e6f73e84a1449a43bf7127c Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Thu, 18 Apr 2024 10:35:23 +0200 Subject: [PATCH 08/11] reduce diff --- light-client-verifier/src/predicates.rs | 26 ++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/light-client-verifier/src/predicates.rs b/light-client-verifier/src/predicates.rs index b0f96c206..00eb1d0b2 100644 --- a/light-client-verifier/src/predicates.rs +++ b/light-client-verifier/src/predicates.rs @@ -184,18 +184,6 @@ pub trait VerificationPredicates: Send + Sync { } } - /// Check that there is enough signers overlap between the given, untrusted - /// validator set and the untrusted signed header. - fn has_sufficient_signers_overlap( - &self, - untrusted_sh: &SignedHeader, - untrusted_validators: &ValidatorSet, - calculator: &dyn VotingPowerCalculator, - ) -> Result<(), VerificationError> { - calculator.check_signers_overlap(untrusted_sh, untrusted_validators)?; - Ok(()) - } - /// Check that there is enough a) validators overlap between the trusted /// validator set and the untrusted signed header and b) signers overlap /// between the given, untrusted validator set and the untrusted signed @@ -217,6 +205,18 @@ pub trait VerificationPredicates: Send + Sync { Ok(()) } + /// Check that there is enough signers overlap between the given, untrusted + /// validator set and the untrusted signed header. + fn has_sufficient_signers_overlap( + &self, + untrusted_sh: &SignedHeader, + untrusted_validators: &ValidatorSet, + calculator: &dyn VotingPowerCalculator, + ) -> Result<(), VerificationError> { + calculator.check_signers_overlap(untrusted_sh, untrusted_validators)?; + Ok(()) + } + /// Check that the hash of the next validator set in the trusted block matches /// the hash of the validator set in the untrusted one. fn valid_next_validator_set( @@ -638,7 +638,7 @@ mod tests { let signed_header = light_block.signed_header; let vp = ProdPredicates; - let mut trust_threshold = TrustThreshold::new(1, 3).expect("Cannot make trust threshold"); + let mut trust_threshold = TrustThreshold::ONE_THIRD; let voting_power_calculator = ProdVotingPowerCalculator::default(); // Test scenarios --> From bb45038f89c461011ba96aa215173b874a4b30f3 Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Thu, 18 Apr 2024 10:53:04 +0200 Subject: [PATCH 09/11] test --- light-client-verifier/src/predicates.rs | 62 ++++++++++++++----------- light-client-verifier/src/verifier.rs | 2 +- 2 files changed, 36 insertions(+), 28 deletions(-) diff --git a/light-client-verifier/src/predicates.rs b/light-client-verifier/src/predicates.rs index 00eb1d0b2..b8a88ac4e 100644 --- a/light-client-verifier/src/predicates.rs +++ b/light-client-verifier/src/predicates.rs @@ -188,7 +188,7 @@ pub trait VerificationPredicates: Send + Sync { /// validator set and the untrusted signed header and b) signers overlap /// between the given, untrusted validator set and the untrusted signed /// header. - fn has_sufficient_signers_and_validators_overlap( + fn has_sufficient_validators_and_signers_overlap( &self, untrusted_sh: &SignedHeader, trusted_validators: &ValidatorSet, @@ -629,30 +629,27 @@ mod tests { } } - // TODO: Fix the test. DO NOT SUBMIT - /* #[test] - fn test_has_sufficient_validators_overlap() { + fn test_has_sufficient_validators_and_signers_overlap() { let light_block: LightBlock = TestgenLightBlock::new_default(1).generate().unwrap().into(); let val_set = light_block.validators; let signed_header = light_block.signed_header; let vp = ProdPredicates; - let mut trust_threshold = TrustThreshold::ONE_THIRD; let voting_power_calculator = ProdVotingPowerCalculator::default(); // Test scenarios --> - // 1. > trust_threshold validators overlap - let result_ok = vp.has_sufficient_validators_overlap( + // 1. Validators and signers overlap ≥ trust_threshold. + vp.has_sufficient_validators_and_signers_overlap( &signed_header, &val_set, - &trust_threshold, + &TrustThreshold::TWO_THIRDS, + &val_set, &voting_power_calculator, - ); - - assert!(result_ok.is_ok()); + ) + .unwrap(); - // 2. < trust_threshold validators overlap + // 2. Validators overlap < threshold; signers overlap ≥ threshold. let mut vals = val_set.validators().clone(); vals.push( Validator::new("extra-val") @@ -662,30 +659,41 @@ mod tests { ); let bad_valset = Set::without_proposer(vals); - trust_threshold = TrustThreshold::new(2, 3).expect("Cannot make trust threshold"); - - let result_err = vp.has_sufficient_validators_overlap( + let result = vp.has_sufficient_validators_and_signers_overlap( &signed_header, &bad_valset, - &trust_threshold, + &TrustThreshold::TWO_THIRDS, + &val_set, &voting_power_calculator, ); - match result_err { + let expected_tally = VotingPowerTally { + total: 200, + tallied: 100, + trust_threshold: TrustThreshold::TWO_THIRDS, + }; + match result { Err(VerificationError(VerificationErrorDetail::NotEnoughTrust(e), _)) => { - assert_eq!( - e.tally, - VotingPowerTally { - total: 200, - tallied: 100, - trust_threshold, - } - ); + assert_eq!(expected_tally, e.tally) + }, + _ => panic!("expected NotEnoughTrust error, got: {result:?}"), + } + + // 3. Validators overlap ≥ threshold; signers overlap < threshold. + let result = vp.has_sufficient_validators_and_signers_overlap( + &signed_header, + &val_set, + &TrustThreshold::TWO_THIRDS, + &bad_valset, + &voting_power_calculator, + ); + match result { + Err(VerificationError(VerificationErrorDetail::InsufficientSignersOverlap(e), _)) => { + assert_eq!(expected_tally, e.tally) }, - _ => panic!("expected NotEnoughTrust error"), + _ => panic!("expected InsufficientSignersOverlap error, got: {result:?}"), } } - */ #[test] fn test_has_sufficient_signers_overlap() { diff --git a/light-client-verifier/src/verifier.rs b/light-client-verifier/src/verifier.rs index 1cee4a229..caec451ef 100644 --- a/light-client-verifier/src/verifier.rs +++ b/light-client-verifier/src/verifier.rs @@ -226,7 +226,7 @@ where let result = if need_both { self.predicates - .has_sufficient_signers_and_validators_overlap( + .has_sufficient_validators_and_signers_overlap( untrusted.signed_header, trusted.next_validators, &options.trust_threshold, From 4bd8f0affdf0a4fe5a8927121b01fc8ad11d56cd Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Thu, 18 Apr 2024 11:00:24 +0200 Subject: [PATCH 10/11] changelog --- ...-avoid-chucking-signature-multiple-times.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 .changelog/unreleased/breaking-changes/1410-avoid-chucking-signature-multiple-times.md diff --git a/.changelog/unreleased/breaking-changes/1410-avoid-chucking-signature-multiple-times.md b/.changelog/unreleased/breaking-changes/1410-avoid-chucking-signature-multiple-times.md new file mode 100644 index 000000000..026b1209e --- /dev/null +++ b/.changelog/unreleased/breaking-changes/1410-avoid-chucking-signature-multiple-times.md @@ -0,0 +1,18 @@ +- `[light-client-verifier]` Rework VerificationPredicates and VotingPowerCalculator + by introducing methods which check validators and signers overlap at once. + The motivation of this is to avoid checking the same signature multiple + times. + + Consider a validator is in old and new set. Previously their signature would + be verified twice. Once by call to `has_sufficient_validators_overlap` + method and second time by call to `has_sufficient_signers_overlap` method. + + With the new interface, `has_sufficient_validators_and_signers_overlap` is + called and it can be implemented to remember which signatures have been + verified. + + As a side effect of those changes, signatures are now verified in the order + of validator’s power which may further reduce number of signatures which + need to be verified. + + ([\#1410](https://github.com/informalsystems/tendermint-rs/pull/1410)) From 079418fa527627da2ec54efcf5a87c4d301276e5 Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Thu, 18 Apr 2024 12:17:37 +0200 Subject: [PATCH 11/11] docs --- .../src/operations/voting_power.rs | 48 +++++++++++++++++-- light-client-verifier/src/predicates.rs | 22 +++++++-- 2 files changed, 61 insertions(+), 9 deletions(-) diff --git a/light-client-verifier/src/operations/voting_power.rs b/light-client-verifier/src/operations/voting_power.rs index cb369e907..1f7e9f0ed 100644 --- a/light-client-verifier/src/operations/voting_power.rs +++ b/light-client-verifier/src/operations/voting_power.rs @@ -81,9 +81,19 @@ pub trait VotingPowerCalculator: Send + Sync { .fold(0u64, |total, val_info| total + val_info.power.value()) } - /// Check a) against the given threshold that there is enough trust between - /// an untrusted header and a trusted validator set and b) if there is 2/3rd - /// overlap between an untrusted header and untrusted validator set. + /// Check that there is enough trust between an untrusted header and given + /// trusted and untrusted validator sets. + /// + /// First of all, checks that enough validators from the + /// `trusted_validators` set signed the `untrusted_header` to reach given + /// `trust_threshold`. + /// + /// Second of all, checks that enough validators from the + /// `untrusted_validators` set signed the `untrusted_header` to reach + /// a trust threshold of ⅔. + /// + /// If both of those conditions aren’t met, it’s unspecified which error is + /// returned. fn check_enough_trust_and_signers( &self, untrusted_header: &SignedHeader, @@ -121,7 +131,10 @@ pub trait VotingPowerCalculator: Send + Sync { /// set. /// /// Note that the returned tally may be lower than actual tally so long as - /// it meets the `trust_threshold`. + /// it meets the `trust_threshold`. Furthermore, the method isn’t + /// guaranteed to verify all the signatures present in the signed header. + /// If there are invalid signatures, the method may or may not return an + /// error depending on which validators those signatures correspond to. /// /// If you have two separate sets of validators and need to check voting /// power for both of them, prefer [`Self::voting_power_in_sets`] method. @@ -136,7 +149,32 @@ pub trait VotingPowerCalculator: Send + Sync { /// validator sets. /// /// This is equivalent to calling [`Self::voting_power_in`] on each set - /// separately but may be more optimised. + /// separately but may be more optimised. Implementators are encouraged to + /// write a properly optimised method which avoids checking the same + /// signature twice but for a simple unoptimised implementation the + /// following works: + /// + /// ```ignore + /// fn voting_power_in_sets( + /// &self, + /// signed_header: &SignedHeader, + /// first_set: (&ValidatorSet, TrustThreshold), + /// second_set: (&ValidatorSet, TrustThreshold), + /// ) -> Result<(VotingPowerTally, VotingPowerTally), VerificationError> { + /// let first_tally = self.voting_power_in( + /// signed_header, + /// first_set.0, + /// first_set.1, + /// )?; + /// let second_tally = self.voting_power_in( + /// signed_header, + /// first_set.0, + /// first_set.1, + /// )?; + /// Ok((first_tally, second_tally)) + /// } + /// + /// ``` fn voting_power_in_sets( &self, signed_header: &SignedHeader, diff --git a/light-client-verifier/src/predicates.rs b/light-client-verifier/src/predicates.rs index b8a88ac4e..b7bbc4011 100644 --- a/light-client-verifier/src/predicates.rs +++ b/light-client-verifier/src/predicates.rs @@ -184,10 +184,24 @@ pub trait VerificationPredicates: Send + Sync { } } - /// Check that there is enough a) validators overlap between the trusted - /// validator set and the untrusted signed header and b) signers overlap - /// between the given, untrusted validator set and the untrusted signed - /// header. + /// Checks that there is enough overlap between validators and the untrusted + /// signed header. + /// + /// First of all, checks that enough validators from the + /// `trusted_validators` set signed the untrusted header to reach given + /// `trust_threshold`. + /// + /// Second of all, checks that enough validators from the + /// `untrusted_validators` set signed the untrusted header to reach a trust + /// threshold of ⅔. + /// + /// If both of those conditions aren’t met, it’s unspecified which error is + /// returned. + /// + /// Note also that the method isn’t guaranteed to verify all the signatures + /// present in the signed header. If there are invalid signatures, the + /// method may or may not return an error depending on which validators + /// those signatures correspond to. fn has_sufficient_validators_and_signers_overlap( &self, untrusted_sh: &SignedHeader,