nik/liana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge #905: spend: warn when overpaying fee

Browse Source 
5a15c744e796c9ebe4f6c1eb3010b824189f7855 commands: return warnings from spend creation (jp1ac4)
da474ad6ce1a7553bdc16a586565ef509d74f8ee spend: add warning when adding change to fee (jp1ac4)
8d84f0de863c8fea97d7402651482b908b6d5b87 spend: return max possible change from coin selection (jp1ac4)
e4d8330f34cf82e43bd7d1dc54e56ebf4cfe0baf spend: use debug log level in coin selection (jp1ac4)

Pull request description:

  This is a first step towards #811. The GUI will be updated in a follow-up PR.

  It adds a warning if there is any change/excess available that has been added to the fee, building on the assumption from https://github.com/bitcoindevkit/coin-select/pull/14 that the decision whether to include change depends on whether the excess is over a threshold. The function `select_coins_for_spend()` now returns this threshold so that the caller can check if the possible change amount is above it.

  I've used an `enum` for the different warnings, but could use strings directly.

  If the approach looks fine, I'll add some tests.

ACKs for top commit:
  darosior:
    ACK 5a15c744e796c9ebe4f6c1eb3010b824189f7855

Tree-SHA512: 12716b1e299d3154c520667c66aeb7a176a770f4a1d53125a8334d7c5a7e7bb2ce1dcb795ad5998bffc1303d9cb34c651cf8d3ef3dee8210572d75069012bddd
This commit is contained in:
Antoine Poinsot 2024-01-13 13:18:30 +01:00
commit 501dce4372
No known key found for this signature in database
GPG Key ID: E13FC145CD3F4304
4 changed files with 185 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
doc/API.md
View File

@ -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`

84
src/commands/mod.rs
View File

@ -490,7 +490,11 @@ impl DaemonControl {
// derivation index in case any address in the transaction outputs was ours and from the
// future.
let change_info = change_address.info;
let CreateSpendRes { psbt, has_change } = create_spend(
let CreateSpendRes {
psbt,
has_change,
warnings,
} = create_spend(
&self.config.main_descriptor,
&self.secp,
&mut tx_getter,
@ -506,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> {
@ -810,6 +817,7 @@ impl DaemonControl {
let CreateSpendRes {
psbt: rbf_psbt,
has_change,
warnings,
} = match create_spend(
&self.config.main_descriptor,
&self.secp,
@ -854,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(),
});
}
}
@ -985,7 +996,9 @@ impl DaemonControl {
}
let sweep_addr_info = sweep_addr.info;
let CreateSpendRes { psbt, has_change } = create_spend(
let CreateSpendRes {
psbt, has_change, ..
} = create_spend(
&self.config.main_descriptor,
&self.secp,
&mut tx_getter,
@ -1098,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<String>,
}
#[derive(Debug, Clone, Serialize, Deserialize)]
@ -1136,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},
@ -1369,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()
@ -1445,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.

78
src/spend.rs
View File

@ -14,6 +14,7 @@ use miniscript::bitcoin::{
psbt::{Input as PsbtIn, Output as PsbtOut, Psbt},
secp256k1,
};
use serde::{Deserialize, Serialize};
/// We would never create a transaction with an output worth less than this.
/// That's 1$ at 20_000$ per BTC.
@ -170,6 +171,21 @@ pub struct CandidateCoin {
pub sequence: Option<bitcoin::Sequence>,
}
/// A coin selection result.
///
/// A change output should only be added if `change_amount > 0`.
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct CoinSelectionRes {
/// Selected candidates.
pub selected: Vec<CandidateCoin>,
/// Change amount that should be included according to the change policy used
/// for selection.
pub change_amount: bitcoin::Amount,
/// Maximum change amount possible with the selection irrespective of any change
/// policy.
pub max_change_amount: bitcoin::Amount,
}
/// Metric based on [`LowestFee`] that aims to minimize transaction fees
/// with the additional option to only find solutions with a change output.
///
@ -254,7 +270,7 @@ fn select_coins_for_spend(
min_fee: u64,
max_sat_weight: u32,
must_have_change: bool,
) -> Result<(Vec<CandidateCoin>, bitcoin::Amount), InsufficientFunds> {
) -> Result<CoinSelectionRes, InsufficientFunds> {
let out_value_nochange = base_tx.output.iter().map(|o| o.value.to_sat()).sum();
// Create the coin selector from the given candidates. NOTE: the coin selector keeps track
@ -344,7 +360,7 @@ fn select_coins_for_spend(
#[cfg(debug)]
let bnb_rounds = bnb_rounds / 1_000;
if let Err(e) = selector.run_bnb(lowest_fee_change_cond, bnb_rounds) {
log::warn!(
log::debug!(
"Coin selection error: '{}'. Selecting coins by descending value per weight unit...",
e.to_string()
);
@ -374,14 +390,27 @@ fn select_coins_for_spend(
// By now, selection is complete and we can check how much change to give according to our policy.
let drain = change_policy(&selector, target);
let change_amount = bitcoin::Amount::from_sat(drain.value);
Ok((
// Max available change is given by the excess when adding a change output with zero value.
let drain_novalue = bdk_coin_select::Drain {
weights: drain_weights,
value: 0,
};
let max_change_amount = bitcoin::Amount::from_sat(
selector
.excess(target, drain_novalue)
.max(0) // negative excess would mean insufficient funds to pay for change output
.try_into()
.expect("value is non-negative"),
);
Ok(CoinSelectionRes {
selected: selector
.selected_indices()
.iter()
.map(|i| candidate_coins[*i])
.collect(),
change_amount,
))
max_change_amount,
})
}
// Get the derived descriptor for this coin
@ -427,11 +456,31 @@ pub enum SpendTxFees {
Rbf(u64, u64),
}
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
pub enum CreateSpendWarning {
ChangeAddedToFee(u64),
}
impl fmt::Display for CreateSpendWarning {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
CreateSpendWarning::ChangeAddedToFee(amt) => write!(
f,
"Change amount of {} sat{} added to fee as it was too small to create a transaction output.",
amt,
if *amt > 1 {"s"} else {""},
),
}
}
}
pub struct CreateSpendRes {
/// The created PSBT.
pub psbt: Psbt,
/// Whether the created PSBT has a change output.
pub has_change: bool,
/// Warnings relating to the PSBT.
pub warnings: Vec<CreateSpendWarning>,
}
/// Create a PSBT for a transaction spending some, or all, of `candidate_coins` to `destinations`.
@ -480,6 +529,7 @@ pub fn create_spend(
// 3. Add the selected coins as inputs to the transaction.
// 4. Finalize the PSBT and sanity check it before returning it.
let mut warnings = Vec::new();
let (feerate_vb, min_fee) = match fees {
SpendTxFees::Regular(feerate) => (feerate, 0),
SpendTxFees::Rbf(feerate, fee) => (feerate, fee),
@ -535,7 +585,11 @@ pub fn create_spend(
};
// Now select the coins necessary using the provided candidates and determine whether
// there is any leftover to create a change output.
let (selected_coins, change_amount) = {
let CoinSelectionRes {
selected,
change_amount,
max_change_amount,
} = {
// At this point the transaction still has no input and no change output, as expected
// by the coins selection helper function.
assert!(tx.input.is_empty());
@ -590,11 +644,15 @@ pub fn create_spend(
bip32_derivation,
..PsbtOut::default()
});
} else if max_change_amount.to_sat() > 0 {
warnings.push(CreateSpendWarning::ChangeAddedToFee(
max_change_amount.to_sat(),
));
}
// Iterate through selected coins and add necessary information to the PSBT inputs.
let mut psbt_ins = Vec::with_capacity(selected_coins.len());
for cand in &selected_coins {
let mut psbt_ins = Vec::with_capacity(selected.len());
for cand in &selected {
let sequence = cand
.sequence
.unwrap_or(bitcoin::Sequence::ENABLE_RBF_NO_LOCKTIME);
@ -636,5 +694,9 @@ pub fn create_spend(
sanity_check_psbt(main_descriptor, &psbt)?;
// TODO: maybe check for common standardness rules (max size, ..)?
Ok(CreateSpendRes { psbt, has_change })
Ok(CreateSpendRes {
psbt,
has_change,
warnings,
})
}

41
tests/test_spend.py
View File

@ -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)