refactor(core,chain): extract BlockQueries::resolve_candidates

evanlinjin · claude · evanlinjin · commit c037f3536fbc · 2026-06-17T07:36:45.000Z
Both canonicalization phases checked a tx's anchors against fetched
blocks with identical logic: scan for a resolved, matching block; else
request the missing heights; else wait on in-flight queries; else give
up. The two copies had already drifted (the phase-2 copy still carried
the old `has_unresolved_heights` shape with a latent empty-`Query`
edge).

Extract that logic as `BlockQueries::resolve_candidates`, returning a
`BlockCandidateResolution` enum (`Confirmed` / `Query` / `Awaiting` /
`NotConfirmed`). It lives on `BlockQueries` since it is an operation on
that type, and is decoupled from `Anchor`: it is generic over the
candidate `A` and takes a `Fn(&amp;A) -&gt; BlockId` closure, requiring only
what it uses (a `BlockId` per candidate).

Each call site now maps the four outcomes onto its own queue handling
and `TaskProgress`. Adds unit tests covering all four outcomes,
including the `Awaiting` branch that the synchronous driver never hits.

Co-Authored-By: Claude Opus 4.8 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/crates/chain/src/canonical_task.rs b/crates/chain/src/canonical_task.rs
@@ -5,7 +5,9 @@ use alloc::boxed::Box;
 use alloc::collections::BTreeSet;
 use alloc::sync::Arc;
 use alloc::vec::Vec;
-use bdk_core::{BlockId, BlockQueries, ChainTask, TaskProgress, ToBlockHash};
+use bdk_core::{
+    BlockCandidateResolution, BlockId, BlockQueries, ChainTask, TaskProgress, ToBlockHash,
+};
 use bitcoin::{Transaction, Txid};
 
 type CanonicalMap<A> = HashMap<Txid, CanonicalEntry<CanonicalReason<A>>>;
@@ -131,58 +133,36 @@ impl<'g, A: Anchor, B: ToBlockHash> ChainTask<B> for CanonicalTask<'g, A, B> {
                         return TaskProgress::Advanced;
                     }
 
-                    let mut best_anchor = Option::<A>::None;
-                    for a in anchors.iter() {
-                        let h = a.anchor_block().height;
-                        // A block that is either unresolved (`None`), absent at this height
-                        // (`Some(None)`), or present but mismatched does not confirm this anchor.
-                        if let Some(Some(b)) = self.queries.get(h) {
-                            if b.to_blockhash() == a.anchor_block().hash {
-                                best_anchor = Some(a.clone());
-                                break;
-                            }
-                        }
-                    }
-
-                    if let Some(a) = best_anchor {
-                        self.mark_canonical(txid, tx, CanonicalReason::from_anchor(a));
-                        return TaskProgress::Advanced;
-                    }
-
-                    // No resolved anchor matched. `request` returns only heights that are neither
-                    // resolved nor already in-flight, so a non-empty result is genuinely new work
-                    // to fetch. This keeps `Query` additive and never empty.
-                    let heights = self
+                    match self
                         .queries
-                        .request(anchors.iter().map(|a| a.anchor_block().height));
-                    if !heights.is_empty() {
-                        self.unprocessed_anchored_txs
-                            .push_front((txid, tx, anchors));
-                        return TaskProgress::Query(heights);
-                    }
-
-                    // No new heights to request. If any anchor height is still in-flight (requested
-                    // but unresolved), we can't decide this tx yet — wait for the driver. Otherwise
-                    // every anchor height is resolved and none matched, so the tx is not confirmed.
-                    if anchors
-                        .iter()
-                        .any(|a| self.queries.get(a.anchor_block().height).is_none())
+                        .resolve_candidates(anchors, |a| a.anchor_block())
                     {
-                        self.unprocessed_anchored_txs
-                            .push_front((txid, tx, anchors));
-                        return TaskProgress::AwaitingQueries;
+                        BlockCandidateResolution::Confirmed(a) => {
+                            self.mark_canonical(txid, tx, CanonicalReason::from_anchor(a));
+                        }
+                        BlockCandidateResolution::Query(heights) => {
+                            self.unprocessed_anchored_txs
+                                .push_front((txid, tx, anchors));
+                            return TaskProgress::Query(heights);
+                        }
+                        BlockCandidateResolution::Awaiting => {
+                            self.unprocessed_anchored_txs
+                                .push_front((txid, tx, anchors));
+                            return TaskProgress::AwaitingQueries;
+                        }
+                        BlockCandidateResolution::NotConfirmed => {
+                            // No anchor confirms this tx; defer to the leftover stage.
+                            self.unprocessed_leftover_txs.push_back((
+                                txid,
+                                tx,
+                                anchors
+                                    .iter()
+                                    .last()
+                                    .expect("must have at least one anchor")
+                                    .confirmation_height_upper_bound(),
+                            ));
+                        }
                     }
-
-                    // No confirmed anchor found.
-                    self.unprocessed_leftover_txs.push_back((
-                        txid,
-                        tx,
-                        anchors
-                            .iter()
-                            .last()
-                            .expect("must have at least one anchor")
-                            .confirmation_height_upper_bound(),
-                    ));
                 } else {
                     self.current_stage.next_stage();
                 }
diff --git a/crates/chain/src/canonical_view_task.rs b/crates/chain/src/canonical_view_task.rs
@@ -6,7 +6,10 @@ use crate::tx_graph::TxDescendants;
 use alloc::collections::BTreeSet;
 use alloc::vec::Vec;
 
-use bdk_core::{BlockId, BlockQueries, ChainTask, TaskProgress, ToBlockHash, ToBlockTime};
+use bdk_core::{
+    BlockCandidateResolution, BlockId, BlockQueries, ChainTask, TaskProgress, ToBlockHash,
+    ToBlockTime,
+};
 use bitcoin::{OutPoint, Txid};
 
 use crate::{canonical::CanonicalEntry, Anchor, CanonicalView, ChainPosition, TxGraph};
@@ -132,44 +135,24 @@ impl<'g, A: Anchor, B: ToBlockHash> ChainTask<B> for CanonicalViewTask<'g, A, B>
         match self.current_stage {
             ViewStage::ResolvingPositions => {
                 if let Some((txid, anchors)) = self.unprocessed_anchor_checks.pop_front() {
-                    let mut best_anchor = Option::<A>::None;
-                    for a in anchors.iter() {
-                        let h = a.anchor_block().height;
-                        // Only a resolved, present, matching block confirms this anchor.
-                        if let Some(Some(b)) = self.queries.get(h) {
-                            if b.to_blockhash() == a.anchor_block().hash {
-                                best_anchor = Some(a.clone());
-                                break;
-                            }
-                        }
-                    }
-
-                    if let Some(anchor) = best_anchor {
-                        self.direct_anchors.insert(txid, anchor);
-                        return TaskProgress::Advanced;
-                    }
-
-                    // No resolved anchor matched. `request` is additive, so a non-empty result is
-                    // genuinely new work to fetch and is never empty.
-                    let heights = self
+                    match self
                         .queries
-                        .request(anchors.iter().map(|a| a.anchor_block().height));
-                    if !heights.is_empty() {
-                        self.unprocessed_anchor_checks.push_front((txid, anchors));
-                        return TaskProgress::Query(heights);
-                    }
-
-                    // No new heights. If any anchor height is still in-flight, wait for the driver;
-                    // otherwise all are resolved and none matched (no direct anchor for this tx).
-                    if anchors
-                        .iter()
-                        .any(|a| self.queries.get(a.anchor_block().height).is_none())
+                        .resolve_candidates(anchors, |a| a.anchor_block())
                     {
-                        self.unprocessed_anchor_checks.push_front((txid, anchors));
-                        return TaskProgress::AwaitingQueries;
+                        BlockCandidateResolution::Confirmed(anchor) => {
+                            self.direct_anchors.insert(txid, anchor);
+                        }
+                        BlockCandidateResolution::Query(heights) => {
+                            self.unprocessed_anchor_checks.push_front((txid, anchors));
+                            return TaskProgress::Query(heights);
+                        }
+                        BlockCandidateResolution::Awaiting => {
+                            self.unprocessed_anchor_checks.push_front((txid, anchors));
+                            return TaskProgress::AwaitingQueries;
+                        }
+                        // No anchor confirms this tx; leave it without a direct anchor.
+                        BlockCandidateResolution::NotConfirmed => {}
                     }
-
-                    // No confirmed anchor found for this tx
                     TaskProgress::Advanced
                 } else {
                     self.current_stage = ViewStage::FetchingMtpBlocks;
diff --git a/crates/core/src/block_queries.rs b/crates/core/src/block_queries.rs
@@ -1,4 +1,5 @@
 use crate::collections::BTreeMap;
+use crate::{BlockId, ToBlockHash};
 use alloc::collections::BTreeSet;
 use alloc::vec::Vec;
 
@@ -60,8 +61,164 @@ impl<B> BlockQueries<B> {
     }
 }
 
+impl<B: ToBlockHash> BlockQueries<B> {
+    /// Resolves a set of candidate blocks against the blocks fetched so far.
+    ///
+    /// Each candidate's [`BlockId`] is obtained via `block_id`. A candidate is *in-chain* when the
+    /// block resolved at its height is present and its hash matches the candidate's; the first such
+    /// candidate is returned as [`Confirmed`](BlockCandidateResolution::Confirmed).
+    ///
+    /// When no candidate is confirmed, the result tells the caller how to make progress:
+    /// - [`Query`](BlockCandidateResolution::Query) — heights that still need fetching. These come
+    ///   from [`request`](Self::request), so the list is *additive* (each height announced once)
+    ///   and never empty.
+    /// - [`Awaiting`](BlockCandidateResolution::Awaiting) — nothing new to request, but some
+    ///   heights are still in-flight; wait for them to resolve.
+    /// - [`NotConfirmed`](BlockCandidateResolution::NotConfirmed) — every candidate height is
+    ///   resolved and none matched.
+    pub fn resolve_candidates<A: Clone>(
+        &mut self,
+        candidates: &BTreeSet<A>,
+        block_id: impl Fn(&A) -> BlockId,
+    ) -> BlockCandidateResolution<A> {
+        // A candidate is confirmed only by a resolved, present block whose hash matches. Unresolved
+        // (`None`), absent (`Some(None)`), or mismatched blocks do not confirm it.
+        for c in candidates.iter() {
+            let id = block_id(c);
+            if let Some(Some(b)) = self.get(id.height) {
+                if b.to_blockhash() == id.hash {
+                    return BlockCandidateResolution::Confirmed(c.clone());
+                }
+            }
+        }
+
+        let heights = self.request(candidates.iter().map(|c| block_id(c).height));
+        if !heights.is_empty() {
+            return BlockCandidateResolution::Query(heights);
+        }
+
+        // Nothing new to request: wait if any candidate height is still in-flight, otherwise all
+        // are resolved and none matched.
+        if candidates
+            .iter()
+            .any(|c| self.get(block_id(c).height).is_none())
+        {
+            return BlockCandidateResolution::Awaiting;
+        }
+
+        BlockCandidateResolution::NotConfirmed
+    }
+}
+
 impl<B> Default for BlockQueries<B> {
     fn default() -> Self {
         Self::new()
     }
 }
+
+/// Outcome of [`BlockQueries::resolve_candidates`].
+///
+/// A "candidate" is some value `A` that claims to live at a particular [`BlockId`] (height +
+/// hash). Resolution checks those claims against the blocks fetched so far and reports which of
+/// four states applies. The generic `A` is carried through opaquely so the confirmed candidate can
+/// be returned to the caller.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum BlockCandidateResolution<A> {
+    /// One candidate's block is resolved and its hash matches — this candidate is in-chain.
+    Confirmed(A),
+    /// These *newly requested* heights must be fetched before the candidates can be decided.
+    /// Always non-empty.
+    Query(Vec<u32>),
+    /// Nothing new to request, but some candidate heights are still in-flight; the candidates
+    /// can't be decided until the driver resolves them.
+    Awaiting,
+    /// Every candidate height is resolved and none match — no candidate is in-chain.
+    NotConfirmed,
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use alloc::vec;
+    use bitcoin::{hashes::Hash, BlockHash};
+
+    fn bhash(n: u8) -> BlockHash {
+        BlockHash::from_byte_array([n; 32])
+    }
+
+    fn id(height: u32, hash: u8) -> BlockId {
+        BlockId {
+            height,
+            hash: bhash(hash),
+        }
+    }
+
+    fn candidates<const N: usize>(ids: [BlockId; N]) -> BTreeSet<BlockId> {
+        BTreeSet::from(ids)
+    }
+
+    #[test]
+    fn confirmed_when_block_resolved_and_matches() {
+        let mut q = BlockQueries::<BlockHash>::new();
+        q.request([5].into_iter());
+        q.resolve(5, Some(bhash(5)));
+
+        let cands = candidates([id(5, 5)]);
+        let res = q.resolve_candidates(&cands, |c| *c);
+        assert_eq!(res, BlockCandidateResolution::Confirmed(id(5, 5)));
+    }
+
+    #[test]
+    fn confirmed_picks_matching_candidate_among_many() {
+        let mut q = BlockQueries::<BlockHash>::new();
+        q.request([4, 5].into_iter());
+        q.resolve(4, Some(bhash(99))); // height 4 resolved but mismatched
+        q.resolve(5, Some(bhash(5))); // height 5 matches
+
+        let cands = candidates([id(4, 4), id(5, 5)]);
+        let res = q.resolve_candidates(&cands, |c| *c);
+        assert_eq!(res, BlockCandidateResolution::Confirmed(id(5, 5)));
+    }
+
+    #[test]
+    fn query_returns_unrequested_heights() {
+        let mut q = BlockQueries::<BlockHash>::new();
+        let cands = candidates([id(7, 7)]);
+        let res = q.resolve_candidates(&cands, |c| *c);
+        assert_eq!(res, BlockCandidateResolution::Query(vec![7]));
+        // The request is recorded, so it is now reported as outstanding.
+        assert!(q.unresolved().eq([7]));
+    }
+
+    #[test]
+    fn awaiting_when_heights_in_flight() {
+        let mut q = BlockQueries::<BlockHash>::new();
+        q.request([7].into_iter()); // requested but not resolved
+
+        let cands = candidates([id(7, 7)]);
+        let res = q.resolve_candidates(&cands, |c| *c);
+        assert_eq!(res, BlockCandidateResolution::Awaiting);
+    }
+
+    #[test]
+    fn not_confirmed_when_resolved_and_mismatched() {
+        let mut q = BlockQueries::<BlockHash>::new();
+        q.request([7].into_iter());
+        q.resolve(7, Some(bhash(99))); // present but wrong hash
+
+        let cands = candidates([id(7, 7)]);
+        let res = q.resolve_candidates(&cands, |c| *c);
+        assert_eq!(res, BlockCandidateResolution::NotConfirmed);
+    }
+
+    #[test]
+    fn not_confirmed_when_block_absent() {
+        let mut q = BlockQueries::<BlockHash>::new();
+        q.request([7].into_iter());
+        q.resolve(7, None); // chain source has no block at this height
+
+        let cands = candidates([id(7, 7)]);
+        let res = q.resolve_candidates(&cands, |c| *c);
+        assert_eq!(res, BlockCandidateResolution::NotConfirmed);
+    }
+}
