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 ?
| /// 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 ?
| I: IntoIterator<Item = FullTxOut<ConfirmationBlockTime>> + 'a, | ||
| F: Fn(&FullTxOut<ConfirmationBlockTime>) -> bool + 'a, | ||
| { | ||
| let current_height = params.maturity_height.unwrap_or(self.chain.tip().height()); |
There was a problem hiding this comment.
nit: it'd be clear if you stick to maturity_height, or just height.
| 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.
| pub struct PsbtParams<C> { | ||
| /// Set of selected UTXO outpoints. | ||
| /// Set of selected UTXO outpoints, `HashSet` ensures uniqueness | ||
| pub(crate) set: HashSet<OutPoint>, |
There was a problem hiding this comment.
question: woulndn't be using something as select be clearer ?
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.
| // For each txid being replaced, verify that at least one of its original inputs | ||
| // remains in the selected set. A replacement must conflict with every transaction it | ||
| // replaces — two transactions cannot spend the same UTXO. |
There was a problem hiding this comment.
| // For each txid being replaced, verify that at least one of its original inputs | |
| // remains in the selected set. A replacement must conflict with every transaction it | |
| // replaces — two transactions cannot spend the same UTXO. | |
| // A replacement must conflict (double-spend) with each tx it replaces - so require at least | |
| // one of each original's inputs to remain selected. |
wdyt?
| .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.
| let descendant_fee: Amount = descendants | ||
| .iter() | ||
| .filter_map(|&txid| { | ||
| let tx = self.tx_graph.graph().get_tx(txid)?; |
There was a problem hiding this comment.
| let tx = self.tx_graph.graph().get_tx(txid)?; | |
| let tx = self.tx_graph.graph().get_tx(txid).expect("tx must exist as they were obtained via the TxGraph"); |
We obtained descendants directly from TxGraph. So they must exist.
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.
///
| let err = bdk_coin_select::InsufficientFunds { | ||
| missing: target_amount.to_sat(), | ||
| }; | ||
| return Err(CreatePsbtError::InsufficientFunds(err))?; |
There was a problem hiding this comment.
nit: it looks strange to throw an error while returning an error. The below should be more idiomatic.
| return Err(CreatePsbtError::InsufficientFunds(err))?; | |
| return Err(CreatePsbtError::InsufficientFunds(err).into()); |
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 |
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