Merge branch 'origin/tomas/gats-lifetimes-refactor' (#966)

* origin/tomas/gats-lifetimes-refactor:
  [ci] wasm checksums update
  core/storage: remove redundant `StorageWrite` impl for mut ref
  changelog: #966
  core/storage_api: use GATs to hide lifetime in StorageRead trait
  core/storage/mockDB: remove unused `reverse_order` flag
  [ci] wasm checksums update
  changelog: add #912
  wasm_for_tests: make
  storage: remove rev_iter_prefix, which doesn't work as expected
  test in apps/ledger/storage: test prefix iterators

.changelog/unreleased/bugs/912-remove-prefix-iter.md

@@ -0,0 +1,3 @@
+- Removed 'rev_iter_prefix' from storage API as its implementation
+  depends on RocksDB and it doesn't work as expected.
+  ([#912](https://github.com/anoma/namada/pull/912))

.changelog/unreleased/improvements/966-gats-lifetimes-refactor.md

@@ -0,0 +1,2 @@
+- Hide the explicit lifetime from StorageRead trait.
+  ([#966](https://github.com/anoma/namada/pull/966))

apps/src/lib/node/ledger/storage/mod.rs

@@ -50,10 +50,13 @@ fn new_blake2b() -> Blake2b {
 
 #[cfg(test)]
 mod tests {
+    use borsh::BorshSerialize;
+    use itertools::Itertools;
     use namada::ledger::storage::types;
-    use namada::types::address;
+    use namada::ledger::storage_api;
     use namada::types::chain::ChainId;
     use namada::types::storage::{BlockHash, BlockHeight, Key};
+    use namada::types::{address, storage};
     use proptest::collection::vec;
     use proptest::prelude::*;
     use proptest::test_runner::Config;
@@ -344,4 +347,45 @@ mod tests {
 
         Ok(())
     }
+
+    /// Test the prefix iterator with RocksDB.
+    #[test]
+    fn test_persistent_storage_prefix_iter() {
+        let db_path =
+            TempDir::new().expect("Unable to create a temporary DB directory");
+        let mut storage = PersistentStorage::open(
+            db_path.path(),
+            ChainId::default(),
+            address::nam(),
+            None,
+        );
+
+        let prefix = storage::Key::parse("prefix").unwrap();
+        let mismatched_prefix = storage::Key::parse("different").unwrap();
+        // We'll write sub-key in some random order to check prefix iter's order
+        let sub_keys = [2_i32, 1, i32::MAX, -1, 260, -2, i32::MIN, 5, 0];
+
+        for i in sub_keys.iter() {
+            let key = prefix.push(i).unwrap();
+            let value = i.try_to_vec().unwrap();
+            storage.write(&key, value).unwrap();
+
+            let key = mismatched_prefix.push(i).unwrap();
+            let value = (i / 2).try_to_vec().unwrap();
+            storage.write(&key, value).unwrap();
+        }
+        storage.commit().unwrap();
+
+        // Then try to iterate over their prefix
+        let iter = storage_api::iter_prefix(&storage, &prefix)
+            .unwrap()
+            .map(Result::unwrap);
+
+        // The order has to be sorted by sub-key value
+        let expected = sub_keys
+            .iter()
+            .sorted()
+            .map(|i| (prefix.push(i).unwrap(), *i));
+        itertools::assert_equal(iter, expected);
+    }
 }

apps/src/lib/node/ledger/storage/rocksdb.rs

@@ -891,11 +891,7 @@ impl<'iter> DBIter<'iter> for RocksDB {
         &'iter self,
         prefix: &Key,
     ) -> PersistentPrefixIterator<'iter> {
-        iter_prefix(self, prefix, Direction::Forward)
-    }
-
-    fn rev_iter_prefix(&'iter self, prefix: &Key) -> Self::PrefixIter {
-        iter_prefix(self, prefix, Direction::Reverse)
+        iter_prefix(self, prefix)
     }
 
     fn iter_results(&'iter self) -> PersistentPrefixIterator<'iter> {
@@ -922,7 +918,6 @@ impl<'iter> DBIter<'iter> for RocksDB {
 fn iter_prefix<'iter>(
     db: &'iter RocksDB,
     prefix: &Key,
-    direction: Direction,
 ) -> PersistentPrefixIterator<'iter> {
     let db_prefix = "subspace/".to_owned();
     let prefix = format!("{}{}", db_prefix, prefix);
@@ -937,7 +932,7 @@ fn iter_prefix<'iter>(
     read_opts.set_iterate_upper_bound(upper_prefix);
 
     let iter = db.0.iterator_opt(
-        IteratorMode::From(prefix.as_bytes(), direction),
+        IteratorMode::From(prefix.as_bytes(), Direction::Forward),
         read_opts,
     );
     PersistentPrefixIterator(PrefixIterator::new(iter, db_prefix))

core/src/ledger/storage/mockdb.rs

@@ -446,42 +446,14 @@ impl<'iter> DBIter<'iter> for MockDB {
         let db_prefix = "subspace/".to_owned();
         let prefix = format!("{}{}", db_prefix, prefix);
         let iter = self.0.borrow().clone().into_iter();
-        MockPrefixIterator::new(
-            MockIterator {
-                prefix,
-                iter,
-                reverse_order: false,
-            },
-            db_prefix,
-        )
-    }
-
-    fn rev_iter_prefix(&'iter self, prefix: &Key) -> Self::PrefixIter {
-        let db_prefix = "subspace/".to_owned();
-        let prefix = format!("{}{}", db_prefix, prefix);
-        let iter = self.0.borrow().clone().into_iter();
-        MockPrefixIterator::new(
-            MockIterator {
-                prefix,
-                iter,
-                reverse_order: true,
-            },
-            db_prefix,
-        )
+        MockPrefixIterator::new(MockIterator { prefix, iter }, db_prefix)
     }
 
     fn iter_results(&'iter self) -> MockPrefixIterator {
         let db_prefix = "results/".to_owned();
         let prefix = "results".to_owned();
         let iter = self.0.borrow().clone().into_iter();
-        MockPrefixIterator::new(
-            MockIterator {
-                prefix,
-                iter,
-                reverse_order: false,
-            },
-            db_prefix,
-        )
+        MockPrefixIterator::new(MockIterator { prefix, iter }, db_prefix)
     }
 }
 
@@ -491,8 +463,6 @@ pub struct MockIterator {
     prefix: String,
     /// The concrete iterator
     pub iter: btree_map::IntoIter<String, Vec<u8>>,
-    /// Is the iterator in reverse order?
-    reverse_order: bool,
 }
 
 /// A prefix iterator for the [`MockDB`].
@@ -502,23 +472,12 @@ impl Iterator for MockIterator {
     type Item = Result<KVBytes>;
 
     fn next(&mut self) -> Option<Self::Item> {
-        if self.reverse_order {
-            for (key, val) in (&mut self.iter).rev() {
-                if key.starts_with(&self.prefix) {
-                    return Some(Ok((
-                        Box::from(key.as_bytes()),
-                        Box::from(val.as_slice()),
-                    )));
-                }
-            }
-        } else {
-            for (key, val) in &mut self.iter {
-                if key.starts_with(&self.prefix) {
-                    return Some(Ok((
-                        Box::from(key.as_bytes()),
-                        Box::from(val.as_slice()),
-                    )));
-                }
+        for (key, val) in &mut self.iter {
+            if key.starts_with(&self.prefix) {
+                return Some(Ok((
+                    Box::from(key.as_bytes()),
+                    Box::from(val.as_slice()),
+                )));
             }
         }
         None

core/src/ledger/storage/mod.rs

@@ -305,10 +305,6 @@ pub trait DBIter<'iter> {
     /// ordered by the storage keys.
     fn iter_prefix(&'iter self, prefix: &Key) -> Self::PrefixIter;
 
-    /// Read account subspace key value pairs with the given prefix from the DB,
-    /// reverse ordered by the storage keys.
-    fn rev_iter_prefix(&'iter self, prefix: &Key) -> Self::PrefixIter;
-
     /// Read results subspace key value pairs from the DB
     fn iter_results(&'iter self) -> Self::PrefixIter;
 }
@@ -515,15 +511,6 @@ where
         (self.db.iter_prefix(prefix), prefix.len() as _)
     }
 
-    /// Returns a prefix iterator, reverse ordered by storage keys, and the gas
-    /// cost
-    pub fn rev_iter_prefix(
-        &self,
-        prefix: &Key,
-    ) -> (<D as DBIter<'_>>::PrefixIter, u64) {
-        (self.db.rev_iter_prefix(prefix), prefix.len() as _)
-    }
-
     /// Returns a prefix iterator and the gas cost
     pub fn iter_results(&self) -> (<D as DBIter<'_>>::PrefixIter, u64) {
         (self.db.iter_results(), 0)
@@ -1005,12 +992,15 @@ where
     }
 }
 
-impl<'iter, D, H> StorageRead<'iter> for Storage<D, H>
+impl<D, H> StorageRead for Storage<D, H>
 where
     D: DB + for<'iter_> DBIter<'iter_>,
     H: StorageHasher,
 {
-    type PrefixIter = <D as DBIter<'iter>>::PrefixIter;
+    type PrefixIter<'iter> = <D as DBIter<'iter>>::PrefixIter
+where
+        Self: 'iter
+    ;
 
     fn read_bytes(
         &self,
@@ -1026,23 +1016,16 @@ where
         self.block.tree.has_key(key).into_storage_result()
     }
 
-    fn iter_prefix(
+    fn iter_prefix<'iter>(
         &'iter self,
         prefix: &crate::types::storage::Key,
-    ) -> std::result::Result<Self::PrefixIter, storage_api::Error> {
+    ) -> std::result::Result<Self::PrefixIter<'iter>, storage_api::Error> {
         Ok(self.db.iter_prefix(prefix))
     }
 
-    fn rev_iter_prefix(
+    fn iter_next<'iter>(
         &'iter self,
-        prefix: &crate::types::storage::Key,
-    ) -> std::result::Result<Self::PrefixIter, storage_api::Error> {
-        Ok(self.db.rev_iter_prefix(prefix))
-    }
-
-    fn iter_next(
-        &self,
-        iter: &mut Self::PrefixIter,
+        iter: &mut Self::PrefixIter<'iter>,
     ) -> std::result::Result<Option<(String, Vec<u8>)>, storage_api::Error>
     {
         Ok(iter.next().map(|(key, val, _gas)| (key, val)))
@@ -1119,44 +1102,6 @@ where
     }
 }
 
-impl<D, H> StorageWrite for &mut Storage<D, H>
-where
-    D: DB + for<'iter> DBIter<'iter>,
-    H: StorageHasher,
-{
-    fn write<T: borsh::BorshSerialize>(
-        &mut self,
-        key: &crate::types::storage::Key,
-        val: T,
-    ) -> storage_api::Result<()> {
-        let val = val.try_to_vec().unwrap();
-        self.write_bytes(key, val)
-    }
-
-    fn write_bytes(
-        &mut self,
-        key: &crate::types::storage::Key,
-        val: impl AsRef<[u8]>,
-    ) -> storage_api::Result<()> {
-        let _ = self
-            .db
-            .write_subspace_val(self.block.height, key, val)
-            .into_storage_result()?;
-        Ok(())
-    }
-
-    fn delete(
-        &mut self,
-        key: &crate::types::storage::Key,
-    ) -> storage_api::Result<()> {
-        let _ = self
-            .db
-            .delete_subspace_val(self.block.height, key)
-            .into_storage_result()?;
-        Ok(())
-    }
-}
-
 impl From<MerkleTreeError> for Error {
     fn from(error: MerkleTreeError) -> Self {
         Self::MerkleTreeError(error)

core/src/ledger/storage_api/collections/lazy_map.rs

@@ -323,7 +323,7 @@ where
     /// Returns whether the set contains a value.
     pub fn contains<S>(&self, storage: &S, key: &K) -> Result<bool>
     where
-        S: for<'iter> StorageRead<'iter>,
+        S: StorageRead,
     {
         storage.has_key(&self.get_data_key(key))
     }
@@ -363,7 +363,7 @@ where
     /// map.
     pub fn iter<'iter>(
         &'iter self,
-        storage: &'iter impl StorageRead<'iter>,
+        storage: &'iter impl StorageRead,
     ) -> Result<
         impl Iterator<
             Item = Result<(
@@ -406,7 +406,7 @@ where
         val: V,
     ) -> Result<Option<V>>
     where
-        S: StorageWrite + for<'iter> StorageRead<'iter>,
+        S: StorageWrite + StorageRead,
     {
         let previous = self.get(storage, &key)?;
 
@@ -420,7 +420,7 @@ where
     /// was previously in the map.
     pub fn remove<S>(&self, storage: &mut S, key: &K) -> Result<Option<V>>
     where
-        S: StorageWrite + for<'iter> StorageRead<'iter>,
+        S: StorageWrite + StorageRead,
     {
         let value = self.get(storage, key)?;
 
@@ -433,7 +433,7 @@ where
     /// Returns the value corresponding to the key, if any.
     pub fn get<S>(&self, storage: &S, key: &K) -> Result<Option<V>>
     where
-        S: for<'iter> StorageRead<'iter>,
+        S: StorageRead,
     {
         let data_key = self.get_data_key(key);
         Self::read_key_val(storage, &data_key)
@@ -442,7 +442,7 @@ where
     /// Returns whether the map contains no elements.
     pub fn is_empty<S>(&self, storage: &S) -> Result<bool>
     where
-        S: for<'iter> StorageRead<'iter>,
+        S: StorageRead,
     {
         let mut iter =
             storage_api::iter_prefix_bytes(storage, &self.get_data_prefix())?;
@@ -457,7 +457,7 @@ where
     #[allow(clippy::len_without_is_empty)]
     pub fn len<S>(&self, storage: &S) -> Result<u64>
     where
-        S: for<'iter> StorageRead<'iter>,
+        S: StorageRead,
     {
         let iter =
             storage_api::iter_prefix_bytes(storage, &self.get_data_prefix())?;
@@ -473,7 +473,7 @@ where
     /// map.
     pub fn iter<'iter>(
         &self,
-        storage: &'iter impl StorageRead<'iter>,
+        storage: &'iter impl StorageRead,
     ) -> Result<impl Iterator<Item = Result<(K, V)>> + 'iter> {
         let iter = storage_api::iter_prefix(storage, &self.get_data_prefix())?;
         Ok(iter.map(|key_val_res| {
@@ -493,7 +493,7 @@ where
         storage_key: &storage::Key,
     ) -> Result<Option<V>>
     where
-        S: for<'iter> StorageRead<'iter>,
+        S: StorageRead,
     {
         let res = storage.read(storage_key)?;
         Ok(res)