From afcf99a81be152b25e2acf62ae1bb7b3ca1368dc Mon Sep 17 00:00:00 2001 From: Ismail Khoffi Date: Wed, 19 Feb 2020 18:19:23 +0100 Subject: [PATCH] Valid TrustThresholdFraction and further simplify tests code (#142) * Valid TrustThresholdFraction for real * Further simplify tests: init requester with final state * Apply review suggestions and assert_bisection_err fn * remove redundant clone * add another err case * unpublicize verify_commit_full and verify_commit_trusting (ref #136) * ensure the untrusted_header.bft_time() > trusted_header.bft_time() * Fix failing test and test and add another erroring one * remove dupl test * Consistent comments in tests * Refactor init_requester (#152) * added bisection test for insufficient commits * refactored init_requester, TODO: update tests * fixed tests * Added bisection test for insufficient commits * fixed failures * cargo fmt * Changed tests to use ValsAndCommit struct * satisfy clippy * Dealing with the merge conflict's aftermath Co-authored-by: Shivani Joshi <46731446+Shivani912@users.noreply.github.com> --- tendermint/src/lite/error.rs | 4 + tendermint/src/lite/types.rs | 21 ++- tendermint/src/lite/verifier.rs | 307 ++++++++++++++++++++++++-------- 3 files changed, 252 insertions(+), 80 deletions(-) diff --git a/tendermint/src/lite/error.rs b/tendermint/src/lite/error.rs index a45594ef3..e884def17 100644 --- a/tendermint/src/lite/error.rs +++ b/tendermint/src/lite/error.rs @@ -24,6 +24,10 @@ pub enum Kind { #[error("expected height >= {expected} (got: {got})")] NonIncreasingHeight { got: u64, expected: u64 }, + /// Header time is in the past compared to already trusted header. + #[error("untrusted header time <= trusted header time")] + NonIncreasingTime, + /// Invalid validator hash. #[error("header's validator hash does not match actual validator hash ({header_val_hash:?}!={val_hash:?})")] InvalidValidatorSet { diff --git a/tendermint/src/lite/types.rs b/tendermint/src/lite/types.rs index 4cde4b740..1ef1e9b66 100644 --- a/tendermint/src/lite/types.rs +++ b/tendermint/src/lite/types.rs @@ -94,8 +94,13 @@ pub struct TrustThresholdFraction { } impl TrustThresholdFraction { - pub fn new(numerator: u64, denominator: u64) -> Result { - if numerator <= denominator && denominator > 0 { + /// Instantiate a TrustThresholdFraction if the given denominator and + /// numerator are valid. + /// + /// The parameters are valid iff `1/3 <= numerator/denominator <= 1`. + /// In any other case we return [`Error::InvalidTrustThreshold`]. + pub fn new(numerator: u64, denominator: u64) -> Result { + if numerator <= denominator && denominator > 0 && 3 * numerator >= denominator { return Ok(Self { numerator, denominator, @@ -103,7 +108,8 @@ impl TrustThresholdFraction { } Err(Kind::InvalidTrustThreshold { got: format!("{}/{}", numerator, denominator), - }) + } + .into()) } } @@ -227,6 +233,10 @@ pub(super) mod mocks { next_vals, } } + + pub fn set_time(&mut self, new_time: SystemTime) { + self.time = new_time + } } impl Header for MockHeader { @@ -430,8 +440,13 @@ mod tests { TrustThresholdFraction::new(1, 3).expect("mustn't panic") ); assert!(TrustThresholdFraction::new(2, 3).is_ok()); + assert!(TrustThresholdFraction::new(1, 1).is_ok()); assert!(TrustThresholdFraction::new(3, 1).is_err()); + assert!(TrustThresholdFraction::new(1, 4).is_err()); + assert!(TrustThresholdFraction::new(1, 5).is_err()); + assert!(TrustThresholdFraction::new(2, 7).is_err()); + assert!(TrustThresholdFraction::new(0, 1).is_err()); assert!(TrustThresholdFraction::new(1, 0).is_err()); } } diff --git a/tendermint/src/lite/verifier.rs b/tendermint/src/lite/verifier.rs index b1b9e674d..e40b387dc 100644 --- a/tendermint/src/lite/verifier.rs +++ b/tendermint/src/lite/verifier.rs @@ -98,7 +98,7 @@ where /// NOTE: These validators are expected to be the correct validators for the commit, /// but since we're using voting_power_in, we can't actually detect if there's /// votes from validators not in the set. -pub fn verify_commit_full(vals: &C::ValidatorSet, commit: &C) -> Result<(), Error> +fn verify_commit_full(vals: &C::ValidatorSet, commit: &C) -> Result<(), Error> where C: Commit, { @@ -121,7 +121,7 @@ where /// NOTE the given validators do not necessarily correspond to the validator set for this commit, /// but there may be some intersection. The trust_level parameter allows clients to require more /// than +1/3 by implementing the TrustLevel trait accordingly. -pub fn verify_commit_trusting( +fn verify_commit_trusting( validators: &C::ValidatorSet, commit: &C, trust_level: L, @@ -178,7 +178,10 @@ where let trusted_height = trusted_header.height(); let untrusted_height = untrusted_sh.header().height(); - // TODO: ensure the untrusted_header.bft_time() > trusted_header.bft_time() + // ensure the untrusted_header.bft_time() > trusted_header.bft_time() + if untrusted_header.bft_time().into() <= trusted_header.bft_time().into() { + return Err(Kind::NonIncreasingTime.into()); + } match untrusted_height.cmp(&trusted_height.checked_add(1).expect("height overflow")) { Ordering::Less => { @@ -416,52 +419,64 @@ mod tests { // create an initial trusted state from the given vals fn init_trusted_state( - vals_vec: Vec, + vals_and_commit_vec: ValsAndCommit, next_vals_vec: Vec, height: u64, ) -> MockState { - let time = init_time(); - let vals = &MockValSet::new(vals_vec.clone()); + // time has to be increasing: + let time = init_time() + Duration::new(height * 2, 0); + let vals = &MockValSet::new(vals_and_commit_vec.vals_vec); let next_vals = &MockValSet::new(next_vals_vec); let header = MockHeader::new(height, time, vals.hash(), next_vals.hash()); - let commit = MockCommit::new(header.hash(), vals_vec); - let sh = &SignedHeader::new(commit, header); + let commit = MockCommit::new(header.hash(), vals_and_commit_vec.commit_vec); + let sh = &MockSignedHeader::new(commit, header); MockState::new(sh, vals) } // create the next state with the given vals and commit. - fn next_state( - vals_vec: Vec, - commit_vec: Vec, - ) -> (MockSignedHeader, MockValSet, MockValSet) { + fn next_state(vals_and_commit: ValsAndCommit) -> (MockSignedHeader, MockValSet, MockValSet) { let time = init_time() + Duration::new(10, 0); let height = 10; - let vals = MockValSet::new(vals_vec); + let vals = MockValSet::new(vals_and_commit.vals_vec); let next_vals = vals.clone(); let header = MockHeader::new(height, time, vals.hash(), next_vals.hash()); - let commit = MockCommit::new(header.hash(), commit_vec); + let commit = MockCommit::new(header.hash(), vals_and_commit.commit_vec); (MockSignedHeader::new(commit, header), vals, next_vals) } + #[derive(Clone)] + struct ValsAndCommit { + vals_vec: Vec, + commit_vec: Vec, + } + + impl ValsAndCommit { + pub fn new(vals_vec: Vec, commit_vec: Vec) -> ValsAndCommit { + ValsAndCommit { + vals_vec, + commit_vec, + } + } + } // Init a mock Requester. // For each pair of lists of validators (cur and next vals) we // init a trusted state (signed header and vals); - // these are (signed) headers and validators will be returned by the Requester. - fn init_requester(vals_for_height: Vec>) -> MockRequester { + // Note: for bisection up-to height n, provide n+2 validators as validators for n+1 + // will be requested to verify height n. + fn init_requester(vals_and_commit_for_height: Vec) -> MockRequester { let mut req = MockRequester::new(); - let max_height = vals_for_height.len(); - for (h, vals) in vals_for_height.iter().enumerate() { + let max_height = vals_and_commit_for_height.len() as u64; + for (h, vac) in vals_and_commit_for_height.iter().enumerate() { let height = (h + 1) as u64; // height starts with 1 ... - if height < max_height as u64 { - let next_vals = vals_for_height.get(h + 1).expect("next_vals missing"); - let ts = init_trusted_state(vals.to_owned(), next_vals.to_owned(), height); + if height < max_height { + let next_vals = vals_and_commit_for_height + .get(h + 1) + .expect("next_vals missing") + .vals_vec + .clone(); + let ts = &init_trusted_state(vac.to_owned(), next_vals.to_owned(), height); req.signed_headers.insert(height, ts.last_header().clone()); req.validators.insert(height, ts.validators().to_owned()); - } else { - // these will be requested in the last call of verify_bisection_inner - // and verified against next_validators of the header in the last SignedHeader: - let vals = &MockValSet::new(vals.clone()); - req.validators.insert(height, vals.to_owned()); } } req @@ -470,11 +485,10 @@ mod tests { // make a state with the given vals and commit and ensure we get the expected error kind. fn assert_single_err( ts: &TrustedState, - vals: Vec, - commit: Vec, - err: &Kind, + vals_and_commit: ValsAndCommit, + err: Error, ) { - let (un_sh, un_vals, un_next_vals) = next_state(vals, commit); + let (un_sh, un_vals, un_next_vals) = next_state(vals_and_commit); let result = verify_single_inner( ts, &un_sh, @@ -487,8 +501,8 @@ mod tests { } // make a state with the given vals and commit and ensure we get no error. - fn assert_single_ok(ts: &MockState, vals: Vec, commit: Vec) { - let (un_sh, un_vals, un_next_vals) = next_state(vals, commit); + fn assert_single_ok(ts: &MockState, vals_and_commit: ValsAndCommit) { + let (un_sh, un_vals, un_next_vals) = next_state(vals_and_commit); assert!(verify_single_inner( ts, &un_sh, @@ -499,13 +513,14 @@ mod tests { .is_ok()); } - // make a sequence of states with the given vals for the requester + // use the sequence of states with the given vals for the requester // and ensure bisection yields no error. fn assert_bisection_ok( req: &MockRequester, ts: &TrustedState, untrusted_height: u64, expected_num_of_requests: usize, + expected_final_state: &MockState, ) { let mut cache: Vec = Vec::new(); let ts_new = verify_bisection_inner( @@ -516,7 +531,7 @@ mod tests { cache.as_mut(), ) .expect("should have passed"); - assert_eq!(ts_new.last_header().header().height(), untrusted_height); + assert_eq!(ts_new, expected_final_state.to_owned()); assert_eq!(cache.len(), expected_num_of_requests); assert_uniqueness(cache); } @@ -527,24 +542,49 @@ mod tests { assert_eq!(cache, uniq); } + // use the sequence of states with the given vals for the requester + // and ensure we get the expected error. + fn assert_bisection_err( + req: &MockRequester, + ts: &TrustedState, + untrusted_height: u64, + err: Error, + ) { + let mut cache: Vec = Vec::new(); + let result = verify_bisection_inner( + &ts, + untrusted_height, + TrustThresholdFraction::default(), + req, + cache.as_mut(), + ); + assert!(result.is_err()); + assert_eq!( + result.unwrap_err().kind().to_string(), + err.kind().to_string() + ); + } + // valid to skip, but invalid commit. 1 validator. #[test] fn test_verify_single_skip_1_val_verify() { - let ts = &init_trusted_state(vec![0], vec![0], 1); + let vac = ValsAndCommit::new(vec![0], vec![0]); + let ts = &init_trusted_state(vac, vec![0], 1); // 100% overlap, but wrong commit. // NOTE: This should be an invalid commit error since there's // a vote from a validator not in the set! // but voting_power_in isn't smart enough to see this ... // TODO(ismail): https://github.com/interchainio/tendermint-rs/issues/140 + let invalid_vac = ValsAndCommit::new(vec![1], vec![0]); assert_single_err( ts, - vec![1], - vec![0], - &Kind::InvalidCommit { + invalid_vac, + Kind::InvalidCommit { total: 1, signed: 0, - }, + } + .into(), ); } @@ -552,15 +592,22 @@ mod tests { // test if we can skip to it. #[test] fn test_verify_single_skip_1_val_skip() { - let ts = &init_trusted_state(vec![0], vec![0], 1); + let mut vac = ValsAndCommit::new(vec![0], vec![0]); + let ts = &init_trusted_state(vac.clone(), vec![0], 1); //***** // Ok // 100% overlap (original signer is present in commit) - assert_single_ok(ts, vec![0], vec![0]); - assert_single_ok(ts, vec![0, 1], vec![0, 1]); - assert_single_ok(ts, vec![0, 1, 2], vec![0, 1, 2]); - assert_single_ok(ts, vec![0, 1, 2, 3], vec![0, 1, 2, 3]); + assert_single_ok(ts, vac); + + vac = ValsAndCommit::new(vec![0, 1], vec![0, 1]); + assert_single_ok(ts, vac); + + vac = ValsAndCommit::new(vec![0, 1, 2], vec![0, 1, 2]); + assert_single_ok(ts, vac); + + vac = ValsAndCommit::new(vec![0, 1, 2, 3], vec![0, 1, 2, 3]); + assert_single_ok(ts, vac); //***** // Err @@ -571,28 +618,36 @@ mod tests { }; // 0% overlap - new val set without the original signer - assert_single_err(ts, vec![1], vec![1], &err); + vac = ValsAndCommit::new(vec![1], vec![1]); + assert_single_err(ts, vac, err.clone().into()); // 0% overlap - val set contains original signer, but they didn't sign - assert_single_err(ts, vec![0, 1, 2, 3], vec![1, 2, 3], &err); + vac = ValsAndCommit::new(vec![0, 1, 2, 3], vec![1, 2, 3]); + assert_single_err(ts, vac, err.into()); } // valid commit and data, starting with 2 validators. // test if we can skip to it. #[test] fn test_verify_single_skip_2_val_skip() { - let ts = &init_trusted_state(vec![0, 1], vec![0, 1], 1); + let mut vac = ValsAndCommit::new(vec![0, 1], vec![0, 1]); + let ts = &init_trusted_state(vac.clone(), vec![0, 1], 1); //************* // OK // 100% overlap (both original signers still present) - assert_single_ok(ts, vec![0, 1], vec![0, 1]); - assert_single_ok(ts, vec![0, 1, 2], vec![0, 1, 2]); + assert_single_ok(ts, vac); + + vac = ValsAndCommit::new(vec![0, 1, 2], vec![0, 1, 2]); + assert_single_ok(ts, vac); // 50% overlap (one original signer still present) - assert_single_ok(ts, vec![0], vec![0]); - assert_single_ok(ts, vec![0, 1, 2, 3], vec![1, 2, 3]); + vac = ValsAndCommit::new(vec![0], vec![0]); + assert_single_ok(ts, vac); + + vac = ValsAndCommit::new(vec![0, 1, 2, 3], vec![1, 2, 3]); + assert_single_ok(ts, vac); //************* // Err @@ -603,28 +658,36 @@ mod tests { }; // 0% overlap (neither original signer still present) - assert_single_err(ts, vec![2], vec![2], &err); + vac = ValsAndCommit::new(vec![2], vec![2]); + assert_single_err(ts, vac, err.clone().into()); // 0% overlap (original signer is still in val set but not in commit) - assert_single_err(ts, vec![0, 2, 3, 4], vec![2, 3, 4], &err); + vac = ValsAndCommit::new(vec![0, 2, 3, 4], vec![2, 3, 4]); + assert_single_err(ts, vac, err.into()); } // valid commit and data, starting with 3 validators. // test if we can skip to it. #[test] fn test_verify_single_skip_3_val_skip() { - let ts = &init_trusted_state(vec![0, 1, 2], vec![0, 1, 2], 1); + let mut vac = ValsAndCommit::new(vec![0, 1, 2], vec![0, 1, 2]); + let ts = &init_trusted_state(vac.clone(), vec![0, 1, 2], 1); //************* // OK // 100% overlap (both original signers still present) - assert_single_ok(ts, vec![0, 1, 2], vec![0, 1, 2]); - assert_single_ok(ts, vec![0, 1, 2, 3], vec![0, 1, 2, 3]); + assert_single_ok(ts, vac); + + vac = ValsAndCommit::new(vec![0, 1, 2, 3], vec![0, 1, 2, 3]); + assert_single_ok(ts, vac); // 66% overlap (two original signers still present) - assert_single_ok(ts, vec![0, 1], vec![0, 1]); - assert_single_ok(ts, vec![0, 1, 2, 3], vec![1, 2, 3]); + vac = ValsAndCommit::new(vec![0, 1], vec![0, 1]); + assert_single_ok(ts, vac); + + vac = ValsAndCommit::new(vec![0, 1, 2, 3], vec![1, 2, 3]); + assert_single_ok(ts, vac); //************* // Err @@ -636,11 +699,15 @@ mod tests { }; // 33% overlap (one original signer still present) - assert_single_err(ts, vec![0], vec![0], &err); - assert_single_err(ts, vec![0, 3], vec![0, 3], &err); + vac = ValsAndCommit::new(vec![0], vec![0]); + assert_single_err(ts, vac, err.clone().into()); + + vac = ValsAndCommit::new(vec![0, 3], vec![0, 3]); + assert_single_err(ts, vac, err.clone().into()); // 0% overlap (neither original signer still present) - assert_single_err(ts, vec![3], vec![2], &err); + vac = ValsAndCommit::new(vec![3], vec![2]); + assert_single_err(ts, vac, err.into()); let err = Kind::InsufficientVotingPower { total: 3, @@ -649,37 +716,123 @@ mod tests { }; // 0% overlap (original signer is still in val set but not in commit) - assert_single_err(ts, vec![0, 3, 4, 5], vec![3, 4, 5], &err); + vac = ValsAndCommit::new(vec![0, 3, 4, 5], vec![3, 4, 5]); + assert_single_err(ts, vac, err.into()); } #[test] fn test_verify_bisection_1_val() { - let req = init_requester(vec![vec![0], vec![0], vec![0]]); + let vac = ValsAndCommit::new(vec![0], vec![0]); + let final_state = init_trusted_state(vac.clone(), vec![0], 2); + let req = init_requester(vec![vac.clone(), vac.clone(), vac.clone(), vac.clone()]); let sh = req.signed_header(1).expect("first sh not present"); let vals = req.validator_set(1).expect("init. valset not present"); let ts = &MockState::new(&sh, &vals); - assert_bisection_ok(&req, &ts, 2, 1); - - let req = init_requester(vec![vec![0], vec![0], vec![0], vec![0]]); - assert_bisection_ok(&req, &ts, 3, 1); + assert_bisection_ok(&req, &ts, 2, 1, &final_state); + + let final_state = init_trusted_state(vac.clone(), vec![0], 3); + // let vac = ValsAndCommit::new(vec![0], vec![0]); + let req = init_requester(vec![ + vac.clone(), + vac.clone(), + vac.clone(), + vac.clone(), + vac, + ]); + assert_bisection_ok(&req, &ts, 3, 1, &final_state); } #[test] - fn test_verify_bisection_1_val_4_heights_check_all_intermediate() { - let mut vals_per_height: Vec> = Vec::new(); - vals_per_height.push(vec![0, 1, 2, 3, 4, 5]); // 1 - vals_per_height.push(vec![0, 1, 2]); // 2 -> 50% val change - vals_per_height.push(vec![0, 1]); // 3 -> 33% change - vals_per_height.push(vec![1, 2]); // 4 -> 50% change - vals_per_height.push(vec![0, 2]); // 5 -> 50% <- too much change (from 1), need to bisect... - vals_per_height.push(vec![0, 2]); // 6 -> (only need to validate 5) - let req = init_requester(vals_per_height); + fn test_verify_bisection() { + //************* + // OK + + let mut vals_and_commit_for_height: Vec = Vec::new(); + vals_and_commit_for_height.push(ValsAndCommit::new( + vec![0, 1, 2, 3, 4, 5], + vec![0, 1, 2, 3, 4, 5], + )); // 1 + vals_and_commit_for_height.push(ValsAndCommit::new(vec![0, 1, 2], vec![0, 1, 2])); // 2 -> 50% val change + vals_and_commit_for_height.push(ValsAndCommit::new(vec![0, 1], vec![0, 1])); // 3 -> 33% change + vals_and_commit_for_height.push(ValsAndCommit::new(vec![1, 2], vec![1, 2])); // 4 -> 50% change + vals_and_commit_for_height.push(ValsAndCommit::new(vec![0, 2], vec![0, 2])); // 5 -> 50% <- too much change (from 1), need to bisect... + vals_and_commit_for_height.push(ValsAndCommit::new(vec![0, 2], vec![0, 2])); // 6 -> (only needed to validate 5) + vals_and_commit_for_height.push(ValsAndCommit::new(vec![0, 2], vec![0, 2])); // 7 -> (only used to construct state 6) + let vac = ValsAndCommit::new(vec![0, 2], vec![0, 2]); + let final_ts = init_trusted_state(vac, vec![0, 2], 5); + let req = init_requester(vals_and_commit_for_height); let sh = req.signed_header(1).expect("first sh not present"); let vals = req.validator_set(1).expect("init. valset not present"); let ts = &MockState::new(&sh, &vals); - assert_bisection_ok(&req, &ts, 5, 3); + assert_bisection_ok(&req, &ts, 5, 3, &final_ts); + + //************* + // Err + + // fails due to missing vals for height 6: + let mut faulty_req = req; + faulty_req.validators.remove(&6_u64); + assert_bisection_err(&faulty_req, &ts, 5, Kind::RequestFailed.into()); + + // Error: can't bisect from trusted height 1 to height 1 + // (here because non-increasing time is caught first) + let vac = ValsAndCommit::new(vec![0, 1, 2], vec![0, 1, 2]); + let req = init_requester(vec![vac.clone(), vac.clone(), vac]); + assert_bisection_err(&req, &ts, 1, Kind::NonIncreasingTime.into()); + + // can't bisect from trusted height 1 to height 1 (here we tamper with time but + // expect to fail on NonIncreasingHeight): + let vac = ValsAndCommit::new(vec![0, 1, 2], vec![0, 1, 2]); + let mut req = init_requester(vec![vac.clone(), vac.clone(), vac]); + let sh = req.signed_headers.get(&1_u64).unwrap(); + let mut time_tampered_header = sh.header().clone(); + time_tampered_header.set_time(init_time() + Duration::new(5, 0)); + let tampered_commit = MockCommit::new(time_tampered_header.hash(), vec![0, 1, 2]); + let new_sh = MockSignedHeader::new(tampered_commit, time_tampered_header); + // replace the signed header: + req.signed_headers.insert(1, new_sh); + assert_bisection_err( + &req, + &ts, + 1, + Kind::NonIncreasingHeight { + got: 1, + expected: 2, + } + .into(), + ); + } + + // can't bisect from height 1 to height 3 + // because there isn't enough signatures/commits for header at height 3 + // errors with an 'InvalidCommit' + #[test] + fn test_bisection_not_enough_commits() { + let vals_vec = vec![0, 1, 2, 4]; + let commit_vec = vec![0, 1, 2, 4]; + let vac1 = ValsAndCommit::new(vals_vec.clone(), commit_vec); + let vac2 = ValsAndCommit::new(vals_vec.clone(), vec![0]); + let req = &init_requester(vec![ + vac1.clone(), + vac2.clone(), + vac2.clone(), + vac2.clone(), + vac2, + ]); + let ts = &init_trusted_state(vac1, vals_vec, 1); + + assert_bisection_err( + req, + ts, + 3, + Kind::InvalidCommit { + total: 4, + signed: 1, + } + .into(), + ); } #[test]