feat(utxo-locking)!: Add commands to lock, unlock and list locked UTXOs#294
feat(utxo-locking)!: Add commands to lock, unlock and list locked UTXOs#294tvpeter wants to merge 13 commits into
Conversation
- add BdkCli helper struct and impl methods - add integration tests for the key subcommand operations
- Add integration tests for wallets, descriptor, compile and config modules
- add integration tests for offline wallet operations
- Add test for creating, signing, broadcasting tx - Add test for combining psbt, fee bump and other psbt related commands - Add bip322 tests
- add saving proxy_opts for electrum and esplora clients to config and reading values - fix prepare_home_dir util fn
- Update bdk_wallet to v3.1.0 - Replaced Network enum with NetworkKind - Replaced `include_output_redeem_witness_script` with `add_global_xpubs` - update bdk_redb to v0.2.0
- update bdk_kyoto to v0.17.0 - add ew broadcast/subscriber API for KyotoClient
- add wallet commands to lock, unlock and list locked utxos - update unspent command to show status of each outpoint
-Temporary disable bip322 feature
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #294 +/- ##
==========================================
+ Coverage 56.89% 57.47% +0.57%
==========================================
Files 22 22
Lines 3661 3694 +33
==========================================
+ Hits 2083 2123 +40
+ Misses 1578 1571 -7
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:
|
-add test for locking, unlocking and listing locked utxos
e26cf5e to
aba6c66
Compare
|
Concept ACK, but I have a few concerns I want to point out. |
|
I noticed the bip322 feature, its commands, and integration tests have been commented out. I assume this was because of the hard dependency conflict between bdk_bip322 (which requires bdk_wallet ^2.1.0) and the bdk_wallet v3.1.0 dependency upgrade and that you left the code here to make it easier to restore later. However, leaving large blocks of dead code may lead to code rot. Instead of commenting it out, could we cleanly remove the bip322 code in an isolated commit and open a tracking issue to reintroduce it? When bdk_bip322 is updated, we can simply git revert that removal commit to restore the work without needing to re-implement the feature from scratch. |
Thank you for your suggestion. The reason the bip322 feature is currently commented out is that the maintainer of bdk_bip322 @aagbotemi, promised to update the library soon when I reached out to him. I commented out the feature to ensure that the CI checks are passing and to get reviews. |
| for out_point in &self.utxos { | ||
| if wallet.get_utxo(*out_point).is_none() { | ||
| eprintln!("warning: {out_point} is not a known wallet UTXO; locking anyway"); | ||
| } | ||
| wallet.lock_outpoint(*out_point); | ||
| } |
There was a problem hiding this comment.
This prints a warning but proceeds to lock the outpoint even when wallet.get_utxo returns None. Is this intentional?
If it is intentional, does locking this unknown outpoint pollute the persisted lock-state DB with a nonexistent record? Since this outpoint will never appear in list_unspent and thus can never be spent by the wallet, will this orphaned lock record sit in the database forever and cause permanent bloat?
There was a problem hiding this comment.
If it is intentional, does locking this unknown outpoint pollute the persisted lock-state DB with a nonexistent record?
My understanding - the use case for locking a currently-unknown outpoint is that the user already knows the UTXO but the wallet doesn't yet (it hasn't synced). After a sync the wallet learns the UTXO, so it shows up among the locked ones and the lock takes effect for coin selection.
Since bdk-cli can't know in advance whether a locked outpoint will become a known wallet UTXO later or not, I can't think of a good solution against the possible DB bloat here - filtering out "unknown" outpoints would also drop these legitimate pre-sync locks.
That said, bdk-cli targets technically advanced users, so I'd lean on the assumption that the user knows what they're doing and won't lock arbitrary UTXOs.
|
Regarding the tests, I've been considering a few edge cases:
|
- update bdk_bip322 crate with replacement crate bdk_message_signer - re-enable bip322 feature
For exhaustive tests that cover all edge cases, check out the UTXO Locking feature in |
| #[derive(Parser, Debug, Clone, PartialEq)] | ||
| pub struct LockUtxoCommand { | ||
| /// Outpoint(s) to lock, format TXID:VOUT. | ||
| #[arg(env = "TXID:VOUT", long = "utxo", required = true, value_parser = parse_outpoint)] |
There was a problem hiding this comment.
env = "TXID:VOUT" tells clap to read this argument from an env var named TXID:VOUT when it isn't passed on the command line.
The catch is that an env var with that name can't actually be set: a name containing : isn't a valid shell identifier, so export rejects it:
$ export TXID:VOUT=foo
export: not valid in this context: TXID:VOUT
Might be worth renaming the env var to a more shell-compatible name (without :).
|
|
||
| if self.offline_signer { | ||
| tx_builder.include_output_redeem_witness_script(); | ||
| tx_builder.add_global_xpubs(); |
There was a problem hiding this comment.
This makes tx building (finish()) error with CreateTxError::MissingKeyOrigin if any descriptor key has no origin - a bare xpub with no [fingerprint/path] prefix (a master key is fine).
The previous include_output_redeem_witness_script() didn't have this requirement - in the bdk_wallet version currently on master (2.4.0) that flag was actually never read (checked the source), so it was effectively a no-op. add_global_xpubs() does populate the xpubs but is stricter, so in some cases it now surfaces this error.
bdk-cli's own descriptors always carry an origin, so they're fine. Just flagging that an imported watch-only descriptor with a bare origin-less xpub (wpkh(tpub.../0/*)) would error here where it previously produced a PSBT. Might be worth a docs note about the origin requirement.
(Same swap in create_sp_tx, bump_fee, create_dns_tx.)
| for out_point in &self.utxos { | ||
| if wallet.get_utxo(*out_point).is_none() { | ||
| eprintln!("warning: {out_point} is not a known wallet UTXO; locking anyway"); | ||
| } | ||
| wallet.lock_outpoint(*out_point); | ||
| } |
There was a problem hiding this comment.
If it is intentional, does locking this unknown outpoint pollute the persisted lock-state DB with a nonexistent record?
My understanding - the use case for locking a currently-unknown outpoint is that the user already knows the UTXO but the wallet doesn't yet (it hasn't synced). After a sync the wallet learns the UTXO, so it shows up among the locked ones and the lock takes effect for coin selection.
Since bdk-cli can't know in advance whether a locked outpoint will become a known wallet UTXO later or not, I can't think of a good solution against the possible DB bloat here - filtering out "unknown" outpoints would also drop these legitimate pre-sync locks.
That said, bdk-cli targets technically advanced users, so I'd lean on the assumption that the user knows what they're doing and won't lock arbitrary UTXOs.

Description
This PR updates the Wallet API to v3.1.0 and adds wallet subcommands to lock and unlock UTXOs. Locked outpoints are excluded from coin selection, and the lock state is stored in the wallet. It also updates the
bdk_redbto v0.2.0 andbdk_kyototo v0.17.0.Fixes #293 and builds upon #289 and #278
Notes to the reviewers
Changelog notice
lock_utxocommandunlock_utxocommandlocked_uxtoscommandNetworkenum withNetworkKindinclude_output_redeem_witness_scriptwithadd_global_xpubsin TxBuildersubmit_packagewithbroadcast_randomfor broadcasting transactions in KyotoClientChecklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
CHANGELOG.md