Fix VotingPowerCalculator::voting_power_in.

- Provide two helper methods for checking validators overlap and if
  there is enough trust w.r.t. the trusted state.
- Only count `Commit` signatures towards the voting power.
- Do not rely on `signed_votes` method to get the votes.

light-client/src/operations/voting_power.rs

@@ -1,19 +1,77 @@
 use crate::{
-    bail,
+    bail, ensure,
     predicates::errors::VerificationError,
-    types::{SignedHeader, ValidatorSet},
+    types::{Commit, SignedHeader, TrustThreshold, ValidatorSet},
 };
 
-use anomaly::BoxError;
+use serde::{Deserialize, Serialize};
+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 VotingPower {
+    pub total: u64,
+    pub tallied: u64,
+    pub trust_threshold: TrustThreshold,
+}
+
+impl fmt::Display for VotingPower {
+    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 voting_power_in(
+
+    fn check_enough_trust(
+        &self,
+        untrusted_header: &SignedHeader,
+        untrusted_validators: &ValidatorSet,
+        trust_threshold: TrustThreshold,
+    ) -> Result<VotingPower, VerificationError> {
+        println!("check_validators_overlap");
+        let voting_power =
+            self.voting_power_of(untrusted_header, untrusted_validators, trust_threshold)?;
+
+        if trust_threshold.is_enough_power(voting_power.tallied, voting_power.total) {
+            Ok(voting_power)
+        } else {
+            Err(VerificationError::NotEnoughTrust(voting_power))
+        }
+    }
+
+    fn check_signers_overlap(
+        &self,
+        untrusted_header: &SignedHeader,
+        untrusted_validators: &ValidatorSet,
+    ) -> Result<VotingPower, VerificationError> {
+        println!("check_signers_overlap");
+        let two_thirds = TrustThreshold::new(2, 3).unwrap();
+        let voting_power =
+            self.voting_power_of(untrusted_header, untrusted_validators, two_thirds)?;
+
+        if two_thirds.is_enough_power(voting_power.tallied, voting_power.total) {
+            Ok(voting_power)
+        } else {
+            Err(VerificationError::InsufficientSignersOverlap(voting_power))
+        }
+    }
+
+    fn voting_power_of(
         &self,
         signed_header: &SignedHeader,
-        validators: &ValidatorSet,
-    ) -> Result<u64, BoxError>;
+        validator_set: &ValidatorSet,
+        trust_threshold: TrustThreshold,
+    ) -> Result<VotingPower, VerificationError>;
 }
 
 #[derive(Copy, Clone, Debug)]
@@ -24,39 +82,110 @@ impl VotingPowerCalculator for ProdVotingPowerCalculator {
         validators.total_power()
     }
 
-    fn voting_power_in(
+    fn voting_power_of(
         &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<VotingPower, VerificationError> {
+        let signatures = &signed_header.commit.signatures;
+
+        let mut tallied_voting_power = 0_u64;
+        for (idx, signature) in signatures.into_iter().enumerate() {
+            let vote = vote_from_non_absent_signature(signature, idx as u64, &signed_header.commit);
+            let vote = match vote {
+                Some(vote) => vote,
+                None => continue, // Ok, some signatures can be absent
+            };
+
+            // TODO: Check that we didn't see a vote from this validator twice ...
+            let validator = match validator_set.validator(vote.validator_address) {
+                Some(validator) => validator,
+                None => {
+                    // println!(
+                    //     "  > couldn't find validator with address {}",
+                    //     vote.validator_address,
+                    // );
+
+                    continue;
+                }
             };
 
-            // 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,
-                )));
+                });
             }
 
-            signed_power += val.power();
+            // 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.
+            }
+
+            // TODO: Break out when we have enough voting power
         }
 
-        Ok(signed_power)
+        let voting_power = VotingPower {
+            total: self.total_power_of(validator_set),
+            tallied: tallied_voting_power,
+            trust_threshold,
+        };
+
+        Ok(voting_power)
     }
 }
+
+fn vote_from_non_absent_signature(
+    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.clone(),
+            timestamp.clone(),
+            signature.clone(),
+            Some(commit.block_id.clone()),
+        ),
+        CommitSig::BlockIDFlagNil {
+            validator_address,
+            timestamp,
+            signature,
+        } => (
+            validator_address.clone(),
+            timestamp.clone(),
+            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::VotingPower;
+use crate::types::{Hash, Height, Time, Validator};
 
 /// The various errors which can be raised by the verifier component,
 /// when validating or verifying a light block.
@@ -15,20 +16,26 @@ pub enum VerificationError {
     #[error("implementation specific: {0}")]
     ImplementationSpecific(String),
 
+    #[error("not enough trust because insufficient validators overlap: {0}")]
+    NotEnoughTrust(VotingPower),
+
+    #[error("insufficient signers overlap: {0}")]
+    InsufficientSignersOverlap(VotingPower),
+
     #[error(
-        "insufficient validators overlap: total_power={total_power} signed_power={signed_power} trust_threshold={trust_threshold}"
+        "validators and signatures count do no match: {validators_count} != {signatures_count}"
     )]
-    InsufficientValidatorsOverlap {
-        total_power: u64,
-        signed_power: u64,
-        trust_threshold: TrustThreshold,
+    ValidatorsAndSignaturesCountMismatch {
+        validators_count: usize,
+        signatures_count: usize,
     },
 
-    #[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("Couldn't verify signature `{signature:?}` with validator `{validator:?}` on sign_bytes `{sign_bytes:?}`")]
+    InvalidSignature {
+        signature: Vec<u8>,
+        validator: Validator,
+        sign_bytes: Vec<u8>,
+    },
 
     #[error("invalid commit: {0}")]
     InvalidCommit(String),
@@ -76,7 +83,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

light-client/src/types.rs

@@ -9,6 +9,7 @@ use tendermint::{
         Commit as TMCommit,
     },
     lite::TrustThresholdFraction,
+    validator::Info as TMValidatorInfo,
     validator::Set as TMValidatorSet,
 };
 
@@ -30,6 +31,9 @@ pub type Header = TMHeader;
 /// Set of validators
 pub type ValidatorSet = TMValidatorSet;
 
+/// Info about a single validator
+pub type Validator = TMValidatorInfo;
+
 /// A commit contains the justification (ie. a set of signatures)
 /// that a block was consensus, as committed by a set previous block of validators.
 pub type Commit = TMCommit;

light-client/tests/light_client.rs

@@ -104,7 +104,11 @@ fn run_test_case(tc: TestCase<LightBlock>) {
 
                 latest_trusted = Trusted::new(new_state.signed_header, new_state.next_validators);
             }
-            Err(_) => {
+            Err(e) => {
+                dbg!(e);
+                // if !expects_err {
+                //     dbg!(e);
+                // }
                 assert!(expects_err);
             }
         }
@@ -240,9 +244,10 @@ fn run_bisection_test(tc: TestBisection<LightBlock>) {
             assert!(!expects_err);
         }
         Err(e) => {
-            if !expects_err {
-                dbg!(e);
-            }
+            dbg!(e);
+            // if !expects_err {
+            //     dbg!(e);
+            // }
             assert!(expects_err);
         }
     }