From 21c899f9ec2121e06189a1c6a63c4d7c9f213585 Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Mon, 7 Apr 2025 16:00:53 +0100 Subject: [PATCH 1/5] gui: fix docstring --- liana-gui/src/lianalite/client/backend/api.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/liana-gui/src/lianalite/client/backend/api.rs b/liana-gui/src/lianalite/client/backend/api.rs index 131e04c9..3f4c5d06 100644 --- a/liana-gui/src/lianalite/client/backend/api.rs +++ b/liana-gui/src/lianalite/client/backend/api.rs @@ -407,7 +407,7 @@ pub mod payload { pub recipients: Vec, /// The outpoints of coins to use as transaction inputs. If empty, /// coins will be selected automatically from the set of confirmed coins - /// and those unconfirmed coins at a change address, excluding immature + /// and those unconfirmed coins that are from self, excluding immature /// coins. pub inputs: &'a [bitcoin::OutPoint], // The feerate to use for this transaction. From ce711ae10afa343c7228abe4be55cb615c3d971b Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Mon, 7 Apr 2025 09:30:49 +0100 Subject: [PATCH 2/5] commands: add test for `create_recovery` --- lianad/src/commands/mod.rs | 124 +++++++++++++++++++++++++++++++++++++ lianad/src/testutils.rs | 16 ++++- 2 files changed, 137 insertions(+), 3 deletions(-) diff --git a/lianad/src/commands/mod.rs b/lianad/src/commands/mod.rs index c190083a..964ab34c 100644 --- a/lianad/src/commands/mod.rs +++ b/lianad/src/commands/mod.rs @@ -1384,6 +1384,7 @@ mod tests { locktime::absolute, Amount, OutPoint, ScriptBuf, Sequence, Transaction, Txid, Witness, }; + use spend::InsufficientFunds; use std::{collections::BTreeMap, str::FromStr}; #[test] @@ -2659,4 +2660,127 @@ mod tests { ms.shutdown(); } + + #[test] + fn create_recovery() { + let dummy_tx = bitcoin::Transaction { + version: TxVersion::TWO, + lock_time: absolute::LockTime::Blocks(absolute::Height::ZERO), + input: vec![], + output: vec![], + }; + let dummy_op = bitcoin::OutPoint::new(dummy_tx.compute_txid(), 0); + let ms = DummyLiana::new_timelock(DummyBitcoind::new(), DummyDatabase::new(), 10); + let control = &ms.control(); + let mut db_conn = control.db().lock().unwrap().connection(); + db_conn.new_txs(&[dummy_tx]); + + // Arguments sanity checking + let dummy_addr = + bitcoin::Address::from_str("bc1qnsexk3gnuyayu92fc3tczvc7k62u22a22ua2kv").unwrap(); + // Feerate cannot be less than 1. + assert_eq!( + control.create_recovery(dummy_addr.clone(), 0, None), + Err(CommandError::InvalidFeerate(0)) + ); + // If we ask to sweep to an address from another network, it will fail. + let invalid_addr = + bitcoin::Address::from_str("tb1qfufcrdyarcg5eph608c6l8vktrc9re6agu4se2").unwrap(); + assert!(matches!( + control.create_recovery(invalid_addr, 1, None), + Err(CommandError::Address( + address::error::ParseError::NetworkValidation { .. } + )) + )); + + // We have no coins to create recovery. + assert!(matches!( + control.create_recovery(dummy_addr.clone(), 1, None), + Err(CommandError::RecoveryNotAvailable), + )); + + // Add unconfirmed coin. + let dummy_coin = Coin { + outpoint: dummy_op, + is_immature: false, + block_info: None, + amount: bitcoin::Amount::from_sat(100_000), + derivation_index: bip32::ChildNumber::from(13), + is_change: false, + spend_txid: None, + spend_block: None, + is_from_self: false, + }; + db_conn.new_unspent_coins(&[dummy_coin]); + // Recovery not available for unconfirmed coins. + assert!(matches!( + control.create_recovery(dummy_addr.clone(), 1, None), + Err(CommandError::RecoveryNotAvailable), + )); + + // Confirm coin such that timelock (10) has not expired at next block (101). + db_conn.confirm_coins(&[(dummy_op, 92, 100_000)]); + assert!(matches!( + control.create_recovery(dummy_addr.clone(), 1, None), + Err(CommandError::RecoveryNotAvailable), + )); + + // If we use a smaller timelock value it works, even though we don't have any such + // recovery timelock (see https://github.com/wizardsardine/liana/issues/1089). + assert!(control + .create_recovery(dummy_addr.clone(), 1, Some(9)) + .is_ok()); + + // Remove coin, re-add and confirm such that recovery available at next block. + db_conn.remove_coins(&[dummy_op]); + db_conn.new_unspent_coins(&[dummy_coin]); + db_conn.confirm_coins(&[(dummy_op, 91, 100_000)]); + let res = control.create_recovery(dummy_addr.clone(), 1, None); + assert!(res.is_ok()); + let psbt = res.unwrap().psbt; + assert_eq!(psbt.outputs.len(), 1); + assert_eq!(psbt.unsigned_tx.output.len(), 1); + assert_eq!( + psbt.unsigned_tx.output.first().unwrap().script_pubkey, + dummy_addr.assume_checked_ref().script_pubkey() + ); + // Amount is coin value minus fee. + assert_eq!( + psbt.unsigned_tx.output.first().unwrap().value, + Amount::from_sat(100_000 - 127) + ); + + // If we pass a larger timelock, it no longer works: + assert!(matches!( + control.create_recovery(dummy_addr.clone(), 1, Some(11)), + Err(CommandError::RecoveryNotAvailable), + )); + + // If the coin is spending, it is no longer recoverable. + db_conn.spend_coins(&[( + dummy_op, + Txid::from_str("84f09bddfe0f036d0390edf655636ad6092c3ab8f09b2bb1503caa393463f241") + .unwrap(), + )]); + assert!(matches!( + control.create_recovery(dummy_addr.clone(), 1, None), + Err(CommandError::RecoveryNotAvailable), + )); + + // Now remove the coin and re-add, but this time with an amount that is too small to create an output. + // This will give a coin selection error due to insufficient funds. + db_conn.remove_coins(&[dummy_op]); + let mut dummy_coin = dummy_coin; + dummy_coin.amount = Amount::from_sat(5_000 + 126); + db_conn.new_unspent_coins(&[dummy_coin]); + db_conn.confirm_coins(&[(dummy_op, 91, 100_000)]); + assert_eq!( + control.create_recovery(dummy_addr.clone(), 1, None), + Err(CommandError::SpendCreation( + SpendCreationError::CoinSelection(InsufficientFunds { missing: 1 }) + )), + ); + + ms.shutdown(); + } } diff --git a/lianad/src/testutils.rs b/lianad/src/testutils.rs index dd05f716..3d783f8c 100644 --- a/lianad/src/testutils.rs +++ b/lianad/src/testutils.rs @@ -558,6 +558,7 @@ impl DummyLiana { bitcoin_interface: impl BitcoinInterface + 'static, database: impl DatabaseInterface + 'static, rpc_server: bool, + timelock: u16, ) -> DummyLiana { let tmp_dir = tmp_dir(); fs::create_dir_all(&tmp_dir).unwrap(); @@ -574,7 +575,7 @@ impl DummyLiana { let heir_key = descriptors::PathInfo::Single(descriptor::DescriptorPublicKey::from_str("[aabbccdd]xpub68JJTXc1MWK8PEQozKsRatrUHXKFNkD1Cb1BuQU9Xr5moCv87anqGyXLyUd4KpnDyZgo3gz4aN1r3NiaoweFW8UutBsBbgKHzaD5HkTkifK/<0;1>/*").unwrap()); let policy = descriptors::LianaPolicy::new_legacy( owner_key, - [(10_000, heir_key)].iter().cloned().collect(), + [(timelock, heir_key)].iter().cloned().collect(), ) .unwrap(); let desc = descriptors::LianaDescriptor::new(policy); @@ -597,7 +598,16 @@ impl DummyLiana { bitcoin_interface: impl BitcoinInterface + 'static, database: impl DatabaseInterface + 'static, ) -> DummyLiana { - Self::_new(bitcoin_interface, database, false) + Self::_new(bitcoin_interface, database, false, 10_000) + } + + /// Creates a new DummyLiana interface with the specified recovery path timelock. + pub fn new_timelock( + bitcoin_interface: impl BitcoinInterface + 'static, + database: impl DatabaseInterface + 'static, + timelock: u16, + ) -> DummyLiana { + Self::_new(bitcoin_interface, database, false, timelock) } /// Creates a new DummyLiana interface which also spins up an RPC server. @@ -605,7 +615,7 @@ impl DummyLiana { bitcoin_interface: impl BitcoinInterface + 'static, database: impl DatabaseInterface + 'static, ) -> DummyLiana { - Self::_new(bitcoin_interface, database, true) + Self::_new(bitcoin_interface, database, true, 10_000) } pub fn control(&self) -> &DaemonControl { From 8cc723fb3bca3af661e7ace47ee8cb838d7d0db9 Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Fri, 4 Apr 2025 15:35:23 +0100 Subject: [PATCH 3/5] commands: allow to specify coins for recovery --- liana-gui/src/daemon/embedded.rs | 2 +- lianad/src/commands/mod.rs | 204 ++++++++++++++++++++++++++----- lianad/src/jsonrpc/api.rs | 2 +- lianad/src/jsonrpc/rpc.rs | 3 +- 4 files changed, 176 insertions(+), 35 deletions(-) diff --git a/liana-gui/src/daemon/embedded.rs b/liana-gui/src/daemon/embedded.rs index 94935ab3..113c22a7 100644 --- a/liana-gui/src/daemon/embedded.rs +++ b/liana-gui/src/daemon/embedded.rs @@ -217,7 +217,7 @@ impl Daemon for EmbeddedDaemon { ) -> Result { self.command(|daemon| { daemon - .create_recovery(address, feerate_vb, sequence) + .create_recovery(address, &[], feerate_vb, sequence) .map(|res| res.psbt) .map_err(|e| DaemonError::Unexpected(e.to_string())) }) diff --git a/lianad/src/commands/mod.rs b/lianad/src/commands/mod.rs index 964ab34c..0b9190d6 100644 --- a/lianad/src/commands/mod.rs +++ b/lianad/src/commands/mod.rs @@ -68,6 +68,8 @@ pub enum CommandError { /// An error that might occur in the racy rescan triggering logic. RescanTrigger(String), RecoveryNotAvailable, + // Include timelock in error as it may not have been set explicitly by the user. + OutpointNotRecoverable(bitcoin::OutPoint, /* timelock */ u16), /// Overflowing or unhardened derivation index. InvalidDerivationIndex, RbfError(RbfErrorInfo), @@ -120,6 +122,11 @@ impl fmt::Display for CommandError { f, "No coin currently spendable through this timelocked recovery path." ), + Self::OutpointNotRecoverable(op, t) => write!( + f, + "Coin at '{}' is not recoverable with timelock '{}'", + op, t + ), Self::InvalidDerivationIndex => { write!(f, "Unhardened or overflowing BIP32 derivation index.") } @@ -1141,16 +1148,22 @@ impl DaemonControl { ListTransactionsResult { transactions } } - /// Create a transaction that sweeps all coins for which a timelocked recovery path is - /// currently available to a provided address with the provided feerate. + /// Create a transaction that sweeps coins using a timelocked recovery path to a + /// provided address with the provided feerate. /// /// The `timelock` parameter can be used to specify which recovery path to use. By default, /// we'll use the first recovery path available. /// + /// If `coins_outpoints` is empty, all coins for which the given recovery path is currently + /// available will be used. Otherwise, only those specified will be considered. An error will + /// be returned if any coins specified by `coins_outpoints` are unknown, already spent or + /// otherwise not currently recoverable using the given recovery path. + /// /// Note that not all coins may be spendable through a single recovery path at the same time. pub fn create_recovery( &self, address: bitcoin::Address, + coins_outpoints: &[bitcoin::OutPoint], feerate_vb: u64, timelock: Option, ) -> Result { @@ -1167,26 +1180,42 @@ impl DaemonControl { let timelock = timelock.unwrap_or_else(|| self.config.main_descriptor.first_timelock_value()); let height_delta: i32 = timelock.into(); - let sweepable_coins: Vec<_> = db_conn - .coins(&[CoinStatus::Confirmed], &[]) - .into_values() - .filter_map(|c| { - // We are interested in coins available at the *next* block - if c.block_info - .map(|b| current_height + 1 >= b.height + height_delta) - .unwrap_or(false) - { - Some(coin_to_candidate( - &c, - /*must_select=*/ true, - /*sequence=*/ Some(bitcoin::Sequence::from_height(timelock)), - /*ancestor_info=*/ None, - )) - } else { - None + let coins = if coins_outpoints.is_empty() { + db_conn.coins(&[CoinStatus::Confirmed], &[]) + } else { + // We could have used the same DB call for both cases by specifying the status and outpoints, + // but in order to give more helpful errors, we filter the DB call here only for outpoints + // and then check for coin status separately. + let coins_by_op = db_conn.coins(&[], coins_outpoints); + for op in coins_outpoints { + let coin = coins_by_op + .get(op) + .ok_or(CommandError::UnknownOutpoint(*op))?; + // We only check for spent coins here. Unconfirmed coins (including immature) + // will fail the check for recoverability further below. + if coin.is_spent() { + return Err(CommandError::AlreadySpent(*op)); } - }) - .collect(); + } + coins_by_op + }; + let mut sweepable_coins = Vec::with_capacity(coins.len()); + for (op, c) in coins { + // We are interested in coins available at the *next* block + if c.block_info + .map(|b| current_height + 1 >= b.height + height_delta) + .unwrap_or(false) + { + sweepable_coins.push(coin_to_candidate( + &c, + /*must_select=*/ true, + /*sequence=*/ Some(bitcoin::Sequence::from_height(timelock)), + /*ancestor_info=*/ None, + )); + } else if !coins_outpoints.is_empty() { + return Err(CommandError::OutpointNotRecoverable(op, timelock)); + } + } if sweepable_coins.is_empty() { return Err(CommandError::RecoveryNotAvailable); } @@ -2669,7 +2698,8 @@ mod tests { input: vec![], output: vec![], }; - let dummy_op = bitcoin::OutPoint::new(dummy_tx.compute_txid(), 0); + let dummy_txid = dummy_tx.compute_txid(); + let dummy_op = bitcoin::OutPoint::new(dummy_txid, 0); let ms = DummyLiana::new_timelock(DummyBitcoind::new(), DummyDatabase::new(), 10); let control = &ms.control(); let mut db_conn = control.db().lock().unwrap().connection(); @@ -2680,14 +2710,14 @@ mod tests { bitcoin::Address::from_str("bc1qnsexk3gnuyayu92fc3tczvc7k62u22a22ua2kv").unwrap(); // Feerate cannot be less than 1. assert_eq!( - control.create_recovery(dummy_addr.clone(), 0, None), + control.create_recovery(dummy_addr.clone(), &[], 0, None), Err(CommandError::InvalidFeerate(0)) ); // If we ask to sweep to an address from another network, it will fail. let invalid_addr = bitcoin::Address::from_str("tb1qfufcrdyarcg5eph608c6l8vktrc9re6agu4se2").unwrap(); assert!(matches!( - control.create_recovery(invalid_addr, 1, None), + control.create_recovery(invalid_addr, &[], 1, None), Err(CommandError::Address( address::error::ParseError::NetworkValidation { .. } )) @@ -2695,9 +2725,14 @@ mod tests { // We have no coins to create recovery. assert!(matches!( - control.create_recovery(dummy_addr.clone(), 1, None), + control.create_recovery(dummy_addr.clone(), &[], 1, None), Err(CommandError::RecoveryNotAvailable), )); + // Coin is unknown. + assert_eq!( + control.create_recovery(dummy_addr.clone(), &[dummy_op], 1, None), + Err(CommandError::UnknownOutpoint(dummy_op)), + ); // Add unconfirmed coin. let dummy_coin = Coin { @@ -2714,28 +2749,39 @@ mod tests { db_conn.new_unspent_coins(&[dummy_coin]); // Recovery not available for unconfirmed coins. assert!(matches!( - control.create_recovery(dummy_addr.clone(), 1, None), + control.create_recovery(dummy_addr.clone(), &[], 1, None), Err(CommandError::RecoveryNotAvailable), )); + assert_eq!( + control.create_recovery(dummy_addr.clone(), &[dummy_op], 1, None), + Err(CommandError::OutpointNotRecoverable(dummy_op, 10)), + ); // Confirm coin such that timelock (10) has not expired at next block (101). db_conn.confirm_coins(&[(dummy_op, 92, 100_000)]); assert!(matches!( - control.create_recovery(dummy_addr.clone(), 1, None), + control.create_recovery(dummy_addr.clone(), &[], 1, None), Err(CommandError::RecoveryNotAvailable), )); + assert_eq!( + control.create_recovery(dummy_addr.clone(), &[dummy_op], 1, None), + Err(CommandError::OutpointNotRecoverable(dummy_op, 10)), + ); // If we use a smaller timelock value it works, even though we don't have any such // recovery timelock (see https://github.com/wizardsardine/liana/issues/1089). assert!(control - .create_recovery(dummy_addr.clone(), 1, Some(9)) + .create_recovery(dummy_addr.clone(), &[], 1, Some(9)) + .is_ok()); + assert!(control + .create_recovery(dummy_addr.clone(), &[dummy_op], 1, Some(9)) .is_ok()); // Remove coin, re-add and confirm such that recovery available at next block. db_conn.remove_coins(&[dummy_op]); db_conn.new_unspent_coins(&[dummy_coin]); db_conn.confirm_coins(&[(dummy_op, 91, 100_000)]); - let res = control.create_recovery(dummy_addr.clone(), 1, None); + let res = control.create_recovery(dummy_addr.clone(), &[], 1, None); assert!(res.is_ok()); let psbt = res.unwrap().psbt; assert_eq!(psbt.outputs.len(), 1); @@ -2752,9 +2798,13 @@ mod tests { // If we pass a larger timelock, it no longer works: assert!(matches!( - control.create_recovery(dummy_addr.clone(), 1, Some(11)), + control.create_recovery(dummy_addr.clone(), &[], 1, Some(11)), Err(CommandError::RecoveryNotAvailable), )); + assert_eq!( + control.create_recovery(dummy_addr.clone(), &[dummy_op], 1, Some(11)), + Err(CommandError::OutpointNotRecoverable(dummy_op, 11)), + ); // If the coin is spending, it is no longer recoverable. db_conn.spend_coins(&[( @@ -2763,9 +2813,13 @@ mod tests { .unwrap(), )]); assert!(matches!( - control.create_recovery(dummy_addr.clone(), 1, None), + control.create_recovery(dummy_addr.clone(), &[], 1, None), Err(CommandError::RecoveryNotAvailable), )); + assert_eq!( + control.create_recovery(dummy_addr.clone(), &[dummy_op], 1, None), + Err(CommandError::AlreadySpent(dummy_op)), + ); // Now remove the coin and re-add, but this time with an amount that is too small to create an output. // This will give a coin selection error due to insufficient funds. @@ -2775,12 +2829,98 @@ mod tests { db_conn.new_unspent_coins(&[dummy_coin]); db_conn.confirm_coins(&[(dummy_op, 91, 100_000)]); assert_eq!( - control.create_recovery(dummy_addr.clone(), 1, None), + control.create_recovery(dummy_addr.clone(), &[], 1, None), + Err(CommandError::SpendCreation( + SpendCreationError::CoinSelection(InsufficientFunds { missing: 1 }) + )), + ); + assert_eq!( + control.create_recovery(dummy_addr.clone(), &[dummy_op], 1, None), Err(CommandError::SpendCreation( SpendCreationError::CoinSelection(InsufficientFunds { missing: 1 }) )), ); + // Add a new coin so that we have enough funds for the recovery. + let dummy_op_2 = bitcoin::OutPoint::new(dummy_txid, 1); + let dummy_coin_2 = Coin { + outpoint: dummy_op_2, + is_immature: false, + block_info: None, + amount: bitcoin::Amount::from_sat(10_000), + derivation_index: bip32::ChildNumber::from(1378), + is_change: false, + spend_txid: None, + spend_block: None, + is_from_self: false, + }; + db_conn.new_unspent_coins(&[dummy_coin_2]); + db_conn.confirm_coins(&[(dummy_op_2, 92, 200_000)]); + // Coin cannot be used as the timelock will still be in place at the next block. + assert_eq!( + control.create_recovery(dummy_addr.clone(), &[], 1, None), + Err(CommandError::SpendCreation( + SpendCreationError::CoinSelection(InsufficientFunds { missing: 1 }) + )), + ); + // If we try to specify the new coin, we'll get an error that the coin is not recoverable. + assert_eq!( + control.create_recovery(dummy_addr.clone(), &[dummy_op, dummy_op_2], 1, None), + Err(CommandError::OutpointNotRecoverable(dummy_op_2, 10)), + ); + // Using a shorter timelock parameter works: + assert!(control + .create_recovery(dummy_addr.clone(), &[], 1, Some(9)) + .is_ok()); + assert!(control + .create_recovery(dummy_addr.clone(), &[dummy_op, dummy_op_2], 1, Some(9)) + .is_ok()); + + // Now re-add the coin with a confirmation one block earlier. + db_conn.remove_coins(&[dummy_op_2]); + db_conn.new_unspent_coins(&[dummy_coin_2]); + db_conn.confirm_coins(&[(dummy_op_2, 91, 200_000)]); + + // Now both coins are used in the recovery and we have enough funds. + let res = control.create_recovery(dummy_addr.clone(), &[], 1, None); + assert!(res.is_ok()); + let psbt = res.unwrap().psbt; + assert_eq!(psbt.outputs.len(), 1); + assert_eq!(psbt.unsigned_tx.output.len(), 1); + assert_eq!( + psbt.unsigned_tx.output.first().unwrap().script_pubkey, + dummy_addr.assume_checked_ref().script_pubkey() + ); + // Amount is coin value minus fee. + assert_eq!( + psbt.unsigned_tx.output.first().unwrap().value, + Amount::from_sat(/* coin 1 */ 5_000 + 126 + /* coin 2 */10_000 - /* fee */ 211) + ); + + // Do the same again, now specifying the outpoints explicitly. + let res = control.create_recovery(dummy_addr.clone(), &[dummy_op, dummy_op_2], 1, None); + assert!(res.is_ok()); + let psbt = res.unwrap().psbt; + assert_eq!(psbt.outputs.len(), 1); + assert_eq!(psbt.unsigned_tx.output.len(), 1); + assert_eq!( + psbt.unsigned_tx.output.first().unwrap().script_pubkey, + dummy_addr.assume_checked_ref().script_pubkey() + ); + assert_eq!( + psbt.unsigned_tx.output.first().unwrap().value, + Amount::from_sat(/* coin 1 */ 5_000 + 126 + /* coin 2 */10_000 - /* fee */ 211) + ); + + // Now check that increasing the feerate increases the fee. + let res = control.create_recovery(dummy_addr.clone(), &[], 2, None); + assert!(res.is_ok()); + let psbt = res.unwrap().psbt; + assert_eq!( + psbt.unsigned_tx.output.first().unwrap().value, + Amount::from_sat(/* coin 1 */ 5_000 + 126 + /* coin 2 */10_000 - /* fee */ 2 * 211) + ); + ms.shutdown(); } } diff --git a/lianad/src/jsonrpc/api.rs b/lianad/src/jsonrpc/api.rs index 808156a1..02e5426e 100644 --- a/lianad/src/jsonrpc/api.rs +++ b/lianad/src/jsonrpc/api.rs @@ -343,7 +343,7 @@ fn create_recovery(control: &DaemonControl, params: Params) -> Result for Error { | commands::CommandError::InvalidDerivationIndex | commands::CommandError::RbfError(..) | commands::CommandError::EmptyFilterList - | commands::CommandError::RecoveryNotAvailable => { + | commands::CommandError::RecoveryNotAvailable + | commands::CommandError::OutpointNotRecoverable(..) => { Error::new(ErrorCode::InvalidParams, e.to_string()) } commands::CommandError::RescanTrigger(..) => { From d4151d88d690d49b779402f61d6d506460fd8d87 Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Tue, 8 Apr 2025 10:34:07 +0100 Subject: [PATCH 4/5] rpc: allow to choose outpoints in `createrecovery` To maintain backwards compatibility, the `outpoints` parameter is the final positional argument and can be omitted entirely. --- doc/API.md | 21 +++++++---- liana-gui/src/daemon/client/mod.rs | 1 + lianad/src/jsonrpc/api.rs | 20 +++++++++- tests/test_rpc.py | 59 +++++++++++++++++++++++++++++- 4 files changed, 91 insertions(+), 10 deletions(-) diff --git a/doc/API.md b/doc/API.md index a7b0f9ff..b6ce771e 100644 --- a/doc/API.md +++ b/doc/API.md @@ -406,8 +406,13 @@ Confirmation time is based on the timestamp of blocks. ### `createrecovery` -Create a transaction that sweeps all coins for which a timelocked recovery path is -currently available to a provided address with the provided feerate. +Create a transaction that sweeps coins using a timelocked recovery path to a provided address +with the provided feerate. + +If `outpoints` is empty or missing, then all coins for which the given recovery path is currently +available will be used. Otherwise, only those specified will be considered. An error will +be returned if any coins specified by `outpoints` are unknown, already spent or otherwise +not currently recoverable using the given recovery path. The `timelock` parameter can be used to specify which recovery path to use. By default, we'll use the first recovery path available. If created for a later timelock a recovery @@ -421,11 +426,13 @@ cover the requested feerate. #### Request -| Field | Type | Description | -| ---------- | ----------------- | ----------------------------------------------------------------------------------------- | -| `address` | str | The Bitcoin address to sweep the coins to. | -| `feerate` | integer | Target feerate for the transaction, in satoshis per virtual byte. | -| `timelock` | int or `null` | Recovery path to be used, identified by the number of blocks after which it is available. | +| Field | Type | Description | +| ---------- | ---------------------- | ----------------------------------------------------------------------------------------- | +| `address` | str | The Bitcoin address to sweep the coins to. | +| `feerate` | integer | Target feerate for the transaction, in satoshis per virtual byte. | +| `timelock` | int or `null` | Recovery path to be used, identified by the number of blocks after which it is available. | +| `outpoints`| list of str (optional) | List of the coins to be recovered, as `txid:vout`. | + #### Response diff --git a/liana-gui/src/daemon/client/mod.rs b/liana-gui/src/daemon/client/mod.rs index 53651d36..e663e475 100644 --- a/liana-gui/src/daemon/client/mod.rs +++ b/liana-gui/src/daemon/client/mod.rs @@ -185,6 +185,7 @@ impl Daemon for Lianad { feerate_vb: u64, sequence: Option, ) -> Result { + // The `outpoints` parameter is omitted, which means all recoverable coins will be used. let res: CreateRecoveryResult = self.call( "createrecovery", Some(vec![json!(address), json!(feerate_vb), json!(sequence)]), diff --git a/lianad/src/jsonrpc/api.rs b/lianad/src/jsonrpc/api.rs index 02e5426e..dc2cf5a0 100644 --- a/lianad/src/jsonrpc/api.rs +++ b/lianad/src/jsonrpc/api.rs @@ -342,8 +342,26 @@ fn create_recovery(control: &DaemonControl, params: Params) -> Result>>() + }) + .ok_or_else(|| Error::invalid_params("Invalid 'outpoints' parameter.")) + }) + .transpose()? + .unwrap_or_default(); // missing is same as empty array - let res = control.create_recovery(address, &[], feerate, timelock)?; + let res = control.create_recovery(address, &outpoints, feerate, timelock)?; Ok(serde_json::json!(&res)) } diff --git a/tests/test_rpc.py b/tests/test_rpc.py index c5df1bee..5334c4ed 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -1037,6 +1037,7 @@ def test_create_recovery(lianad, bitcoind): wait_for( lambda: lianad.rpc.getinfo()["block_height"] == bitcoind.rpc.getblockcount() ) + first_outpoints = [c["outpoint"] for c in lianad.rpc.listcoins()["coins"]] # There's nothing to sweep with pytest.raises( @@ -1044,6 +1045,26 @@ def test_create_recovery(lianad, bitcoind): match="No coin currently spendable through this timelocked recovery path", ): lianad.rpc.createrecovery(bitcoind.rpc.getnewaddress(), 2) + # Same if we specify timelock: + with pytest.raises( + RpcError, + match="No coin currently spendable through this timelocked recovery path", + ): + lianad.rpc.createrecovery(bitcoind.rpc.getnewaddress(), 2, 10) + # And if we use empty array for outpoints: + with pytest.raises( + RpcError, + match="No coin currently spendable through this timelocked recovery path", + ): + lianad.rpc.createrecovery(bitcoind.rpc.getnewaddress(), 2, 10, []) + # If we specify a coin, the error will be different: + with pytest.raises( + RpcError, + match=f"Coin at '{first_outpoints[0]}' is not recoverable with timelock '10'", + ): + lianad.rpc.createrecovery( + bitcoind.rpc.getnewaddress(), 2, 10, [f"{first_outpoints[0]}"] + ) # Receive another coin, it will be one block after the others txid = bitcoind.rpc.sendtoaddress(lianad.rpc.getnewaddress()["address"], 0.4) @@ -1055,9 +1076,17 @@ def test_create_recovery(lianad, bitcoind): wait_for( lambda: lianad.rpc.getinfo()["block_height"] == bitcoind.rpc.getblockcount() ) - res = lianad.rpc.createrecovery(bitcoind.rpc.getnewaddress(), 18) + new_outpoint = [ + c["outpoint"] for c in lianad.rpc.listcoins()["coins"] if txid in c["outpoint"] + ][0] + reco_address = bitcoind.rpc.getnewaddress() + res = lianad.rpc.createrecovery(reco_address, 18) reco_psbt = PSBT.from_base64(res["psbt"]) + # Do the same passing all three coins explicitly: + res_op = lianad.rpc.createrecovery(reco_address, 18, 10, first_outpoints) + reco_psbt_op = PSBT.from_base64(res_op["psbt"]) + # Check locktime being set correctly. tip_height = bitcoind.rpc.getblockcount() assert tip_height > 100 @@ -1065,8 +1094,34 @@ def test_create_recovery(lianad, bitcoind): assert tip_height - 100 <= locktime <= tip_height assert len(reco_psbt.tx.vin) == 3, "The last coin's timelock hasn't matured yet" - assert len(reco_psbt.tx.vout) == 1 + assert len(reco_psbt.tx.vout) == len(reco_psbt_op.tx.vout) == 1 + # The inputs are the same for both explicit and implicit outpoints: + assert sorted(i.prevout.serialize() for i in reco_psbt.tx.vin) == sorted( + i.prevout.serialize() for i in reco_psbt_op.tx.vin + ) + assert reco_psbt.tx.vout[0].nValue == reco_psbt_op.tx.vout[0].nValue + assert reco_psbt.tx.vout[0].scriptPubKey == reco_psbt_op.tx.vout[0].scriptPubKey assert int(0.5999 * COIN) < int(reco_psbt.tx.vout[0].nValue) < int(0.6 * COIN) + + # Now use only 2 of the 3 coins: + res_op_2 = lianad.rpc.createrecovery( + bitcoind.rpc.getnewaddress(), 18, 10, first_outpoints[:2] + ) + reco_psbt_op_2 = PSBT.from_base64(res_op_2["psbt"]) + assert len(reco_psbt_op_2.tx.vin) == 2 + assert sorted( + f"{i.prevout.hash:064x}:{i.prevout.n}" for i in reco_psbt_op_2.tx.vin + ) == sorted(first_outpoints[:2]) + + # If we try to include the newest coin, an error will be returned: + with pytest.raises( + RpcError, + match=f"Coin at '{new_outpoint}' is not recoverable with timelock '10'", + ): + lianad.rpc.createrecovery( + bitcoind.rpc.getnewaddress(), 2, 10, [first_outpoints[0], new_outpoint] + ) + txid = sign_and_broadcast(lianad, bitcoind, reco_psbt, recovery=True) # And by mining one more block we'll be able to sweep the last coin. From 2527bcea2eef753fb776e1618c7d4b9b79801a3c Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Tue, 8 Apr 2025 13:06:58 +0100 Subject: [PATCH 5/5] fix: timelock parameter is optional The timelock parameter is optional in that it can be omitted, but if present it must have an integer value rather than a value of `null`. --- doc/API.md | 2 +- liana-gui/src/daemon/client/mod.rs | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/doc/API.md b/doc/API.md index b6ce771e..63bfc62f 100644 --- a/doc/API.md +++ b/doc/API.md @@ -430,7 +430,7 @@ cover the requested feerate. | ---------- | ---------------------- | ----------------------------------------------------------------------------------------- | | `address` | str | The Bitcoin address to sweep the coins to. | | `feerate` | integer | Target feerate for the transaction, in satoshis per virtual byte. | -| `timelock` | int or `null` | Recovery path to be used, identified by the number of blocks after which it is available. | +| `timelock` | int (optional) | Recovery path to be used, identified by the number of blocks after which it is available. | | `outpoints`| list of str (optional) | List of the coins to be recovered, as `txid:vout`. | diff --git a/liana-gui/src/daemon/client/mod.rs b/liana-gui/src/daemon/client/mod.rs index e663e475..d08abc5d 100644 --- a/liana-gui/src/daemon/client/mod.rs +++ b/liana-gui/src/daemon/client/mod.rs @@ -186,10 +186,13 @@ impl Daemon for Lianad { sequence: Option, ) -> Result { // The `outpoints` parameter is omitted, which means all recoverable coins will be used. - let res: CreateRecoveryResult = self.call( - "createrecovery", - Some(vec![json!(address), json!(feerate_vb), json!(sequence)]), - )?; + let mut params = serde_json::Map::new(); + params.insert("address".to_string(), json!(address)); + params.insert("feerate".to_string(), json!(feerate_vb)); + if let Some(sequence) = sequence { + params.insert("timelock".to_string(), json!(sequence)); + } + let res: CreateRecoveryResult = self.call("createrecovery", Some(params))?; Ok(res.psbt) }