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)) diff --git a/light-client-verifier/src/operations/voting_power.rs b/light-client-verifier/src/operations/voting_power.rs index e0f98e26c..1f7e9f0ed 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, @@ -30,6 +31,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!( @@ -52,22 +81,38 @@ 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 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, trusted_validators: &ValidatorSet, trust_threshold: TrustThreshold, + untrusted_validators: &ValidatorSet, ) -> 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)) - } + 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 @@ -77,28 +122,65 @@ 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. + /// 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`. 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. /// - /// 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. 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, + first_set: (&ValidatorSet, TrustThreshold), + second_set: (&ValidatorSet, TrustThreshold), + ) -> Result<(VotingPowerTally, VotingPowerTally), VerificationError>; } /// Default implementation of a `VotingPowerCalculator`, parameterized with @@ -120,55 +202,137 @@ 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-nil vote. +struct NonAbsentCommitVote { + signed_vote: SignedVote, + /// Flag indicating whether the signature has already been verified. + verified: bool, +} + +impl NonAbsentCommitVote { + /// Returns a signed non-nil vote for given commit. + /// + /// 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`. + 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() + } } -/// Error during validator lookup. -enum LookupError { - NotFound, - AlreadySeen, +/// Collection of non-absent commit votes. +struct NonAbsentCommitVotes { + /// Votes sorted by validator address. + votes: Vec, } -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 } +impl NonAbsentCommitVotes { + pub fn new(signed_header: &SignedHeader) -> Result { + let mut votes = signed_header + .commit + .signatures + .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, + 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) } } @@ -184,117 +348,55 @@ impl VotingPowerCalculator for ProvidedVotingPowerCalcul validator_set: &ValidatorSet, 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; - - // 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() { - tallied_voting_power += 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 trust_threshold.is_enough_power(tallied_voting_power, total_voting_power) { + if power.check().is_ok() { break; } } - - let voting_power = VotingPowerTally { - total: total_voting_power, - tallied: tallied_voting_power, - trust_threshold, - }; - - 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..b7bbc4011 100644 --- a/light-client-verifier/src/predicates.rs +++ b/light-client-verifier/src/predicates.rs @@ -184,21 +184,43 @@ 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( + /// 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, 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_enough_trust_and_signers( + untrusted_sh, + trusted_validators, + *trust_threshold, + untrusted_validators, + )?; Ok(()) } - /// Check that there is enough signers overlap between the given, untrusted validator set - /// and the untrusted signed header. + /// 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, @@ -622,27 +644,26 @@ mod tests { } #[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::new(1, 3).expect("Cannot make trust threshold"); 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, - ); + ) + .unwrap(); - assert!(result_ok.is_ok()); - - // 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") @@ -652,27 +673,39 @@ 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:?}"), } } diff --git a/light-client-verifier/src/verifier.rs b/light-client-verifier/src/verifier.rs index ba8e65146..caec451ef 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_validators_and_signers_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 } }