light-client: treat Trusted status as a special case of Verified …

…as per the spec (#419)

* Refactor LightStore::latest_trusted_or_verified

* Look for trusted or verified blocks in bisection loop

Only looking for blocks with `Status::Verified` might miss
a block that has previously gone through fork detection and
marked as `Trusted`. We would then fetch it again from the
peer, go through verification again, and then override its
status to `Verified`, thus downgrading it from `Trusted` to
`Verified`.

This commit fixes this by asking the light store for blocks which
are either trusted or verified.

* Small readability improvement in get_or_fetch_block

* Ensure blocks are not downgraded from trusted to verified

* Preserve Trusted status when updating a block that was just verified

* Fix typo in comment

Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>

* Remove Status::least_verified method

* Update changelog

* Rename LightStore::get_any to get_non_failed

* Fix contracts to take into account the Trusted status

* Rename utils module to std_ext with inner modules

* Improve doc of `get_or_fetch_block`

Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>

CHANGES.md

@@ -5,10 +5,12 @@ Light Client:
 - Expose latest_trusted from Supervisor Handle ([#394])
 - Turn `Handle` into a trait for ease of integration and testability ([#401])
 - Improve `Supervisor` ergonomics according to [ADR-007] ([#403])
+- Treat `Trusted` status as a special case of `Verified` as per the spec ([#419])
 
 [#394]: https://github.com/informalsystems/tendermint-rs/pull/394
 [#401]: https://github.com/informalsystems/tendermint-rs/pull/401
 [#403]: https://github.com/informalsystems/tendermint-rs/pull/403
+[#419]: https://github.com/informalsystems/tendermint-rs/pull/419
 [ADR-007]: https://github.com/informalsystems/tendermint-rs/blob/master/docs/architecture/adr-007-light-client-supervisor-ergonomics.md
 
 ## [0.14.1] (2020-06-23)

light-client/src/contracts.rs

@@ -12,6 +12,7 @@ pub fn trusted_store_contains_block_at_target_height(
     target_height: Height,
 ) -> bool {
     light_store.get(target_height, Status::Verified).is_some()
+        || light_store.get(target_height, Status::Trusted).is_some()
 }
 
 pub fn is_within_trust_period(
@@ -29,15 +30,6 @@ pub fn light_store_contains_block_within_trusting_period(
     now: Time,
 ) -> bool {
     light_store
-        .all(Status::Verified)
+        .all(Status::Trusted)
         .any(|lb| is_within_trust_period(&lb, trusting_period, now))
 }
-
-// pub fn target_height_greater_than_all_blocks_in_trusted_store(
-//     light_store: &dyn LightStore,
-//     target_height: Height,
-// ) -> bool {
-//     light_store
-//         .all(Status::Verified)
-//         .all(|lb| lb.height() < target_height)
-// }

light-client/src/errors.rs

@@ -37,7 +37,7 @@ pub enum ErrorKind {
     ForkDetected(Vec<PeerId>),
 
     #[error("no initial trusted state")]
-    NoInitialTrustedState(Status),
+    NoInitialTrustedState,
 
     #[error("no trusted state")]
     NoTrustedState(Status),
@@ -52,7 +52,6 @@ pub enum ErrorKind {
     TrustedStateOutsideTrustingPeriod {
         trusted_state: Box<LightBlock>,
         options: Options,
-        status: Status,
     },
 
     #[error("bisection for target at height {0} failed when reached trusted state at height {1}")]

light-client/src/fork_detector.rs

@@ -92,7 +92,7 @@ impl ForkDetector for ProdForkDetector {
         for witness in witnesses {
             let mut state = State::new(MemoryStore::new());
 
-            let witness_block = witness
+            let (witness_block, _) = witness
                 .light_client
                 .get_or_fetch_block(verified_block.height(), &mut state)?;
 

light-client/src/lib.rs

@@ -22,6 +22,7 @@ pub mod operations;
 pub mod peer_list;
 pub mod predicates;
 pub mod state;
+mod std_ext;
 pub mod store;
 pub mod supervisor;
 pub mod types;

light-client/src/light_client.rs

@@ -154,7 +154,7 @@ impl LightClient {
         state: &mut State,
     ) -> Result<LightBlock, Error> {
         // Let's first look in the store to see whether we have already successfully verified this block
-        if let Some(light_block) = state.light_store.get(target_height, Status::Verified) {
+        if let Some(light_block) = state.light_store.get_trusted_or_verified(target_height) {
             return Ok(light_block);
         }
 
@@ -169,7 +169,7 @@ impl LightClient {
             let trusted_state = state
                 .light_store
                 .latest_trusted_or_verified()
-                .ok_or_else(|| ErrorKind::NoInitialTrustedState(Status::Verified))?;
+                .ok_or_else(|| ErrorKind::NoInitialTrustedState)?;
 
             if target_height < trusted_state.height() {
                 bail!(ErrorKind::TargetLowerThanTrustedState {
@@ -183,7 +183,6 @@ impl LightClient {
                 bail!(ErrorKind::TrustedStateOutsideTrustingPeriod {
                     trusted_state: Box::new(trusted_state),
                     options,
-                    status: Status::Verified
                 });
             }
 
@@ -195,8 +194,9 @@ impl LightClient {
                 return Ok(trusted_state);
             }
 
-            // Fetch the block at the current height from our peer
-            let current_block = self.get_or_fetch_block(current_height, state)?;
+            // 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
@@ -205,18 +205,20 @@ impl LightClient {
 
             match verdict {
                 Verdict::Success => {
-                    // Verification succeeded, add the block to the light store with `verified` status
-                    state.light_store.update(&current_block, Status::Verified);
+                    // Verification succeeded, add the block to the light store with
+                    // the `Verified` status or higher if already trusted.
+                    let new_status = Status::most_trusted(Status::Verified, status);
+                    state.light_store.update(&current_block, new_status);
                 }
                 Verdict::Invalid(e) => {
-                    // Verification failed, add the block to the light store with `failed` status, and abort.
+                    // Verification failed, add the block to the light store with `Failed` status, and abort.
                     state.light_store.update(&current_block, Status::Failed);
 
                     bail!(ErrorKind::InvalidLightBlock(e))
                 }
                 Verdict::NotEnoughTrust(_) => {
                     // The current block cannot be trusted because of missing overlap in the validator sets.
-                    // Add the block to the light store with `unverified` status.
+                    // Add the block to the light store with `Unverified` status.
                     // This will engage bisection in an attempt to raise the height of the highest
                     // trusted state until there is enough overlap.
                     state.light_store.update(&current_block, Status::Unverified);
@@ -230,35 +232,33 @@ impl LightClient {
         }
     }
 
-    /// Look in the light store for a block from the given peer at the given height.
-    /// If one cannot be found, fetch the block from the given peer.
+    /// Look in the light store for a block from the given peer at the given height,
+    /// which has not previously failed verification (ie. its status is not `Failed`).
+    ///
+    /// If one cannot be found, fetch the block from the given peer and store
+    /// it in the light store with `Unverified` status.
     ///
     /// ## Postcondition
     /// - The provider of block that is returned matches the given peer.
-    #[post(ret.as_ref().map(|lb| lb.provider == self.peer).unwrap_or(true))]
+    #[post(ret.as_ref().map(|(lb, _)| lb.provider == self.peer).unwrap_or(true))]
     pub fn get_or_fetch_block(
         &self,
-        current_height: Height,
+        height: Height,
         state: &mut State,
-    ) -> Result<LightBlock, Error> {
-        let current_block = state
-            .light_store
-            .get(current_height, Status::Verified)
-            .or_else(|| state.light_store.get(current_height, Status::Unverified));
+    ) -> Result<(LightBlock, Status), Error> {
+        let block = state.light_store.get_non_failed(height);
 
-        if let Some(current_block) = current_block {
-            return Ok(current_block);
+        if let Some(block) = block {
+            return Ok(block);
         }
 
-        self.io
-            .fetch_light_block(self.peer, AtHeight::At(current_height))
-            .map(|current_block| {
-                state
-                    .light_store
-                    .insert(current_block.clone(), Status::Unverified);
+        let block = self
+            .io
+            .fetch_light_block(self.peer, AtHeight::At(height))
+            .map_err(ErrorKind::Io)?;
+
+        state.light_store.insert(block.clone(), Status::Unverified);
 
-                current_block
-            })
-            .map_err(|e| ErrorKind::Io(e).into())
+        Ok((block, Status::Unverified))
     }
 }

light-client/src/std_ext.rs

@@ -0,0 +1,26 @@
+pub mod cmp {
+
+    /// Stable version of `std::cmp::max_by_key`.
+    pub fn max_by_key<A, B: Ord>(a: A, b: A, key: impl Fn(&A) -> B) -> A {
+        if key(&a) >= key(&b) {
+            a
+        } else {
+            b
+        }
+    }
+}
+
+pub mod option {
+
+    /// Choose between two optional values.
+    ///
+    /// If either value is `None`, the other one will be returned.
+    /// If both values are `Some`, then the given function will pick one of the two values.
+    pub fn select<A>(a: Option<A>, b: Option<A>, f: impl FnOnce(A, A) -> A) -> Option<A> {
+        match (a, b) {
+            (None, b) => b,
+            (a, None) => a,
+            (Some(a), Some(b)) => Some(f(a, b)),
+        }
+    }
+}

light-client/src/store.rs

@@ -4,6 +4,7 @@
 //! - a transient, in-memory implementation for testing purposes
 //! - a persistent, on-disk, sled-backed implementation for production
 
+use crate::std_ext;
 use crate::types::{Height, LightBlock, Status};
 
 pub mod memory;
@@ -37,21 +38,36 @@ pub trait LightStore: std::fmt::Debug + Send {
     /// Get an iterator of all light blocks with the given status.
     fn all(&self, status: Status) -> Box<dyn Iterator<Item = LightBlock>>;
 
+    /// Get a block at a given height whatever its verification status as long as it hasn't failed
+    /// verification (ie. its status is not `Status::Failed`).
+    fn get_non_failed(&self, height: Height) -> Option<(LightBlock, Status)> {
+        None.or_else(|| {
+            self.get(height, Status::Trusted)
+                .map(|lb| (lb, Status::Trusted))
+        })
+        .or_else(|| {
+            self.get(height, Status::Verified)
+                .map(|lb| (lb, Status::Verified))
+        })
+        .or_else(|| {
+            self.get(height, Status::Unverified)
+                .map(|lb| (lb, Status::Unverified))
+        })
+    }
+
     /// Get the light block of greatest height with the trusted or verified status.
     fn latest_trusted_or_verified(&self) -> Option<LightBlock> {
         let latest_trusted = self.latest(Status::Trusted);
         let latest_verified = self.latest(Status::Verified);
 
-        match (latest_trusted, latest_verified) {
-            (None, latest_verified) => latest_verified,
-            (latest_trusted, None) => latest_trusted,
-            (Some(latest_trusted), Some(latest_verified)) => {
-                if latest_trusted.height() > latest_verified.height() {
-                    Some(latest_trusted)
-                } else {
-                    Some(latest_verified)
-                }
-            }
-        }
+        std_ext::option::select(latest_trusted, latest_verified, |t, v| {
+            std_ext::cmp::max_by_key(t, v, |lb| lb.height())
+        })
+    }
+
+    /// Get the light block of the given height with the trusted or verified status.
+    fn get_trusted_or_verified(&self, height: Height) -> Option<LightBlock> {
+        self.get(height, Status::Trusted)
+            .or_else(|| self.get(height, Status::Verified))
     }
 }

light-client/src/types.rs

@@ -49,30 +49,29 @@ pub type SignedHeader = TMSignedHeader;
 pub type TrustedState = LightBlock;
 
 /// Verification status of a light block.
-#[derive(Copy, Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
+#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)]
 pub enum Status {
+    /// The light block has failed verification.
+    Failed,
     /// The light has not been verified yet.
     Unverified,
     /// The light block has been successfully verified.
     Verified,
     /// The light block has been successfully verified and has passed fork detection.
     Trusted,
-    /// The light block has failed verification.
-    Failed,
 }
 
 impl Status {
     /// Return a slice of all the possible values for this enum.
-    pub fn iter() -> &'static [Status] {
-        static ALL: &[Status] = &[
-            Status::Unverified,
-            Status::Verified,
-            Status::Trusted,
-            Status::Failed,
-        ];
-
+    pub fn iter() -> &'static [Self] {
+        use Status::*;
+        static ALL: &[Status] = &[Unverified, Verified, Trusted, Failed];
         ALL
     }
+
+    pub fn most_trusted(a: Self, b: Self) -> Self {
+        std::cmp::max(a, b)
+    }
 }
 
 /// A light block is the core data structure used by the light client.
@@ -116,3 +115,40 @@ impl LightBlock {
         self.signed_header.header.height.into()
     }
 }
+
+#[cfg(test)]
+mod tests {
+
+    mod status {
+        use crate::types::Status;
+        use Status::*;
+
+        #[test]
+        fn ord_impl() {
+            assert!(Trusted > Verified);
+            assert!(Verified > Unverified);
+            assert!(Unverified > Failed);
+        }
+
+        #[test]
+        fn most_trusted() {
+            for (a, b) in cross(Status::iter()) {
+                if a > b {
+                    assert_eq!(Status::most_trusted(a, b), a);
+                } else {
+                    assert_eq!(Status::most_trusted(a, b), b);
+                }
+            }
+        }
+
+        fn cross<T>(xs: &[T]) -> Vec<(T, T)>
+        where
+            T: Copy,
+        {
+            xs.iter()
+                .copied()
+                .flat_map(|y| xs.iter().copied().map(move |x| (x, y)))
+                .collect()
+        }
+    }
+}