Skip to content

Add canonicalization modifier: assume not canonical. #1809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

evanlinjin
Copy link
Member

Description

This is useful for creating RBF transactions where you do not want to spend from txs you are replacing or descendants of txs you are replacing.

I noticed that some users think it is useful to implicitly cancel a tx. This can also be used for that. cc. #1799, #1764.

Notes to the reviewers

This PR is based on #1808. Please review and merge that first.

Changelog notice

WIP

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

Introduce `CanonicalizationMods` which is passed in to
`CanonicalIter::new`.

`CanonicalizationMods::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.
Also change `TxTemplate` API to allow for testing with
`assume_canonical`.
This is useful for crafting RBF transactions where you do not want to
pick inputs from txs you are replacing or descendants of txs you are
replacing.
@evanlinjin evanlinjin self-assigned this Jan 23, 2025
@evanlinjin evanlinjin added the api A breaking API change label Jan 23, 2025
@evanlinjin evanlinjin added this to the 1.1.0 milestone Jan 23, 2025
@LLFourn
Copy link
Contributor

LLFourn commented Jan 28, 2025

As in #1808 it would be nice to have a draft PR that uses this to clean up RBF in wallet to see if we've got everything we need here.

@notmandatory notmandatory removed this from the 1.1.0 milestone Feb 4, 2025
@notmandatory
Copy link
Member

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

@evanlinjin
Copy link
Member Author

I've realized there's a flaw with the assume_not_canonical approach.

Consider a case where transaction A has one output A:0. Both B and C spend A:0, making them conflicting transactions. If we simply assume that B is not canonical, C will still end up spending A:0 in the canonical result — assuming nothing else conflicts with A, C, or their ancestors.

Since this feature is intended to support RBF behavior, this logic doesn't actually help us. The correct CanonicalizationParams field would be something like:

assume_not_spent: HashSet<OutPoint>,

@evanlinjin evanlinjin closed this May 1, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Done in BDK Chain May 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants