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..bf038cd708 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,14 @@ 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 this wallet. Recovered \ + funds would be locked under the same descriptor again. Set \ + `allow_own_address` to allow it." + ) + } Self::OutpointNotRecoverable(op, t) => { write!(f, "Coin at '{op}' is not recoverable with timelock '{t}'",) } @@ -1251,12 +1261,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 +1325,21 @@ 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() { + if !allow_own_address.unwrap_or(false) { + return Err(CommandError::RecoveryToOwnAddress(sweep_addr.addr)); + } + warnings.push(format!( + "Address {} belongs to the same wallet. Recovered funds would \ + be locked under the same descriptor again.", + sweep_addr.addr + )); + } + let locktime = self.anti_fee_sniping_locktime(); let CreateSpendRes { psbt, has_change, .. @@ -1329,7 +1357,7 @@ impl DaemonControl { self.maybe_increase_last_deriv_index(&mut db_conn, &sweep_addr_info); } - Ok(CreateRecoveryResult { psbt }) + Ok(CreateRecoveryResult { psbt, warnings }) } } @@ -1523,6 +1551,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 +3167,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 +3179,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 +3194,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 +3218,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 +3267,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 +3302,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 +3318,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 +3347,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 +3377,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 +3393,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 +3409,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..0efb975b2b 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 this 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"])