feat(chain): add new `list_ordered_canonical_txs` method #2027

oleonardolima · 2025-09-11T01:17:15Z

Description

It introduces a new method for topological ordering canonical transactions, list_ordered_canonical_txs. It now ensures the dependency-based transaction processing, guaranteeing that parent transactions always appears before their children transaction.

The existing list_canonical_txs and try_list_canonical_txs methods have been deprecated in favor of the new ordered version.

Notes to the reviewers

"[...] For those reviewing, and wondering why we don't have a fallible try version of this method, it's because we don't have a fallible ChainOracle implementation - we will get rid of ChainOracle trait soon anyway."

This PR is intended for a point release so that bdk_wallet 2.x users can get a topologically sorted list of transactions (we need a point release on bdk_wallet 2.x as well).

It might be useful to take a look at the new test scenarios that I've added, it shows some specific scenarios where the current implementation and output of canonical_txs didn't output the transactions in topological order.

Let me know if you think the TopologicalIter algorithm and/or API could be improved.

Changelog notice

#### Added
- New `list_ordered_canonical_txs` method to `TxGraph` that returns canonical transactions in topological order, ensuring parent transactions always appear before their children

#### Deprecated
- `list_canonical_txs` method - use `list_ordered_canonical_txs` instead for guaranteed topological ordering
- `try_list_canonical_txs` method - use `list_ordered_canonical_txs` instead for guaranteed topological ordering

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

evanlinjin · 2025-09-11T03:09:28Z

Nice diagrams

crates/chain/src/canonical_iter.rs

evanlinjin

The internals have completely changed. I don't think a point release would make any sense (we also have already merged other breaking changes). I recommend to just break the API.

Get rid of CanonicalIter. Just have a single function that returns impl Iterator<Item = ...>.

crates/chain/src/canonical_iter.rs

evanlinjin · 2025-09-19T04:22:54Z

crates/chain/src/canonical_iter.rs

 impl<A: Anchor, C: ChainOracle> Iterator for CanonicalIter<'_, A, C> {
    type Item = Result<(Txid, Arc<Transaction>, CanonicalReason<A>), C::Error>;

    fn next(&mut self) -> Option<Self::Item> {


Ideally, we would want most of this called in CanonicalIter::new. ~~However, I see that it requires us to return an error which would be a breaking change. We also can't store the error in CanonicalIter as the error type is a type parameter on ChainOracle.~~

Edit: Let's break the API. CanonicalIter no longer makes sense. We should just have a single function that returns TopologicalIter/impl Iterator<Item = ...>.

crates/chain/src/canonical_iter.rs

evanlinjin · 2025-09-25T10:52:47Z

crates/chain/tests/test_tx_graph.rs

+    }
+}
+
+fn is_txs_in_topological_order(txs: Vec<Txid>, tx_graph: TxGraph<BlockId>) -> bool {


Good thinking.

evanlinjin · 2025-09-25T11:26:04Z

crates/chain/src/tx_graph.rs

+    /// reorders the transactions based on their spending relationships.
+    ///
+    /// [`list_canonical_txs`]: Self::list_canonical_txs
+    pub fn list_ordered_canonical_txs<'a, C: ChainOracle<Error = Infallible>>(


For those reviewing, and wondering why we don't have a fallible try version of this method, it's because we don't have a fallible ChainOracle implementation - we will get rid of ChainOracle trait soon anyway.

This PR is intended for a point release so that bdk_wallet 2.x users can get a topologically sorted list of transactions (we need a point release on bdk_wallet 2.x as well).

evanlinjin

Looks good, just a few nits.

crates/chain/src/canonical_iter.rs

evanlinjin · 2025-09-25T13:52:05Z

crates/chain/src/canonical_iter.rs

+            use crate::ChainPosition;
+            use core::cmp::Ordering;
+
+            match (&a_tx.chain_position, &b_tx.chain_position) {
+                // Both confirmed: sort by confirmation height
+                (
+                    ChainPosition::Confirmed {
+                        anchor: a_anchor, ..
+                    },
+                    ChainPosition::Confirmed {
+                        anchor: b_anchor, ..
+                    },
+                ) => {
+                    let a_height = a_anchor.confirmation_height_upper_bound();
+                    let b_height = b_anchor.confirmation_height_upper_bound();
+                    a_height.cmp(&b_height)
+                }
+                // Confirmed comes before unconfirmed
+                (ChainPosition::Confirmed { .. }, ChainPosition::Unconfirmed { .. }) => {
+                    Ordering::Less
+                }
+                // Unconfirmed comes after confirmed
+                (ChainPosition::Unconfirmed { .. }, ChainPosition::Confirmed { .. }) => {
+                    Ordering::Greater
+                }
+                // Both unconfirmed: sort by first_seen (earlier timestamp first)
+                (
+                    ChainPosition::Unconfirmed {
+                        first_seen: a_first_seen,
+                        ..
+                    },
+                    ChainPosition::Unconfirmed {
+                        first_seen: b_first_seen,
+                        ..
+                    },
+                ) => {
+                    // Earlier timestamps come first
+                    a_first_seen.cmp(b_first_seen)
+                }
+            }


Wouldn't this be easier? Also, shouldn't we be sorting by first-seen (not last-seen)? I think the ChainPosition Ord impl already sorts by first-seen.

Suggested change

use crate::ChainPosition;

use core::cmp::Ordering;

match (&a_tx.chain_position, &b_tx.chain_position) {

// Both confirmed: sort by confirmation height

(

ChainPosition::Confirmed {

anchor: a_anchor, ..

},

ChainPosition::Confirmed {

anchor: b_anchor, ..

},

) => {

let a_height = a_anchor.confirmation_height_upper_bound();

let b_height = b_anchor.confirmation_height_upper_bound();

a_height.cmp(&b_height)

}

// Confirmed comes before unconfirmed

(ChainPosition::Confirmed { .. }, ChainPosition::Unconfirmed { .. }) => {

Ordering::Less

}

// Unconfirmed comes after confirmed

(ChainPosition::Unconfirmed { .. }, ChainPosition::Confirmed { .. }) => {

Ordering::Greater

}

// Both unconfirmed: sort by first_seen (earlier timestamp first)

(

ChainPosition::Unconfirmed {

first_seen: a_first_seen,

..

},

ChainPosition::Unconfirmed {

first_seen: b_first_seen,

..

},

) => {

// Earlier timestamps come first

a_first_seen.cmp(b_first_seen)

}

}

a_tx.cmp(&b_tx)

I think it can work, but I mean it's already using the first_seen.

oleonardolima · 2025-09-29T01:53:31Z

I think I'll keep this one for release/0.23.x, and open a new PR for master with the breaking changes.

It deprecates the existing `list_canonical_txs` and `try_list_canonical_txs` in favor of `list_ordered_canonical_txs` which guarantees topological ordering. The new `list_ordered_canonical_txs` ensures that spending transactions always appear after their inputs, topologically ordered in "spending order". - Add `TopologicalIterator` for level-based topological sorting - Use the `ChainPosition` for sorting within the same level - Add the new method to the canonicalization benchmarks - Update the new test to verify topological ordering correctness Co-Authored-By: Claude <noreply@anthropic.com>

- Update the `TopologicalIter` to take an Iterator<Item = CanonicalTx<'a, Arc<Transaction>, A> of canonical txs. - Update it's documentation to mention the Kahn's Algorithm. - Update it to use `a_tx.cmp(&b_tx)` instead.

ValuedMammal · 2025-09-30T19:47:51Z

I don't think you have to deprecate list_canonical_txs. It should just return the txs in topological order. CanonicalIter already canonicalizes the graph topologically (albeit reversed), only that it has an issue with splitting packages of related txs that should be canonicalized as a single batch.

oleonardolima added this to the Wallet 3.0.0 milestone Sep 11, 2025

oleonardolima requested review from LLFourn and evanlinjin September 11, 2025 01:17

oleonardolima self-assigned this Sep 11, 2025

oleonardolima added tests module-blockchain labels Sep 11, 2025

oleonardolima added this to BDK Chain Sep 11, 2025

oleonardolima force-pushed the test/add-canonical-iter-topological-order-tests branch 2 times, most recently from 6b76092 to a28cfdf Compare September 11, 2025 03:07

oleonardolima force-pushed the test/add-canonical-iter-topological-order-tests branch 11 times, most recently from 08e4ce8 to 70ee41b Compare September 16, 2025 04:49

This comment was marked as outdated.

Sign in to view

oleonardolima force-pushed the test/add-canonical-iter-topological-order-tests branch 2 times, most recently from 6239c8f to 0aab769 Compare September 16, 2025 06:10

oleonardolima moved this to In Progress in BDK Chain Sep 16, 2025

evanlinjin reviewed Sep 18, 2025

View reviewed changes

crates/chain/src/canonical_iter.rs Outdated Show resolved Hide resolved

oleonardolima force-pushed the test/add-canonical-iter-topological-order-tests branch from d57a580 to 3b79cba Compare September 19, 2025 01:52

evanlinjin requested changes Sep 19, 2025

View reviewed changes

oleonardolima force-pushed the test/add-canonical-iter-topological-order-tests branch 2 times, most recently from 0a17091 to 71c3744 Compare September 23, 2025 04:20

oleonardolima moved this from In Progress to Needs Review in BDK Chain Sep 24, 2025

oleonardolima requested review from evanlinjin and LagginTimes September 24, 2025 01:06

oleonardolima force-pushed the test/add-canonical-iter-topological-order-tests branch from 278d049 to c2a266d Compare September 24, 2025 01:28

evanlinjin reviewed Sep 25, 2025

View reviewed changes

oleonardolima force-pushed the test/add-canonical-iter-topological-order-tests branch from 0524b20 to cc0f010 Compare September 26, 2025 01:29

oleonardolima changed the base branch from master to release/chain-0.23.x September 29, 2025 06:30

oleonardolima modified the milestones: Wallet 3.0.0, Chain 0.24.0, Wallet 2.2.0 Sep 29, 2025

oleonardolima changed the base branch from release/chain-0.23.x to release/chain-0.23.2 September 29, 2025 06:38

oleonardolima force-pushed the test/add-canonical-iter-topological-order-tests branch from cc0f010 to ac95bd7 Compare September 29, 2025 06:41

oleonardolima added 4 commits September 29, 2025 16:45

fix(example_cli): clippy warnings

0a33219

fix(electrum): clippy warnings

377da74

chore(docs): fix typo in docs

834aa5e

chore(test): rename txid_to_name to proper tx_name_to_txid

1e3b8cc

oleonardolima force-pushed the test/add-canonical-iter-topological-order-tests branch from ac95bd7 to 49bf2b3 Compare September 29, 2025 06:45

oleonardolima and others added 2 commits September 29, 2025 16:47

test: add scenarios for canonical_txs topological order

fd0b35d

oleonardolima force-pushed the test/add-canonical-iter-topological-order-tests branch from 49bf2b3 to 573915f Compare September 29, 2025 06:47

refactor(chain)!: update TopologicalIter to take an iterator

d9b9318

- Update the `TopologicalIter` to take an Iterator<Item = CanonicalTx<'a, Arc<Transaction>, A> of canonical txs. - Update it's documentation to mention the Kahn's Algorithm. - Update it to use `a_tx.cmp(&b_tx)` instead.

oleonardolima force-pushed the test/add-canonical-iter-topological-order-tests branch from 573915f to d9b9318 Compare September 29, 2025 06:54

oleonardolima mentioned this pull request Sep 30, 2025

Release v2.2.0 bitcoindevkit/bdk_wallet#322

Open

2 tasks

oleonardolima requested review from ValuedMammal and evanlinjin September 30, 2025 00:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(chain): add new `list_ordered_canonical_txs` method #2027

feat(chain): add new `list_ordered_canonical_txs` method #2027

oleonardolima commented Sep 11, 2025 •

edited

Loading

Uh oh!

evanlinjin commented Sep 11, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

evanlinjin left a comment

Uh oh!

Uh oh!

Uh oh!

evanlinjin Sep 19, 2025

Uh oh!

Uh oh!

evanlinjin Sep 25, 2025

Uh oh!

evanlinjin Sep 25, 2025

Uh oh!

evanlinjin left a comment

Uh oh!

Uh oh!

Uh oh!

evanlinjin Sep 25, 2025

Uh oh!

oleonardolima Sep 26, 2025

Uh oh!

oleonardolima commented Sep 29, 2025

Uh oh!

ValuedMammal commented Sep 30, 2025

Uh oh!

Uh oh!

feat(chain): add new list_ordered_canonical_txs method #2027

Are you sure you want to change the base?

feat(chain): add new list_ordered_canonical_txs method #2027

Conversation

oleonardolima commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

Uh oh!

evanlinjin commented Sep 11, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

evanlinjin Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

evanlinjin Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

evanlinjin Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

evanlinjin Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

oleonardolima Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

oleonardolima commented Sep 29, 2025

Uh oh!

ValuedMammal commented Sep 30, 2025

Uh oh!

Uh oh!

feat(chain): add new `list_ordered_canonical_txs` method #2027

feat(chain): add new `list_ordered_canonical_txs` method #2027

oleonardolima commented Sep 11, 2025 •

edited

Loading