Fix an off-by-one error in BridgeTree garbage collection.

proptest-regressions/bridgetree.txt

@@ -4,4 +4,5 @@
 #
 # It is recommended to check this file in to source control so that
 # everyone who runs the test benefits from these saved cases.
-cc ed8f0851cbfcde17e671dca49e83151f01d277c881abdb09c738d00ead7fc74b # shrinks to tree = BridgeTree {   depth: 8,   bridges: [MerkleBridge { prior_position: None, auth_fragments: {}, frontier: NonEmptyFrontier { position: Position(1), leaf: Right("a", "a"), ommers: [] } }, MerkleBridge { prior_position: Some(Position(1)), auth_fragments: {Position(1): AuthFragment { position: Position(1), altitudes_observed: 1, values: ["ek"] }}, frontier: NonEmptyFrontier { position: Position(3), leaf: Right("e", "k"), ommers: ["aa"] } }],  saved: {(Position(1), "a"): 0},   checkpoints: [],   max_checkpoints: 10 }
+cc a64e0d40c9287f2975a27447d266e4dd7228e44de99d000df39c4610598f486a # shrinks to tree = BridgeTree {   depth: 8,   prior_bridges: [MerkleBridge { prior_position: None, auth_fragments: {}, frontier: NonEmptyFrontier { position: Position(0), leaf: Left("a"), ommers: [] } }, MerkleBridge { prior_position: Some(Position(0)), auth_fragments: {}, frontier: NonEmptyFrontier { position: Position(1), leaf: Right("a", "a"), ommers: [] } }],   current_bridge: Some(MerkleBridge { prior_position: Some(Position(1)), auth_fragments: {Position(1): AuthFragment { position: Position(1), altitudes_observed: 0, values: [] }}, frontier: NonEmptyFrontier { position: Position(1), leaf: Right("a", "a"), ommers: [] } }),   saved: {Position(1): 1},   checkpoints: [Checkpoint { bridges_len: 1, is_witnessed: false, forgotten: {} }],   max_checkpoints: 10 }
+cc 736aee7c92f418b3b7391b0ae253ca4dc18f9b6cc625c0c34f0e568d26421e92 # shrinks to tree = BridgeTree {   depth: 8,   prior_bridges: [],   current_bridge: None,   saved: {},   checkpoints: [],   max_checkpoints: 10 }

src/bridgetree.rs

@@ -702,7 +702,8 @@ impl<H: Hashable + Ord + Debug, const DEPTH: u8> Debug for BridgeTree<H, DEPTH>
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub enum BridgeTreeError {
     IncorrectIncompleteIndex,
-    InvalidWitnessIndex,
+    InvalidWitnessIndex(usize),
+    PositionMismatch { expected: Position, found: Position },
     InvalidSavePoints,
     ContinuityError,
     CheckpointMismatch,
@@ -762,31 +763,25 @@ impl<H: Hashable + Ord + Clone, const DEPTH: u8> BridgeTree<H, DEPTH> {
         }
     }
 
-    /// Construct a new BridgeTree that will start recording changes from the state of
-    /// the specified frontier.
-    pub fn from_frontier(max_checkpoints: usize, frontier: NonEmptyFrontier<H>) -> Self {
-        Self {
-            prior_bridges: vec![],
-            current_bridge: Some(MerkleBridge::from_parts(None, BTreeMap::new(), frontier)),
-            saved: BTreeMap::new(),
-            checkpoints: vec![],
-            max_checkpoints,
-        }
-    }
-
-    pub fn from_parts(
-        prior_bridges: Vec<MerkleBridge<H>>,
-        current_bridge: Option<MerkleBridge<H>>,
-        saved: BTreeMap<Position, usize>,
-        checkpoints: Vec<Checkpoint>,
+    fn check_consistency_internal(
+        prior_bridges: &[MerkleBridge<H>],
+        current_bridge: &Option<MerkleBridge<H>>,
+        saved: &BTreeMap<Position, usize>,
+        checkpoints: &[Checkpoint],
         max_checkpoints: usize,
-    ) -> Result<Self, BridgeTreeError> {
+    ) -> Result<(), BridgeTreeError> {
         // check that saved values correspond to bridges
-        if saved
-            .iter()
-            .any(|(pos, i)| i >= &prior_bridges.len() || &prior_bridges[*i].position() != pos)
-        {
-            return Err(BridgeTreeError::InvalidWitnessIndex);
+        for (pos, i) in saved {
+            if i >= &prior_bridges.len() {
+                return Err(BridgeTreeError::InvalidWitnessIndex(*i));
+            }
+            let found = prior_bridges[*i].position();
+            if &found != pos {
+                return Err(BridgeTreeError::PositionMismatch {
+                    expected: *pos,
+                    found,
+                });
+            }
         }
 
         if checkpoints.len() > max_checkpoints
@@ -813,6 +808,35 @@ impl<H: Hashable + Ord + Clone, const DEPTH: u8> BridgeTree<H, DEPTH> {
             return Err(BridgeTreeError::ContinuityError);
         }
 
+        Ok(())
+    }
+
+    /// Construct a new BridgeTree that will start recording changes from the state of
+    /// the specified frontier.
+    pub fn from_frontier(max_checkpoints: usize, frontier: NonEmptyFrontier<H>) -> Self {
+        Self {
+            prior_bridges: vec![],
+            current_bridge: Some(MerkleBridge::from_parts(None, BTreeMap::new(), frontier)),
+            saved: BTreeMap::new(),
+            checkpoints: vec![],
+            max_checkpoints,
+        }
+    }
+
+    pub fn from_parts(
+        prior_bridges: Vec<MerkleBridge<H>>,
+        current_bridge: Option<MerkleBridge<H>>,
+        saved: BTreeMap<Position, usize>,
+        checkpoints: Vec<Checkpoint>,
+        max_checkpoints: usize,
+    ) -> Result<Self, BridgeTreeError> {
+        Self::check_consistency_internal(
+            &prior_bridges,
+            &current_bridge,
+            &saved,
+            &checkpoints,
+            max_checkpoints,
+        )?;
         Ok(BridgeTree {
             prior_bridges,
             current_bridge,
@@ -821,6 +845,16 @@ impl<H: Hashable + Ord + Clone, const DEPTH: u8> BridgeTree<H, DEPTH> {
             max_checkpoints,
         })
     }
+
+    pub fn check_consistency(&self) -> Result<(), BridgeTreeError> {
+        Self::check_consistency_internal(
+            &self.prior_bridges,
+            &self.current_bridge,
+            &self.saved,
+            &self.checkpoints,
+            self.max_checkpoints,
+        )
+    }
 }
 
 impl<H: Hashable + Ord + Clone, const DEPTH: u8> crate::Frontier<H> for BridgeTree<H, DEPTH> {
@@ -853,7 +887,7 @@ impl<H: Hashable + Ord + Clone, const DEPTH: u8> crate::Frontier<H> for BridgeTr
     }
 }
 
-impl<H: Hashable + Ord + Clone, const DEPTH: u8> Tree<H> for BridgeTree<H, DEPTH> {
+impl<H: Hashable + Ord + Clone + Debug, const DEPTH: u8> Tree<H> for BridgeTree<H, DEPTH> {
     fn current_position(&self) -> Option<Position> {
         self.current_bridge.as_ref().map(|b| b.position())
     }
@@ -1089,7 +1123,11 @@ impl<H: Hashable + Ord + Clone, const DEPTH: u8> Tree<H> for BridgeTree<H, DEPTH
                 }
             }
 
+            // unwrap is safe because we know that prior_bridges was nonempty.
             if let Some(last_bridge) = cur {
+                if let Some(idx) = self.saved.get_mut(&last_bridge.position()) {
+                    *idx -= merged;
+                }
                 self.prior_bridges.push(last_bridge);
             }
 
@@ -1097,6 +1135,9 @@ impl<H: Hashable + Ord + Clone, const DEPTH: u8> Tree<H> for BridgeTree<H, DEPTH
                 c.rewrite_indices(|idx| idx - merged);
             }
         }
+        if let Err(e) = self.check_consistency() {
+            panic!("Consistency check failed with {:?} for tree {:?}", e, self);
+        }
     }
 }
 

src/lib.rs

@@ -888,7 +888,7 @@ pub(crate) mod tests {
                     None
                 }
                 Authpath(p) => tree.authentication_path(*p).map(|xs| (*p, xs)),
-                GarbageCollect => None
+                GarbageCollect => None,
             }
         }
 
@@ -1080,8 +1080,7 @@ pub(crate) mod tests {
                 Unwitness(position) => {
                     tree.remove_witness(position);
                 }
-                WitnessedPositions => {
-                }
+                WitnessedPositions => {}
                 Checkpoint => {
                     tree_checkpoints.push(tree_size);
                     tree.checkpoint();
@@ -1110,8 +1109,7 @@ pub(crate) mod tests {
                         );
                     }
                 }
-                GarbageCollect => {
-                }
+                GarbageCollect => {}
             }
         }
 

src/sample.rs

@@ -193,7 +193,7 @@ impl<H: Hashable + PartialEq + Clone> Tree<H> for CompleteTree<H> {
         }
     }
 
-    fn garbage_collect(&mut self) { 
+    fn garbage_collect(&mut self) {
         // Garbage collection of the sample tree is a no-op.
     }
 }