Skip to content

Conversation

@ValuedMammal
Copy link
Collaborator

@ValuedMammal ValuedMammal commented Aug 12, 2025

Description

Use the new bdk_tx transaction building library to create PSBTs in BDK Wallet. Primary benefits include the use of bdk_coin_select as well as miniscript::plan module under the hood.

fix #164
fix #204

Notes to the reviewers

Remaining TODOs are listed in the table below #297 (comment).

It may help to look over the high level design and rationale which is available here.

Changelog notice

### Changed

- `TxOrdering::Custom` is generic over the input and output types.

### Added

- `psbt::params` module
- `PsbtParams` struct
- `CoinSelectionStrategy` enum
- `UtxoFilter` struct

Added `pub` methods on `Wallet`

- `Wallet::create_psbt{_with_rng}`
- `Wallet::replace_by_fee{_with_rng}`
- `Wallet::replace_by_fee_and_recipients`
- `Wallet::transactions_with_params`
- `Wallet::insert_tx`

- docs: Add `examples/psbt.rs`
- docs: Add `examples/rbf.rs`

Checklists

All Submissions:

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'm linking the issue being fixed by this PR

@ValuedMammal ValuedMammal moved this to In Progress in BDK Wallet Aug 12, 2025
@ValuedMammal ValuedMammal added this to the Wallet 3.0.0 milestone Aug 12, 2025
@ValuedMammal ValuedMammal self-assigned this Aug 12, 2025
@coveralls
Copy link

coveralls commented Aug 26, 2025

Pull Request Test Coverage Report for Build 19016316365

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 524 of 656 (79.88%) changed or added relevant lines in 5 files are covered.
  • 299 unchanged lines in 8 files lost coverage.
  • Overall coverage decreased (-0.03%) to 84.779%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/wallet/tx_builder.rs 7 12 58.33%
src/wallet/error.rs 0 19 0.0%
src/wallet/mod.rs 317 357 88.8%
src/psbt/params.rs 183 251 72.91%
Files with Coverage Reduction New Missed Lines %
src/wallet/signer.rs 3 81.11%
src/wallet/params.rs 9 76.24%
src/descriptor/template.rs 10 97.46%
src/descriptor/mod.rs 13 88.04%
src/descriptor/dsl.rs 16 95.36%
src/keys/mod.rs 69 79.49%
src/descriptor/policy.rs 89 79.17%
src/wallet/mod.rs 90 84.1%
Totals Coverage Status
Change from base Build 18318022693: -0.03%
Covered Lines: 7536
Relevant Lines: 8889

💛 - Coveralls

@thunderbiscuit
Copy link
Member

I like the create_psbt name for the function. The Params struct I need to do a quick search but off the top of my head it feels like we have a few of those scattered across the crates named exactly that, so it gets confusing. In my exploration PR I used TransactionParams, but that's a bit long and verbose.

What do you envision the API for replace-by-fee transactions look like in this new bdk-tx world? I'm picturing something like Wallet::create_replacement_psbt which takes a WalletTx and another Params struct.

@ValuedMammal ValuedMammal moved this from In Progress to Needs Review in BDK Wallet Sep 11, 2025
@ValuedMammal ValuedMammal marked this pull request as ready for review September 11, 2025 15:39
@ValuedMammal ValuedMammal changed the title [WIP] Implement create_psbt for Wallet Implement create_psbt for Wallet Sep 11, 2025
Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

So this is more of a conceptual review/question set, as I'm getting acquainted with the PR, and it also requires knowledge and understanding of the bdk-tx crate/workflow.

One general question I have is do you think is missing in functionality between this and the current TxBuilder? Can we make a todo list that compares functionality with the current TxBuilder to better visualize how close of a replacement this is, or if it only provides part of the functionality for now (and if so which parts)?

I haven't had time to look/test the examples and my day is over, but I'll come back to this on Monday.

}

/// Set the definite descriptor used for generating the change output.
pub fn change_descriptor(&mut self, desc: DefiniteDescriptor) -> &mut Self {
Copy link
Member

Choose a reason for hiding this comment

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

Is it correct to think that if not defined here, the change just goes to:

  1. The next index on Keychain::Internal if available
  2. If no internal keychain is available, the next index on the KeychainKind::External

Copy link
Member

@thunderbiscuit thunderbiscuit Sep 16, 2025

Choose a reason for hiding this comment

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

Ok awesome thanks for confirming. In this case, I'd just mention this in the docs (that this method is basically "if you want to send change elsewhere than your default change location").

/// `ReplaceParams` provides a thin wrapper around [`PsbtParams`] and is intended for
/// crafting Replace-By-Fee transactions (RBF).
#[derive(Debug, Default)]
pub struct ReplaceParams {
Copy link
Member

Choose a reason for hiding this comment

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

Can you tell me more about why we want this new ReplaceParams type? Naively I would have thought you'd just call PsbtParams::replace and you'd get a sort of pre-populated PsbtParams ready for replacing your tx, but I assume this doesn't quite work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Naively I would have thought you'd just call PsbtParams::replace and you'd get a sort of pre-populated PsbtParams ready for replacing your tx

That works too.

Copy link
Member

Choose a reason for hiding this comment

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

If that works that's my preferred approach, unless there is something I'm not seeing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry I didn't elaborate before. We want to provide some separation to make it difficult to misuse the API. Once you've committed to replacing a tx, you can't go back and fiddle with the params, instead you're limited to doing only what is permitted by the ReplaceParams, at least that's the general idea. This prevents a situation where some params override others and the implementation becomes unwieldy.

Right now the key difference is that you're not allowed to add more utxos (outpoints) in addition the ones being replaced, because it may lead to creating an invalid tx. Still open to suggestions for improvement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed ReplaceParams. Now calling PsbtParams::replace_txs will return a new PsbtParams that is marked by the "RBF" context. I think it's a cleaner approach overall without compromising type safety.

/// # Ok::<_, anyhow::Error>(())
/// ```
#[cfg(feature = "std")]
pub fn create_psbt(&self, params: PsbtParams) -> Result<(Psbt, Finalizer), CreatePsbtError> {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we return this Finalizer? I'd like to see that expanded upon in the docs. When looking into it I see that the Finalizer is

pub struct Finalizer {
    pub(crate) plans: HashMap<OutPoint, Plan>,
}

so probably something that helps the signers figure out what to sign and how to sign once you give them a psbt, but I'm not sure. The docs on Finalizer are also a bit... brief 😂😂

Copy link
Contributor

Choose a reason for hiding this comment

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

I still need to take a deeper look into bdk-tx, but I think the name could be misleading with Input Finalizer from the PSBT BIP-174 (if it's not following the same proposed idea).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is meant to handle the role of a PSBT input finalizer as described in BIP174.

Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

I did an initial review, with simple comments/questions. Still need to take a deeper look into bdk-tx and do a more thorough review here.

Comment on lines 264 to 267
/// No Bnb solution.
Bnb(bdk_coin_select::NoBnbSolution),
/// Non-sufficient funds
InsufficientFunds(bdk_coin_select::InsufficientFunds),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we have a generic CS error instead ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you elaborate on what you mean by generic CS error?

/// # Ok::<_, anyhow::Error>(())
/// ```
#[cfg(feature = "std")]
pub fn create_psbt(&self, params: PsbtParams) -> Result<(Psbt, Finalizer), CreatePsbtError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I still need to take a deeper look into bdk-tx, but I think the name could be misleading with Input Finalizer from the PSBT BIP-174 (if it's not following the same proposed idea).

@ValuedMammal
Copy link
Collaborator Author

Thank you for the quick review @thunderbiscuit @oleonardolima.

@ValuedMammal
Copy link
Collaborator Author

ValuedMammal commented Sep 20, 2025

TxBuilder vs PsbtParams feature comparison

subject to change

Feature TxBuilder PsbtParams Status
internal_policy_path NA
external_policy_path NA
manually_selected_only Done
include_output_redeem_witness_script NA
current_height Done
fee_rate Done
fee_absolute TODO
sighash (type) TODO
ordering Done
add_global_xpubs Done
allow_dust Done
unspendable NA
recipients Done
utxos Done
drain_wallet Done
drain_to/change_script Done
locktime Done
version Done
fallback_sequence Done
bumping_fee Done
only_witness_utxo Done
change_policy NA
add_foreign_utxo / planned_input Done
utxo_filter Done

N/A:
- *_policy_path: deprecated, ref: add_assets
- include_output_redeem_witness_script: deprecated, not used
- unspendable: NA, ref: utxo_filter
- change_policy: NA, ref: utxo_filter

@ValuedMammal
Copy link
Collaborator Author

ValuedMammal commented Oct 1, 2025

  • We need a way to implement fee_absolute or find out if this is possible using bdk_coin_select.
  • Note that UtxoFilter can accomplish a number of things from the old interface, such as "change policy" and "unspendable".
  • To prevent multiple parameters from trying to dictate the current height, the current_height option can be achieved via the assets.absolute_timelock.
  • current_height renamed to maturity_height, as it's only used as a parameter to is_mature.

@ValuedMammal
Copy link
Collaborator Author

We need a way to implement fee_absolute or find out if this is possible using bdk_coin_select.

I think a straightforward approach is to set a target feerate of 0 and add the fee amount to the output value_sum. The effect is to raise the cost of meeting the target by the amount of the target fee.

I can see it being relevant to CPFP, since there we're less concerned about the feerate of an individual tx and more so with the fee needed to bump a package to a target feerate.

@codecov
Copy link

codecov bot commented Dec 2, 2025

Codecov Report

❌ Patch coverage is 91.30435% with 60 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.82%. Comparing base (bec219a) to head (14c1d2c).

Files with missing lines Patch % Lines
src/wallet/error.rs 0.00% 19 Missing ⚠️
src/psbt/params.rs 93.68% 18 Missing ⚠️
src/wallet/mod.rs 94.95% 18 Missing ⚠️
src/wallet/tx_builder.rs 58.33% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #297      +/-   ##
==========================================
+ Coverage   85.24%   85.82%   +0.57%     
==========================================
  Files          23       24       +1     
  Lines        8230     8916     +686     
==========================================
+ Hits         7016     7652     +636     
- Misses       1214     1264      +50     
Flag Coverage Δ
rust 85.82% <91.30%> (+0.57%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ValuedMammal ValuedMammal force-pushed the feat/create_psbt branch 2 times, most recently from f00d309 to 3988abb Compare December 3, 2025 18:44
`TxOrdering` is made generic by exposing the generic from
`TxSort` function. The benefit is that we're no longer limited
to ordering lists of only `TxIn` or `TxOut`.

We use bitcoin `TxIn` and `TxOut` as the default type parameter
to maintain backward compatibility.

Add `sort_with_rng` for `TxOrdering<In, Out>` for sorting
two generic mutable slices.

This will be useful in a later commit by applying a
`TxOrdering` to the Input/Output sets of a `Selection`.
We add the `psbt::params` module along with new types
including `PsbtParams` and `SelectionStrategy`.

`PsbtParams` is mostly inspired by `TxParams` from `tx_builder.rs`,
except that we've removed support for `policy_path` in favor of
`add_assets` API. Further enhancements include `.utxo_filter` and
`.canonical_params` member fields.

`PsbtParams<C>` contains a type parameter `C` indicating the context
in which the parameters can be used. Methods related to PSBT
creation exist within the `CreateTx` context, and methods
related to replacements (RBF) exist within the `ReplaceTx`
context.

In `lib.rs` re-export everything under `psbt` module.

- deps: Add `bdk_tx` 0.1.0
- deps: Add `bdk_coin_select` 0.4.1
We use the new `PsbtParams` to add methods on `Wallet` for
creating PSBTs, including RBF transactions.

`Wallet::create_psbt` and `Wallet::replace_by_fee` each have
no-std counterparts that take an additional `impl RngCore` parameter.

Also adds a high level convenience method
`Wallet::replace_by_fee_and_recipients` that exposes the minimum
information needed to create an RBF.

This commit re-introduces the `Wallet::insert_tx` API for adding
transaction data to the wallet that may or may not be canonical from
the point of view of the TxGraph.

Added `Wallet::transactions_with_params` that allows customizing
the internal canonicalization logic.

Added new errors to `wallet::errors`

- `CreatePsbtError`
- `ReplaceByFeeError`
Adds additional tests to exercise `PsbtParams` logic and
PSBT creation.

- test-utils: Add `insert_tx_anchor` test helper for adding
a transaction to the wallet with associated anchor block.
- `examples/psbt.rs`
- `examples/rbf.rs`
@ValuedMammal ValuedMammal added new feature New feature or request and removed summit To review at rust bitcoin summit. labels Dec 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new feature New feature or request

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

Flexible RBF construction Redesign TxBuilder

5 participants