From 93b0fadd9ef14f6e0dd237d7443f49fc2c38cc61 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Sun, 29 Dec 2019 13:15:09 -0500 Subject: [PATCH] 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 | 13 +++++---- tendermint/src/lite/verifier.rs | 51 ++++++++++++++++++++++----------- 2 files changed, 43 insertions(+), 21 deletions(-) diff --git a/tendermint/src/lite/types.rs b/tendermint/src/lite/types.rs index db22fcd9f..9d0e74e26 100644 --- a/tendermint/src/lite/types.rs +++ b/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; /// 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, } diff --git a/tendermint/src/lite/verifier.rs b/tendermint/src/lite/verifier.rs index 52652b753..46d36015d 100644 --- a/tendermint/src/lite/verifier.rs +++ b/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(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