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

@@ -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 */

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 */

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)

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

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;