Fix VotingPowerCalculator::voting_power_in

light-client/src/callback.rs

@@ -20,7 +20,7 @@ impl<A> Callback<A> {
     }
 
     /// Call the underlying closure on `result`.
-    pub fn call(self, result: A) -> () {
+    pub fn call(self, result: A) {
         (self.inner)(result);
     }
 }

light-client/src/operations/voting_power.rs

@@ -1,19 +1,76 @@
 use crate::{
     bail,
     predicates::errors::VerificationError,
-    types::{SignedHeader, ValidatorSet},
+    types::{Commit, SignedHeader, TrustThreshold, ValidatorSet},
 };
 
-use anomaly::BoxError;
+use serde::{Deserialize, Serialize};
+use std::collections::HashSet;
+use std::fmt;
+
+use tendermint::block::CommitSig;
+use tendermint::lite::types::TrustThreshold as _;
 use tendermint::lite::types::ValidatorSet as _;
+use tendermint::vote::{SignedVote, Vote};
+
+#[derive(Copy, Clone, Debug, PartialEq, Serialize, Deserialize)]
+pub struct VotingPowerTally {
+    pub total: u64,
+    pub tallied: u64,
+    pub trust_threshold: TrustThreshold,
+}
+
+impl fmt::Display for VotingPowerTally {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(
+            f,
+            "VotingPower(total={} tallied={} trust_threshold={})",
+            self.total, self.tallied, self.trust_threshold
+        )
+    }
+}
 
 pub trait VotingPowerCalculator: Send {
     fn total_power_of(&self, validators: &ValidatorSet) -> u64;
+
+    fn check_enough_trust(
+        &self,
+        untrusted_header: &SignedHeader,
+        untrusted_validators: &ValidatorSet,
+        trust_threshold: TrustThreshold,
+    ) -> Result<(), VerificationError> {
+        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::NotEnoughTrust(voting_power))
+        }
+    }
+
+    fn check_signers_overlap(
+        &self,
+        untrusted_header: &SignedHeader,
+        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::InsufficientSignersOverlap(voting_power))
+        }
+    }
+
     fn voting_power_in(
         &self,
         signed_header: &SignedHeader,
-        validators: &ValidatorSet,
-    ) -> Result<u64, BoxError>;
+        validator_set: &ValidatorSet,
+        trust_threshold: TrustThreshold,
+    ) -> Result<VotingPowerTally, VerificationError>;
 }
 
 #[derive(Copy, Clone, Debug)]
@@ -27,36 +84,105 @@ impl VotingPowerCalculator for ProdVotingPowerCalculator {
     fn voting_power_in(
         &self,
         signed_header: &SignedHeader,
-        validators: &ValidatorSet,
-    ) -> Result<u64, BoxError> {
-        // NOTE: We don't know the validators that committed this block,
-        //       so we have to check for each vote if its validator is already known.
-        let mut signed_power = 0_u64;
-
-        for vote in &signed_header.signed_votes() {
-            // Only count if this vote is from a known validator.
-            // TODO: we still need to check that we didn't see a vote from this validator twice ...
-            let val_id = vote.validator_id();
-            let val = match validators.validator(val_id) {
-                Some(v) => v,
-                None => continue,
+        validator_set: &ValidatorSet,
+        trust_threshold: TrustThreshold,
+    ) -> Result<VotingPowerTally, VerificationError> {
+        let signatures = &signed_header.commit.signatures;
+
+        let mut tallied_voting_power = 0_u64;
+        let mut seen_validators = HashSet::new();
+
+        // Get non-absent votes from the signatures
+        let non_absent_votes = signatures.iter().enumerate().flat_map(|(idx, signature)| {
+            if let Some(vote) = non_absent_vote(signature, idx as u64, &signed_header.commit) {
+                Some((signature, vote))
+            } else {
+                None
+            }
+        });
+
+        for (signature, vote) in non_absent_votes {
+            // Ensure we only count a validator's power once
+            if seen_validators.contains(&vote.validator_address) {
+                bail!(VerificationError::DuplicateValidator(
+                    vote.validator_address
+                ));
+            } else {
+                seen_validators.insert(vote.validator_address);
+            }
+
+            let validator = match validator_set.validator(vote.validator_address) {
+                Some(validator) => validator,
+                None => continue, // Cannot find matching validator, so we skip the vote
             };
 
-            // check vote is valid from validator
-            let sign_bytes = vote.sign_bytes();
+            let signed_vote = SignedVote::new(
+                (&vote).into(),
+                signed_header.header.chain_id.as_str(),
+                vote.validator_address,
+                vote.signature,
+            );
 
-            if !val.verify_signature(&sign_bytes, vote.signature()) {
-                bail!(VerificationError::ImplementationSpecific(format!(
-                    "Couldn't verify signature {:?} with validator {:?} on sign_bytes {:?}",
-                    vote.signature(),
-                    val,
+            // Check vote is valid
+            let sign_bytes = signed_vote.sign_bytes();
+            if !validator.verify_signature(&sign_bytes, signed_vote.signature()) {
+                bail!(VerificationError::InvalidSignature {
+                    signature: signed_vote.signature().to_vec(),
+                    validator,
                     sign_bytes,
-                )));
+                });
+            }
+
+            // 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.
             }
 
-            signed_power += val.power();
+            // TODO: Break out of the loop when we have enough voting power.
+            // See https://github.com/informalsystems/tendermint-rs/issues/235
         }
 
-        Ok(signed_power)
+        let voting_power = VotingPowerTally {
+            total: self.total_power_of(validator_set),
+            tallied: tallied_voting_power,
+            trust_threshold,
+        };
+
+        Ok(voting_power)
     }
 }
+
+fn non_absent_vote(commit_sig: &CommitSig, validator_index: u64, commit: &Commit) -> Option<Vote> {
+    let (validator_address, timestamp, signature, block_id) = match commit_sig {
+        CommitSig::BlockIDFlagAbsent { .. } => return None,
+        CommitSig::BlockIDFlagCommit {
+            validator_address,
+            timestamp,
+            signature,
+        } => (
+            *validator_address,
+            *timestamp,
+            signature.clone(),
+            Some(commit.block_id.clone()),
+        ),
+        CommitSig::BlockIDFlagNil {
+            validator_address,
+            timestamp,
+            signature,
+        } => (*validator_address, *timestamp, signature.clone(), None),
+    };
+
+    Some(Vote {
+        vote_type: tendermint::vote::Type::Precommit,
+        height: commit.height,
+        round: commit.round,
+        block_id,
+        timestamp,
+        validator_address,
+        validator_index,
+        signature,
+    })
+}

light-client/src/predicates.rs

@@ -144,52 +144,14 @@ pub trait VerificationPredicates: Send {
         Ok(())
     }
 
-    fn has_sufficient_voting_power(
-        &self,
-        signed_header: &SignedHeader,
-        validators: &ValidatorSet,
-        trust_threshold: &TrustThreshold,
-        calculator: &dyn VotingPowerCalculator,
-    ) -> Result<(), VerificationError> {
-        // FIXME: Do not discard underlying error
-        let total_power = calculator.total_power_of(validators);
-        let voting_power = calculator
-            .voting_power_in(signed_header, validators)
-            .map_err(|e| VerificationError::ImplementationSpecific(e.to_string()))?;
-
-        ensure!(
-            voting_power * trust_threshold.denominator > total_power * trust_threshold.numerator,
-            VerificationError::InsufficientVotingPower {
-                total_power,
-                voting_power,
-            }
-        );
-
-        Ok(())
-    }
-
     fn has_sufficient_validators_overlap(
         &self,
         untrusted_sh: &SignedHeader,
         trusted_validators: &ValidatorSet,
         trust_threshold: &TrustThreshold,
         calculator: &dyn VotingPowerCalculator,
     ) -> Result<(), VerificationError> {
-        // FIXME: Do not discard underlying error
-        let total_power = calculator.total_power_of(trusted_validators);
-        let voting_power = calculator
-            .voting_power_in(untrusted_sh, trusted_validators)
-            .map_err(|e| VerificationError::ImplementationSpecific(e.to_string()))?;
-
-        ensure!(
-            voting_power * trust_threshold.denominator > total_power * trust_threshold.numerator,
-            VerificationError::InsufficientValidatorsOverlap {
-                total_power,
-                signed_power: voting_power,
-                trust_threshold: *trust_threshold,
-            }
-        );
-
+        calculator.check_enough_trust(untrusted_sh, trusted_validators, *trust_threshold)?;
         Ok(())
     }
 
@@ -199,20 +161,7 @@ pub trait VerificationPredicates: Send {
         untrusted_validators: &ValidatorSet,
         calculator: &dyn VotingPowerCalculator,
     ) -> Result<(), VerificationError> {
-        // FIXME: Do not discard underlying error
-        let total_power = calculator.total_power_of(untrusted_validators);
-        let signed_power = calculator
-            .voting_power_in(untrusted_sh, untrusted_validators)
-            .map_err(|e| VerificationError::ImplementationSpecific(e.to_string()))?;
-
-        ensure!(
-            signed_power * 3 > total_power * 2,
-            VerificationError::InsufficientCommitPower {
-                total_power,
-                signed_power,
-            }
-        );
-
+        calculator.check_signers_overlap(untrusted_sh, untrusted_validators)?;
         Ok(())
     }
 

light-client/src/predicates/errors.rs

@@ -3,7 +3,8 @@ use serde::{Deserialize, Serialize};
 use thiserror::Error;
 
 use crate::errors::ErrorExt;
-use crate::types::{Hash, Height, Time, TrustThreshold};
+use crate::operations::voting_power::VotingPowerTally;
+use crate::types::{Hash, Height, Time, Validator, ValidatorAddress};
 
 /// The various errors which can be raised by the verifier component,
 /// when validating or verifying a light block.
@@ -15,21 +16,29 @@ pub enum VerificationError {
     #[error("implementation specific: {0}")]
     ImplementationSpecific(String),
 
-    #[error(
-        "insufficient validators overlap: total_power={total_power} signed_power={signed_power} trust_threshold={trust_threshold}"
-    )]
-    InsufficientValidatorsOverlap {
-        total_power: u64,
-        signed_power: u64,
-        trust_threshold: TrustThreshold,
+    #[error("not enough trust because insufficient validators overlap: {0}")]
+    NotEnoughTrust(VotingPowerTally),
+
+    #[error("insufficient signers overlap: {0}")]
+    InsufficientSignersOverlap(VotingPowerTally),
+
+    // #[error(
+    //     "validators and signatures count do no match: {validators_count} != {signatures_count}"
+    // )]
+    // ValidatorsAndSignaturesCountMismatch {
+    //     validators_count: usize,
+    //     signatures_count: usize,
+    // },
+    #[error("duplicate validator with address {0}")]
+    DuplicateValidator(ValidatorAddress),
+
+    #[error("Couldn't verify signature `{signature:?}` with validator `{validator:?}` on sign_bytes `{sign_bytes:?}`")]
+    InvalidSignature {
+        signature: Vec<u8>,
+        validator: Validator,
+        sign_bytes: Vec<u8>,
     },
 
-    #[error("insufficient voting power: total_power={total_power} voting_power={voting_power}")]
-    InsufficientVotingPower { total_power: u64, voting_power: u64 },
-
-    #[error("invalid commit power: total_power={total_power} signed_power={signed_power}")]
-    InsufficientCommitPower { total_power: u64, signed_power: u64 },
-
     #[error("invalid commit: {0}")]
     InvalidCommit(String),
 
@@ -76,7 +85,7 @@ impl ErrorExt for VerificationError {
     /// Whether this error means that the light block
     /// cannot be trusted w.r.t. the latest trusted state.
     fn not_enough_trust(&self) -> bool {
-        if let Self::InsufficientValidatorsOverlap { .. } = self {
+        if let Self::NotEnoughTrust { .. } = self {
             true
         } else {
             false