Merge branch 'tomas+brent/fix-iter-prefix' into tomas+brent/lazy-map-…

…epoched-MOVE-WRITELOG

* tomas+brent/fix-iter-prefix:
  storage: extend iter_prefix test
  storage: rename `fn commit` to `commit_block`
  write_log: allow to commit genesis txs
  write_log: add prefix iter
  shared: Add WlStorage, combining WriteLog with Storage
  core/storage: add a warning to `iter_prefix`
  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))

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

@@ -548,7 +548,7 @@ where
             .commit_block(&mut self.storage)
             .expect("Expected committing block write log success");
         // store the block's data in DB
-        self.storage.commit().unwrap_or_else(|e| {
+        self.storage.commit_block().unwrap_or_else(|e| {
             tracing::error!(
                 "Encountered a storage error while committing a block {:?}",
                 e

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

@@ -50,10 +50,13 @@ fn new_blake2b() -> Blake2b {
 
 #[cfg(test)]
 mod tests {
-    use namada::ledger::storage::types;
-    use namada::types::address;
+    use itertools::Itertools;
+    use namada::ledger::storage::write_log::WriteLog;
+    use namada::ledger::storage::{types, WlStorage};
+    use namada::ledger::storage_api::{self, StorageWrite};
     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;
@@ -129,7 +132,7 @@ mod tests {
         storage
             .write(&key, value_bytes.clone())
             .expect("write failed");
-        storage.commit().expect("commit failed");
+        storage.commit_block().expect("commit failed");
 
         // save the last state and drop the storage
         let root = storage.merkle_root().0;
@@ -184,7 +187,7 @@ mod tests {
                 .expect("write failed");
             expected.push((key.to_string(), value_bytes));
         }
-        storage.commit().expect("commit failed");
+        storage.commit_block().expect("commit failed");
 
         let (iter, gas) = storage.iter_prefix(&prefix);
         assert_eq!(gas, prefix.len() as u64);
@@ -299,7 +302,7 @@ mod tests {
             } else {
                 storage.delete(&key)?;
             }
-            storage.commit()?;
+            storage.commit_block()?;
         }
 
         // 2. We try to read from these heights to check that we get back
@@ -344,4 +347,105 @@ 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 mut write_log = WriteLog::default();
+
+        let mut storage = WlStorage::new(&mut write_log, &mut storage);
+
+        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, 260, -2, 5, 0];
+
+        for i in sub_keys.iter() {
+            let key = prefix.push(i).unwrap();
+            storage.write(&key, i).unwrap();
+
+            let key = mismatched_prefix.push(i).unwrap();
+            storage.write(&key, i / 2).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.clone());
+
+        // Commit genesis state
+        storage.commit_genesis().unwrap();
+
+        // Again, try to iterate over their prefix
+        let iter = storage_api::iter_prefix(&storage, &prefix)
+            .unwrap()
+            .map(Result::unwrap);
+        itertools::assert_equal(iter, expected);
+
+        let more_sub_keys = [1_i32, i32::MIN, -10, 123, i32::MAX, 10];
+        debug_assert!(
+            !more_sub_keys.iter().any(|x| sub_keys.contains(x)),
+            "assuming no repetition"
+        );
+        for i in more_sub_keys.iter() {
+            let key = prefix.push(i).unwrap();
+            storage.write(&key, i).unwrap();
+
+            let key = mismatched_prefix.push(i).unwrap();
+            storage.write(&key, i / 2).unwrap();
+        }
+
+        let iter = storage_api::iter_prefix(&storage, &prefix)
+            .unwrap()
+            .map(Result::unwrap);
+
+        // The order has to be sorted by sub-key value
+        let merged = itertools::merge(sub_keys.iter(), more_sub_keys.iter());
+        let expected = merged
+            .clone()
+            .sorted()
+            .map(|i| (prefix.push(i).unwrap(), *i));
+        itertools::assert_equal(iter, expected);
+
+        // Delete some keys
+        let delete_keys = [2, 0, -10, 123];
+        for i in delete_keys.iter() {
+            let key = prefix.push(i).unwrap();
+            storage.delete(&key).unwrap()
+        }
+
+        // Check that iter_prefix doesn't return deleted keys anymore
+        let iter = storage_api::iter_prefix(&storage, &prefix)
+            .unwrap()
+            .map(Result::unwrap);
+        let expected = merged
+            .filter(|x| !delete_keys.contains(x))
+            .sorted()
+            .map(|i| (prefix.push(i).unwrap(), *i));
+        itertools::assert_equal(iter, expected.clone());
+
+        // Commit genesis state
+        storage.commit_genesis().unwrap();
+
+        // And check again
+        let iter = storage_api::iter_prefix(&storage, &prefix)
+            .unwrap()
+            .map(Result::unwrap);
+        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

@@ -302,14 +302,14 @@ pub trait DBIter<'iter> {
     /// The concrete type of the iterator
     type PrefixIter: Debug + Iterator<Item = (String, Vec<u8>, u64)>;
 
+    /// WARNING: This only works for values that have been committed to DB.
+    /// To be able to see values written or deleted, but not yet committed,
+    /// use the `StorageWithWriteLog`.
+    ///
     /// Read account subspace key value pairs with the given prefix from the DB,
     /// 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;
 }
@@ -434,7 +434,7 @@ where
     }
 
     /// Persist the current block's state to the database
-    pub fn commit(&mut self) -> Result<()> {
+    pub fn commit_block(&mut self) -> Result<()> {
         let state = BlockStateWrite {
             merkle_tree_stores: self.block.tree.stores(),
             header: self.header.as_ref(),
@@ -508,23 +508,18 @@ where
         }
     }
 
-    /// Returns a prefix iterator, ordered by storage keys, and the gas cost
+    /// WARNING: This only works for values that have been committed to DB.
+    /// To be able to see values written or deleted, but not yet committed,
+    /// use the `StorageWithWriteLog`.
+    ///
+    /// Returns a prefix iterator, ordered by storage keys, and the gas cost.
     pub fn iter_prefix(
         &self,
         prefix: &Key,
     ) -> (<D as DBIter<'_>>::PrefixIter, u64) {
         (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)
@@ -1034,13 +1029,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,