refactor(chain)!: replace CanonicalIter with sans-io CanonicalizationTask

[oleonardolima]

fixes #1816

Description

It completes a refactoring of the canonicalization API in bdk_chain, migrating from an iterator-based approach CanonicalIter to a sans-IO/task-based pattern CanonicalizationTask. This change improves the separation of concerns between canonicalization logic and I/O operations, making the code more testable and flexible.

Old API:

// Direct coupling between canonicalization logic and `ChainOracle`
let view = tx_graph.canonical_view(&chain, chain_tip, params)?;

New API:

// Step 1: Create a task (pure logic, no I/O)
let task = tx_graph.canonicalization_task(params);

// Step 2: Execute with a chain oracle (handles I/O)
let view = chain.canonicalize(task, Some(chain_tip));

The new flow works as follows:

1. Task: CanonicalizationTask encapsulates all canonicalization logic without performing any chain queries.
2. Query: The task generates CanonicalizationRequests for anchor verification as needed, allowing the ChainOracle to batch or optimize these queries.
3. Resolution: The ChainOracle (e.g., LocalChain) processes requests and returns CanonicalizationResponse's indicating which anchors are the best in chain.
4. CanonicalView: Once all queries are resolved, the task builds a complete CanonicalView containing all canonical transactions with their chain positions.

This sans-IO pattern enables:

• Tasks can be tested with mock responses without a real chain
• Different chain oracles can implement their own optimization strategies
• Clear separation between business logic and I/O operations

Notes to the reviewers

The changes are splitted in multiple commits, as I think it could help reviewing it. Also it depends on #2029 PR, as it's built on top of it.

Changelog notice

  ### Changed
  - **Breaking:** Replace `TxGraph::canonical_iter()` and `TxGraph::canonical_view()` with `TxGraph::canonicalization_task()`
  - **Breaking:** Remove `CanonicalIter` in favor of `CanonicalizationTask`
  - **Breaking:** Change canonicalization to use a two-step process: create task, then execute with chain oracle
  - Move `CanonicalReason`, `ObservedIn`, and `CanonicalizationParams` from `canonical_iter` module to `canonical_task` module
  - Add `LocalChain::canonicalize()` method to execute canonicalization tasks

  ### Removed
  - **Breaking:** Remove `canonical_iter` module and `CanonicalIter` type
  - **Breaking:** Remove `TxGraph::try_canonical_view()` and `TxGraph::canonical_view()` methods
  - **Breaking:** Remove `CanonicalView::new()` public constructor

  ### Added
  - New sans-IO `CanonicalizationTask` for stateless canonicalization
  - `CanonicalizationRequest` and `CanonicalizationResponse` types for anchor verification

Checklists

All Submissions:

• I followed the contribution guidelines

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[evanlinjin]

Great work.

This is my initial round of reviews.

Are you planning to introduce topological ordering in a separate PR?

[evanlinjin]

What do you think about the following naming:

• CanonicalizationTask -> CanonicalResolver.
• TxGraph::canonicalization_task -> TxGraph::resolver.
• LocalChain::canonicalize -> LocalChain::resolve.

[oleonardolima]

I've been thinking about this, I agree with the CanonicalResolver, though the TxGraph::resolver and LocalChain::resolve seems a bit off.

What do you think about (?):

• CanonicalResolver
• TxGraph::canonical_resolver
• LocalChain::canonical_resolve

[evanlinjin]

Nit: Have the detected_self_double_spend early return instead of having the else branch.

Rationale: Early return is easier to read and results in less nesting.

[oleonardolima]

I should've addressed this, let me double check.

[evanlinjin]

I'm wondering whether it will be better to remove this from ChainRequest and have a new method on ChainQuery called something like query_tip(&self) -> BlockId.

Rationale: I'm assuming that we always want this tip to be consistent across the lifetime of the ChainQuery impl.

Counter-argument: We may want to have a ChainQuery impl which queries across conflicting chains - what would this be used for though?

WDYT

[ValuedMammal]

Yes it seems weird that the request would include the chain tip when the chain oracle should be aware of its own tip.

[evanlinjin]

@ValuedMammal although the chain oracle knows the tip, we want the tip to stay consistent across requests. We are just deferring the responsibility to the ChainQuery to make the tip consistent (instead of having each ChainRequest be responsible for it).

[ValuedMammal]

Rather than pass chain_tip to the CanonicalizationTask constructor, where it is merely copied into every request, I think the chain_tip parameter could be passed to LocalChain::canonicalize.

[evanlinjin]

I think it's better API to have:

pub type ChainResponse<B = BlockId> = Vec<(B, bool)>;

Rationale: We only care about whether a block is in the chain or not. However, with Option<B>, we now need to worry about the order the data is coming in.

Counter-argument: This increasing internal complexity.

Counter-counter-argument: No, this decreases internal complexity - allowing us to represent things in the most simplistic manner internally - whether something exists or not.

    /// We don't have it yet. This is our `ChainRequest`
    pending_blocks: BTreeSet<BlockId>,
    /// We have it!
    blocks: HashMap<BlockId, bool>,

At the anchored-tx/transitive-tx stages, we try to advance with the blocks we have. If we are missing anything, just populate pending_blocks then try again later. Very simple.

[oleonardolima]

Indeed, it looks like a better API. I'll give it a try and see if I face any other tradeoffs in the code.

[evanlinjin]

Were we going to get rid of this?

[evanlinjin]

@oleonardolima You mentioned to me that you aren't happy with this, and @LLFourn brought up splitting things into two steps in the past.

Problem Description

Currently, CanonicalizationTask convolutes two concerns in one step:

1. Canonicalization - determining which transactions are canonical and why (CanonicalReason).
2. Position resolution and topological ordering - mapping reasons into ChainPosition (confirmed vs unconfirmed) and in the future, ordering transactions topologically.

This makes the logic harder to follow than it needs to be. I'm thinking of splitting into two tasks:

• CanonicalTask - the core algorithm. Resolves conflicts and outputs CanonicalTxs (transactions + CanonicalReason, no position data).

• CanonicalViewTask - thin wrapper that takes CanonicalTxs and resolves reasons into ChainPosition (and doing topological ordering in the future), producing the final CanonicalView.

  Both types would be public, so callers that don't need position data can stop at step 1.

Proposed API

  let canonical_txs = chain.resolve(tx_graph.canonical_task(tip, params));
  let view = chain.resolve(canonical_txs.view_task());

Maybe even a helper method on LocalChain:

  impl LocalChain {
      pub fn canonical_view(
          &self,
          tx_graph: &TxGraph<A>,
          tip: BlockId,
          canon_params: CanonicalizationParams,
          view_params: CanonicalViewParams,
      ) -> CanonicalView<A> {
          let canonical_txs = self.resolve(tx_graph.canonical_task(tip, canon_params));
          self.resolve(canonical_txs.view_task(view_params))
      }
  }

Additionally, since CanonicalTxs and CanonicalView will want to share the majority of the methods, we can do something like:

struct Canonical<A, P> { ... }

type CanonicalTxs<A> = Canonical<A, CanonicalReason<A>>;
type CanonicalView<A> = Canonical<A, ChainPosition<A>>;

FullTxOut and CanonicalTx will also need that P generic as well. I would also rename FullTxOut::chain_position to pos.

[evanlinjin]

Before ACK and merge (@evanlinjin's list)

• Rename CanonicalizationTask to CanonicalTask.
• Consider renaming ChainQuery to ChainTask.
• Remove and inline single-used private methods: try_advance, process_assumed_txs (I would add another variant to CanonicalStage) process_last_seen, process_leftover_txs. ¹
• Remove is_finished (unless if you can provide justification for it's existence). My argument: "No more queries" instantly means "actually finished" - there is no state inbetween to represent.
• Remove chain_tip from ChainRequest. Add ChainQuery::tip() -> BlockId method. ²
• Change CanonicalTask to return CanonicalTxs & introduce CanonicalViewTask that returns CanonicalView. ³
• Remove all generics on ChainTask (ChainQuery) as we will flesh out this trait in follow-up PRs.

Edit:

I got Claude to look into the majority of these. Let me know if these commits are useful: https://github.com/evanlinjin/bdk/tree/refactor/canonical-iter-api-evanlinjin

Footnotes

1. https://github.com/bitcoindevkit/bdk/pull/2038#discussion_r2658462257 ↩

2. https://github.com/bitcoindevkit/bdk/pull/2038#discussion_r2401496855 ↩

3. https://github.com/bitcoindevkit/bdk/pull/2038#discussion_r2802430801 ↩