refactor(bip158)!: make constructor FilterIter::new_with_checkpoint fallible

The logic of `find_base` is moved inside the `new_with_checkpoint`
constructor, which will error if a point of agreement (PoA) isn't
found. As a result, `get_tip` is now only responsible for setting
the stop height, which means it can be called multiple times on
a single instance of FilterIter.

`cp` field is removed from FilterIter struct, since it is now
only used in the constructor to find a base and set the start
height.

There's no longer a need to remember conflicting blocks on the way
to finding the PoA, now that we store all newly fetched headers
during `next`.

Author: ValuedMammal

crates/bitcoind_rpc/examples/filter_iter.rs

@@ -55,7 +55,7 @@ fn main() -> anyhow::Result<()> {
     // Initialize block emitter
     let cp = chain.tip();
     let start_height = cp.height();
-    let mut emitter = FilterIter::new_with_checkpoint(&rpc_client, cp);
+    let mut emitter = FilterIter::new_with_checkpoint(&rpc_client, cp)?;
     for (_, desc) in graph.index.keychains() {
         let spks = SpkIterator::new_with_range(desc, 0..SPK_COUNT).map(|(_, spk)| spk);
         emitter.add_spks(spks);

crates/bitcoind_rpc/src/bip158.rs

@@ -33,8 +33,6 @@ pub struct FilterIter<'c, C> {
     client: &'c C,
     // SPK inventory
     spks: Vec<ScriptBuf>,
-    // local cp
-    cp: Option<CheckPoint>,
     // block headers
     headers: BTreeMap<Height, HashedHeader>,
     // heights of matching blocks
@@ -58,7 +56,6 @@ impl<'c, C: RpcApi> FilterIter<'c, C> {
         Self {
             client,
             spks: vec![],
-            cp: None,
             headers: BTreeMap::new(),
             matched: BTreeSet::new(),
             height,
@@ -68,10 +65,19 @@ impl<'c, C: RpcApi> FilterIter<'c, C> {
     }
 
     /// Construct [`FilterIter`] from a given `client` and [`CheckPoint`].
-    pub fn new_with_checkpoint(client: &'c C, cp: CheckPoint) -> Self {
-        let mut filter_iter = Self::new_with_height(client, cp.height());
-        filter_iter.cp = Some(cp);
-        filter_iter
+    ///
+    /// # Errors
+    ///
+    /// If no point of agreement is found between `cp` and the remote node, then
+    /// a [`Error::ReorgDepthExceeded`] error is returned.
+    pub fn new_with_checkpoint(client: &'c C, cp: CheckPoint) -> Result<Self, Error> {
+        let mut iter = Self::new_with_height(client, cp.height());
+
+        // Start scanning from point of agreement + 1.
+        let base = iter.find_base(cp.clone())?;
+        iter.height = base.height.saturating_add(1);
+
+        Ok(iter)
     }
 
     /// Extends `self` with an iterator of spks.
@@ -86,7 +92,9 @@ impl<'c, C: RpcApi> FilterIter<'c, C> {
 
     /// Get the remote tip.
     ///
-    /// Returns `None` if the remote height is less than the height of this [`FilterIter`].
+    /// This will set the stop height to that of the new tip.
+    ///
+    /// Returns `None` if the remote height is not at least the height of this [`FilterIter`].
     pub fn get_tip(&mut self) -> Result<Option<BlockId>, Error> {
         let tip_hash = self.client.get_best_block_hash()?;
         let header = self.client.get_block_header_info(&tip_hash)?;
@@ -95,12 +103,6 @@ impl<'c, C: RpcApi> FilterIter<'c, C> {
             return Ok(None);
         }
 
-        // start scanning from point of agreement + 1
-        if let Some(cp) = self.cp.as_ref() {
-            let base = self.find_base_with(cp.clone())?;
-            self.height = base.height.saturating_add(1);
-        }
-
         self.stop = tip_height;
 
         Ok(Some(BlockId {
@@ -223,8 +225,14 @@ impl<C: RpcApi> Iterator for FilterIter<'_, C> {
 }
 
 impl<C: RpcApi> FilterIter<'_, C> {
-    /// Returns the point of agreement between `self` and the given `cp`.
-    fn find_base_with(&mut self, mut cp: CheckPoint) -> Result<BlockId, Error> {
+    /// Returns the point of agreement (PoA) between `self` and the given `cp`.
+    ///
+    /// This ensures that the scan may proceed from a block that still exists
+    /// in the best chain.
+    ///
+    /// If no PoA is found between `cp` and the remote node, then a [`Error::ReorgDepthExceeded`]
+    /// error is returned.
+    fn find_base(&mut self, mut cp: CheckPoint) -> Result<BlockId, Error> {
         loop {
             let height = cp.height();
             let (fetched_hash, header) = match self.headers.get(&height).copied() {
@@ -240,24 +248,24 @@ impl<C: RpcApi> FilterIter<'_, C> {
                 self.headers.insert(height, (fetched_hash, header));
                 return Ok(cp.block_id());
             }
-            // Remember conflicts.
-            self.headers.insert(height, (fetched_hash, header));
             cp = cp.prev().ok_or(Error::ReorgDepthExceeded)?;
         }
     }
 
     /// Returns a chain update from the newly scanned blocks.
     ///
-    /// Returns `None` if this [`FilterIter`] was not constructed using a [`CheckPoint`], or
-    /// if not all events have been emitted (by calling `next`).
+    /// This should only be called once all events have been consumed (by calling `next`).
+    ///
+    /// Returns `None` if the height of this `FilterIter` is not yet past the stop height.
     pub fn chain_update(&self) -> Option<CheckPoint> {
-        if self.cp.is_none() || self.headers.is_empty() || self.height <= self.stop {
+        if self.headers.is_empty() || self.height <= self.stop {
             return None;
         }
 
-        // We return blocks up to and including the initial height, all of the matching blocks,
+        // We return blocks up to the initial height, all of the matching blocks,
         // and blocks in the terminal range.
         let tail_range = (self.stop + 1).saturating_sub(Self::CHAIN_SUFFIX_LEN)..=self.stop;
+
         Some(
             CheckPoint::from_block_ids(self.headers.iter().filter_map(|(&height, &(hash, _))| {
                 if height <= self.start

crates/bitcoind_rpc/tests/test_filter_iter.rs

@@ -97,7 +97,7 @@ fn get_tip_and_chain_update() -> anyhow::Result<()> {
     .into_iter()
     .for_each(|test| {
         let cp = CheckPoint::from_block_ids(test.chain).unwrap();
-        let mut iter = FilterIter::new_with_checkpoint(env.rpc_client(), cp);
+        let mut iter = FilterIter::new_with_checkpoint(env.rpc_client(), cp).unwrap();
         assert_eq!(iter.get_tip().unwrap(), Some(new_tip));
         for _res in iter.by_ref() {}
         let update_cp = iter.chain_update().unwrap();
@@ -177,11 +177,8 @@ fn filter_iter_error_wrong_network() -> anyhow::Result<()> {
         hash: bitcoin::hashes::Hash::hash(b"wrong-hash"),
     };
     let cp = CheckPoint::new(block_id);
-    let mut iter = FilterIter::new_with_checkpoint(rpc, cp);
-    let err = iter
-        .get_tip()
-        .expect_err("`get_tip` should fail to find PoA");
-    assert!(matches!(err, Error::ReorgDepthExceeded));
+    let res = FilterIter::new_with_checkpoint(rpc, cp);
+    assert!(matches!(res, Err(Error::ReorgDepthExceeded)));
 
     Ok(())
 }