From 5a15c744e796c9ebe4f6c1eb3010b824189f7855 Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Thu, 4 Jan 2024 21:27:36 +0000 Subject: [PATCH] commands: return warnings from spend creation --- doc/API.md | 7 ++-- src/commands/mod.rs | 79 ++++++++++++++++++++++++++++++++++++++++++--- tests/test_spend.py | 41 +++++++++++++++++------ 3 files changed, 110 insertions(+), 17 deletions(-) diff --git a/doc/API.md b/doc/API.md index 35a9b3b9..d629bc7c 100644 --- a/doc/API.md +++ b/doc/API.md @@ -179,9 +179,10 @@ This command will refuse to create any output worth less than 5k sats. #### Response -| Field | Type | Description | -| -------------- | --------- | ---------------------------------------------------- | -| `psbt` | string | PSBT of the spending transaction, encoded as base64. | +| Field | Type | Description | +| -------------- | ----------------- | ---------------------------------------------------- | +| `psbt` | string | PSBT of the spending transaction, encoded as base64. | +| `warnings` | list of string | Warnings, if any, generated during spend creation. | ### `updatespend` diff --git a/src/commands/mod.rs b/src/commands/mod.rs index f300787c..751726fa 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -491,7 +491,9 @@ impl DaemonControl { // future. let change_info = change_address.info; let CreateSpendRes { - psbt, has_change, .. + psbt, + has_change, + warnings, } = create_spend( &self.config.main_descriptor, &self.secp, @@ -508,7 +510,10 @@ impl DaemonControl { self.maybe_increase_next_deriv_index(&mut db_conn, &change_info); } - Ok(CreateSpendResult { psbt }) + Ok(CreateSpendResult { + psbt, + warnings: warnings.iter().map(|w| w.to_string()).collect(), + }) } pub fn update_spend(&self, mut psbt: Psbt) -> Result<(), CommandError> { @@ -812,7 +817,7 @@ impl DaemonControl { let CreateSpendRes { psbt: rbf_psbt, has_change, - .. + warnings, } = match create_spend( &self.config.main_descriptor, &self.secp, @@ -857,7 +862,10 @@ impl DaemonControl { self.maybe_increase_next_deriv_index(&mut db_conn, &change_address.info); } - return Ok(CreateSpendResult { psbt: rbf_psbt }); + return Ok(CreateSpendResult { + psbt: rbf_psbt, + warnings: warnings.iter().map(|w| w.to_string()).collect(), + }); } } @@ -1103,6 +1111,7 @@ pub struct ListCoinsResult { pub struct CreateSpendResult { #[serde(serialize_with = "ser_to_string", deserialize_with = "deser_fromstr")] pub psbt: Psbt, + pub warnings: Vec, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -1141,6 +1150,7 @@ mod tests { use super::*; use crate::{bitcoin::Block, database::BlockInfo, spend::InsaneFeeInfo, testutils::*}; + use bdk_coin_select::InsufficientFunds; use bitcoin::{ bip32::{self, ChildNumber}, blockdata::transaction::{TxIn, TxOut, Version as TxVersion}, @@ -1374,6 +1384,8 @@ mod tests { assert_eq!(tx.input.len(), 1); assert_eq!(tx.input[0].previous_output, dummy_op); assert_eq!(tx.output.len(), 2); + // It has change so no warnings expected. + assert!(res.warnings.is_empty()); assert_eq!( tx.output[0].script_pubkey, dummy_addr.payload().script_pubkey() @@ -1450,6 +1462,65 @@ mod tests { dummy_addr.payload().script_pubkey() ); assert_eq!(tx.output[0].value.to_sat(), 95_000); + // change = 100_000 - 95_000 - /* fee without change */ 127 - /* extra fee for change output */ 43 = 4830 + assert_eq!(res.warnings, vec!["Change amount of 4830 sats added to fee as it was too small to create a transaction output."]); + + // Increase the target value by the change amount and the warning will disappear. + *destinations.get_mut(&dummy_addr).unwrap() = 95_000 + 4_830; + let res = control + .create_spend(&destinations, &[dummy_op], 1, None) + .unwrap(); + let tx = res.psbt.unsigned_tx; + assert_eq!(tx.output.len(), 1); + assert!(res.warnings.is_empty()); + + // Now increase target also by the extra fee that was paying for change and we can still create the spend. + *destinations.get_mut(&dummy_addr).unwrap() = + 95_000 + 4_830 + /* fee for change output */ 43; + let res = control + .create_spend(&destinations, &[dummy_op], 1, None) + .unwrap(); + let tx = res.psbt.unsigned_tx; + assert_eq!(tx.output.len(), 1); + assert!(res.warnings.is_empty()); + + // Now increase the target by 1 more sat and we will have insufficient funds. + *destinations.get_mut(&dummy_addr).unwrap() = + 95_000 + 4_830 + /* fee for change output */ 43 + 1; + assert_eq!( + control.create_spend(&destinations, &[dummy_op], 1, None), + Err(CommandError::SpendCreation( + SpendCreationError::CoinSelection(InsufficientFunds { missing: 1 }) + )) + ); + + // Now decrease the target so that the lost change is just 1 sat. + *destinations.get_mut(&dummy_addr).unwrap() = + 100_000 - /* fee without change */ 127 - /* extra fee for change output */ 43 - 1; + let res = control + .create_spend(&destinations, &[dummy_op], 1, None) + .unwrap(); + // Message uses "sat" instead of "sats" when value is 1. + assert_eq!(res.warnings, vec!["Change amount of 1 sat added to fee as it was too small to create a transaction output."]); + + // Now decrease the target value so that we have enough for a change output. + *destinations.get_mut(&dummy_addr).unwrap() = + 95_000 - /* fee without change */ 127 - /* extra fee for change output */ 43; + let res = control + .create_spend(&destinations, &[dummy_op], 1, None) + .unwrap(); + let tx = res.psbt.unsigned_tx; + assert_eq!(tx.output.len(), 2); + assert_eq!(tx.output[1].value.to_sat(), 5_000); + assert!(res.warnings.is_empty()); + + // Now increase the target by 1 and we'll get a warning again, this time for 1 less than the dust threshold. + *destinations.get_mut(&dummy_addr).unwrap() = + 95_000 - /* fee without change */ 127 - /* extra fee for change output */ 43 + 1; + let res = control + .create_spend(&destinations, &[dummy_op], 1, None) + .unwrap(); + assert_eq!(res.warnings, vec!["Change amount of 4999 sats added to fee as it was too small to create a transaction output."]); // Now if we mark the coin as spent, we won't create another Spend transaction containing // it. diff --git a/tests/test_spend.py b/tests/test_spend.py index 94ba1a59..82ac0f58 100644 --- a/tests/test_spend.py +++ b/tests/test_spend.py @@ -25,6 +25,8 @@ def test_spend_change(lianad, bitcoind): spend_psbt = PSBT.from_base64(res["psbt"]) assert len(spend_psbt.o) == 3 assert len(spend_psbt.tx.vout) == 3 + # Since the transaction contains a change output there is no warning. + assert len(res["warnings"]) == 0 # Sign and broadcast this first Spend transaction. signed_psbt = lianad.signer.sign_psbt(spend_psbt) @@ -46,6 +48,8 @@ def test_spend_change(lianad, bitcoind): } res = lianad.rpc.createspend(destinations, outpoints, 2) spend_psbt = PSBT.from_base64(res["psbt"]) + assert len(spend_psbt.o) == 2 + assert len(res["warnings"]) == 0 # We can sign and broadcast it. signed_psbt = lianad.signer.sign_psbt(spend_psbt) @@ -110,6 +114,8 @@ def test_coin_marked_spent(lianad, bitcoind): res = lianad.rpc.createspend(destinations, [outpoint], 6) psbt = PSBT.from_base64(res["psbt"]) sign_and_broadcast(psbt) + assert len(psbt.o) == 2 + assert len(res["warnings"]) == 0 # Spend the second coin without a change output outpoint = next( @@ -123,30 +129,43 @@ def test_coin_marked_spent(lianad, bitcoind): res = lianad.rpc.createspend(destinations, [outpoint], 1) psbt = PSBT.from_base64(res["psbt"]) sign_and_broadcast(psbt) + assert len(psbt.o) == 1 + assert len(res["warnings"]) == 1 + assert ( + res["warnings"][0] + == "Change amount of 830 sats added to fee as it was too small to create a transaction output." + ) # Spend the third coin to an address of ours, no change - outpoints = [ - c["outpoint"] - for c in lianad.rpc.listcoins()["coins"] - if deposit_c in c["outpoint"] - ] + coins_c = [c for c in lianad.rpc.listcoins()["coins"] if deposit_c in c["outpoint"]] destinations = { lianad.rpc.getnewaddress()["address"]: int(0.03 * COIN) - 1_000, } - res = lianad.rpc.createspend(destinations, [outpoints[0]], 1) + outpoint_3 = [c["outpoint"] for c in coins_c if c["amount"] == 0.03 * COIN][0] + res = lianad.rpc.createspend(destinations, [outpoint_3], 1) psbt = PSBT.from_base64(res["psbt"]) sign_and_broadcast(psbt) + assert len(psbt.o) == 1 + assert len(res["warnings"]) == 1 + assert ( + res["warnings"][0] + == "Change amount of 818 sats added to fee as it was too small to create a transaction output." + ) # Spend the fourth coin to an address of ours, with change + outpoint_4 = [c["outpoint"] for c in coins_c if c["amount"] == 0.04 * COIN][0] destinations = { lianad.rpc.getnewaddress()["address"]: int(0.04 * COIN / 2), } - res = lianad.rpc.createspend(destinations, [outpoints[1]], 18) + res = lianad.rpc.createspend(destinations, [outpoint_4], 18) psbt = PSBT.from_base64(res["psbt"]) sign_and_broadcast(psbt) + assert len(psbt.o) == 2 + assert len(res["warnings"]) == 0 - # Batch spend the fourth and fifth coins - outpoint = next( + # Batch spend the fifth and sixth coins + outpoint_5 = [c["outpoint"] for c in coins_c if c["amount"] == 0.05 * COIN][0] + outpoint_6 = next( c["outpoint"] for c in lianad.rpc.listcoins()["coins"] if deposit_d in c["outpoint"] @@ -156,9 +175,11 @@ def test_coin_marked_spent(lianad, bitcoind): lianad.rpc.getnewaddress()["address"]: int(0.01 * COIN), bitcoind.rpc.getnewaddress(): int(0.01 * COIN), } - res = lianad.rpc.createspend(destinations, [outpoints[2], outpoint], 2) + res = lianad.rpc.createspend(destinations, [outpoint_5, outpoint_6], 2) psbt = PSBT.from_base64(res["psbt"]) sign_and_broadcast(psbt) + assert len(psbt.o) == 4 + assert len(res["warnings"]) == 0 # All the spent coins must have been detected as such all_deposits = (deposit_a, deposit_b, deposit_c, deposit_d)