From 04b6ad06cb85685270a588e60e7668c79afb60bc Mon Sep 17 00:00:00 2001 From: niftynei Date: Mon, 12 Jul 2021 12:06:29 -0500 Subject: [PATCH] change fees: more accurate rounding for change amount We were getting off-by-one for the total amount that the change is for, since it rounds the fee *down*, independent of the total weight of the entire tx. We fix this by using the diff btw the fee of the total weight (w/ and w/o the change output) --- bitcoin/tx.c | 14 +++++++++++--- bitcoin/tx.h | 6 +++++- plugins/spender/multiwithdraw.c | 3 ++- plugins/txprepare.c | 11 ++++++++++- wallet/reservation.c | 2 +- 5 files changed, 29 insertions(+), 7 deletions(-) diff --git a/bitcoin/tx.c b/bitcoin/tx.c index 5dac3209f..c7ffe12ee 100644 --- a/bitcoin/tx.c +++ b/bitcoin/tx.c @@ -899,14 +899,22 @@ size_t bitcoin_tx_simple_input_weight(bool p2sh) bitcoin_tx_simple_input_witness_weight()); } -struct amount_sat change_amount(struct amount_sat excess, u32 feerate_perkw) +struct amount_sat change_amount(struct amount_sat excess, u32 feerate_perkw, + size_t total_weight) { size_t outweight; + struct amount_sat change_fee; /* Must be able to pay for its own additional weight */ outweight = bitcoin_tx_output_weight(BITCOIN_SCRIPTPUBKEY_P2WPKH_LEN); - if (!amount_sat_sub(&excess, - excess, amount_tx_fee(feerate_perkw, outweight))) + + /* Rounding can cause off by one errors, so we do this */ + if (!amount_sat_sub(&change_fee, + amount_tx_fee(feerate_perkw, outweight + total_weight), + amount_tx_fee(feerate_perkw, total_weight))) + return AMOUNT_SAT(0); + + if (!amount_sat_sub(&excess, excess, change_fee)) return AMOUNT_SAT(0); /* Must be non-dust */ diff --git a/bitcoin/tx.h b/bitcoin/tx.h index 3f941cee5..255155ee3 100644 --- a/bitcoin/tx.h +++ b/bitcoin/tx.h @@ -289,7 +289,11 @@ size_t bitcoin_tx_simple_input_weight(bool p2sh); * If it's not worth (or possible) to make change, returns AMOUNT_SAT(0). * Otherwise returns the amount of the change output to add (@excess minus * the additional fee for the change output itself). + * + * We pass in the total_weight of the tx (up until this point) so as + * to avoid any off-by-one errors with rounding the change fee (down) */ -struct amount_sat change_amount(struct amount_sat excess, u32 feerate_perkw); +struct amount_sat change_amount(struct amount_sat excess, u32 feerate_perkw, + size_t total_weight); #endif /* LIGHTNING_BITCOIN_TX_H */ diff --git a/plugins/spender/multiwithdraw.c b/plugins/spender/multiwithdraw.c index bb36c971c..76a047a10 100644 --- a/plugins/spender/multiwithdraw.c +++ b/plugins/spender/multiwithdraw.c @@ -474,7 +474,8 @@ mw_after_fundpsbt(struct command *cmd, } /* Handle any change output. */ - mw->change_amount = change_amount(excess_sat, feerate_per_kw); + mw->change_amount = change_amount(excess_sat, feerate_per_kw, + estimated_final_weight); mw->change_needed = !amount_sat_eq(mw->change_amount, AMOUNT_SAT(0)); if (mw->change_needed) diff --git a/plugins/txprepare.c b/plugins/txprepare.c index 64e9b4832..84da4161a 100644 --- a/plugins/txprepare.c +++ b/plugins/txprepare.c @@ -277,6 +277,7 @@ static struct command_result *psbt_created(struct command *cmd, const jsmntok_t *psbttok; struct out_req *req; struct amount_sat excess; + u32 weight; psbttok = json_get_member(buf, result, "psbt"); txp->psbt = json_tok_psbt(txp, buf, psbttok); @@ -300,6 +301,14 @@ static struct command_result *psbt_created(struct command *cmd, result->end - result->start, buf + result->start); + if (!json_to_number(buf, json_get_member(buf, result, + "estimated_final_weight"), + &weight)) + return command_fail(cmd, LIGHTNINGD, + "Unparsable estimated_final_weight: '%.*s'", + result->end - result->start, + buf + result->start); + /* If we have an "all" output, now we can derive its value: excess * in this case will be total value after inputs paid for themselves. */ if (txp->all_output_idx != -1) { @@ -313,7 +322,7 @@ static struct command_result *psbt_created(struct command *cmd, } /* So, do we need change? */ - txp->change_amount = change_amount(excess, txp->feerate); + txp->change_amount = change_amount(excess, txp->feerate, weight); if (amount_sat_eq(txp->change_amount, AMOUNT_SAT(0))) return finish_txprepare(cmd, txp); diff --git a/wallet/reservation.c b/wallet/reservation.c index 7ca74557d..db16cfc86 100644 --- a/wallet/reservation.c +++ b/wallet/reservation.c @@ -369,7 +369,7 @@ static struct command_result *finish_psbt(struct command *cmd, u8 *b32script; /* Checks for dust, returns 0sat if below dust */ - change = change_amount(excess, feerate_per_kw); + change = change_amount(excess, feerate_per_kw, weight); if (!amount_sat_greater(change, AMOUNT_SAT(0))) { excess_as_change = false; goto fee_calc;