Refactor Light Client verification predicates interface for use from …

…IBC (#960)

* Reduce to default hashing implementations

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Refactor verification predicates interface to use minimal input parameters

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Swap order of fields back to the way they were

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Add Eq and PartialEq impls to pacify IBC TendermintClient constraints

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Refactor Light Client verifier to require only minimal state without peer ID

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Introduce VerificationState as a subset of LightBlock parameters for verification

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Rename VerificationState to VerifyParams for clarity

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Refactor light client verifier

This commit refactors the light client verifier to require the minimum
possible amount of detail in its parameters to perform verification.
(The LightBlock structure provides far more data than is actually
necessary)

This refactor makes it much easier to reuse the verification code from
ibc-rs.

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Add .changelog entry

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Document updates to data structures and verify method

Signed-off-by: Thane Thomson <connect@thanethomson.com>

.changelog/unreleased/breaking-changes/956-light-client-predicates-ibc.md

@@ -0,0 +1,4 @@
+- `[tendermint_light_client]` The light client verification functionality has
+  been refactored (including breaking changes to the API) such that it can be
+  more easily used from both `tendermint_light_client` and `ibc-rs`
+  ([#956](https://github.com/informalsystems/tendermint-rs/issues/956))

light-client-js/src/lib.rs

@@ -32,7 +32,12 @@ pub fn verify(untrusted: &JsValue, trusted: &JsValue, options: &JsValue, now: &J
     let result = deserialize_params(untrusted, trusted, options, now).map(
         |(untrusted, trusted, options, now)| {
             let verifier = ProdVerifier::default();
-            verifier.verify(&untrusted, &trusted, &options, now)
+            verifier.verify(
+                untrusted.as_untrusted_state(),
+                trusted.as_trusted_state(),
+                &options,
+                now,
+            )
         },
     );
     JsValue::from_serde(&result).unwrap()

light-client/src/builder/light_client.rs

@@ -170,19 +170,27 @@ impl LightClientBuilder<NoTrustedState> {
         let now = self.clock.now();
 
         self.predicates
-            .is_within_trust_period(header, self.options.trusting_period, now)
+            .is_within_trust_period(header.time, self.options.trusting_period, now)
             .map_err(Error::invalid_light_block)?;
 
         self.predicates
-            .is_header_from_past(header, self.options.clock_drift, now)
+            .is_header_from_past(header.time, self.options.clock_drift, now)
             .map_err(Error::invalid_light_block)?;
 
         self.predicates
-            .validator_sets_match(light_block, &*self.hasher)
+            .validator_sets_match(
+                &light_block.validators,
+                light_block.signed_header.header.validators_hash,
+                &*self.hasher,
+            )
             .map_err(Error::invalid_light_block)?;
 
         self.predicates
-            .next_validators_match(light_block, &*self.hasher)
+            .next_validators_match(
+                &light_block.next_validators,
+                light_block.signed_header.header.next_validators_hash,
+                &*self.hasher,
+            )
             .map_err(Error::invalid_light_block)?;
 
         Ok(())

light-client/src/components/verifier.rs

@@ -2,14 +2,15 @@
 
 use crate::operations::voting_power::VotingPowerTally;
 use crate::predicates as preds;
+use crate::types::{TrustedBlockState, UntrustedBlockState};
 use crate::{
     errors::ErrorExt,
     light_client::Options,
     operations::{
         CommitValidator, Hasher, ProdCommitValidator, ProdHasher, ProdVotingPowerCalculator,
         VotingPowerCalculator,
     },
-    types::{LightBlock, Time},
+    types::Time,
 };
 use preds::{
     errors::{VerificationError, VerificationErrorDetail},
@@ -55,75 +56,184 @@ pub trait Verifier: Send + Sync {
     /// Perform the verification.
     fn verify(
         &self,
-        untrusted: &LightBlock,
-        trusted: &LightBlock,
+        untrusted: UntrustedBlockState<'_>,
+        trusted: TrustedBlockState<'_>,
         options: &Options,
         now: Time,
     ) -> Verdict;
 }
 
-/// Production implementation of the verifier.
-///
-/// For testing purposes, this implementation is parametrized by:
-/// - A set of predicates used to validate a light block
-/// - A voting power calculator
-/// - A commit validator
-/// - A header hasher
-///
-/// For regular use, one can construct a standard implementation with `ProdVerifier::default()`.
-pub struct ProdVerifier {
-    predicates: Box<dyn VerificationPredicates>,
-    voting_power_calculator: Box<dyn VotingPowerCalculator>,
-    commit_validator: Box<dyn CommitValidator>,
-    hasher: Box<dyn Hasher>,
+macro_rules! verdict {
+    ($e:expr) => {
+        let result = $e;
+        if result.is_err() {
+            return result.into();
+        }
+    };
 }
 
-impl ProdVerifier {
-    /// Constructs a new instance of this struct
-    pub fn new(
-        predicates: impl VerificationPredicates + 'static,
-        voting_power_calculator: impl VotingPowerCalculator + 'static,
-        commit_validator: impl CommitValidator + 'static,
-        hasher: impl Hasher + 'static,
-    ) -> Self {
+/// Predicate verifier encapsulating components necessary to facilitate
+/// verification.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct PredicateVerifier<P, C, V, H> {
+    predicates: P,
+    voting_power_calculator: C,
+    commit_validator: V,
+    hasher: H,
+}
+
+impl<P, C, V, H> Default for PredicateVerifier<P, C, V, H>
+where
+    P: Default,
+    C: Default,
+    V: Default,
+    H: Default,
+{
+    fn default() -> Self {
         Self {
-            predicates: Box::new(predicates),
-            voting_power_calculator: Box::new(voting_power_calculator),
-            commit_validator: Box::new(commit_validator),
-            hasher: Box::new(hasher),
+            predicates: P::default(),
+            voting_power_calculator: C::default(),
+            commit_validator: V::default(),
+            hasher: H::default(),
         }
     }
 }
 
-impl Default for ProdVerifier {
-    fn default() -> Self {
-        Self::new(
-            ProdPredicates::default(),
-            ProdVotingPowerCalculator::default(),
-            ProdCommitValidator::default(),
-            ProdHasher::default(),
-        )
+impl<P, C, V, H> PredicateVerifier<P, C, V, H>
+where
+    P: VerificationPredicates,
+    C: VotingPowerCalculator,
+    V: CommitValidator,
+    H: Hasher,
+{
+    /// Constructor.
+    pub fn new(predicates: P, voting_power_calculator: C, commit_validator: V, hasher: H) -> Self {
+        Self {
+            predicates,
+            voting_power_calculator,
+            commit_validator,
+            hasher,
+        }
     }
 }
 
-impl Verifier for ProdVerifier {
+impl<P, C, V, H> Verifier for PredicateVerifier<P, C, V, H>
+where
+    P: VerificationPredicates,
+    C: VotingPowerCalculator,
+    V: CommitValidator,
+    H: Hasher,
+{
+    /// Validate the given light block state.
+    ///
+    /// - Ensure the latest trusted header hasn't expired
+    /// - Ensure the header validator hashes match the given validators
+    /// - Ensure the header next validator hashes match the given next
+    ///   validators
+    /// - Additional implementation specific validation via `commit_validator`
+    /// - Check that the untrusted block is more recent than the trusted state
+    /// - If the untrusted block is the very next block after the trusted block,
+    ///   check that their (next) validator sets hashes match.
+    /// - Otherwise, ensure that the untrusted block has a greater height than
+    ///   the trusted block.
+    ///
+    /// **NOTE**: If the untrusted state's `next_validators` field is `None`,
+    /// this will not (and will not be able to) check whether the untrusted
+    /// state's `next_validators_hash` field is valid.
     fn verify(
         &self,
-        untrusted: &LightBlock,
-        trusted: &LightBlock,
+        untrusted: UntrustedBlockState<'_>,
+        trusted: TrustedBlockState<'_>,
         options: &Options,
         now: Time,
     ) -> Verdict {
-        preds::verify(
-            &*self.predicates,
-            &*self.voting_power_calculator,
-            &*self.commit_validator,
-            &*self.hasher,
-            trusted,
-            untrusted,
-            options,
+        // Ensure the latest trusted header hasn't expired
+        verdict!(self.predicates.is_within_trust_period(
+            trusted.header_time,
+            options.trusting_period,
             now,
-        )
-        .into()
+        ));
+
+        // Ensure the header isn't from a future time
+        verdict!(self.predicates.is_header_from_past(
+            untrusted.signed_header.header.time,
+            options.clock_drift,
+            now,
+        ));
+
+        // Ensure the header validator hashes match the given validators
+        verdict!(self.predicates.validator_sets_match(
+            untrusted.validators,
+            untrusted.signed_header.header.validators_hash,
+            &self.hasher,
+        ));
+
+        // TODO(thane): Is this check necessary for IBC?
+        if let Some(untrusted_next_validators) = untrusted.next_validators {
+            // Ensure the header next validator hashes match the given next validators
+            verdict!(self.predicates.next_validators_match(
+                untrusted_next_validators,
+                untrusted.signed_header.header.next_validators_hash,
+                &self.hasher,
+            ));
+        }
+
+        // Ensure the header matches the commit
+        verdict!(self.predicates.header_matches_commit(
+            &untrusted.signed_header.header,
+            untrusted.signed_header.commit.block_id.hash,
+            &self.hasher,
+        ));
+
+        // Additional implementation specific validation
+        verdict!(self.predicates.valid_commit(
+            untrusted.signed_header,
+            untrusted.validators,
+            &self.commit_validator,
+        ));
+
+        // Check that the untrusted block is more recent than the trusted state
+        verdict!(self
+            .predicates
+            .is_monotonic_bft_time(untrusted.signed_header.header.time, trusted.header_time,));
+
+        let trusted_next_height = trusted.height.increment();
+
+        if untrusted.height() == trusted_next_height {
+            // If the untrusted block is the very next block after the trusted block,
+            // check that their (next) validator sets hashes match.
+            verdict!(self.predicates.valid_next_validator_set(
+                untrusted.signed_header.header.validators_hash,
+                trusted.next_validators_hash,
+            ));
+        } else {
+            // Otherwise, ensure that the untrusted block has a greater height than
+            // the trusted block.
+            verdict!(self
+                .predicates
+                .is_monotonic_height(untrusted.signed_header.header.height, trusted.height));
+
+            // Check there is enough overlap between the validator sets of
+            // the trusted and untrusted blocks.
+            verdict!(self.predicates.has_sufficient_validators_overlap(
+                untrusted.signed_header,
+                trusted.next_validators,
+                &options.trust_threshold,
+                &self.voting_power_calculator,
+            ));
+        }
+
+        // Verify that more than 2/3 of the validators correctly committed the block.
+        verdict!(self.predicates.has_sufficient_signers_overlap(
+            untrusted.signed_header,
+            untrusted.validators,
+            &self.voting_power_calculator,
+        ));
+
+        Verdict::Success
     }
 }
+
+/// The default production implementation of the [`PredicateVerifier`].
+pub type ProdVerifier =
+    PredicateVerifier<ProdPredicates, ProdVotingPowerCalculator, ProdCommitValidator, ProdHasher>;

light-client/src/light_client.rs

@@ -201,22 +201,22 @@ impl LightClient {
             let now = self.clock.now();
 
             // Get the latest trusted state
-            let trusted_state = state
+            let trusted_block = state
                 .light_store
                 .highest_trusted_or_verified()
                 .ok_or_else(Error::no_initial_trusted_state)?;
 
-            if target_height < trusted_state.height() {
+            if target_height < trusted_block.height() {
                 return Err(Error::target_lower_than_trusted_state(
                     target_height,
-                    trusted_state.height(),
+                    trusted_block.height(),
                 ));
             }
 
             // Check invariant [LCV-INV-TP.1]
-            if !is_within_trust_period(&trusted_state, self.options.trusting_period, now) {
+            if !is_within_trust_period(&trusted_block, self.options.trusting_period, now) {
                 return Err(Error::trusted_state_outside_trusting_period(
-                    Box::new(trusted_state),
+                    Box::new(trusted_block),
                     self.options,
                 ));
             }
@@ -226,18 +226,21 @@ impl LightClient {
 
             // If the trusted state is now at a height equal to the target height, we are done.
             // [LCV-DIST-LIFE.1]
-            if target_height == trusted_state.height() {
-                return Ok(trusted_state);
+            if target_height == trusted_block.height() {
+                return Ok(trusted_block);
             }
 
             // Fetch the block at the current height from the light store if already present,
             // or from the primary peer otherwise.
             let (current_block, status) = self.get_or_fetch_block(current_height, state)?;
 
             // Validate and verify the current block
-            let verdict = self
-                .verifier
-                .verify(&current_block, &trusted_state, &self.options, now);
+            let verdict = self.verifier.verify(
+                current_block.as_untrusted_state(),
+                trusted_block.as_trusted_state(),
+                &self.options,
+                now,
+            );
 
             match verdict {
                 Verdict::Success => {

light-client/src/operations/commit_validator.rs

@@ -26,17 +26,16 @@ pub trait CommitValidator: Send + Sync {
 }
 
 /// Production-ready implementation of a commit validator
+#[derive(Debug, Clone, PartialEq, Eq)]
 pub struct ProdCommitValidator {
-    hasher: Box<dyn Hasher>,
+    hasher: ProdHasher,
 }
 
 impl ProdCommitValidator {
     /// Create a new commit validator using the given [`Hasher`]
     /// to compute the hash of headers and validator sets.
-    pub fn new(hasher: impl Hasher + 'static) -> Self {
-        Self {
-            hasher: Box::new(hasher),
-        }
+    pub fn new(hasher: ProdHasher) -> Self {
+        Self { hasher }
     }
 }