multifundchannel: use excess_as_change flag to simplify logic.

This was added to fundpsbt/utxopsbt in v0.10, but the spender plugin
didn't take advantage of it, instead calculating its own change amount
and output.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

plugins/spender/multifundchannel.c

@@ -15,14 +15,14 @@
 
 extern const struct chainparams *chainparams;
 
-/* Flag set when any of the destinations has a value of "all".  */
+/* Which destinations has a value of "all", or -1.  */
-static bool has_all(const struct multifundchannel_command *mfc)
+static struct multifundchannel_destination *all_dest(const struct multifundchannel_command *mfc)
 {
 	for (size_t i = 0; i < tal_count(mfc->destinations); i++) {
 		if (mfc->destinations[i].all)
-			return true;
+			return &mfc->destinations[i];
 	}
-	return false;
+	return NULL;
 }
 
 size_t dest_count(const struct multifundchannel_command *mfc,
@@ -162,19 +162,15 @@ mfc_cleanup_done(struct command *cmd,
 		return command_still_pending(cmd);
 }
 
-/* Cleans up a txid by doing `txdiscard` on it.  */
+static struct command_result *unreserve_call(struct command *cmd,
-static void
+					     struct wally_psbt *psbt,
-mfc_cleanup_psbt(struct command *cmd,
+					     void *cb, void *cbdata)
-		 struct multifundchannel_cleanup *cleanup,
-		 struct wally_psbt *psbt)
 {
 	struct wally_psbt *pruned_psbt;
 	struct out_req *req = jsonrpc_request_start(cmd->plugin,
 						    cmd,
 						    "unreserveinputs",
-						    &mfc_cleanup_done,
+						    cb, cb, cbdata);
-						    &mfc_cleanup_done,
-						    cleanup);
 
 	/* We might have peer's inputs on this, get rid of them */
 	tal_wally_start();
@@ -191,7 +187,16 @@ mfc_cleanup_psbt(struct command *cmd,
 
 	json_add_psbt(req->js, "psbt", take(pruned_psbt));
 	json_add_u32(req->js, "reserve", 2016);
-	send_outreq(cmd->plugin, req);
+	return send_outreq(cmd->plugin, req);
+}
+
+/* Cleans up a txid by doing `txdiscard` on it.  */
+static void
+mfc_cleanup_psbt(struct command *cmd,
+		 struct multifundchannel_cleanup *cleanup,
+		 struct wally_psbt *psbt)
+{
+	unreserve_call(cmd, psbt, mfc_cleanup_done, cleanup);
 }
 
 /* Cleans up a `openchannel_init` by doing `openchannel_abort` for the channel*/
@@ -820,7 +825,8 @@ perform_fundchannel_complete(struct multifundchannel_command *mfc)
 	return command_still_pending(mfc->cmd);
 }
 
-/*~ The PSBT we are holding currently has no outputs.
+/*~ The PSBT we are holding currently has no outputs
+  (except an optional change output).
 We now proceed to filling in those outputs now that
 we know what the funding scriptpubkeys are.
 
@@ -856,7 +862,7 @@ perform_funding_tx_finalize(struct multifundchannel_command *mfc)
 
 	/* Construct a deck of destinations.  */
 	deck = tal_arr(tmpctx, struct multifundchannel_destination *,
-		       v1_dest_count + mfc->change_needed);
+		       v1_dest_count);
 
 	deck_i = 0;
 	for (i = 0; i < tal_count(mfc->destinations); i++) {
@@ -867,10 +873,6 @@ perform_funding_tx_finalize(struct multifundchannel_command *mfc)
 		deck[deck_i++] = &mfc->destinations[i];
 	}
 
-	/* Add a NULL into the deck as a proxy for change output, if
-	 * needed.  */
-	if (mfc->change_needed)
-		deck[v1_dest_count] = NULL;
 	/* Fisher-Yates shuffle.  */
 	for (i = tal_count(deck); i > 1; --i) {
 		size_t j = pseudorand(i);
@@ -884,38 +886,27 @@ perform_funding_tx_finalize(struct multifundchannel_command *mfc)
 
 	/* Now that we have our outputs shuffled, add outputs to the PSBT.  */
 	for (unsigned int outnum = 0; outnum < tal_count(deck); ++outnum) {
+		struct multifundchannel_destination *dest;
+
 		if (outnum != 0)
 			tal_append_fmt(&content, ", ");
-		if (deck[outnum]) {
+
-			/* Funding outpoint.  */
+		/* Funding outpoint.  */
-			struct multifundchannel_destination *dest;
+		dest = deck[outnum];
-			dest = deck[outnum];
+		(void) psbt_insert_output(mfc->psbt,
-			(void) psbt_insert_output(mfc->psbt,
+					  dest->funding_script,
-						  dest->funding_script,
+					  dest->amount,
-						  dest->amount,
+					  outnum);
-						  outnum);
+		/* The actual output index will be based on the
-			/* The actual output index will be based on the
+		 * serial_id if this contains any v2 outputs */
-			 * serial_id if this contains any v2 outputs */
+		if (v2_dest_count == 0)
-			if (v2_dest_count == 0)
+			dest->outnum = outnum;
-				dest->outnum = outnum;
+		tal_append_fmt(&content, "%s: %s",
-			tal_append_fmt(&content, "%s: %s",
+			       type_to_string(tmpctx, struct node_id,
-				       type_to_string(tmpctx, struct node_id,
+					      &dest->id),
-						      &dest->id),
+			       type_to_string(tmpctx,
-				       type_to_string(tmpctx,
+					      struct amount_sat,
-						      struct amount_sat,
+					      &dest->amount));
-						      &dest->amount));
-		} else {
-			/* Change output.  */
-			assert(mfc->change_needed);
-			(void) psbt_insert_output(mfc->psbt,
-						  mfc->change_scriptpubkey,
-						  mfc->change_amount,
-						  outnum);
-			tal_append_fmt(&content, "change: %s",
-				       type_to_string(tmpctx,
-						      struct amount_sat,
-						      &mfc->change_amount));
-		}
 	}
 
 	if (v2_dest_count > 0) {
@@ -1206,139 +1197,19 @@ mfc_psbt_acquired(struct multifundchannel_command *mfc)
 	return perform_channel_start(mfc);
 }
 
+/* Limited recursion if we discover 'all' is too big for non-wumbo! */
 static struct command_result *
-after_newaddr(struct command *cmd,
+perform_fundpsbt(struct multifundchannel_command *mfc, u32 feerate);
-	      const char *buf,
-	      const jsmntok_t *result,
-	      struct multifundchannel_command *mfc)
-{
-	const jsmntok_t *field;
-
-	field = json_get_member(buf, result, "bech32");
-	if (!field)
-		plugin_err(cmd->plugin,
-			   "No bech32 field in newaddr result: %.*s",
-			   json_tok_full_len(result),
-			   json_tok_full(buf, result));
-	if (json_to_address_scriptpubkey(mfc, chainparams, buf, field,
-					 &mfc->change_scriptpubkey)
-	 != ADDRESS_PARSE_SUCCESS)
-		plugin_err(cmd->plugin,
-			   "Unparseable bech32 field in newaddr result: %.*s",
-			   json_tok_full_len(result),
-			   json_tok_full(buf, result));
-
-	return mfc_psbt_acquired(mfc);
-}
 
 static struct command_result *
-acquire_change_address(struct multifundchannel_command *mfc)
+retry_fundpsbt_capped_all(struct command *cmd,
+			  const char *buf,
+			  const jsmntok_t *result,
+			  struct multifundchannel_command *mfc)
 {
-	struct out_req *req;
+	/* We've unreserved this, now free it and try again! */
-	req = jsonrpc_request_start(mfc->cmd->plugin, mfc->cmd,
+	tal_free(mfc->psbt);
-				    "newaddr",
+	return perform_fundpsbt(mfc, mfc->feerate_per_kw);
-				    &after_newaddr, &mfc_forward_error,
-				    mfc);
-	json_add_string(req->js, "addresstype", "bech32");
-	return send_outreq(mfc->cmd->plugin, req);
-}
-
-static struct command_result *
-handle_mfc_change(struct multifundchannel_command *mfc)
-{
-	size_t change_weight;
-	struct amount_sat change_fee, fee_paid, total_fee;
-	struct amount_sat change_min_limit;
-
-	/* Determine if adding a change output is worth it.
-	 * Get the weight of a change output and how much it
-	 * costs.
-	 */
-	change_weight = bitcoin_tx_output_weight(
-				BITCOIN_SCRIPTPUBKEY_P2WPKH_LEN);
-
-	/* To avoid 'off-by-one' errors due to rounding down
-	 * (which we do in `amount_tx_fee`), we find the total calculated
-	 * fees (estimated_weight + change weight @ feerate) and subtract
-	 * the originally calculated fees (estimated_weight @ feerate) */
-	fee_paid = amount_tx_fee(mfc->feerate_per_kw,
-				 mfc->estimated_final_weight);
-	total_fee = amount_tx_fee(mfc->feerate_per_kw,
-				  mfc->estimated_final_weight + change_weight);
-	if (!amount_sat_sub(&change_fee, total_fee, fee_paid))
-		abort();
-
-	/* The limit is equal to the change_fee plus the dust limit.  */
-	if (!amount_sat_add(&change_min_limit,
-			    change_fee, chainparams->dust_limit))
-		plugin_err(mfc->cmd->plugin,
-			   "Overflow dust limit and change fee.");
-
-	/* Is the excess over the limit?  */
-	if (amount_sat_greater(mfc->excess_sat, change_min_limit)) {
-		bool ok = amount_sat_sub(&mfc->change_amount,
-					 mfc->excess_sat, change_fee);
-		assert(ok);
-		mfc->change_needed = true;
-		if (!mfc->change_scriptpubkey)
-			return acquire_change_address(mfc);
-	} else
-		mfc->change_needed = false;
-
-	return mfc_psbt_acquired(mfc);
-}
-
-/* If one of the destinations specified "all", figure out how much that is.  */
-static struct command_result *
-compute_mfc_all(struct multifundchannel_command *mfc)
-{
-	size_t all_index = SIZE_MAX;
-	struct multifundchannel_destination *all_dest;
-
-	assert(has_all(mfc));
-
-	for (size_t i = 0; i < tal_count(mfc->destinations); ++i) {
-		struct multifundchannel_destination *dest;
-		dest = &mfc->destinations[i];
-		if (dest->all) {
-			assert(all_index == SIZE_MAX);
-			all_index = i;
-			continue;
-		}
-		/* Subtract the amount from the excess.  */
-		if (!amount_sat_sub(&mfc->excess_sat,
-				    mfc->excess_sat, dest->amount))
-			/* Not enough funds!  */
-			return mfc_fail(mfc, FUND_CANNOT_AFFORD,
-					"Insufficient funds.");
-	}
-	assert(all_index != SIZE_MAX);
-	all_dest = &mfc->destinations[all_index];
-
-	/* Is the excess above the dust amount?  */
-	if (amount_sat_less(mfc->excess_sat, chainparams->dust_limit))
-		return mfc_fail(mfc, FUND_OUTPUT_IS_DUST,
-				"Output 'all' %s would be dust",
-				type_to_string(tmpctx, struct amount_sat,
-					       &mfc->excess_sat));
-
-	/* Assign the remainder to the 'all' output.  */
-	all_dest->amount = mfc->excess_sat;
-	if (!feature_negotiated(plugin_feature_set(mfc->cmd->plugin),
-				all_dest->their_features,
-				OPT_LARGE_CHANNELS)
-	 && amount_sat_greater(all_dest->amount,
-			       chainparams->max_funding))
-		all_dest->amount = chainparams->max_funding;
-	/* Remove it from the excess.  */
-	bool ok = amount_sat_sub(&mfc->excess_sat,
-				 mfc->excess_sat, all_dest->amount);
-	assert(ok);
-	/* Remove the 'all' flag.  */
-	all_dest->all = false;
-
-	/* Continue.  */
-	return handle_mfc_change(mfc);
 }
 
 static struct command_result *
@@ -1348,7 +1219,7 @@ after_fundpsbt(struct command *cmd,
 	       struct multifundchannel_command *mfc)
 {
 	const jsmntok_t *field;
-	struct amount_msat msat;
+	struct multifundchannel_destination *all;
 
 	plugin_log(mfc->cmd->plugin, LOG_DBG,
 		   "mfc %"PRIu64": %s done.",
@@ -1379,17 +1250,38 @@ after_fundpsbt(struct command *cmd,
 	if (!field || !json_to_u32(buf, field, &mfc->estimated_final_weight))
 		goto fail;
 
-	/* msat LOL.  */
+	all = all_dest(mfc);
-	field = json_get_member(buf, result, "excess_msat");
+	if (all) {
-	if (!field || !parse_amount_msat(&msat,
+		struct amount_msat msat;
-					 buf + field->start,
-					 field->end - field->start)
-	    || !amount_msat_to_sat(&mfc->excess_sat, msat))
-		goto fail;
 
-	if (has_all(mfc))
+		/* excess_msat is amount to use for "all".  */
-		return compute_mfc_all(mfc);
+		field = json_get_member(buf, result, "excess_msat");
-	return handle_mfc_change(mfc);
+		if (!field || !parse_amount_msat(&msat,
+						 buf + field->start,
+						 field->end - field->start)
+		    || !amount_msat_to_sat(&all->amount, msat))
+			goto fail;
+
+		/* Remove the 'all' flag.  */
+		all->all = false;
+
+		/* It's first grade, Spongebob! */
+		if (!feature_negotiated(plugin_feature_set(mfc->cmd->plugin),
+					all->their_features,
+					OPT_LARGE_CHANNELS)
+		    && amount_sat_greater(all->amount,
+					  chainparams->max_funding)) {
+			/* Oh, crap!  Set this amount and retry! */
+			plugin_log(mfc->cmd->plugin, LOG_INFORM,
+				   "'all' was too large for non-wumbo channel, trimming from %s to %s",
+				   fmt_amount_sat(tmpctx, all->amount),
+				   fmt_amount_sat(tmpctx, chainparams->max_funding));
+			all->amount = chainparams->max_funding;
+			return unreserve_call(mfc->cmd, mfc->psbt,
+					      retry_fundpsbt_capped_all, mfc);
+		}
+	}
+	return mfc_psbt_acquired(mfc);
 
 fail:
 	plugin_err(mfc->cmd->plugin,
@@ -1446,7 +1338,7 @@ perform_fundpsbt(struct multifundchannel_command *mfc, u32 feerate)
 	 */
 	json_add_u32(req->js, "reserve", 2016);
 	/* How much do we need to reserve?  */
-	if (has_all(mfc))
+	if (all_dest(mfc) != NULL)
 		json_add_string(req->js, "satoshi", "all");
 	else {
 		struct amount_sat sum = AMOUNT_SAT(0);
@@ -1511,6 +1403,8 @@ perform_fundpsbt(struct multifundchannel_command *mfc, u32 feerate)
 				tal_fmt(tmpctx, "%u", 110));
 	}
 
+	/* Handle adding a change output if required. */
+	json_add_bool(req->js, "excess_as_change", true);
 
 	return send_outreq(mfc->cmd->plugin, req);
 }
@@ -2092,7 +1986,6 @@ json_multifundchannel(struct command *cmd,
 	mfc->minchannels = minchannels ? *minchannels : tal_count(mfc->destinations);
 	mfc->removeds = tal_arr(mfc, struct multifundchannel_removed, 0);
 	mfc->psbt = NULL;
-	mfc->change_scriptpubkey = NULL;
 	mfc->txid = NULL;
 	mfc->final_tx = NULL;
 	mfc->final_txid = NULL;

plugins/spender/multifundchannel.h

@@ -218,21 +218,6 @@ struct multifundchannel_command {
 	/* The expected weight of the PSBT after adding in all the outputs.
 	 * In weight units (sipa).  */
 	u32 estimated_final_weight;
-	/* Excess satoshi from the PSBT.
-	 * If "all" this is the entire amount; if not "all" this is the
-	 * proposed change amount, which if dusty should be donated to
-	 * the miners.
-	 */
-	struct amount_sat excess_sat;
-
-	/* A convenient change address. NULL at the start, filled in
-	 * if we detect we need it.  */
-	const u8 *change_scriptpubkey;
-	/* Whether we need a change output.  */
-	bool change_needed;
-	/* The change amount.  */
-	struct amount_sat change_amount;
-
 	/* The txid of the final funding transaction.  */
 	struct bitcoin_txid *txid;
 

tests/test_connection.py

@@ -1,5 +1,6 @@
 from fixtures import *  # noqa: F401,F403
 from fixtures import TEST_NETWORK
+from decimal import Decimal
 from ephemeral_port_reserve import reserve  # type: ignore
 from pyln.client import RpcError, Millisatoshi
 import pyln.proto.wire as wire
@@ -1111,6 +1112,7 @@ def test_funding_all_too_much(node_factory):
 
     addr, txid = l1.fundwallet(2**24 + 10000)
     l1.rpc.fundchannel(l2.info['id'], "all")
+    assert l1.daemon.is_in_log("'all' was too large for non-wumbo channel, trimming")
 
     # One reserved, confirmed output spent above, and one change.
     outputs = l1.rpc.listfunds()['outputs']
@@ -1842,7 +1844,14 @@ def test_multifunding_v1_v2_mixed(node_factory, bitcoind):
                     {"id": '{}@localhost:{}'.format(l4.info['id'], l4.port),
                      "amount": 50000}]
 
-    l1.rpc.multifundchannel(destinations)
+    # There should be change!
+    tx = l1.rpc.multifundchannel(destinations)['tx']
+    decoded = bitcoind.rpc.decoderawtransaction(tx)
+    assert len(decoded['vout']) == len(destinations) + 1
+    # Feerate should be about right, too!
+    fee = Decimal(2000000) / 10**8 * len(decoded['vin']) - sum(v['value'] for v in decoded['vout'])
+    assert 7450 < fee * 10**8 / decoded['weight'] * 1000 < 7550
+
     mine_funding_to_announce(bitcoind, [l1, l2, l3, l4], wait_for_mempool=1)
 
     for node in [l1, l2, l3, l4]: