Improve sled-based light store (#773)

* Encode heights used as keys in sled-based light store in big-endian representation

One needs to represent the keys in big-endian form for
sled's iterators and ordered operations to work properly.

See [1] and [2] for more information.

[1] https://github.com/spacejam/sled#a-note-on-lexicographic-ordering-and-endianness
[2] https://github.com/spacejam/sled/blob/aa3fcf37d844e2d1382e0a58267dc3b3ae3b25aa/examples/structured.rs#L23-L30

* Use sled trees instead of key prefixes

* Use next_back instead of max_by to get the latest block in the light store

This avoids a full linear scan, and is even likely constant time.

* Rename LightStore::latest* to highest* and introduce LightStore::lowest

* Feature-guard the sled-based lightstore

* Hide sled dependency by introducing SledStore::open

* Update changelog

* Formatting

* Add LightStore::lowest_trusted_or_verified

* Add more tests

CHANGELOG.md

@@ -11,11 +11,14 @@
 
 ### BUG FIXES
 
+* `[light-client]` Fix potential block ordering problem with sled-based lightstore ([#769])
+* `[light-client]` Improve the API of the light store. ([#428])
 * `[light-client]` The `sled`-backed lightstore is now feature-guarded under
-   the `lightstore-sled` feature, which is enabled by default for now. ([#772])
+   the `lightstore-sled` feature, which is enabled by default for now. ([#428])
 
+[#769]: https://github.com/informalsystems/tendermint-rs/issues/769
 [#764]: https://github.com/informalsystems/tendermint-rs/issues/764
-[#772]: https://github.com/informalsystems/tendermint-rs/pull/772
+[#428]: https://github.com/informalsystems/tendermint-rs/issues/428
 
 ## v0.17.1
 
@@ -36,8 +39,8 @@ Client's correct and reliable operation.
 
 ### BUG FIXES:
 
-- `[tendermint]` `Time` values were not always formatted properly,
-  causing the light client to sometimes return malformed light blocks. ([#774])
+* `[tendermint]` `Time` values were not always formatted properly,
+  causing the light client to sometimes return malformed light blocks ([#774])
 
 [#774]: https://github.com/informalsystems/tendermint-rs/issues/774
 

light-client/Cargo.toml

@@ -57,3 +57,4 @@ tendermint-testgen = { path = "../testgen"}
 serde_json = "1.0.51"
 gumdrop = "0.8.0"
 rand = "0.7.3"
+tempdir = "0.3.7"

light-client/src/builder/light_client.rs

@@ -133,7 +133,7 @@ impl LightClientBuilder<NoTrustedState> {
     pub fn trust_from_store(self) -> Result<LightClientBuilder<HasTrustedState>, Error> {
         let trusted_state = self
             .light_store
-            .latest_trusted_or_verified()
+            .highest_trusted_or_verified()
             .ok_or(error::Kind::NoTrustedStateInStore)?;
 
         self.trust_light_block(trusted_state)

light-client/src/components/scheduler.rs

@@ -20,7 +20,7 @@ pub trait Scheduler: Send + Sync {
     ///
     /// ## Postcondition
     /// - The resulting height must be valid according to `valid_schedule`. [LCV-SCHEDULE-POST.1]
-    #[pre(light_store.latest_trusted_or_verified().is_some())]
+    #[pre(light_store.highest_trusted_or_verified().is_some())]
     #[post(valid_schedule(ret, target_height, current_height, light_store))]
     fn schedule(
         &self,
@@ -53,15 +53,15 @@ where
 ///
 /// ## Postcondition
 /// - The resulting height must be valid according to `valid_schedule`. [LCV-SCHEDULE-POST.1]
-#[pre(light_store.latest_trusted_or_verified().is_some())]
+#[pre(light_store.highest_trusted_or_verified().is_some())]
 #[post(valid_schedule(ret, target_height, current_height, light_store))]
 pub fn basic_bisecting_schedule(
     light_store: &dyn LightStore,
     current_height: Height,
     target_height: Height,
 ) -> Height {
     let trusted_height = light_store
-        .latest_trusted_or_verified()
+        .highest_trusted_or_verified()
         .map(|lb| lb.height())
         .unwrap();
 
@@ -105,7 +105,7 @@ pub fn valid_schedule(
     light_store: &dyn LightStore,
 ) -> bool {
     let latest_trusted_height = light_store
-        .latest_trusted_or_verified()
+        .highest_trusted_or_verified()
         .map(|lb| lb.height())
         .unwrap();
 

light-client/src/light_client.rs

@@ -172,7 +172,7 @@ impl LightClient {
             // Get the latest trusted state
             let trusted_state = state
                 .light_store
-                .latest_trusted_or_verified()
+                .highest_trusted_or_verified()
                 .ok_or(ErrorKind::NoInitialTrustedState)?;
 
             if target_height < trusted_state.height() {

light-client/src/store.rs

@@ -39,7 +39,10 @@ pub trait LightStore: Debug + Send + Sync {
     fn remove(&mut self, height: Height, status: Status);
 
     /// Get the light block of greatest height with the given status.
-    fn latest(&self, status: Status) -> Option<LightBlock>;
+    fn highest(&self, status: Status) -> Option<LightBlock>;
+
+    /// Get the light block of lowest height with the given status.
+    fn lowest(&self, status: Status) -> Option<LightBlock>;
 
     /// Get an iterator of all light blocks with the given status.
     fn all(&self, status: Status) -> Box<dyn Iterator<Item = LightBlock>>;
@@ -62,15 +65,25 @@ pub trait LightStore: Debug + Send + Sync {
     }
 
     /// 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);
+    fn highest_trusted_or_verified(&self) -> Option<LightBlock> {
+        let latest_trusted = self.highest(Status::Trusted);
+        let latest_verified = self.highest(Status::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 lowest height with the trusted or verified status.
+    fn lowest_trusted_or_verified(&self) -> Option<LightBlock> {
+        let latest_trusted = self.lowest(Status::Trusted);
+        let latest_verified = self.lowest(Status::Verified);
+
+        std_ext::option::select(latest_trusted, latest_verified, |t, v| {
+            std_ext::cmp::min_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)

light-client/src/store/memory.rs

@@ -65,14 +65,22 @@ impl LightStore for MemoryStore {
         self.insert(light_block.clone(), status);
     }
 
-    fn latest(&self, status: Status) -> Option<LightBlock> {
+    fn highest(&self, status: Status) -> Option<LightBlock> {
         self.store
             .iter()
             .filter(|(_, e)| e.status == status)
             .max_by_key(|(&height, _)| height)
             .map(|(_, e)| e.light_block.clone())
     }
 
+    fn lowest(&self, status: Status) -> Option<LightBlock> {
+        self.store
+            .iter()
+            .filter(|(_, e)| e.status == status)
+            .min_by_key(|(&height, _)| height)
+            .map(|(_, e)| e.light_block.clone())
+    }
+
     fn all(&self, status: Status) -> Box<dyn Iterator<Item = LightBlock>> {
         let light_blocks: Vec<_> = self
             .store

light-client/src/store/sled.rs

@@ -5,26 +5,25 @@ pub mod utils;
 use std::path::Path;
 
 use crate::{
-    store::sled::utils::*,
+    store::sled::utils::HeightIndexedDb,
     types::{Height, LightBlock},
 };
 
 use super::{LightStore, Status};
-use ::sled::Db as SledDb;
 
-const UNVERIFIED_PREFIX: &str = "light_store/unverified";
-const VERIFIED_PREFIX: &str = "light_store/verified";
-const TRUSTED_PREFIX: &str = "light_store/trusted";
-const FAILED_PREFIX: &str = "light_store/failed";
+const UNVERIFIED: &str = "unverified";
+const VERIFIED: &str = "verified";
+const TRUSTED: &str = "trusted";
+const FAILED: &str = "failed";
 
 /// Persistent store backed by an on-disk `sled` database.
 #[derive(Debug, Clone)]
 pub struct SledStore {
-    db: SledDb,
-    unverified_db: KeyValueDb<Height, LightBlock>,
-    verified_db: KeyValueDb<Height, LightBlock>,
-    trusted_db: KeyValueDb<Height, LightBlock>,
-    failed_db: KeyValueDb<Height, LightBlock>,
+    unverified_db: HeightIndexedDb<LightBlock>,
+    verified_db: HeightIndexedDb<LightBlock>,
+    trusted_db: HeightIndexedDb<LightBlock>,
+    failed_db: HeightIndexedDb<LightBlock>,
+    db: sled::Db,
 }
 
 impl SledStore {
@@ -34,17 +33,17 @@ impl SledStore {
     }
 
     /// Create a new persistent store from a sled database that is already open.
-    pub fn new(db: SledDb) -> Self {
+    pub fn new(db: sled::Db) -> Self {
         Self {
+            unverified_db: HeightIndexedDb::new(db.open_tree(UNVERIFIED).unwrap()),
+            verified_db: HeightIndexedDb::new(db.open_tree(VERIFIED).unwrap()),
+            trusted_db: HeightIndexedDb::new(db.open_tree(TRUSTED).unwrap()),
+            failed_db: HeightIndexedDb::new(db.open_tree(FAILED).unwrap()),
             db,
-            unverified_db: KeyValueDb::new(UNVERIFIED_PREFIX),
-            verified_db: KeyValueDb::new(VERIFIED_PREFIX),
-            trusted_db: KeyValueDb::new(TRUSTED_PREFIX),
-            failed_db: KeyValueDb::new(FAILED_PREFIX),
         }
     }
 
-    fn db(&self, status: Status) -> &KeyValueDb<Height, LightBlock> {
+    fn db(&self, status: Status) -> &HeightIndexedDb<LightBlock> {
         match status {
             Status::Unverified => &self.unverified_db,
             Status::Verified => &self.verified_db,
@@ -56,38 +55,95 @@ impl SledStore {
 
 impl LightStore for SledStore {
     fn get(&self, height: Height, status: Status) -> Option<LightBlock> {
-        self.db(status).get(&self.db, &height).ok().flatten()
+        self.db(status).get(height).ok().flatten()
     }
 
     fn update(&mut self, light_block: &LightBlock, status: Status) {
         let height = light_block.height();
 
         for other in Status::iter() {
             if status != *other {
-                self.db(*other).remove(&self.db, &height).ok();
+                self.db(*other).remove(height).ok();
             }
         }
 
-        self.db(status).insert(&self.db, &height, light_block).ok();
+        self.db(status).insert(height, light_block).ok();
     }
 
     fn insert(&mut self, light_block: LightBlock, status: Status) {
         self.db(status)
-            .insert(&self.db, &light_block.height(), &light_block)
+            .insert(light_block.height(), &light_block)
             .ok();
     }
 
     fn remove(&mut self, height: Height, status: Status) {
-        self.db(status).remove(&self.db, &height).ok();
+        self.db(status).remove(height).ok();
     }
 
-    fn latest(&self, status: Status) -> Option<LightBlock> {
-        self.db(status)
-            .iter(&self.db)
-            .max_by(|first, second| first.height().cmp(&second.height()))
+    fn highest(&self, status: Status) -> Option<LightBlock> {
+        self.db(status).iter().next_back()
+    }
+
+    fn lowest(&self, status: Status) -> Option<LightBlock> {
+        self.db(status).iter().next()
     }
 
     fn all(&self, status: Status) -> Box<dyn Iterator<Item = LightBlock>> {
-        Box::new(self.db(status).iter(&self.db))
+        Box::new(self.db(status).iter())
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use tempdir::TempDir;
+    use tendermint_testgen::{light_block::TMLightBlock as TGLightBlock, Generator, LightChain};
+
+    #[test]
+    fn highest_returns_latest_block() {
+        with_blocks(10, |mut db, blocks| {
+            let initial_block = blocks[0].clone();
+            db.insert(initial_block.clone(), Status::Verified);
+            assert_eq!(db.lowest(Status::Verified).as_ref(), Some(&initial_block));
+
+            for block in blocks.into_iter().skip(1) {
+                db.insert(block, Status::Verified);
+                assert_eq!(db.lowest(Status::Verified).as_ref(), Some(&initial_block));
+            }
+        })
+    }
+
+    #[test]
+    fn lowest_returns_earliest_block() {
+        with_blocks(10, |mut db, blocks| {
+            for block in blocks {
+                db.insert(block.clone(), Status::Verified);
+                assert_eq!(db.highest(Status::Verified), Some(block));
+            }
+        })
+    }
+
+    fn with_blocks(height: u64, f: impl FnOnce(SledStore, Vec<LightBlock>)) {
+        let tmp_dir = TempDir::new("tendermint_light_client_sled_test").unwrap();
+        let db = SledStore::open(tmp_dir).unwrap();
+
+        let chain = LightChain::default_with_length(height);
+        let blocks = chain
+            .light_blocks
+            .into_iter()
+            .map(|lb| lb.generate().unwrap())
+            .map(testgen_to_lb)
+            .collect::<Vec<_>>();
+
+        f(db, blocks)
+    }
+
+    fn testgen_to_lb(tm_lb: TGLightBlock) -> LightBlock {
+        LightBlock {
+            signed_header: tm_lb.signed_header,
+            validators: tm_lb.validators,
+            next_validators: tm_lb.next_validators,
+            provider: tm_lb.provider,
+        }
     }
 }