Skip to content

refactor(chain,core)!: replace CanonicalIter with sans-IO CanonicalTask + ChainQuery trait#2038

Open
oleonardolima wants to merge 18 commits intobitcoindevkit:masterfrom
oleonardolima:refactor/canonical-iter-api
Open

refactor(chain,core)!: replace CanonicalIter with sans-IO CanonicalTask + ChainQuery trait#2038
oleonardolima wants to merge 18 commits intobitcoindevkit:masterfrom
oleonardolima:refactor/canonical-iter-api

Conversation

@oleonardolima
Copy link
Copy Markdown
Collaborator

@oleonardolima oleonardolima commented Sep 18, 2025

fixes #1816

Description

Replaces the iterator-based CanonicalIter with a two-phase sans-IO canonicalization pipeline, and introduces a generic ChainQuery trait in bdk_core to decouple canonicalization from chain sources.

Old API:

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

New API:

// Option A: Two-phase (full control)
let canonical_txs = chain.canonicalize(tx_graph.canonical_task(tip, params));
let view = chain.canonicalize(canonical_txs.view_task(&tx_graph));

// Option B: Convenience method
let view = chain.canonical_view(&tx_graph, tip, params);

Phase 1: CanonicalTask

Determines which transactions are canonical by processing them in stages:

  1. Assumed txs — transactions assumed canonical via CanonicalParams
  2. Anchored txs — transactions anchored in the best chain (descending height)
  3. Seen txs — unconfirmed transactions by descending last-seen time
  4. Remaining txs — leftover anchored transactions not in the best chain

Produces a CanonicalTxs<A> containing each canonical transaction with its CanonicalReason.

Phase 2: CanonicalViewTask

Resolves CanonicalReasons into concrete ChainPositions (confirmed height or unconfirmed with last-seen), producing the final CanonicalView<A>.

Both phases implement the ChainQuery trait, so any chain source can drive them via the same next_query/resolve_query loop.

Key structural changes

  • ChainQuery trait added to bdk_core — a generic sans-IO interface (next_queryresolve_queryfinish) for any algorithm that needs to verify blocks against a chain source.
  • ChainOracle trait removed — replaced by ChainQuery. LocalChain::canonicalize() now drives any ChainQuery implementor.
  • Canonical<A, P> generic containerCanonicalTxs<A> (phase 1 output) and CanonicalView<A> (phase 2 output) are type aliases over Canonical<A, P>.
  • Module splitcanonical_view.rs split into canonical.rs (types: Canonical, CanonicalTx, CanonicalTxOut) and canonical_view_task.rs (phase 2 task). canonical_iter.rs replaced by canonical_task.rs.

Notes to the reviewers

The changes are split into multiple commits for easier review. Also depends on #2029.

Changelog notice

  ### Added
  - `bdk_core::ChainQuery` trait — generic sans-IO interface for chain verification queries
  - `bdk_core::ChainRequest` / `ChainResponse` type aliases
  - `CanonicalTask` — phase 1 sans-IO canonicalization (determines canonical txs)
  - `CanonicalViewTask` — phase 2 sans-IO canonicalization (resolves chain positions)
  - `Canonical<A, P>` generic container with `CanonicalTxs<A>` and `CanonicalView<A>` aliases
  - `LocalChain::canonicalize()` — drives any `ChainQuery` implementor
  - `LocalChain::canonical_view()` — convenience method for full two-phase canonicalization

  ### Changed
  - **Breaking:** Replace `TxGraph::canonical_iter()` / `TxGraph::canonical_view()` with `TxGraph::canonical_task()`
  - **Breaking:** Canonicalization now uses a two-phase sans-IO process via `ChainQuery`
  - **Breaking:** `ChainQuery`, `ChainRequest`, `ChainResponse` have no generics (use `BlockId` directly)
  - **Breaking:** Chain tip moved from `ChainRequest` to `ChainQuery::tip()`

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

Checklists

All Submissions:

New Features:

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

Comment thread crates/chain/src/canonical_task.rs Outdated
@oleonardolima oleonardolima added this to the Wallet 3.0.0 milestone Sep 18, 2025
@notmandatory notmandatory moved this to In Progress in BDK Chain Sep 18, 2025
@oleonardolima oleonardolima force-pushed the refactor/canonical-iter-api branch 6 times, most recently from d851ba6 to c02636d Compare September 23, 2025 00:54
@oleonardolima oleonardolima added module-blockchain api A breaking API change labels Sep 23, 2025
@oleonardolima oleonardolima force-pushed the refactor/canonical-iter-api branch from c02636d to 78c0538 Compare September 23, 2025 01:08
@oleonardolima oleonardolima force-pushed the refactor/canonical-iter-api branch 3 times, most recently from 677e25a to 9e27ab1 Compare September 29, 2025 01:47
Comment thread crates/chain/src/canonical.rs
@oleonardolima oleonardolima marked this pull request as ready for review October 2, 2025 06:18
Copy link
Copy Markdown
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work.

This is my initial round of reviews.

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

Comment thread crates/core/src/chain_query.rs Outdated
Comment on lines +72 to +74
let chain_tip = chain.tip().block_id();
let task = graph.canonicalization_task(chain_tip, Default::default());
let canonical_view = chain.canonicalize(task);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about the following naming:

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

@oleonardolima oleonardolima Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, this one is a bit outdated.

As of 031de40 we have:

  • CanonicalizationTask -> CanonicalTask; and also new CanonicalViewTask.
  • TxGraph::canonicalization_task -> TxGraph::canonical_task.
  • LocalChain::canonicalize is still the same, though we now also have LocalChain::canonical_view.

I'm fine with these names for now, though if there's no consensus on those we can discuss/change in a follow-up PR.

Comment thread crates/chain/src/canonical_task.rs Outdated
Comment thread crates/chain/src/canonical_task.rs Outdated
Comment thread crates/chain/src/local_chain.rs Outdated
Comment thread crates/chain/src/canonical_task.rs Outdated
Comment thread crates/chain/src/canonical_task.rs Outdated
@oleonardolima oleonardolima force-pushed the refactor/canonical-iter-api branch from 9e27ab1 to f6c8b02 Compare October 3, 2025 00:33
Comment thread crates/core/src/chain_query.rs Outdated
Comment thread crates/chain/src/canonical_task.rs Outdated
Comment thread crates/core/src/chain_query.rs Outdated
@oleonardolima oleonardolima force-pushed the refactor/canonical-iter-api branch 4 times, most recently from a276c37 to b7f8fba Compare October 8, 2025 04:53
Comment thread crates/chain/tests/test_tx_graph_conflicts.rs Outdated
@oleonardolima oleonardolima force-pushed the refactor/canonical-iter-api branch 2 times, most recently from 57184dc to 6f8ef26 Compare April 3, 2026 00:23
@ValuedMammal
Copy link
Copy Markdown
Collaborator

I can take another look at it.

@ValuedMammal
Copy link
Copy Markdown
Collaborator

While reviewing I found it easier to structure the commits like this.

Introduces CanonicalTask, a new type that encapsulates block-anchor verification as an explicit query/response loop. The task walks the transaction graph in priority-ordered stages (assumed, anchored, seen, leftover) and emits batches of BlockIds for the caller to verify against a chain oracle, accepting responses via resolve_query. Once driven to completion, finish() returns a CanonicalTxs which can be converted to a CanonicalView via .view().

Migrates the entire codebase to the new CanonicalTask-based API, removing the now-superseded canonical_iter.rs and canonical_view.rs modules. Introduces canonical.rs to house the unified Canonical<A, T, P> type (aliased as CanonicalTxs and CanonicalView) along with the CanonicalReason-to-ChainPosition resolution in .view(). All consumers — tests, benchmarks, examples, and chain-sync integrations — are updated accordingly.

Removes the ChainOracle trait, which is no longer needed now that canonicalization is driven through CanonicalTask's explicit query/response interface rather than a generic trait. LocalChain's is_block_in_chain and get_chain_tip methods are retained as plain inherent methods (returning Option instead of Result<Option, Infallible>), simplifying the API surface without losing any functionality.

Comment on lines +168 to +182
match TxDescendants::new_exclude_root(
self.tx_graph,
*txid,
|_, desc_txid| -> Option<(Txid, &A)> {
// assert the descendant visited is canonical
self.canonical_txs
.contains_key(&desc_txid)
.then(|| {
self.direct_anchors
.get(&desc_txid)
.map(|anchor| (desc_txid, anchor))
})
.flatten()
},
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will stop when a descendant doesn't have a direct anchor which is incorrect as a direct anchor will follow after.

What should happen

  • The closure should return everything (no filter).
  • .next should stop once it hits a direct anchor.

What should be figured out in a follow up

Although TxDescendants does a BFS (which means we will find the "shallowest" confirmed anchor) - this does not guarantee it will be the "earliest confirmed" anchor (which is the ideal value to have).

}
}

CanonicalView::new(self.tip, view_order, view_txs, self.spends.clone())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit

Suggested change
CanonicalView::new(self.tip, view_order, view_txs, self.spends.clone())
CanonicalView::new(self.tip, view_order, view_txs, self.spends)

fn tip(&self) -> BlockId;

/// Returns the next query needed, or `None` if no more queries are required.
fn next_query(&mut self) -> Option<ChainRequest>;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested by Claude

Suggested change
fn next_query(&mut self) -> Option<ChainRequest>;
#[must_use]
fn next_query(&mut self) -> Option<ChainRequest>;

@evanlinjin
Copy link
Copy Markdown
Member

@oleonardolima Thanks for taking the PR this far. To push this forward, I would like to take over.

oleonardolima and others added 15 commits April 27, 2026 10:54
replace `CanonicalIter` with sans-io `CanonicalizationTask`

Introduces new `CanonicalizationTask`, which implements the canonicalization process
through a request/response pattern, that allow us to remove the
`ChainOracle` dependency in the future.

The `CanonicalizationTask` handles direct/transitive anchor determination, also tracks
already confirmed anchors to avoid redundant queries. After all the `CanonicalizationRequest`'s
have been resolved, the `CanonicalizationTask` can be finalized returning the final `CanonicalView`.

It batches all the anchors, which require a chain query, for a given transaction into a single
`CanonicalizationRequest`, instead of having multiple requests for each one.

- Add new `CanonicalizationTask`, relying on
  `Canonicalization{Request|Response}` for chain queries. It
- Replaces the old `CanonicalIter` usage, with new
  `CanonicalizationTask`.

BREAKING CHANGE: It replaces direct `ChainOracle` querying in canonicalization process, with
the new request/response pattern by `CanonicalizationTask`.
The new API introduces a sans-io behavior, separating the
canonicalization logic from `I/O` operations, it should be used as
follows:

1. Create a new `CanonicalizationTask` with a `TxGraph`, by calling:
   `graph.canonicalization_task(params)`
2. Execute the canonicalization process with a chain oracle (e.g
   `LocalChain`, which implements `ChainOracle` trait), by calling:
   `chain.canonicalize(task, chain_tip)`

- Replace `CanonicalView::new()` constructor with internal `CanonicalView::new()` for use by `CanonicalizationTask`
- Remove `TxGraph::try_canonical_view()` and `TxGraph::canonical_view()` methods
- Add `TxGraph::canonicalization_task()` method to create canonicalization tasks
- Add `LocalChain::canonicalize()` method to process tasks and return `CanonicalView`'s
- Update `IndexedTxGraph` to delegate canonicalization to underlying `TxGraph`

BREAKING CHANGE: Remove `CanonicalView::new()` and `TxGraph::canonical_view()` methods in favor of task-based approach
- Adds `CanonicalReason`, `ObservedIn`, and `CanonicalizationParams` to
  `canonical_task.rs` module, instead of using the ones from
  `canonical_iter.rs`.
- Removes the `canonical_iter.rs` file and its module declaration.

BREAKING CHANGE: `CanonicalIter` and all its exports are removed
…icalizationTask`

Introduce a new `ChainQuery` trait in `bdk_core` that provides an
interface for query-based operations against blockchain data. This trait
enables sans-IO patterns for algorithms that need to interact with blockchain
oracles without directly performing I/O.

The `CanonicalizationTask` now implements this trait, making it more composable
and allowing the query pattern to be reused for other blockchain query operations.

- Add `ChainQuery` trait with associated types for Request, Response, Context, and Result
- Implement `ChainQuery` for `CanonicalizationTask` with `BlockId` as context

BREAKING CHANGE: `CanonicalizationTask::finish()` now requires a `BlockId` parameter

Co-Authored-By: Claude <noreply@anthropic.com>
Make `ChainRequest`/`ChainResponse` generic over block identifier types to enable
reuse beyond BlockId. Move `chain_tip` into `ChainRequest` for better encapsulation
and simpler API.

- Make `ChainRequest` and `ChainResponse` generic types with `BlockId` as default
- Add `chain_tip` field to `ChainRequest` to make it self-contained
- Change `ChainQuery` trait to use generic parameter `B` for block identifier type
- Remove `chain_tip` parameter from `LocalChain::canonicalize()` method
- Rename `ChainQuery::Result` to `ChainQuery::Output` for clarity

BREAKING CHANGE:
- `ChainRequest` now has a `chain_tip` field and is generic over block identifier type
- `ChainResponse` is now generic with default type parameter `BlockId`
- `ChainQuery` trait now takes a generic parameter `B = BlockId`
- `LocalChain::canonicalize()` no longer takes a `chain_tip` parameter

Co-authored-by: Claude <noreply@anthropic.com>

refactor(chain): make `LocalChain::canonicalize()` generic over `ChainQuery`

Allow any type implementing `ChainQuery` trait instead of requiring
`CanonicalizationTask` specifically.

Signed-off-by: Leonardo Lima <oleonardolima@users.noreply.github.com>
- Unify both `unprocessed_anchored_txs` and `pending_anchored_txs` in a
  single `unprocessed_anchored_txs` queue.
- Changes the `unprocessed_anchored_txs from `Iterator` to `VecDeque`.
- Removes the `pending_anchored_txs` field and it's usage.
- Collects all `anchored_txs` upfront instead of lazy iteration.
- Add new `CanonicalStage` enum for tracking the different
  canonicalization phases/stages.
- Add new `try_advance()` method for stage progression.
- Add new `is_transitive()` helper to `CanonicalReason`.
- Change internal `confirmed_anchors` to `direct_anchors` for better
  clarity.
- Update the `resolve_query()` to handle staged-based processing.

Co-authored-by: Claude <noreply@anthropic.com>
Inline all stage-processing logic into `next_query()`, removing the
separate `try_advance()` method, `process_*_txs()` helpers, and
`is_finished()` from the `ChainQuery` trait. Add `AssumedTxs` as an
explicit first stage and `CanonicalStage::advance()` for centralized
stage transitions. Document the `ChainQuery` protocol contract.
…`Canonical<A, P>`

Separate concerns by splitting `CanonicalizationTask` into two phases:

1. `CanonicalTask` determines which transactions are canonical and why
   (`CanonicalReason`), outputting `CanonicalTxs<A>`.
2. `CanonicalViewTask` resolves reasons into `ChainPosition`s (confirmed
   vs unconfirmed), outputting `CanonicalView<A>`.

Make `Canonical<A, P>`, `CanonicalTx<P>`, and `FullTxOut<P>` generic over
the position type so the same structs serve both phases. Add
`LocalChain::canonical_view()` convenience method for the common two-step
pipeline.

Renames: `CanonicalizationTask` -> `CanonicalTask`,
`CanonicalizationParams` -> `CanonicalParams`,
`canonicalization_task()` -> `canonical_task()`,
`FullTxOut::chain_position` -> `FullTxOut::pos`.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…Query::tip()`

The chain tip is constant for the lifetime of a query, so it belongs on
the trait rather than being redundantly copied into every request.
`ChainRequest` is now a type alias for `Vec<B>`.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ChainResponse`

These types only ever used `BlockId`, so the generic parameter added
unnecessary complexity. All three are now hardcoded to `BlockId`.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Assumed transactions bypass the `AnchoredTxs` stage and are marked
canonical immediately with `CanonicalReason::Assumed`. Previously,
`view_task()` only queued anchor checks for transitive txs, so directly
assumed txs (`Assumed { descendant: None }`) were never checked and
always resolved to `Unconfirmed` even when they had confirmed anchors.

Queue all `Assumed` txs for anchor checks in `view_task()` and look up
`direct_anchors` for both `Assumed` variants in `finish()`.

Fixes bitcoindevkit#2088

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…onical_view_task.rs`

Move shared types (`CanonicalTx`, `Canonical`, `CanonicalView`, `CanonicalTxs`)
and convenience methods into `canonical.rs`. Keep only the phase-2 task
(`CanonicalViewTask`) in `canonical_view_task.rs`. Also rename `FullTxOut` to
`CanonicalTxOut` and move it to `canonical.rs`.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- ignore `canonical_task.rs` test module in coverage.
- reuse `canonical_view` in `test_tx_graph_conflicts.rs`
@evanlinjin evanlinjin force-pushed the refactor/canonical-iter-api branch from 6f8ef26 to 15ce547 Compare April 27, 2026 11:23
@oleonardolima
Copy link
Copy Markdown
Collaborator Author

@oleonardolima Thanks for taking the PR this far. To push this forward, I would like to take over.

@evanlinjin Alright, I'll take a look into your pushed commits.

oleonardolima and others added 2 commits April 28, 2026 11:44
- add new `test_canonical_view_task.rs` to handle different scenarios
  of chain position resolution.
- fixes the assumed canonical txs chain position resolution, especially for transitively
  assumed canonical transactions, where there's an anchored/confirmed descendant.
@evanlinjin evanlinjin force-pushed the refactor/canonical-iter-api branch from b54a71e to 8aa42d0 Compare April 28, 2026 11:44
Copy link
Copy Markdown
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a5d91e9

However, I agree with @ValuedMammal that the commits are a bit messy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api A breaking API change module-blockchain

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

Remove ChainOracle trait by inverting dependency

5 participants