-
Notifications
You must be signed in to change notification settings - Fork 40
Implement create_psbt
for Wallet
#297
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
base: master
Are you sure you want to change the base?
Conversation
673d602
to
77e1d20
Compare
Pull Request Test Coverage Report for Build 18172758369Details
💛 - Coveralls |
I like the What do you envision the API for replace-by-fee transactions look like in this new bdk-tx world? I'm picturing something like |
2c54be1
to
6f525d1
Compare
6f525d1
to
4493cbd
Compare
create_psbt
for Walletcreate_psbt
for Wallet
There was a problem hiding this 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.
/// | ||
/// Return `None` if `outpoint` doesn't correspond to an indexed txout, or | ||
/// if the assets are not sufficient to create a plan. | ||
fn try_plan(&self, outpoint: OutPoint, assets: &Assets) -> Option<Plan> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a favourite resource for learning about this plan thing? I'm looking at the docs here and I get a rough idea of how it must work but I'm wondering if there is this "standard expected reading" on it or something similar that I've just missed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://hackmd.io/@valuedmammal/B1lxzomj0 It could use an update though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect thank you! Quick and easy read. Cements my high-level understanding of it.
src/psbt/params.rs
Outdated
/// Coin select strategy. | ||
#[derive(Debug, Clone, Copy, Default)] | ||
#[non_exhaustive] | ||
pub enum SelectionStrategy { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the coin selection strategy as a simple enum rather than the CoinSelectionAlgorithm trait. Easier to use and reason about, and most people will only ever need the standard ones (which I assume can eventually get added to this enum) or pick the individual UTXOs themselves.
/// | ||
/// A single outpoint may appear at most once in the list of UTXOs to spend. The caller is | ||
/// responsible for ensuring that elements of `outpoints` correspond to outputs of previous | ||
/// transactions and are currently unspent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the error thrown if this requirement is not met? It's a good place to let people know and I would add it to the docs here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wallet::create_psbt
will throw a CreatePsbtError::UnknownUtxo
if the outpoint doesn't match a previously indexed outpoint (i.e. a wallet-owned tx output). I agree it should be documented somewhere.
} | ||
|
||
/// Set the definite descriptor used for generating the change output. | ||
pub fn change_descriptor(&mut self, desc: DefiniteDescriptor) -> &mut Self { |
There was a problem hiding this comment.
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:
- The next index on Keychain::Internal if available
- If no internal keychain is available, the next index on the KeychainKind::External
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
/// # Ok::<_, anyhow::Error>(()) | ||
/// ``` | ||
#[cfg(feature = "std")] | ||
pub fn create_psbt(&self, params: PsbtParams) -> Result<(Psbt, Finalizer), CreatePsbtError> { |
There was a problem hiding this comment.
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 😂😂
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
wallet/Cargo.toml
Outdated
bip39 = { version = "2.0", optional = true } | ||
bdk_file_store = { version = "0.21.1", optional = true } | ||
anyhow = { version = "1.0.98", optional = true } | ||
tempfile = { version = "3.20.0", optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this can be removed, as it's not being used.
/// No Bnb solution. | ||
Bnb(bdk_coin_select::NoBnbSolution), | ||
/// Non-sufficient funds | ||
InsufficientFunds(bdk_coin_select::InsufficientFunds), |
There was a problem hiding this comment.
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 ?
/// # Ok::<_, anyhow::Error>(()) | ||
/// ``` | ||
#[cfg(feature = "std")] | ||
pub fn create_psbt(&self, params: PsbtParams) -> Result<(Psbt, Finalizer), CreatePsbtError> { |
There was a problem hiding this comment.
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).
Thank you for the quick review @thunderbiscuit @oleonardolima. |
TxBuilder vs PsbtParams feature comparisonsubject to change
* See RbfParams |
Add module psbt/params.rs and introduce PsbtParams. test: Add `test_create_psbt` Introduce `ReplaceParams` Add `Wallet::replace_by_fee_and_recipients` Add `Wallet::replace_by_fee_with_aux_rand` test: `test_sanitize_rbf_set` test: Add `test_replace_by_fee` test: Add `test_spend_non_canonical_txout`
4493cbd
to
a7afecb
Compare
`UtxoFilter` is a user-defined Fn closure which takes a `&FullTxOut` and decides whether to exclude it from coin selection. This can be used, for example, to mark an output unspendable or apply custom UTXO filtering logic.
..by exposing the generic from `TxSort` function. We use bitcoin `TxIn` and `TxOut` as the default type parameter for backward compatibility. Add `sort_with_aux_rand` for `TxOrdering<In, Out>` for sorting two generic mutable slices.
..function on the Selection inputs and outputs.
This is a simple data structure which contains the manually selected outpoints enforcing order and uniqueness.
Check that adding duplicate outpoints only results in a single outpoint added, and that it is contained within the inner set.
|
Description
Use the new
bdk_tx
transaction building library to create PSBTs in BDK Wallet. Primary benefits include the use ofbdk_coin_select
as well asminiscript::plan
module under the hood.close #164
Notes to the reviewers
Remaining TODOs can be done in subsequent PRs, including adding support for sorting transaction inputs and outputs, i.e.
TxOrdering
.Changelog notice
Checklists
All Submissions:
New Features:
Bugfixes:
This pull request breaks the existing API