From dde5488d60f0eaac0d618b0df42a934a6255d64c Mon Sep 17 00:00:00 2001 From: qlrd Date: Sat, 25 Jul 2026 17:30:31 -0300 Subject: [PATCH] lianad: warn `createrecovery` to an address of the same wallet. This commit adds a new error (code 1001, just after the existing 1000) bound to `createrecovery` -- as an error/warning response -- when a user wants to re-lock funds from/to addresses derived from the wallet descriptor. As stated by jp1ac4 the starting point is to check this on the backend since the backend has easier access to known wallet addresses (needs a check on how to deal with derivation indices beyond what the DB knows). The proposed flow is first return an error from `createrecovery`, controlled by a new `Option` parameter `allow_own_address` (defaults to `false`). With this, a user can try again with `allow_own_address=true` (see `tests/test_rpc.py`). In that case, a new `warnings` field is added to the response alongside the `psbt` one, so the user can double-check the entire procedure. refs #1654. --- doc/API.md | 24 ++-- liana-gui/src/app/state/spend/step.rs | 9 +- liana-gui/src/daemon/client/mod.rs | 4 + liana-gui/src/daemon/embedded.rs | 9 +- liana-gui/src/daemon/mod.rs | 1 + .../services/connect/client/backend/mod.rs | 3 + lianad/src/commands/mod.rs | 108 +++++++++++++----- lianad/src/jsonrpc/api.rs | 9 +- lianad/src/jsonrpc/rpc.rs | 7 ++ lianad/src/testutils.rs | 20 +++- tests/test_rpc.py | 16 ++- 11 files changed, 169 insertions(+), 41 deletions(-) diff --git a/doc/API.md b/doc/API.md index 9fbaacdb0c..9d945180fe 100644 --- a/doc/API.md +++ b/doc/API.md @@ -461,19 +461,25 @@ 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 (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`. | +| 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 (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`. | +| `allow_own_address` | bool (optional) | Allow an address that belongs to this same wallet as recovery (default: `false`). | #### Response -| Field | Type | Description | -| -------------- | --------- | ---------------------------------------------------- | -| `psbt` | string | PSBT of the recovery transaction, encoded as base64. | +| Field | Type | Description | +| -------------- | ----------- | ----------------------------------------------------- | +| `psbt` | string | PSBT of the recovery transaction, encoded as base64. | +| `warnings` | list of str | Warnings, if any, generated during recovery creation. | + +An error (code 1001) will be returned if the sweep address is known to belong to this wallet, +since recovered funds would be locked under the same descriptor again. Set `allow_own_address` +to proceed anyway; a warning will then be included in the `warnings` response field instead. ### `updatelabels` diff --git a/liana-gui/src/app/state/spend/step.rs b/liana-gui/src/app/state/spend/step.rs index 2ac6859200..7deb318fe8 100644 --- a/liana-gui/src/app/state/spend/step.rs +++ b/liana-gui/src/app/state/spend/step.rs @@ -434,7 +434,13 @@ impl DefineSpend { // If recovery timelock is set, create a recovery transaction. Otherwise, a regular spend. if let Some(reco_tl) = recovery_timelock { daemon - .create_recovery(max_address.clone(), &outpoints, feerate_vb, Some(reco_tl)) + .create_recovery( + max_address.clone(), + &outpoints, + feerate_vb, + Some(reco_tl), + Some(true), // TODO(#1654) + ) .await // Map the PSBT to `CreateSpendResult` result. We only need the PSBT below. .map(|psbt| CreateSpendResult::Success { @@ -735,6 +741,7 @@ impl Step for DefineSpend { &inputs, feerate_vb, Some(reco_tl), + Some(true), // TODO(#1654) ) .await .map_err(|e| e.into()) diff --git a/liana-gui/src/daemon/client/mod.rs b/liana-gui/src/daemon/client/mod.rs index 6341477d99..003ff789ba 100644 --- a/liana-gui/src/daemon/client/mod.rs +++ b/liana-gui/src/daemon/client/mod.rs @@ -209,6 +209,7 @@ impl Daemon for Lianad { coins_outpoints: &[OutPoint], feerate_vb: u64, sequence: Option, + allow_own_address: Option, ) -> Result { let mut params = serde_json::Map::new(); params.insert("address".to_string(), json!(address)); @@ -217,6 +218,9 @@ impl Daemon for Lianad { if let Some(sequence) = sequence { params.insert("timelock".to_string(), json!(sequence)); } + if let Some(allow_own_address) = allow_own_address { + params.insert("allow_own_address".to_string(), json!(allow_own_address)); + } let res: CreateRecoveryResult = self.call("createrecovery", Some(params))?; Ok(res.psbt) } diff --git a/liana-gui/src/daemon/embedded.rs b/liana-gui/src/daemon/embedded.rs index 5b867bfdf6..5b9107e4fd 100644 --- a/liana-gui/src/daemon/embedded.rs +++ b/liana-gui/src/daemon/embedded.rs @@ -236,10 +236,17 @@ impl Daemon for EmbeddedDaemon { coins_outpoints: &[OutPoint], feerate_vb: u64, sequence: Option, + allow_own_address: Option, ) -> Result { self.command(|daemon| { daemon - .create_recovery(address, coins_outpoints, feerate_vb, sequence) + .create_recovery( + address, + coins_outpoints, + feerate_vb, + sequence, + allow_own_address, + ) .map(|res| res.psbt) .map_err(|e| DaemonError::Unexpected(e.to_string())) }) diff --git a/liana-gui/src/daemon/mod.rs b/liana-gui/src/daemon/mod.rs index b9762cfa9e..78f46473ea 100644 --- a/liana-gui/src/daemon/mod.rs +++ b/liana-gui/src/daemon/mod.rs @@ -182,6 +182,7 @@ pub trait Daemon: Debug { coins_outpoints: &[OutPoint], feerate_vb: u64, sequence: Option, + allow_own_address: Option, ) -> Result; async fn list_txs(&self, txid: &[Txid]) -> Result; async fn get_labels( diff --git a/liana-gui/src/services/connect/client/backend/mod.rs b/liana-gui/src/services/connect/client/backend/mod.rs index c7ebb0fc0e..a5c3964a71 100644 --- a/liana-gui/src/services/connect/client/backend/mod.rs +++ b/liana-gui/src/services/connect/client/backend/mod.rs @@ -930,12 +930,15 @@ impl Daemon for BackendWalletClient { Err(DaemonError::NoAnswer) } + // TODO(#1654): the own-address check is not enforced here (need to check how + // the backend API could use allow_own_address param) async fn create_recovery( &self, address: Address, coins_outpoints: &[OutPoint], feerate_vb: u64, sequence: Option, + _allow_own_address: Option, ) -> Result { let timelock = sequence.ok_or(DaemonError::Unexpected("Missing sequence".to_string()))?; let res: api::DraftPsbt = self diff --git a/lianad/src/commands/mod.rs b/lianad/src/commands/mod.rs index 4f88f3ea5f..1ca0277e78 100644 --- a/lianad/src/commands/mod.rs +++ b/lianad/src/commands/mod.rs @@ -70,6 +70,8 @@ pub enum CommandError { /// An error that might occur in the racy rescan triggering logic. RescanTrigger(String), RecoveryNotAvailable, + /// The sweep address of a recovery one belongs to the same wallet + RecoveryToOwnAddress(bitcoin::Address), // 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. @@ -121,6 +123,13 @@ impl fmt::Display for CommandError { f, "No coin currently spendable through this timelocked recovery path." ), + Self::RecoveryToOwnAddress(addr) => { + write!( + f, + "Recovery address '{addr}' belongs to the same wallet. Recovered \ + funds would be locked under the same descriptor again." + ) + } Self::OutpointNotRecoverable(op, t) => { write!(f, "Coin at '{op}' is not recoverable with timelock '{t}'",) } @@ -1251,12 +1260,16 @@ impl DaemonControl { /// 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. + /// + /// By default an error is returned if the sweep address belongs to this same wallet. Set + /// `allow_own_address` to allow recovered funds to be locked under the same descriptor. pub fn create_recovery( &self, address: bitcoin::Address, coins_outpoints: &[bitcoin::OutPoint], feerate_vb: u64, timelock: Option, + allow_own_address: Option, ) -> Result { if feerate_vb < 1 { return Err(CommandError::InvalidFeerate(feerate_vb)); @@ -1311,7 +1324,18 @@ impl DaemonControl { return Err(CommandError::RecoveryNotAvailable); } + // If DB knows (as derived address) about the provided address, it means + // the sweep address belongs to this wallet. + let mut warnings: Vec = vec![]; let sweep_addr_info = sweep_addr.info; + if sweep_addr_info.is_some() { + let err = CommandError::RecoveryToOwnAddress(sweep_addr.addr.clone()); + if !allow_own_address.unwrap_or(false) { + return Err(err.clone()); + } + warnings.push(err.to_string()); + } + let locktime = self.anti_fee_sniping_locktime(); let CreateSpendRes { psbt, has_change, .. @@ -1329,7 +1353,7 @@ impl DaemonControl { self.maybe_increase_last_deriv_index(&mut db_conn, &sweep_addr_info); } - Ok(CreateRecoveryResult { psbt }) + Ok(CreateRecoveryResult { psbt, warnings }) } } @@ -1523,6 +1547,9 @@ pub struct TransactionInfo { pub struct CreateRecoveryResult { #[serde(serialize_with = "ser_to_string", deserialize_with = "deser_fromstr")] pub psbt: Psbt, + + #[serde(default)] + pub warnings: Vec, } #[cfg(test)] @@ -3136,7 +3163,9 @@ mod tests { }; 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 db = DummyDatabase::new(); + let mut db_handle = db.clone(); + let ms = DummyLiana::new_timelock(DummyBitcoind::new(), db, 10); let control = &ms.control(); let mut db_conn = control.db().lock().unwrap().connection(); db_conn.new_txs(&[dummy_tx]); @@ -3146,14 +3175,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, 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, None), Err(CommandError::Address( address::error::ParseError::NetworkValidation { .. } )) @@ -3161,12 +3190,12 @@ 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, None), Err(CommandError::RecoveryNotAvailable), )); // Coin is unknown. assert_eq!( - control.create_recovery(dummy_addr.clone(), &[dummy_op], 1, None), + control.create_recovery(dummy_addr.clone(), &[dummy_op], 1, None, None), Err(CommandError::UnknownOutpoint(dummy_op)), ); @@ -3185,39 +3214,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, None), Err(CommandError::RecoveryNotAvailable), )); assert_eq!( - control.create_recovery(dummy_addr.clone(), &[dummy_op], 1, None), + control.create_recovery(dummy_addr.clone(), &[dummy_op], 1, None, 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, None), Err(CommandError::RecoveryNotAvailable), )); assert_eq!( - control.create_recovery(dummy_addr.clone(), &[dummy_op], 1, None), + control.create_recovery(dummy_addr.clone(), &[dummy_op], 1, None, 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), None) .is_ok()); assert!(control - .create_recovery(dummy_addr.clone(), &[dummy_op], 1, Some(9)) + .create_recovery(dummy_addr.clone(), &[dummy_op], 1, Some(9), None) .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, None); assert!(res.is_ok()); let psbt = res.unwrap().psbt; assert_eq!(psbt.outputs.len(), 1); @@ -3234,14 +3263,34 @@ 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), None), Err(CommandError::RecoveryNotAvailable), )); assert_eq!( - control.create_recovery(dummy_addr.clone(), &[dummy_op], 1, Some(11)), + control.create_recovery(dummy_addr.clone(), &[dummy_op], 1, Some(11), None), Err(CommandError::OutpointNotRecoverable(dummy_op, 11)), ); + // Recovering to own address is refused (default behaviour) + let own_addr = control.get_new_address(); + db_handle.insert_derived_address( + own_addr.address.clone(), + own_addr.derivation_index, + false, + ); + let own_addr_unchecked = own_addr.address.as_unchecked().clone(); + assert_eq!( + control.create_recovery(own_addr_unchecked.clone(), &[], 1, None, None), + Err(CommandError::RecoveryToOwnAddress(own_addr.address)) + ); + + // allow to recover with own address with a warning attached. + let res = control + .create_recovery(own_addr_unchecked, &[], 1, None, Some(true)) + .unwrap(); + assert_eq!(res.warnings.len(), 1); + assert!(res.warnings[0].contains("belongs to the same wallet")); + // If the coin is spending, it is no longer recoverable. db_conn.spend_coins(&[( dummy_op, @@ -3249,11 +3298,11 @@ mod tests { .unwrap(), )]); assert!(matches!( - control.create_recovery(dummy_addr.clone(), &[], 1, None), + control.create_recovery(dummy_addr.clone(), &[], 1, None, None), Err(CommandError::RecoveryNotAvailable), )); assert_eq!( - control.create_recovery(dummy_addr.clone(), &[dummy_op], 1, None), + control.create_recovery(dummy_addr.clone(), &[dummy_op], 1, None, None), Err(CommandError::AlreadySpent(dummy_op)), ); @@ -3265,13 +3314,13 @@ 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, None), Err(CommandError::SpendCreation( SpendCreationError::CoinSelection(InsufficientFunds { missing: 1 }) )), ); assert_eq!( - control.create_recovery(dummy_addr.clone(), &[dummy_op], 1, None), + control.create_recovery(dummy_addr.clone(), &[dummy_op], 1, None, None), Err(CommandError::SpendCreation( SpendCreationError::CoinSelection(InsufficientFunds { missing: 1 }) )), @@ -3294,22 +3343,28 @@ mod tests { 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), + control.create_recovery(dummy_addr.clone(), &[], 1, None, 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), + control.create_recovery(dummy_addr.clone(), &[dummy_op, dummy_op_2], 1, None, None), Err(CommandError::OutpointNotRecoverable(dummy_op_2, 10)), ); // Using a shorter timelock parameter works: assert!(control - .create_recovery(dummy_addr.clone(), &[], 1, Some(9)) + .create_recovery(dummy_addr.clone(), &[], 1, Some(9), None) .is_ok()); assert!(control - .create_recovery(dummy_addr.clone(), &[dummy_op, dummy_op_2], 1, Some(9)) + .create_recovery( + dummy_addr.clone(), + &[dummy_op, dummy_op_2], + 1, + Some(9), + None + ) .is_ok()); // Now re-add the coin with a confirmation one block earlier. @@ -3318,7 +3373,7 @@ mod tests { 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); + let res = control.create_recovery(dummy_addr.clone(), &[], 1, None, None); assert!(res.is_ok()); let psbt = res.unwrap().psbt; assert_eq!(psbt.outputs.len(), 1); @@ -3334,7 +3389,8 @@ mod tests { ); // Do the same again, now specifying the outpoints explicitly. - let res = control.create_recovery(dummy_addr.clone(), &[dummy_op, dummy_op_2], 1, None); + let res = + control.create_recovery(dummy_addr.clone(), &[dummy_op, dummy_op_2], 1, None, None); assert!(res.is_ok()); let psbt = res.unwrap().psbt; assert_eq!(psbt.outputs.len(), 1); @@ -3349,7 +3405,7 @@ mod tests { ); // Now check that increasing the feerate increases the fee. - let res = control.create_recovery(dummy_addr.clone(), &[], 2, None); + let res = control.create_recovery(dummy_addr.clone(), &[], 2, None, None); assert!(res.is_ok()); let psbt = res.unwrap().psbt; assert_eq!( diff --git a/lianad/src/jsonrpc/api.rs b/lianad/src/jsonrpc/api.rs index 06288faf42..74a472cce5 100644 --- a/lianad/src/jsonrpc/api.rs +++ b/lianad/src/jsonrpc/api.rs @@ -404,8 +404,15 @@ fn create_recovery(control: &DaemonControl, params: Params) -> Result for Error { commands::CommandError::TxBroadcast(_) => { Error::new(ErrorCode::ServerError(BROADCAST_ERROR), e.to_string()) } + commands::CommandError::RecoveryToOwnAddress(_) => Error::new( + ErrorCode::ServerError(RECOVERY_TO_OWN_ADDRESS_ERROR), + e.to_string(), + ), } } } diff --git a/lianad/src/testutils.rs b/lianad/src/testutils.rs index da03225a5c..15ccef38c0 100644 --- a/lianad/src/testutils.rs +++ b/lianad/src/testutils.rs @@ -156,8 +156,10 @@ struct DummyDbState { timestamp: u32, rescan_timestamp: Option, last_poll_timestamp: Option, + derived_addresses: HashMap, } +#[derive(Clone)] pub struct DummyDatabase { db: sync::Arc>, } @@ -191,6 +193,7 @@ impl DummyDatabase { timestamp: now, rescan_timestamp: None, last_poll_timestamp: None, + derived_addresses: HashMap::new(), })), } } @@ -200,6 +203,19 @@ impl DummyDatabase { self.db.write().unwrap().coins.insert(coin.outpoint, coin); } } + + pub fn insert_derived_address( + &mut self, + addr: bitcoin::Address, + index: bip32::ChildNumber, + is_change: bool, + ) { + self.db + .write() + .unwrap() + .derived_addresses + .insert(addr, (index, is_change)); + } } impl DatabaseConnection for DummyDatabase { @@ -363,9 +379,9 @@ impl DatabaseConnection for DummyDatabase { fn derivation_index_by_address( &mut self, - _: &bitcoin::Address, + addr: &bitcoin::Address, ) -> Option<(bip32::ChildNumber, bool)> { - None + self.db.read().unwrap().derived_addresses.get(addr).copied() } fn coins_by_outpoints( diff --git a/tests/test_rpc.py b/tests/test_rpc.py index e7edd69051..c10fe63c73 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -202,7 +202,6 @@ def test_listaddresses(lianad): def test_listrevealedaddresses(lianad, bitcoind): - # Get addresses for reference: addresses = lianad.rpc.listaddresses(0, 10)["addresses"] @@ -1298,8 +1297,23 @@ def test_create_recovery(lianad, bitcoind): ][0] reco_address = bitcoind.rpc.getnewaddress() res = lianad.rpc.createrecovery(reco_address, 18) + + # No warnings because it swept to an external address + assert len(res["warnings"]) == 0 reco_psbt = PSBT.from_base64(res["psbt"]) + # Recover to an address of this same wallet (refuse) + own_address = lianad.rpc.getnewaddress()["address"] + with pytest.raises(RpcError, match="belongs to the same wallet"): + lianad.rpc.createrecovery(own_address, 18) + + # Recover to own address (explicitly allowed) + res_own = lianad.rpc.createrecovery( + address=own_address, feerate=18, allow_own_address=True + ) + assert len(res_own["warnings"]) == 1 + assert "belongs to the same wallet" in res_own["warnings"][0] + # 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"])