Introduce canonicalization parameters by evanlinjin · Pull Request #1808 · bitcoindevkit/bdk

evanlinjin · 2025-01-23T06:22:04Z

Partially Fixes bitcoindevkit/bdk_wallet#40

Description

Add the ability to modify the canonicalization algorithm. Right now, the only modifier is assume_canonical which takes in a Vec (ordered list) of txids and superimposes it on the canonicalization algorithm. Txs later in the list (higher index) have a higher priority (in case of conflicts).

Notes to the reviewers

None at the moment.

Changelog notice

Added CanonicalizationParams to allow the caller to modify the canonicalization algorithm. This in a new parameter on CanonicalIter::new.
Changed TxGraph::insert_tx to allow for updating a transaction's witness field. This is useful for initially introducing an unsigned tx and adding witnesses later on.

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

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

notmandatory · 2025-01-28T00:39:31Z

Would it make sense to add CanonicalizationMods as a field of TxGraph ?

This way the existing TxGraph method signatures don't change, you'd just need a new function to add TxIds to the CanonicalizationMods in the TxGraph.

And if you could only add new TxIds to your CanonicalizationMods this should still be monotone right?

Then you could also add the CanonicalizationMods data to the tx_graph::Changeset so it's persisted. Of course you'd need to update the persistence crates but wouldn't users need/want to persist these mods anyway so a reloaded wallet shows the same balance and utxos?

For Wallet I'm imaging some higher level functions for the things a wallet user needs. For instance (based on suggestions in bitcoindevkit/bdk_wallet#40):

change TxBuilder::finish to insert new unbroadcast tx and add a canonical mod so their inputs won't be double spent
add a canonical mod for a new RBF tx so it has precedence over the unconfirmed tx it's replacing
list unbroadcast tx (found in CanonicalizationMods but not seen)
add an option to TxBuilder to include inputs from unbroadcast tx(s), the new tx has higher canonicalization
remove tx from CanonicalizationMods after seen in mempool (or would this make TxGraph not monotone?)

crates/chain/src/canonical_iter.rs

crates/chain/src/tx_graph.rs

LLFourn · 2025-01-28T06:43:17Z

ConceptACK. Would be nice to have draft PR built on this that resolves bitcoindevkit/bdk_wallet#40 so we know that this is all that's needed at the chain level before merging this.

oleonardolima

cACK 0f00cdb

crates/chain/src/canonical_iter.rs

notmandatory · 2025-02-04T16:10:09Z

Took this out of the bdk_wallet 1.1 milestone since tagging the release tomorrow.

evanlinjin · 2025-02-06T10:40:32Z

Would it make sense to add CanonicalizationMods as a field of TxGraph ?

@notmandatory I do not think so. Different calls to canonicalize will be for different purposes. Especially when we add more fields to CanonicalizationMods (i.e. in #1809).

evanlinjin · 2025-02-06T10:42:34Z

And if you could only add new TxIds to your CanonicalizationMods this should still be monotone right?

We want ordering for these txids. In case of conflict between them, we need a way to prioritize ones over others. This order should be controlled by the caller.

evanlinjin · 2025-04-25T12:32:05Z

@LLFourn I have a draft PR (bitcoindevkit/bdk_wallet#220) which showcases that this API is enough for bdk_wallet to keep track of unbroadcasted transactions (and not accidentally replace them).

crates/chain/tests/test_tx_graph.rs

LagginTimes

Just a few consistency nits:

Commit message 53d6fa1 needs to be updated to rename CanonicalizationMods → CanonicalizationParams.
Same change needs to be done in the PR description changelog.

Besides these, this LGTM!

crates/chain/src/canonical_iter.rs

crates/chain/tests/common/tx_template.rs

Introduce `CanonicalizationParams` which is passed in to `CanonicalIter::new`. `CanonicalizationParams::assume_canonical` is the only field right now. This contains a list of txids that we assume to be canonical, superceding any other canonicalization rules.

LagginTimes

ACK 53afb35

ValuedMammal

ACK 53afb35

All comments addressed

ValuedMammal · 2025-05-01T14:14:11Z

Looks good to me.

@LLFourn Please re-review if you find any issues

evanlinjin requested review from LLFourn, ValuedMammal and notmandatory January 23, 2025 06:22

evanlinjin self-assigned this Jan 23, 2025

evanlinjin added the api A breaking API change label Jan 23, 2025

evanlinjin added this to the 1.1.0 milestone Jan 23, 2025

evanlinjin force-pushed the superimposed_canonicalization branch from 4169060 to 0f00cdb Compare January 23, 2025 10:31

evanlinjin mentioned this pull request Jan 23, 2025

Add canonicalization modifier: assume not canonical. #1809

Closed

5 tasks

LLFourn previously requested changes Jan 28, 2025

View reviewed changes

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

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

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

oleonardolima reviewed Jan 31, 2025

View reviewed changes

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

notmandatory removed this from the 1.1.0 milestone Feb 4, 2025

evanlinjin force-pushed the superimposed_canonicalization branch from c70b40b to b780142 Compare February 28, 2025 06:21

evanlinjin force-pushed the superimposed_canonicalization branch 2 times, most recently from f73f26e to c8f2ddf Compare March 7, 2025 07:38

evanlinjin force-pushed the superimposed_canonicalization branch 2 times, most recently from 37fa16d to d309780 Compare March 21, 2025 07:23

evanlinjin force-pushed the superimposed_canonicalization branch from d309780 to 03f5041 Compare April 17, 2025 05:21

notmandatory added this to BDK Chain Apr 22, 2025

notmandatory removed this from BDK Wallet Apr 22, 2025

notmandatory moved this to In Progress in BDK Chain Apr 22, 2025

notmandatory moved this from In Progress to Needs Review in BDK Chain Apr 22, 2025

evanlinjin added 2 commits April 25, 2025 14:22

feat(chain): Signed txs should displace unsigned txs in TxGraph.

5eb5e86

refactor(chain): Make private fields in CanonicalIter concise.

58fcf32

evanlinjin force-pushed the superimposed_canonicalization branch from 03f5041 to 53d6fa1 Compare April 25, 2025 04:53

evanlinjin mentioned this pull request Apr 25, 2025

Introduce broadcast queue. bitcoindevkit/bdk_wallet#220

Closed

8 tasks

evanlinjin requested review from LLFourn, LagginTimes and oleonardolima April 25, 2025 12:32

LagginTimes reviewed Apr 28, 2025

View reviewed changes

crates/chain/tests/test_tx_graph.rs Outdated Show resolved Hide resolved

LagginTimes reviewed Apr 29, 2025

View reviewed changes

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

crates/chain/tests/common/tx_template.rs Outdated Show resolved Hide resolved

evanlinjin force-pushed the superimposed_canonicalization branch from 0f5176b to 05fe0d5 Compare May 1, 2025 04:21

evanlinjin requested a review from LagginTimes May 1, 2025 04:23

evanlinjin changed the title ~~Introduce canonicalization modifiers.~~ Introduce canonicalization parameters May 1, 2025

evanlinjin and others added 2 commits May 1, 2025 15:24

test(graph): Add additional witness scenarios

53afb35

evanlinjin force-pushed the superimposed_canonicalization branch from 05fe0d5 to 53afb35 Compare May 1, 2025 05:25

LagginTimes approved these changes May 1, 2025

View reviewed changes

ValuedMammal approved these changes May 1, 2025

View reviewed changes

ValuedMammal merged commit 86cdaf4 into bitcoindevkit:master May 1, 2025
17 checks passed

github-project-automation bot moved this from Needs Review to Done in BDK Chain May 1, 2025

ValuedMammal mentioned this pull request May 1, 2025

Release bdk_chain v0.22.0 #1945

Closed

39 tasks

Conversation

evanlinjin commented Jan 23, 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:

Bugfixes:

Uh oh!

notmandatory commented Jan 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LLFourn commented Jan 28, 2025

Uh oh!

oleonardolima left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

notmandatory commented Feb 4, 2025

Uh oh!

evanlinjin commented Feb 6, 2025

Uh oh!

evanlinjin commented Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evanlinjin commented Apr 25, 2025

Uh oh!

Uh oh!

LagginTimes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

LagginTimes left a comment

Choose a reason for hiding this comment

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

ValuedMammal commented May 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

evanlinjin commented Jan 23, 2025 •

edited

Loading

evanlinjin commented Feb 6, 2025 •

edited

Loading