Bucky/some light follow up (#109)

* use ? instead of return Err(err)

* remove unused errors

* move a validation and add TODOs about them

* check_support returns early for sequential case

* drop let _ =

* add comment to voting_power_in about signers

tendermint/src/lite/types.rs

@@ -80,11 +80,14 @@ pub trait Commit {
     fn header_hash(&self) -> Hash;
 
     /// Compute the voting power of the validators that correctly signed the commit,
-    /// have according to their voting power in the passed in validator set.
+    /// according to their voting power in the passed in validator set.
     /// Will return an error in case an invalid signature was included.
     ///
-    /// This method corresponds to the (pure) auxiliary function int the spec:
+    /// This method corresponds to the (pure) auxiliary function in the spec:
     /// `votingpower_in(signers(h.Commit),h.Header.V)`.
+    /// Note this expects the Commit to be able to compute `signers(h.Commit)`,
+    /// ie. the identity of the validators that signed it, so they
+    /// can be cross-referenced with the given `vals`.
     fn voting_power_in(&self, vals: &Self::ValidatorSet) -> Result<u64, Error>;
 
     /// Return the number of votes included in this commit
@@ -144,15 +147,15 @@ pub trait Store {
 pub enum Error {
     Expired,
     DurationOutOfRange,
-    NonSequentialHeight,
-    NonIncreasingHeight,
+
+    InvalidSignature, // TODO: deduplicate with ErrorKind::SignatureInvalid
 
     InvalidValidatorSet,
     InvalidNextValidatorSet,
     InvalidCommitValue, // commit is not for the header we expected
     InvalidCommitLength,
-    InvalidSignature,
 
     InsufficientVotingPower, // TODO(Liamsi): change to same name as spec if this changes (curently ErrTooMuchChange)
+
     RequestFailed,
 }

tendermint/src/lite/verifier.rs

@@ -71,35 +71,58 @@ where
     let h1 = trusted_state.last_header();
     let h1_next_vals = trusted_state.validators();
 
-    if let Err(err) = is_within_trust_period(h1.header(), trusting_period, now) {
-        return Err(err);
+    // TODO(EB): can we move all these checks? it seems the
+    // is_within doesn't need to happen here and the sequential check
+    // for vals hash should be part of validate_vals_and_commit. then this function
+    // is basically just verify_commit_trusting. Would need to update spec as well.
+    // In sequential case, would still need to return early or not call check_support at all.
+    is_within_trust_period(h1.header(), trusting_period, now)?;
+
+    // check the sequential case
+    if h2.header().height() == h1.header().height().increment() {
+        if h2.header().validators_hash() == h1_next_vals.hash() {
+            // It's sequential, so verify_commit_trusting would be
+            // redundant with verify since the validators are the same
+            // when we're not skipping.
+            return Ok(());
+        } else {
+            return Err(Error::InvalidNextValidatorSet);
+        }
     }
 
-    if h2.header().height() == h1.header().height().increment()
-        && h2.header().validators_hash() != h1_next_vals.hash()
-    {
-        return Err(Error::InvalidNextValidatorSet);
-    }
+    // check if enough trusted validators signed to skip to the new height.
     verify_commit_trusting(h1_next_vals, h2.commit(), trust_threshold)
 }
 
 /// Validate the validators and commit against the header.
+// TODO(EB): consider making this a method on Commit so the details are hidden,
+// and so we can remove the votes_len() method (that check would be part of the
+// methods implementation). These checks aren't reflected
+// explicitly in the spec yet, only in the sentence "Additional checks should
+// be done in the implementation to ensure header is well formed".
 fn validate_vals_and_commit<H, V, C>(header: &H, commit: &C, vals: &V) -> Result<(), Error>
 where
     H: Header,
     V: ValidatorSet,
     C: Commit,
 {
-    // ensure the validators in the header matches what we expect from our state.
+    // ensure the header validators match these validators
     if header.validators_hash() != vals.hash() {
         return Err(Error::InvalidValidatorSet);
     }
 
-    // ensure the commit matches the header.
+    // ensure the header matches the commit
     if header.hash() != commit.header_hash() {
         return Err(Error::InvalidCommitValue);
     }
 
+    // ensure the validator size matches the commit size
+    // NOTE: this is commit structure specifc and should be
+    // hidden from the light client ...
+    if vals.len() != commit.votes_len() {
+        return Err(Error::InvalidCommitLength);
+    }
+
     Ok(())
 }
 
@@ -111,9 +134,9 @@ where
 {
     let header = signed_header.header();
     let commit = signed_header.commit();
-    if let Err(e) = validate_vals_and_commit(header, commit, validators) {
-        return Err(e);
-    }
+
+    // basic validatity checks that header, commit, and vals match up
+    validate_vals_and_commit(header, commit, validators)?;
 
     // ensure that +2/3 validators signed correctly
     verify_commit_full(validators, commit)
@@ -126,10 +149,6 @@ where
     C: Commit,
 {
     let total_power = vals.total_power();
-    if vals.len() != commit.votes_len() {
-        return Err(Error::InvalidCommitLength);
-    }
-
     let signed_power = commit.voting_power_in(vals)?;
 
     // check the signers account for +2/3 of the voting power