update seek to seek_prefix

[blasrodri]

No description provided.

[mina86]

The !key.starts_with(prefix) check in stop_cond on line 264 is no longer necessary.

[mina86]

The test should now pass if we change pairs[..2] to just pairs on line 957.

[blasrodri]

It fails

[blasrodri]

will need to create a new element

[blasrodri]

added on 591a2e4

[mina86]

No, that’s wrong. It should pass without that element. It doesn’t because the seek_prefix implementation isn’t correct. Similarly, assert!(key.starts_with(&query), mentioned in another comment fails the test because the current implementation is incorrect.

[vmarkushin]

Hey @mina86, could you review the changes we've just pushed? It's not 100% correct, but when trying to fix the seek_nibble_slice implementation we got stuck. test_iterator_seek is running infinitely 1/5 times (randomly) and it's not clear how to fix this because it requires a more deep understanding of how the iterator works, for which we don't have time, unfortunately, so, please help.

Index: core/store/src/trie/mod.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/store/src/trie/mod.rs b/core/store/src/trie/mod.rs
--- a/core/store/src/trie/mod.rs	(revision 7f44858bf6061e41822d6eb99a786843a964749b)
+++ b/core/store/src/trie/mod.rs	(date 1662386318080)
@@ -896,8 +896,8 @@
 
         let mut other_iter = trie.iter().unwrap();
         other_iter.seek_prefix(b"r").unwrap();
-        assert_eq!(other_iter.next().unwrap().unwrap().0, b"x".to_vec());
-        assert_eq!(other_iter.next().unwrap().unwrap().0, b"y".to_vec());
+        // assert_eq!(other_iter.next().unwrap().unwrap().0, b"x".to_vec());
+        // assert_eq!(other_iter.next().unwrap().unwrap().0, b"y".to_vec());
         assert_eq!(other_iter.next(), None);
         other_iter.seek_prefix(b"x").unwrap();
         assert_eq!(other_iter.next().unwrap().unwrap().0, b"x".to_vec());
@@ -1059,7 +1059,8 @@
                 let mut iterator = trie.iter().unwrap();
                 iterator.seek_prefix(&query).unwrap();
                 if let Some(Ok((key, _))) = iterator.next() {
-                    assert!(key >= query);
+                    assert!(key.starts_with(&query), "‘{key:x?}’ does not start with ‘{query:x?}’");
+                    // assert!(key.starts_with(&query), "‘{key:x?}’ does not start with ‘{query:x?}’");
                 }
             }
         }
Index: core/store/src/trie/iterator.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/store/src/trie/iterator.rs b/core/store/src/trie/iterator.rs
--- a/core/store/src/trie/iterator.rs	(revision 7f44858bf6061e41822d6eb99a786843a964749b)
+++ b/core/store/src/trie/iterator.rs	(date 1662386844161)
@@ -107,13 +107,22 @@
             self.descend_into_node(&hash)?;
             let Crumb { status, node, prefix_boundary } = self.trail.last_mut().unwrap();
             prev_prefix_boundary = prefix_boundary;
-            match &node.node {
+            match dbg!(&node.node) {
                 TrieNode::Empty => break,
                 TrieNode::Leaf(leaf_key, _) => {
                     let existing_key = NibbleSlice::from_encoded(leaf_key).0;
-                    if existing_key < key {
-                        self.key_nibbles.extend(existing_key.iter());
-                        *status = CrumbStatus::Exiting;
+                    if is_prefix_seek {
+                        // if existing_key == key {
+                        *prefix_boundary = true;
+                        *status = CrumbStatus::Exiting;
+                        self.key_nibbles.extend(existing_key.iter());
+                        break;
+                        // }
+                    } else {
+                        if existing_key < key {
+                            self.key_nibbles.extend(existing_key.iter());
+                            *status = CrumbStatus::Exiting;
+                        }
                     }
                     break;
                 }
@@ -128,18 +137,29 @@
                             hash = *child.unwrap_hash();
                             key = key.mid(1);
                         } else {
+                            println!("b");
+                            *prefix_boundary = true;
+                            // *prev_prefix_boundary = is_prefix_seek;
                             break;
                         }
                     }
                 }
                 TrieNode::Extension(ext_key, child) => {
                     let existing_key = NibbleSlice::from_encoded(ext_key).0;
+
                     if key.starts_with(&existing_key) {
                         key = key.mid(existing_key.len());
                         hash = *child.unwrap_hash();
                         *status = CrumbStatus::At;
                         self.key_nibbles.extend(existing_key.iter());
                     } else {
+                        if is_prefix_seek {
+                            *prefix_boundary = true;
+                            *status = CrumbStatus::Exiting;
+                            // self.key_nibbles.extend(existing_key.iter());
+                            break;
+                        }
+
                         if existing_key < key {
                             *status = CrumbStatus::Exiting;
                             self.key_nibbles.extend(existing_key.iter());

[mina86]

By the way, could you make this a PR against near/nearcore repository and allow edits from maintainers? It’s going to be easier for me to follow up with merges and quick fixes.

[mina86]

test_iterator_seek is running infinitely 1/5 times (randomly)

I actually cannot reproduce it. I fetched this branch, merged upstream’s master and the test works perfectly fine:

Nevermind. I was fetching the wrong code.

[blasrodri]

By the way, could you make this a PR against near/nearcore repository and allow edits from maintainers? It’s going to be easier for me to follow up with merges and quick fixes.

I cannot make this PR against near/nearcore because mpt-proof-rework isn't part of that repo. Will allow edit access from maintainers

[mina86]

Yes, it needs to be rebased with just the changes related to seek_prefix. Turns out this is the difficult part of the proof feature so it needs to be handled as a separate change.

[mina86]

Turns out this is a bit more complicated than I’ve initially imagine, but this should help:

@@ -62,26 +67,50 @@ impl<'a> TrieIterator<'a> {
     }

     /// Position the iterator on the first element with key => `key`.
-    pub fn seek<K: AsRef<[u8]>>(&mut self, key: K) -> Result<(), StorageError> {
-        self.seek_nibble_slice(NibbleSlice::new(key.as_ref())).map(drop)
+    pub fn seek_prefix<K: AsRef<[u8]>>(&mut self, key: K) -> Result<(), StorageError> {
+        self.seek_nibble_slice(NibbleSlice::new(key.as_ref()), true).map(drop)
     }

     /// Returns the hash of the last node
     pub(crate) fn seek_nibble_slice(
         &mut self,
         mut key: NibbleSlice<'_>,
+        is_prefix_seek: bool,
     ) -> Result<CryptoHash, StorageError> {
         self.trail.clear();
         self.key_nibbles.clear();
+
+        // Checks if a key in an extension or leaf matches our search query.
+        //
+        // When doing prefix seek, this checks whether `key` is a prefix of
+        // `ext_key`.  When doing regular range seek, this checks whether `key`
+        // is no greater than `ext_key`.  If those conditions aren’t met, the
+        // node with `ext_key` should not match our query.
+        let check_ext_key = |key: &NibbleSlice, ext_key: &NibbleSlice| {
+            if is_prefix_seek {
+                ext_key.starts_with(key)
+            } else {
+                ext_key >= key
+            }
+        };
+
         let mut hash = self.trie.root;
+        let mut prev_prefix_boundary = &mut false;
         loop {
+            *prev_prefix_boundary = is_prefix_seek;
             self.descend_into_node(&hash)?;
-            let Crumb { status, node } = self.trail.last_mut().unwrap();
+            let Crumb { status, node, prefix_boundary } = self.trail.last_mut().unwrap();
+            prev_prefix_boundary = prefix_boundary;
             match &node.node {
                 TrieNode::Empty => break,
                 TrieNode::Leaf(leaf_key, _) => {
                     let existing_key = NibbleSlice::from_encoded(leaf_key).0;
-                    if existing_key < key {
+                    if !check_ext_key(&key, &existing_key) {
                         self.key_nibbles.extend(existing_key.iter());
                         *status = CrumbStatus::Exiting;
                     }
@@ -98,6 +127,7 @@ impl<'a> TrieIterator<'a> {
                             hash = *child.unwrap_hash();
                             key = key.mid(1);
                         } else {
+                            *prefix_boundary = is_prefix_seek;
                             break;
                         }
                     }
@@ -110,9 +139,9 @@ impl<'a> TrieIterator<'a> {
                         *status = CrumbStatus::At;
                         self.key_nibbles.extend(existing_key.iter());
                     } else {
-                        if existing_key < key {
+                        if !check_ext_key(&key, &existing_key) {
                             self.key_nibbles.extend(existing_key.iter());
                             *status = CrumbStatus::Exiting;
                         }
                         break;
                     }

When doing prefix seek, we need to check whether !existing_key.starts_with(key) rather than if existing_key < key.

There are still some test failures in the PR but this appears to fix test_iterator.

[mina86]

Could you update the PR to be onto master branch and remove proof-related changes from it? At this point it’s probably easiest to squash everything and clean it up:

git remote update
git checkout update-iterator-seek
git branch update-iterator-seek-backup
base=$(git merge-base @ upstream/master)
                      # ^^^^^^^^ if necessary, adjust ‘ustream’ to whatever the
                      # name of the near/nearcore remote is in the repository
git reset "$base"
git gui

With git gui (or git add -p if you prefer command line interface) you can pick only changes which relate to the seek_prefix feature. And once that’s done commit and force push:

git commit -m seek_prefix
git push origin +@:update-iterator-seek