feat: Add Wallet::create_psbt - #516
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #516 +/- ##
==========================================
+ Coverage 81.15% 81.74% +0.58%
==========================================
Files 24 25 +1
Lines 5552 6469 +917
Branches 247 295 +48
==========================================
+ Hits 4506 5288 +782
- Misses 970 1082 +112
- Partials 76 99 +23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
oleonardolima
left a comment
There was a problem hiding this comment.
I left a few comments, and questions. I'd like to do some manual testing too, not only with the examples in regtest.
The commit history could use some work, by rewriting and restructuring it, it'll help other reviewers.
| pub type Rbf = ReplaceTx; | ||
|
|
||
| /// Parameters to create a PSBT. | ||
| // TODO: Can we derive `Clone` for this? |
There was a problem hiding this comment.
nit: are you looking forward to address this in a follow-up ?
There was a problem hiding this comment.
To derive Clone for PsbtParams, we need ChangeScript: Clone which in turn depends on rust-bitcoin/rust-miniscript#886.
| /// Marker type representing the PSBT creation state. | ||
| #[derive(Debug)] | ||
| pub struct CreateTx; | ||
|
|
||
| /// Marker type representing the Replace-By-Fee (RBF) state. | ||
| #[derive(Debug)] | ||
| pub struct ReplaceTx; |
There was a problem hiding this comment.
question: food for thought, do you think having these structs is better than having an enum (as it's representing a state), for example ?
There was a problem hiding this comment.
The marker type is used as the generic type parameter and this affords type safety: you can't call replace_by_fee with a PsbtParams<CreateTx> for example. Indeed there may be other variations of the typestate pattern in rust, https://cliffle.com/blog/rust-typestate/.
| txos.into_iter().filter(move |txo| { | ||
| // Exclude outputs that are manually selected. | ||
| if params.set.contains(&txo.outpoint) { | ||
| return false; | ||
| } | ||
| // Filter outputs according to `policy` fn. | ||
| if !policy(txo) { | ||
| return false; | ||
| } | ||
| // Exclude locked UTXOs. | ||
| if self.is_outpoint_locked(txo.outpoint) { | ||
| return false; | ||
| } | ||
| // Exclude immature outputs. | ||
| if !txo.is_mature(current_height) { | ||
| return false; | ||
| } | ||
| // Exclude spent outputs. | ||
| if txo.spent_by.is_some() { | ||
| return false; | ||
| } | ||
| true | ||
| }) |
There was a problem hiding this comment.
nit: IIRC you could do it lazily by chaining the .filter's instead.
There was a problem hiding this comment.
This is a style preference. It's equally lazy as far as I can tell.
evanlinjin
left a comment
There was a problem hiding this comment.
Outstanding review comments from #297.
| /// One of the transactions to be replaced is already confirmed | ||
| TransactionConfirmed(Txid), |
There was a problem hiding this comment.
I don't think this error variant earns it's keep.
-
Most callers would want to check whether the transaction is still unconfirmed before trying to replace it. A caller that doesn't do that obviously wants to create a transaction that conflicts with a confirmed one.
-
The guard is partial. It only catches "already confirmed at build time". The original transaction can be confirmed before the replacement is ready to broadcast.
There was a problem hiding this comment.
I agree with @noahjoeris on this one #297 (comment)
| .iter() | ||
| .filter_map(|&txid| { | ||
| let tx = self.tx_graph.graph().get_tx(txid)?; | ||
| self.calculate_fee(&tx).ok() |
There was a problem hiding this comment.
Do you think we should return an error if an original's descendant cannot have it's fee calculated? The current implementation has a risk of undershooting the RBF fee floor.
There was a problem hiding this comment.
Currently calculating the descendant fee is done on a best-effort basis. IMO failure to calculate a descendant's fee shouldn't prevent you from creating the transaction.
notmandatory
left a comment
There was a problem hiding this comment.
I did an initial review (with LLM help) and new changes look good.
One other suggestion, but doesn't need to be done in this PR, is figure out how we want to document experimental and possibly unstable APIs in the rust docs.
The LLM suggested adding:
/// **Unstable**: requires the `bdk-tx` Cargo feature and the
/// `--cfg bdk_wallet_unstable` rustc flag; may change in any minor release.
///
Prepare the repository for upcoming unstable-gated API commits by proving the cfg workflow end-to-end before any new surface is added. This change wires bdk_wallet_unstable into CI coverage, testing, linting, and docs checks, while keeping no-default-features and no_std builds validated in a stable lane. It also splits local just tasks into stable and unstable paths so pre-push validation correctly exercises both.
Introduce the unstable create_psbt and replace_by_fee APIs behind the bdk-tx feature and --cfg bdk_wallet_unstable. This adds PsbtParams and SelectionStrategy, integrates bdk_tx into wallet PSBT/RBF construction, re-exports bdk_tx when enabled, and includes supporting error types needed by the new flow.
f9507e4 to
a150344
Compare
How is the nLockTime set?First, bdk_tx takes the min_locktime (defaulting to The four scenarios then fall out naturally: Neither min_locktime nor anti_fee_sniping set: min_locktime = LockTime::ZERO. Final locktime is the maximum CLTV requirement across all inputs, or zero if none of the inputs carry a CLTV. Only min_locktime set: The floor is raised from zero to the caller-specified value. Final locktime = max(min_locktime, max(input CLTVs)), unit-compatible. Useful for, e.g., enforcing a block-height floor independently of any script requirements. Only anti_fee_sniping set: min_locktime = zero, so the pre-AFS locktime is just the input CLTVs (or zero). AFS then applies BIP326: with 50/50 probability it sets tx.lock_time to tip_height, otherwise it sets a relative sequence on a Taproot input. If the locktime after CLTV accumulation is already above tip_height (e.g., a CLTV that pins to a future height), AFS leaves it untouched. Note that AFS is not automatically derived from the wallet's chain tip; you must supply the current block height explicitly. Both min_locktime and anti_fee_sniping set: min_locktime acts as the floor going into AFS. The final value is effectively max(min_locktime, input CLTVs, tip_height) — though the exact outcome depends on BIP326's nLockTime vs nSequence coin flip. If min_locktime is already above tip_height, AFS's locktime branch won't change it, so the min_locktime effectively wins. Note that
Some important nuances:
The fallback in So the full picture of locktime inputs is actually three sources, one of which is indirect:
|
|
Tested the PSBT and RBF happy path locally on a fork of |
|
@ValuedMammal Are you looking forward to address the other comments in a follow-up ? |
|
@oleonardolima No follow-up plans yet, except maybe the docs suggestion #516 (review). |
Description
This PR introduces
Wallet::create_psbtandWallet::replace_by_fee- a new PSBT construction API built on top ofbdk-tx. This is a second attempt at integrating the planning module that started with #297. The scope is mostly the same and incorporates a number of improvements noted on #297. Notably the entire surface area is gated behind two compile flags.flags:
bdk-txCargo feature--cfg bdk_wallet_unstablerustc flagEnabling
bdk-txwithout also passingbdk_wallet_unstableis a compile error with a clear diagnostic. This allows us to ship the API for early adopters while reserving the right to break it in a minor release.Users can opt in by adding the following to your project's
.cargo/config.toml:and enable the feature in
Cargo.toml:New public API
Wallet::create_psbtWallet::create_psbt_with_rngWallet::replace_by_feeWallet::replace_by_fee_with_rngPsbtParams<C>CisCreateTxorReplaceTx/RbfSelectionStrategyAll,SingleRandomDraw,LowestFee,CustomCreatePsbtErrorcreate_psbtReplaceByFeeErrorreplace_by_feebdk_txre-exportbdk_txTxOrderingis also generalized toTxOrdering<In, Out>to accommodatebdk-tx's typed input/output types.TxOrdering<In, Out>adds default type parameters(
TxIn,TxOut) that are identical to the previous concrete type, so remainssemver-compatible.
Existing callers that don't use the
Customvariant are unaffected.Differences from #297
create_psbtfor Wallet #297 were trimmed; the public API is narrower and more focused.bdk_txre-export -bdk_wallet::bdk_txis re-exported so callers who needbdk-txtypes directly don't need a separatebdk_txdependency.SelectionStrategy::Allreplacesdrain_wallet- modeling "sweep everything" as a strategy is cleaner than aPsbtParamsoption.SelectionStrategy::Custom- users can supply their own coin-selection algorithm as a closure without having to implement a trait.SingleRandomDrawcorrectness fix - shuffling now only runs whenSingleRandomDrawis active; the extrashuffle_slicecalls are removed.bdk_wallet_unstablecfg gate +bdk-txCargo feature - the fullcreate_psbt/replace_by_feesurface is hidden unless explicitly opted into.Changelog notice
Before submitting