storage: add key sorting (#198)

* database: modify `trait Key`, don't blanket impl

* heed: create `KeyHeed<T>` wrapper type

* fix backend/tests

* blockchain: `impl Key PreRctOutputId`

* database: `StorableStr`, docs, tests

* key: docs, cleanup

* fixes

* heed: simplify types

* storable: remove doc

* heed: use `INTEGER_KEY` instead of custom compare fn

* add docs, tests

* database: document `create_db` invariant

* key: `Lexicographic` -> `Default`

* redb: fix `clear_db` behavior

* fix docs

storage/blockchain/src/open_tables.rs

@@ -169,8 +169,9 @@ mod test {
         env_inner.open_tables(&tx_ro).unwrap();
     }
 
-    /// Tests that directory [`cuprate_database::ConcreteEnv`]
-    /// usage does NOT create all tables.
+    /// Tests that direct usage of
+    /// [`cuprate_database::ConcreteEnv`]
+    /// does NOT create all tables.
     #[test]
     #[should_panic(expected = "`Result::unwrap()` on an `Err` value: TableNotFound")]
     fn test_no_tables_are_created() {

storage/blockchain/src/types.rs

@@ -46,7 +46,7 @@ use bytemuck::{Pod, Zeroable};
 #[cfg(feature = "serde")]
 use serde::{Deserialize, Serialize};
 
-use cuprate_database::StorableVec;
+use cuprate_database::{Key, StorableVec};
 
 //---------------------------------------------------------------------------------------------------- Aliases
 // These type aliases exist as many Monero-related types are the exact same.
@@ -143,6 +143,8 @@ pub struct PreRctOutputId {
     pub amount_index: AmountIndex,
 }
 
+impl Key for PreRctOutputId {}
+
 //---------------------------------------------------------------------------------------------------- BlockInfoV3
 /// Block information.
 ///

storage/database/src/backend/heed/env.rs

@@ -7,26 +7,23 @@ use std::{
     sync::{RwLock, RwLockReadGuard},
 };
 
-use heed::{EnvFlags, EnvOpenOptions};
+use heed::{DatabaseFlags, EnvFlags, EnvOpenOptions};
 
 use crate::{
     backend::heed::{
         database::{HeedTableRo, HeedTableRw},
+        storable::StorableHeed,
         types::HeedDb,
     },
     config::{Config, SyncMode},
     database::{DatabaseIter, DatabaseRo, DatabaseRw},
     env::{Env, EnvInner},
     error::{InitError, RuntimeError},
+    key::{Key, KeyCompare},
     resize::ResizeAlgorithm,
     table::Table,
 };
 
-//---------------------------------------------------------------------------------------------------- Consts
-/// Panic message when there's a table missing.
-const PANIC_MSG_MISSING_TABLE: &str =
-    "cuprate_database::Env should uphold the invariant that all tables are already created";
-
 //---------------------------------------------------------------------------------------------------- ConcreteEnv
 /// A strongly typed, concrete database environment, backed by `heed`.
 pub struct ConcreteEnv {
@@ -268,6 +265,10 @@ where
         tx_ro: &heed::RoTxn<'env>,
     ) -> Result<impl DatabaseRo<T> + DatabaseIter<T>, RuntimeError> {
         // Open up a read-only database using our table's const metadata.
+        //
+        // INVARIANT: LMDB caches the ordering / comparison function from [`EnvInner::create_db`],
+        // and we're relying on that since we aren't setting that here.
+        // <https://github.com/Cuprate/cuprate/pull/198#discussion_r1659422277>
         Ok(HeedTableRo {
             db: self
                 .open_database(tx_ro, Some(T::NAME))?
@@ -282,15 +283,44 @@ where
         tx_rw: &RefCell<heed::RwTxn<'env>>,
     ) -> Result<impl DatabaseRw<T>, RuntimeError> {
         // Open up a read/write database using our table's const metadata.
+        //
+        // INVARIANT: LMDB caches the ordering / comparison function from [`EnvInner::create_db`],
+        // and we're relying on that since we aren't setting that here.
+        // <https://github.com/Cuprate/cuprate/pull/198#discussion_r1659422277>
         Ok(HeedTableRw {
             db: self.create_database(&mut tx_rw.borrow_mut(), Some(T::NAME))?,
             tx_rw,
         })
     }
 
     fn create_db<T: Table>(&self, tx_rw: &RefCell<heed::RwTxn<'env>>) -> Result<(), RuntimeError> {
-        // INVARIANT: `heed` creates tables with `open_database` if they don't exist.
-        self.open_db_rw::<T>(tx_rw)?;
+        // Create a database using our:
+        // - [`Table`]'s const metadata.
+        // - (potentially) our [`Key`] comparison function
+        let mut tx_rw = tx_rw.borrow_mut();
+        let mut db = self.database_options();
+        db.name(T::NAME);
+
+        // Set the key comparison behavior.
+        match <T::Key>::KEY_COMPARE {
+            // Use LMDB's default comparison function.
+            KeyCompare::Default => {
+                db.create(&mut tx_rw)?;
+            }
+
+            // Instead of setting a custom [`heed::Comparator`],
+            // use this LMDB flag; it is ~10% faster.
+            KeyCompare::Number => {
+                db.flags(DatabaseFlags::INTEGER_KEY).create(&mut tx_rw)?;
+            }
+
+            // Use a custom comparison function if specified.
+            KeyCompare::Custom(_) => {
+                db.key_comparator::<StorableHeed<T::Key>>()
+                    .create(&mut tx_rw)?;
+            }
+        }
+
         Ok(())
     }
 
@@ -301,18 +331,18 @@ where
     ) -> Result<(), RuntimeError> {
         let tx_rw = tx_rw.get_mut();
 
-        // Open the table first...
+        // Open the table. We don't care about flags or key
+        // comparison behavior since we're clearing it anyway.
         let db: HeedDb<T::Key, T::Value> = self
             .open_database(tx_rw, Some(T::NAME))?
-            .expect(PANIC_MSG_MISSING_TABLE);
+            .ok_or(RuntimeError::TableNotFound)?;
+
+        db.clear(tx_rw)?;
 
-        // ...then clear it.
-        Ok(db.clear(tx_rw)?)
+        Ok(())
     }
 }
 
 //---------------------------------------------------------------------------------------------------- Tests
 #[cfg(test)]
-mod test {
-    // use super::*;
-}
+mod tests {}

storage/database/src/backend/heed/storable.rs

@@ -1,11 +1,11 @@
 //! `cuprate_database::Storable` <-> `heed` serde trait compatibility layer.
 
 //---------------------------------------------------------------------------------------------------- Use
-use std::{borrow::Cow, marker::PhantomData};
+use std::{borrow::Cow, cmp::Ordering, marker::PhantomData};
 
 use heed::{BoxedError, BytesDecode, BytesEncode};
 
-use crate::storable::Storable;
+use crate::{storable::Storable, Key};
 
 //---------------------------------------------------------------------------------------------------- StorableHeed
 /// The glue struct that implements `heed`'s (de)serialization
@@ -16,7 +16,19 @@ pub(super) struct StorableHeed<T>(PhantomData<T>)
 where
     T: Storable + ?Sized;
 
-//---------------------------------------------------------------------------------------------------- BytesDecode
+//---------------------------------------------------------------------------------------------------- Key
+// If `Key` is also implemented, this can act as the comparison function.
+impl<T> heed::Comparator for StorableHeed<T>
+where
+    T: Key,
+{
+    #[inline]
+    fn compare(a: &[u8], b: &[u8]) -> Ordering {
+        <T as Key>::KEY_COMPARE.as_compare_fn::<T>()(a, b)
+    }
+}
+
+//---------------------------------------------------------------------------------------------------- BytesDecode/Encode
 impl<'a, T> BytesDecode<'a> for StorableHeed<T>
 where
     T: Storable + 'static,
@@ -30,7 +42,6 @@ where
     }
 }
 
-//---------------------------------------------------------------------------------------------------- BytesEncode
 impl<'a, T> BytesEncode<'a> for StorableHeed<T>
 where
     T: Storable + ?Sized + 'a,
@@ -57,6 +68,42 @@ mod test {
     // - simplify trait bounds
     // - make sure the right function is being called
 
+    #[test]
+    /// Assert key comparison behavior is correct.
+    fn compare() {
+        fn test<T>(left: T, right: T, expected: Ordering)
+        where
+            T: Key + Ord + 'static,
+        {
+            println!("left: {left:?}, right: {right:?}, expected: {expected:?}");
+            assert_eq!(
+                <StorableHeed::<T> as heed::Comparator>::compare(
+                    &<StorableHeed::<T> as heed::BytesEncode>::bytes_encode(&left).unwrap(),
+                    &<StorableHeed::<T> as heed::BytesEncode>::bytes_encode(&right).unwrap()
+                ),
+                expected
+            );
+        }
+
+        // Value comparison
+        test::<u8>(0, 255, Ordering::Less);
+        test::<u16>(0, 256, Ordering::Less);
+        test::<u32>(0, 256, Ordering::Less);
+        test::<u64>(0, 256, Ordering::Less);
+        test::<u128>(0, 256, Ordering::Less);
+        test::<usize>(0, 256, Ordering::Less);
+        test::<i8>(-1, 2, Ordering::Less);
+        test::<i16>(-1, 2, Ordering::Less);
+        test::<i32>(-1, 2, Ordering::Less);
+        test::<i64>(-1, 2, Ordering::Less);
+        test::<i128>(-1, 2, Ordering::Less);
+        test::<isize>(-1, 2, Ordering::Less);
+
+        // Byte comparison
+        test::<[u8; 2]>([1, 1], [1, 0], Ordering::Greater);
+        test::<[u8; 3]>([1, 2, 3], [1, 2, 3], Ordering::Equal);
+    }
+
     #[test]
     /// Assert `BytesEncode::bytes_encode` is accurate.
     fn bytes_encode() {

storage/database/src/backend/heed/types.rs

@@ -5,4 +5,7 @@ use crate::backend::heed::storable::StorableHeed;
 
 //---------------------------------------------------------------------------------------------------- Types
 /// The concrete database type for `heed`, usable for reads and writes.
+//
+//                                            Key type        Value type
+//                                               v                v
 pub(super) type HeedDb<K, V> = heed::Database<StorableHeed<K>, StorableHeed<V>>;

storage/database/src/backend/redb/env.rs

@@ -189,7 +189,10 @@ where
         // 3. So it's not being used to open a table since that needs `&tx_rw`
         //
         // Reader-open tables do not affect this, if they're open the below is still OK.
-        redb::WriteTransaction::delete_table(tx_rw, table)?;
+        if !redb::WriteTransaction::delete_table(tx_rw, table)? {
+            return Err(RuntimeError::TableNotFound);
+        }
+
         // Re-create the table.
         // `redb` creates tables if they don't exist, so this should never panic.
         redb::WriteTransaction::open_table(tx_rw, table)?;
@@ -200,6 +203,4 @@ where
 
 //---------------------------------------------------------------------------------------------------- Tests
 #[cfg(test)]
-mod test {
-    // use super::*;
-}
+mod tests {}

storage/database/src/backend/redb/storable.rs

@@ -25,7 +25,7 @@ where
 {
     #[inline]
     fn compare(left: &[u8], right: &[u8]) -> Ordering {
-        <T as Key>::compare(left, right)
+        <T as Key>::KEY_COMPARE.as_compare_fn::<T>()(left, right)
     }
 }
 
@@ -93,8 +93,21 @@ mod test {
             );
         }
 
-        test::<i64>(-1, 2, Ordering::Greater); // bytes are greater, not the value
-        test::<u64>(0, 1, Ordering::Less);
+        // Value comparison
+        test::<u8>(0, 255, Ordering::Less);
+        test::<u16>(0, 256, Ordering::Less);
+        test::<u32>(0, 256, Ordering::Less);
+        test::<u64>(0, 256, Ordering::Less);
+        test::<u128>(0, 256, Ordering::Less);
+        test::<usize>(0, 256, Ordering::Less);
+        test::<i8>(-1, 2, Ordering::Less);
+        test::<i16>(-1, 2, Ordering::Less);
+        test::<i32>(-1, 2, Ordering::Less);
+        test::<i64>(-1, 2, Ordering::Less);
+        test::<i128>(-1, 2, Ordering::Less);
+        test::<isize>(-1, 2, Ordering::Less);
+
+        // Byte comparison
         test::<[u8; 2]>([1, 1], [1, 0], Ordering::Greater);
         test::<[u8; 3]>([1, 2, 3], [1, 2, 3], Ordering::Equal);
     }

storage/database/src/backend/tests.rs

@@ -156,6 +156,20 @@ fn non_manual_resize_2() {
     env.current_map_size();
 }
 
+/// Tests that [`EnvInner::clear_db`] will return
+/// [`RuntimeError::TableNotFound`] if the table doesn't exist.
+#[test]
+fn clear_db_table_not_found() {
+    let (env, _tmpdir) = tmp_concrete_env();
+    let env_inner = env.env_inner();
+    let mut tx_rw = env_inner.tx_rw().unwrap();
+    let err = env_inner.clear_db::<TestTable>(&mut tx_rw).unwrap_err();
+    assert!(matches!(err, RuntimeError::TableNotFound));
+
+    env_inner.create_db::<TestTable>(&tx_rw).unwrap();
+    env_inner.clear_db::<TestTable>(&mut tx_rw).unwrap();
+}
+
 /// Test all `DatabaseR{o,w}` operations.
 #[test]
 fn db_read_write() {
@@ -165,11 +179,11 @@ fn db_read_write() {
     let mut table = env_inner.open_db_rw::<TestTable>(&tx_rw).unwrap();
 
     /// The (1st) key.
-    const KEY: u8 = 0;
+    const KEY: u32 = 0;
     /// The expected value.
     const VALUE: u64 = 0;
     /// How many `(key, value)` pairs will be inserted.
-    const N: u8 = 100;
+    const N: u32 = 100;
 
     /// Assert a u64 is the same as `VALUE`.
     fn assert_value(value: u64) {
@@ -323,19 +337,35 @@ fn db_read_write() {
 
 /// Assert that `key`'s in database tables are sorted in
 /// an ordered B-Tree fashion, i.e. `min_value -> max_value`.
+///
+/// And that it is true for integers, e.g. `0` -> `10`.
 #[test]
 fn tables_are_sorted() {
     let (env, _tmp) = tmp_concrete_env();
     let env_inner = env.env_inner();
+
+    /// Range of keys to insert, `{0, 1, 2 ... 256}`.
+    const RANGE: std::ops::Range<u32> = 0..257;
+
+    // Create tables and set flags / comparison flags.
+    {
+        let tx_rw = env_inner.tx_rw().unwrap();
+        env_inner.create_db::<TestTable>(&tx_rw).unwrap();
+        TxRw::commit(tx_rw).unwrap();
+    }
+
     let tx_rw = env_inner.tx_rw().unwrap();
     let mut table = env_inner.open_db_rw::<TestTable>(&tx_rw).unwrap();
 
-    // Insert `{5, 4, 3, 2, 1, 0}`, assert each new
-    // number inserted is the minimum `first()` value.
-    for key in (0..6).rev() {
-        table.put(&key, &123).unwrap();
+    // Insert range, assert each new
+    // number inserted is the minimum `last()` value.
+    for key in RANGE {
+        table.put(&key, &0).unwrap();
+        table.contains(&key).unwrap();
         let (first, _) = table.first().unwrap();
-        assert_eq!(first, key);
+        let (last, _) = table.last().unwrap();
+        println!("first: {first}, last: {last}, key: {key}");
+        assert_eq!(last, key);
     }
 
     drop(table);
@@ -348,7 +378,7 @@ fn tables_are_sorted() {
         let table = env_inner.open_db_ro::<TestTable>(&tx_ro).unwrap();
         let iter = table.iter().unwrap();
         let keys = table.keys().unwrap();
-        for ((i, iter), key) in (0..6).zip(iter).zip(keys) {
+        for ((i, iter), key) in RANGE.zip(iter).zip(keys) {
             let (iter, _) = iter.unwrap();
             let key = key.unwrap();
             assert_eq!(i, iter);
@@ -359,14 +389,14 @@ fn tables_are_sorted() {
     let mut table = env_inner.open_db_rw::<TestTable>(&tx_rw).unwrap();
 
     // Assert the `first()` values are the minimum, i.e. `{0, 1, 2}`
-    for key in 0..3 {
+    for key in [0, 1, 2] {
         let (first, _) = table.first().unwrap();
         assert_eq!(first, key);
         table.delete(&key).unwrap();
     }
 
-    // Assert the `last()` values are the maximum, i.e. `{5, 4, 3}`
-    for key in (3..6).rev() {
+    // Assert the `last()` values are the maximum, i.e. `{256, 255, 254}`
+    for key in [256, 255, 254] {
         let (last, _) = table.last().unwrap();
         assert_eq!(last, key);
         table.delete(&key).unwrap();