commit followup (#199)

* rename iter -> signed_votes and add validation checks

 - add basic validation to validate method (depending on `BlockIDFlag`)
 - move check for unknown validator to validate method (for CommitSig)

tendermint/src/lite_impl/signed_header.rs

@@ -3,8 +3,9 @@
 use crate::lite::error::{Error, Kind};
 use crate::lite::ValidatorSet;
 use crate::validator::Set;
-use crate::{block, hash, lite, vote};
+use crate::{block, block::BlockIDFlag, hash, lite, vote};
 use anomaly::fail;
+use std::convert::TryFrom;
 
 impl lite::Commit for block::signed_header::SignedHeader {
     type ValidatorSet = Set;
@@ -16,13 +17,9 @@ impl lite::Commit for block::signed_header::SignedHeader {
         // 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 = 0u64;
-        for vote in &self.iter().unwrap() {
-            // skip absent and nil votes
-            // NOTE: do we want to check the validity of votes
-            // for nil ?
-            // TODO: clarify this!
-
-            // check if this vote is from a known validator
+        for vote in &self.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,
@@ -48,6 +45,11 @@ impl lite::Commit for block::signed_header::SignedHeader {
     }
 
     fn validate(&self, vals: &Self::ValidatorSet) -> Result<(), Error> {
+        // TODO: self.commit.block_id cannot be zero in the same way as in go
+        // clarify if this another encoding related issue
+        if self.commit.signatures.len() == 0 {
+            fail!(Kind::ImplementationSpecific, "no signatures for commit");
+        }
         if self.commit.signatures.len() != vals.validators().len() {
             fail!(
                 Kind::ImplementationSpecific,
@@ -58,69 +60,108 @@ impl lite::Commit for block::signed_header::SignedHeader {
         }
 
         for commit_sig in self.commit.signatures.iter() {
-            // returns FaultyFullNode error if it detects a signer that is not present in the validator set
-            if let Some(val_addr) = commit_sig.validator_address {
-                if vals.validator(val_addr) == None {
-                    fail!(
-                        Kind::FaultyFullNode,
-                        "Found a faulty signer ({}) not present in the validator set ({})",
-                        val_addr,
-                        vals.hash()
-                    );
-                }
-            }
+            commit_sig.validate(vals)?;
         }
 
         Ok(())
     }
 }
 
-fn commit_to_votes(commit: block::Commit) -> Result<Vec<vote::Vote>, Error> {
+// this private helper function does *not* do any validation but extracts
+// all non-BlockIDFlagAbsent votes from the commit:
+fn non_absent_votes(commit: &block::Commit) -> Vec<vote::Vote> {
     let mut votes: Vec<vote::Vote> = Default::default();
     for (i, commit_sig) in commit.signatures.iter().enumerate() {
         if commit_sig.is_absent() {
             continue;
         }
 
-        match commit_sig.validator_address {
-            Some(val_addr) => {
-                if let Some(sig) = commit_sig.signature.clone() {
-                    let vote = vote::Vote {
-                        vote_type: vote::Type::Precommit,
-                        height: commit.height,
-                        round: commit.round,
-                        block_id: Option::from(commit.block_id.clone()),
-                        timestamp: commit_sig.timestamp,
-                        validator_address: val_addr,
-                        validator_index: i as u64,
-                        signature: sig,
-                    };
-                    votes.push(vote);
+        if let Some(val_addr) = commit_sig.validator_address {
+            if let Some(sig) = commit_sig.signature.clone() {
+                let vote = vote::Vote {
+                    vote_type: vote::Type::Precommit,
+                    height: commit.height,
+                    round: commit.round,
+                    block_id: Option::from(commit.block_id.clone()),
+                    timestamp: commit_sig.timestamp,
+                    validator_address: val_addr,
+                    validator_index: u64::try_from(i)
+                        .expect("usize to u64 conversion failed for validator index"),
+                    signature: sig,
+                };
+                votes.push(vote);
+            }
+        }
+    }
+    votes
+}
+
+// TODO: consider moving this into commit_sig.rs instead and making it pub
+impl block::commit_sig::CommitSig {
+    fn validate(&self, vals: &Set) -> Result<(), Error> {
+        match self.block_id_flag {
+            BlockIDFlag::BlockIDFlagAbsent => {
+                if self.validator_address.is_some() {
+                    fail!(
+                        Kind::ImplementationSpecific,
+                        "validator address is present for absent CommitSig {:#?}",
+                        self
+                    );
+                }
+                if self.signature.is_some() {
+                    fail!(
+                        Kind::ImplementationSpecific,
+                        "signature is present for absent CommitSig {:#?}",
+                        self
+                    );
                 }
+                // TODO: deal with Time
+                // see https://github.com/informalsystems/tendermint-rs/pull/196#discussion_r401027989
             }
-            None => {
-                fail!(
-                    Kind::ImplementationSpecific,
-                    "validator address missing in commit_sig {:#?}",
-                    commit_sig
-                );
+            BlockIDFlag::BlockIDFlagCommit | BlockIDFlag::BlockIDFlagNil => {
+                if self.validator_address.is_none() {
+                    fail!(
+                        Kind::ImplementationSpecific,
+                        "missing validator address for non-absent CommitSig {:#?}",
+                        self
+                    );
+                }
+                if self.signature.is_none() {
+                    fail!(
+                        Kind::ImplementationSpecific,
+                        "missing signature for non-absent CommitSig {:#?}",
+                        self
+                    );
+                }
+                // TODO: this last check is only necessary if we do full verification (2/3) but the
+                // above checks should actually happen always (even if we skip forward)
+                //
+                // returns ImplementationSpecific error if it detects a signer
+                // that is not present in the validator set:
+                if let Some(val_addr) = self.validator_address {
+                    if vals.validator(val_addr) == None {
+                        fail!(
+                            Kind::ImplementationSpecific,
+                            "Found a faulty signer ({}) not present in the validator set ({})",
+                            val_addr,
+                            vals.hash()
+                        );
+                    }
+                }
             }
         }
+
+        Ok(())
     }
-    Ok(votes)
 }
 
 impl block::signed_header::SignedHeader {
     /// This is a private helper method to iterate over the underlying
     /// votes to compute the voting power (see `voting_power_in` below).
-    fn iter(&self) -> Result<Vec<vote::SignedVote>, Error> {
+    fn signed_votes(&self) -> Vec<vote::SignedVote> {
         let chain_id = self.header.chain_id.to_string();
-        // if let Ok(mut votes) = commit_to_votes(self.commit.clone())
-        let mut votes = match commit_to_votes(self.commit.clone()) {
-            Ok(votes_vec) => votes_vec,
-            Err(e) => return Err(e),
-        };
-        Ok(votes
+        let mut votes = non_absent_votes(&self.commit);
+        votes
             .drain(..)
             .map(|vote| {
                 vote::SignedVote::new(
@@ -130,7 +171,7 @@ impl block::signed_header::SignedHeader {
                     vote.signature,
                 )
             })
-            .collect())
+            .collect()
     }
 }