Merge pull request #21 from zcash/always_rewind_unless_empty

Rewinds are no longer blocked by existing witnesses.

proptest-regressions/lib.txt

@@ -10,3 +10,5 @@ cc aa498d80d63c58720a913f2edcc6ba19da462f7a824f5be906e63c7ca1566076 # shrinks to
 cc 7cebce1664b804d55e259c466ebcc140c4a3708f4ec49ed865d6009736b0a568 # shrinks to ops = [Append("d"), Checkpoint, Witness, Unwitness("d"), Rewind, Unwitness("d")]
 cc 9078a200bceb14aa2f78743f545dab9719db29ddbbb5f1f67fae01805da90f4b # shrinks to ops = [Append("a"), Unwitness("a"), Checkpoint, Checkpoint, Rewind, Append("a"), Rewind, Append("a")]
 cc 20e43678bbf38f4c40fdd9e6f5bbc2b8eebcb1756b599f443f3268d6cf8e9d22 # shrinks to ops = [Append("o"), Checkpoint, Witness, Checkpoint, Unwitness("o"), Rewind, Rewind]
+cc 2b82b514115d3ddb4990853b0178e997632226be6b8f7c9cf07e94a3e9f41690 # shrinks to ops = [Checkpoint, Witness, Rewind, Unwitness(Position(0), "x"), Checkpoint, Rewind, Checkpoint, Unwitness(Position(0), "x"), Unwitness(Position(0), "x"), Witness, Unwitness(Position(0), "x"), Append("x"), Append("b"), Authpath(Position(0), "x"), Authpath(Position(0), "x"), Rewind, Rewind, Authpath(Position(0), "x"), Witness, Rewind, Witness, Rewind, Append("x"), Rewind, Authpath(Position(0), "x"), Checkpoint, Authpath(Position(0), "x"), Witness, Checkpoint, Witness, Checkpoint, Rewind, Unwitness(Position(0), "x"), Unwitness(Position(0), "x"), Rewind, Witness, Rewind, Authpath(Position(0), "x"), Unwitness(Position(0), "x"), Authpath(Position(0), "x"), Witness, Witness, Checkpoint, Rewind, Checkpoint]
+cc 248258b4e2fd558b56611179f6c8493326a8a35bd3fd3ffb55634c14f6fe417a # shrinks to ops = [Append(SipHashable(0)), Checkpoint, Witness, Rewind, Rewind, Authpath(Position(0), SipHashable(0)), Authpath(Position(0), SipHashable(0)), Append(SipHashable(0)), Witness, Authpath(Position(0), SipHashable(0)), Unwitness(Position(0), SipHashable(0)), Unwitness(Position(0), SipHashable(0)), Authpath(Position(0), SipHashable(0)), Authpath(Position(0), SipHashable(0)), Unwitness(Position(0), SipHashable(0)), Witness, Rewind, Checkpoint, Authpath(Position(0), SipHashable(0)), Witness, Rewind, Authpath(Position(0), SipHashable(0)), Authpath(Position(0), SipHashable(0)), Unwitness(Position(0), SipHashable(0)), Rewind, Append(SipHashable(0)), Witness, Authpath(Position(1), SipHashable(0)), Witness, Witness, Unwitness(Position(3), SipHashable(0)), Append(SipHashable(0)), Checkpoint, Append(SipHashable(0)), Witness, Witness, Checkpoint, Unwitness(Position(5), SipHashable(0)), Unwitness(Position(1), SipHashable(0)), Append(SipHashable(0)), Checkpoint, Authpath(Position(2), SipHashable(0)), Witness, Append(SipHashable(0)), Unwitness(Position(6), SipHashable(0)), Authpath(Position(2), SipHashable(0)), Witness, Unwitness(Position(5), SipHashable(0)), Checkpoint, Rewind, Unwitness(Position(1), SipHashable(0)), Checkpoint, Rewind, Witness, Checkpoint, Unwitness(Position(5), SipHashable(0)), Authpath(Position(2), SipHashable(0)), Append(SipHashable(0)), Checkpoint, Append(SipHashable(0)), Checkpoint, Append(SipHashable(0)), Unwitness(Position(4), SipHashable(0)), Rewind, Append(SipHashable(7)), Checkpoint, Append(SipHashable(9)), Authpath(Position(5), SipHashable(0)), Checkpoint, Authpath(Position(10), SipHashable(7)), Rewind, Checkpoint, Rewind]

src/bridgetree.rs

@@ -1050,45 +1050,35 @@ impl<H: Hashable + Ord + Clone, const DEPTH: u8> Tree<H> for BridgeTree<H, DEPTH
     }
 
     /// Rewinds the tree state to the previous checkpoint. This function will
-    /// fail and return false if there is no previous checkpoint or in the event
-    /// witness data would be destroyed in the process.
+    /// return false and leave the tree unmodified if no checkpoints exist.
     fn rewind(&mut self) -> bool {
         match self.checkpoints.pop() {
             Some(mut c) => {
-                if self.saved.values().any(|saved_idx| {
-                    c.bridges_len == 0
-                        || (!c.is_witnessed && *saved_idx >= c.bridges_len - 1)
-                        || (c.is_witnessed && *saved_idx >= c.bridges_len)
-                }) {
-                    // there is a witnessed value at a later position, or the
-                    // current position was witnessed since the checkpoint was
-                    // created, so we restore the removed checkpoint and return
-                    // failure
-                    self.checkpoints.push(c);
-                    false
-                } else {
-                    self.bridges.truncate(c.bridges_len);
-                    self.saved.append(&mut c.forgotten);
+                // drop witnessed values at and above the checkpoint height;
+                // we will re-witness if necessary.
+                self.saved
+                    .retain(|_, saved_idx| *saved_idx + 1 < c.bridges_len);
+                self.bridges.truncate(c.bridges_len);
+                self.saved.append(&mut c.forgotten);
 
-                    let was_duplicate_checkpoint = self
-                        .checkpoints
-                        .last()
-                        .iter()
-                        .any(|c0| c0.bridges_len == c.bridges_len);
-                    let len = self.bridges.len();
-                    if c.is_witnessed {
-                        // if the checkpointed state was witnessed, we need to
-                        // restore the witness, as the successor bridge will have
-                        // been removed by truncation.
-                        self.witness();
-                    } else if len > 0 && was_duplicate_checkpoint {
-                        // if the checkpoint was a duplicate, we need to create
-                        // a successor so that future appends do not mutate the
-                        // state at the tip.
-                        self.bridges.push(self.bridges[len - 1].successor(false));
-                    }
-                    true
+                let was_duplicate_checkpoint = self
+                    .checkpoints
+                    .last()
+                    .iter()
+                    .any(|c0| c0.bridges_len == c.bridges_len);
+                let len = self.bridges.len();
+                if c.is_witnessed {
+                    // if the checkpointed state was witnessed, we need to
+                    // restore the witness, as the successor bridge will have
+                    // been removed by truncation.
+                    self.witness();
+                } else if len > 0 && was_duplicate_checkpoint {
+                    // if the checkpoint was a duplicate, we need to create
+                    // a successor so that future appends do not mutate the
+                    // state at the tip.
+                    self.bridges.push(self.bridges[len - 1].successor(false));
                 }
+                true
             }
             None => false,
         }
@@ -1198,11 +1188,11 @@ mod tests {
         t.witness();
         t.append(&"b".to_string());
         t.append(&"c".to_string());
-        assert!(!t.rewind(), "Rewind is expected to fail.");
         assert!(
             t.drop_oldest_checkpoint(),
             "Checkpoint drop is expected to succeed"
         );
+        assert!(!t.rewind(), "Rewind is expected to fail.");
     }
 
     #[test]

src/lib.rs

@@ -270,12 +270,7 @@ pub trait Tree<H>: Frontier<H> {
     /// that checkpoint record. If there are multiple checkpoints at a given
     /// tree state, the tree state will not be altered until all checkpoints
     /// at that tree state have been removed using `rewind`. This function
-    /// will fail and return false if there is no previous checkpoint or in
-    /// the event witness data would be destroyed in the process.
-    ///
-    /// In the case that this method returns `false`, the user should have
-    /// explicitly called `remove_witness` for each witnessed leaf marked
-    /// since the last checkpoint.
+    /// return false and leave the tree unmodified if no checkpoints exist.
     fn rewind(&mut self) -> bool;
 
     /// Start a recording of append operations performed on a tree.
@@ -371,7 +366,6 @@ pub(crate) mod tests {
         t.checkpoint();
         t.witness();
         t.append(&"a".to_string());
-        assert!(!t.rewind());
         t.append(&"a".to_string());
         t.append(&"a".to_string());
         assert_eq!(t.root(), "aaaa____________");
@@ -599,17 +593,19 @@ pub(crate) mod tests {
 
     pub(crate) fn check_checkpoint_rewind<T: Tree<String>, F: Fn(usize) -> T>(new_tree: F) {
         let mut t = new_tree(100);
-        t.append(&"a".to_string());
-        t.checkpoint();
-        t.append(&"b".to_string());
-        t.witness();
         assert!(!t.rewind());
 
+        let mut t = new_tree(100);
+        t.checkpoint();
+        assert!(t.rewind());
+
         let mut t = new_tree(100);
         t.append(&"a".to_string());
         t.checkpoint();
+        t.append(&"b".to_string());
         t.witness();
-        assert!(!t.rewind());
+        assert!(t.rewind());
+        assert_eq!(Some((Position::from(0), "a".to_string())), t.current_leaf());
 
         let mut t = new_tree(100);
         t.append(&"a".to_string());
@@ -622,7 +618,8 @@ pub(crate) mod tests {
         t.checkpoint();
         t.witness();
         t.append(&"a".to_string());
-        assert!(!t.rewind());
+        assert!(t.rewind());
+        assert_eq!(Some((Position::from(0), "a".to_string())), t.current_leaf());
 
         let mut t = new_tree(100);
         t.append(&"a".to_string());
@@ -636,6 +633,20 @@ pub(crate) mod tests {
     }
 
     pub(crate) fn check_rewind_remove_witness<T: Tree<String>, F: Fn(usize) -> T>(new_tree: F) {
+        let mut tree = new_tree(100);
+        tree.append(&"e".to_string());
+        tree.witness();
+        tree.checkpoint();
+        assert!(tree.rewind());
+        assert!(tree.remove_witness(0usize.into(), &"e".to_string()));
+
+        let mut tree = new_tree(100);
+        tree.append(&"e".to_string());
+        tree.checkpoint();
+        tree.witness();
+        assert!(tree.rewind());
+        assert!(!tree.remove_witness(0usize.into(), &"e".to_string()));
+
         let mut tree = new_tree(100);
         tree.append(&"e".to_string());
         tree.witness();
@@ -657,7 +668,7 @@ pub(crate) mod tests {
         assert!(!tree.remove_witness(0usize.into(), &"a".to_string()));
         tree.checkpoint();
         assert!(tree.witness().is_some());
-        assert!(!tree.rewind());
+        assert!(tree.rewind());
 
         let mut tree = new_tree(100);
         tree.append(&"a".to_string());
@@ -671,50 +682,69 @@ pub(crate) mod tests {
         // test framework itself previously did not correctly handle
         // chain state restoration.
 
-        let ops = vec![
-            Append("a".to_string()),
-            Unwitness(0usize.into(), "a".to_string()),
-            Checkpoint,
-            Witness,
-            Rewind,
-        ];
+        fn append(x: &str) -> Operation<String> {
+            Append(x.to_string())
+        }
+
+        fn unwitness(pos: usize, x: &str) -> Operation<String> {
+            Unwitness(Position::from(pos), x.to_string())
+        }
+
+        let ops = vec![append("x"), Checkpoint, Witness, Rewind, unwitness(0, "x")];
         let result = check_operations(ops);
-        assert!(matches!(result, Ok(())), "Test failed: {:?}", result);
+        assert!(
+            matches!(result, Ok(())),
+            "Reference/Test mismatch: {:?}",
+            result
+        );
 
         let ops = vec![
-            Append("s".to_string()),
-            Witness,
-            Append("m".to_string()),
+            append("d"),
             Checkpoint,
-            Unwitness(0usize.into(), "s".to_string()),
+            Witness,
+            unwitness(0, "d"),
             Rewind,
-            Unwitness(0usize.into(), "s".to_string()),
+            unwitness(0, "d"),
         ];
         let result = check_operations(ops);
-        assert!(matches!(result, Ok(())), "Test failed: {:?}", result);
+        assert!(
+            matches!(result, Ok(())),
+            "Reference/Test mismatch: {:?}",
+            result
+        );
 
         let ops = vec![
-            Append("d".to_string()),
+            append("o"),
             Checkpoint,
             Witness,
-            Unwitness(0usize.into(), "d".to_string()),
+            Checkpoint,
+            unwitness(0, "o"),
+            Rewind,
             Rewind,
-            Unwitness(0usize.into(), "d".to_string()),
         ];
         let result = check_operations(ops);
-        assert!(matches!(result, Ok(())), "Test failed: {:?}", result);
+        assert!(
+            matches!(result, Ok(())),
+            "Reference/Test mismatch: {:?}",
+            result
+        );
 
         let ops = vec![
-            Append("o".to_string()),
-            Checkpoint,
+            append("s"),
             Witness,
+            append("m"),
             Checkpoint,
-            Unwitness(0usize.into(), "o".to_string()),
-            Rewind,
+            unwitness(0, "s"),
             Rewind,
+            unwitness(0, "s"),
+            unwitness(0, "m"),
         ];
         let result = check_operations(ops);
-        assert!(matches!(result, Ok(())), "Test failed: {:?}", result);
+        assert!(
+            matches!(result, Ok(())),
+            "Reference/Test mismatch: {:?}",
+            result
+        );
     }
 
     //

src/sample.rs

@@ -1,6 +1,5 @@
+//! Sample implementation of the Tree interface.
 use super::{Altitude, Frontier, Hashable, Position, Recording, Tree};
-/// Sample implementation of the Tree interface.
-use std::convert::TryInto;
 
 #[derive(Clone, Debug)]
 pub struct TreeState<H: Hashable> {
@@ -144,7 +143,7 @@ impl<H: Hashable + PartialEq + Clone> TreeState<H> {
 #[derive(Clone, Debug)]
 pub struct CompleteTree<H: Hashable> {
     tree_state: TreeState<H>,
-    checkpoints: Vec<(TreeState<H>, bool)>,
+    checkpoints: Vec<TreeState<H>>,
     max_checkpoints: usize,
 }
 
@@ -228,36 +227,18 @@ impl<H: Hashable + PartialEq + Clone> Tree<H> for CompleteTree<H> {
     /// Marks the current tree state as a checkpoint if it is not already a
     /// checkpoint.
     fn checkpoint(&mut self) {
-        let is_witnessed = self
-            .tree_state
-            .current_leaf()
-            .into_iter()
-            .any(|(p, l)| self.tree_state.is_witnessed(p, &l));
-        self.checkpoints
-            .push((self.tree_state.clone(), is_witnessed));
+        self.checkpoints.push(self.tree_state.clone());
         if self.checkpoints.len() > self.max_checkpoints {
             self.drop_oldest_checkpoint();
         }
     }
 
     /// Rewinds the tree state to the previous checkpoint. This function will
-    /// fail and return false if there is no previous checkpoint or in the event
-    /// witness data would be destroyed in the process.
+    /// return false and leave the tree unmodified if no checkpoints exist.
     fn rewind(&mut self) -> bool {
-        if let Some((checkpointed_state, is_witnessed)) = self.checkpoints.pop() {
-            // if there are any witnessed leaves in the current tree state
-            // that would be removed, we don't rewind
-            if self.tree_state.witnesses.iter().any(|&(pos, _)| {
-                let offset: usize = (pos + 1).try_into().unwrap();
-                offset > checkpointed_state.current_offset
-                    || (offset == checkpointed_state.current_offset && !is_witnessed)
-            }) {
-                self.checkpoints.push((checkpointed_state, is_witnessed));
-                false
-            } else {
-                self.tree_state = checkpointed_state;
-                true
-            }
+        if let Some(checkpointed_state) = self.checkpoints.pop() {
+            self.tree_state = checkpointed_state;
+            true
         } else {
             false
         }