remove reverse prefix iter

.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))

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)
@@ -1033,13 +1020,6 @@ where
         Ok(self.db.iter_prefix(prefix))
     }
 
-    fn rev_iter_prefix(
-        &'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,

core/src/ledger/storage_api/mod.rs

@@ -68,16 +68,6 @@ pub trait StorageRead<'iter> {
         prefix: &storage::Key,
     ) -> Result<Self::PrefixIter>;
 
-    /// Storage prefix iterator in reverse order of the storage keys. It will
-    /// try to get an iterator from the storage.
-    ///
-    /// For a more user-friendly iterator API, use [`fn@rev_iter_prefix`] or
-    /// [`fn@rev_iter_prefix_bytes`] instead.
-    fn rev_iter_prefix(
-        &'iter self,
-        prefix: &storage::Key,
-    ) -> Result<Self::PrefixIter>;
-
     /// Storage prefix iterator. It will try to read from the storage.
     fn iter_next(
         &self,
@@ -195,71 +185,3 @@ where
     });
     Ok(iter)
 }
-
-/// Iterate items matching the given prefix, reverse ordered by the storage
-/// keys.
-pub fn rev_iter_prefix_bytes<'a>(
-    storage: &'a impl StorageRead<'a>,
-    prefix: &crate::types::storage::Key,
-) -> Result<impl Iterator<Item = Result<(storage::Key, Vec<u8>)>> + 'a> {
-    let iter = storage.rev_iter_prefix(prefix)?;
-    let iter = itertools::unfold(iter, |iter| {
-        match storage.iter_next(iter) {
-            Ok(Some((key, val))) => {
-                let key = match storage::Key::parse(key).into_storage_result() {
-                    Ok(key) => key,
-                    Err(err) => {
-                        // Propagate key encoding errors into Iterator's Item
-                        return Some(Err(err));
-                    }
-                };
-                Some(Ok((key, val)))
-            }
-            Ok(None) => None,
-            Err(err) => {
-                // Propagate `iter_next` errors into Iterator's Item
-                Some(Err(err))
-            }
-        }
-    });
-    Ok(iter)
-}
-
-/// Iterate Borsh encoded items matching the given prefix, reverse ordered by
-/// the storage keys.
-pub fn rev_iter_prefix<'a, T>(
-    storage: &'a impl StorageRead<'a>,
-    prefix: &crate::types::storage::Key,
-) -> Result<impl Iterator<Item = Result<(storage::Key, T)>> + 'a>
-where
-    T: BorshDeserialize,
-{
-    let iter = storage.rev_iter_prefix(prefix)?;
-    let iter = itertools::unfold(iter, |iter| {
-        match storage.iter_next(iter) {
-            Ok(Some((key, val))) => {
-                let key = match storage::Key::parse(key).into_storage_result() {
-                    Ok(key) => key,
-                    Err(err) => {
-                        // Propagate key encoding errors into Iterator's Item
-                        return Some(Err(err));
-                    }
-                };
-                let val = match T::try_from_slice(&val).into_storage_result() {
-                    Ok(val) => val,
-                    Err(err) => {
-                        // Propagate val encoding errors into Iterator's Item
-                        return Some(Err(err));
-                    }
-                };
-                Some(Ok((key, val)))
-            }
-            Ok(None) => None,
-            Err(err) => {
-                // Propagate `iter_next` errors into Iterator's Item
-                Some(Err(err))
-            }
-        }
-    });
-    Ok(iter)
-}

core/src/ledger/vp_env.rs

@@ -70,13 +70,6 @@ pub trait VpEnv<'view> {
         prefix: &Key,
     ) -> Result<Self::PrefixIter, storage_api::Error>;
 
-    /// Storage prefix iterator, reverse ordered by storage keys. It will try to
-    /// get an iterator from the storage.
-    fn rev_iter_prefix(
-        &self,
-        prefix: &Key,
-    ) -> Result<Self::PrefixIter, storage_api::Error>;
-
     /// Evaluate a validity predicate with given data. The address, changed
     /// storage keys and verifiers will have the same values as the input to
     /// caller's validity predicate.

shared/src/ledger/native_vp/mod.rs

@@ -203,13 +203,6 @@ where
         .into_storage_result()
     }
 
-    fn rev_iter_prefix(
-        &self,
-        prefix: &crate::types::storage::Key,
-    ) -> storage_api::Result<Self::PrefixIter> {
-        self.ctx.rev_iter_prefix(prefix).into_storage_result()
-    }
-
     fn iter_next(
         &self,
         iter: &mut Self::PrefixIter,
@@ -291,13 +284,6 @@ where
         .into_storage_result()
     }
 
-    fn rev_iter_prefix(
-        &self,
-        prefix: &crate::types::storage::Key,
-    ) -> storage_api::Result<Self::PrefixIter> {
-        self.ctx.rev_iter_prefix(prefix).into_storage_result()
-    }
-
     fn iter_next(
         &self,
         iter: &mut Self::PrefixIter,
@@ -450,18 +436,6 @@ where
         .into_storage_result()
     }
 
-    fn rev_iter_prefix(
-        &self,
-        prefix: &Key,
-    ) -> Result<Self::PrefixIter, storage_api::Error> {
-        vp_host_fns::rev_iter_prefix(
-            &mut self.gas_meter.borrow_mut(),
-            self.storage,
-            prefix,
-        )
-        .into_storage_result()
-    }
-
     fn eval(
         &self,
         vp_code: Vec<u8>,

shared/src/ledger/vp_host_fns.rs

@@ -311,22 +311,6 @@ where
     Ok(iter)
 }
 
-/// Storage prefix iterator, reverse ordered by storage keys. It will try to get
-/// an iterator from the storage.
-pub fn rev_iter_prefix<'a, DB, H>(
-    gas_meter: &mut VpGasMeter,
-    storage: &'a Storage<DB, H>,
-    prefix: &Key,
-) -> EnvResult<<DB as storage::DBIter<'a>>::PrefixIter>
-where
-    DB: storage::DB + for<'iter> storage::DBIter<'iter>,
-    H: StorageHasher,
-{
-    let (iter, gas) = storage.rev_iter_prefix(prefix);
-    add_gas(gas_meter, gas)?;
-    Ok(iter)
-}
-
 /// Storage prefix iterator for prior state (before tx execution). It will try
 /// to read from the storage.
 pub fn iter_pre_next<DB>(