withdraw: refactor change output handling

We're not using the change_outnum for withdraw tx's (and the way
we were calculating it was broken as of the addition of 'multiple
outputs'). This removes the change output knowhow from withdraw_tx
entirely, and pushes the responsibility up to the caller to
include the change output in the output set if desired.

Consequently, we also remove the change output knowhow from hsmd.

bitcoin/tx.c

@@ -32,6 +32,16 @@ int wally_tx_clone(struct wally_tx *tx, struct wally_tx **output)
 	return ret;
 }
 
+struct bitcoin_tx_output *new_tx_output(const tal_t *ctx,
+					struct amount_sat amount,
+					const u8 *script)
+{
+	struct bitcoin_tx_output *output = tal(ctx, struct bitcoin_tx_output);
+	output->amount = amount;
+	output->script = tal_dup_arr(output, u8, script, tal_count(script), 0);
+	return output;
+}
+
 int bitcoin_tx_add_output(struct bitcoin_tx *tx, const u8 *script,
 			  u8 *wscript, struct amount_sat amount)
 {

bitcoin/tx.h

@@ -35,6 +35,10 @@ struct bitcoin_tx_output {
 	u8 *script;
 };
 
+struct bitcoin_tx_output *new_tx_output(const tal_t *ctx,
+					struct amount_sat amount,
+					const u8 *script);
+
 /* SHA256^2 the tx in legacy format. */
 void bitcoin_txid(const struct bitcoin_tx *tx, struct bitcoin_txid *txid);
 void wally_txid(const struct wally_tx *wtx, struct bitcoin_txid *txid);

common/withdraw_tx.c

@@ -3,6 +3,7 @@
 #include <bitcoin/pubkey.h>
 #include <bitcoin/script.h>
 #include <ccan/ptrint/ptrint.h>
+#include <common/key_derive.h>
 #include <common/permute_tx.h>
 #include <common/utils.h>
 #include <common/utxo.h>
@@ -13,44 +14,20 @@ struct bitcoin_tx *withdraw_tx(const tal_t *ctx,
 			       const struct chainparams *chainparams,
 			       const struct utxo **utxos,
 			       struct bitcoin_tx_output **outputs,
-			       const struct pubkey *changekey,
-			       struct amount_sat change,
 			       const struct ext_key *bip32_base,
-			       int *change_outnum, u32 nlocktime)
+			       u32 nlocktime)
 {
 	struct bitcoin_tx *tx;
 	int output_count;
 
 	tx = tx_spending_utxos(ctx, chainparams, utxos, bip32_base,
-			       !amount_sat_eq(change, AMOUNT_SAT(0)),
-			       tal_count(outputs), nlocktime,
+			       false, tal_count(outputs), nlocktime,
 			       BITCOIN_TX_DEFAULT_SEQUENCE - 1);
 
 	output_count = bitcoin_tx_add_multi_outputs(tx, outputs);
 	assert(output_count == tal_count(outputs));
 
-	if (!amount_sat_eq(change, AMOUNT_SAT(0))) {
-		/* Add one to the output_count, for the change */
-		output_count++;
-
-		const void *map[output_count];
-		for (size_t i = 0; i < output_count; i++)
-			map[i] = int2ptr(i);
-
-		bitcoin_tx_add_output(tx, scriptpubkey_p2wpkh(tmpctx, changekey),
-				      NULL, change);
-
-		assert(tx->wtx->num_outputs == output_count);
-		permute_outputs(tx, NULL, map);
-
-		/* The change is the last output added, so the last position
-		 * in the map */
-		if (change_outnum)
-			*change_outnum = ptr2int(map[output_count - 1]);
-
-	} else if (change_outnum)
-		*change_outnum = -1;
-
+	permute_outputs(tx, NULL, (const void **)outputs);
 	permute_inputs(tx, (const void **)utxos);
 
 	bitcoin_tx_finalize(tx);

common/withdraw_tx.h

@@ -21,19 +21,14 @@ struct utxo;
  * @chainparams: (in) the params for the created transaction.
  * @utxos: (in/out) tal_arr of UTXO pointers to spend (permuted to match)
  * @outputs: (in) tal_arr of bitcoin_tx_output, scriptPubKeys with amount to send to.
- * @changekey: (in) key to send change to (only used if change_satoshis != 0).
- * @change: (in) amount to send as change.
  * @bip32_base: (in) bip32 base for key derivation, or NULL.
- * @change_outnum: (out) set to output index of change output or -1 if none, unless NULL.
  * @nlocktime: (in) the value to set as the transaction's nLockTime.
  */
 struct bitcoin_tx *withdraw_tx(const tal_t *ctx,
 			       const struct chainparams *chainparams,
 			       const struct utxo **utxos,
 			       struct bitcoin_tx_output **outputs,
-			       const struct pubkey *changekey,
-			       struct amount_sat change,
 			       const struct ext_key *bip32_base,
-			       int *change_outnum, u32 nlocktime);
+			       u32 nlocktime);
 
 #endif /* LIGHTNING_COMMON_WITHDRAW_TX_H */

hsmd/hsm_wire.csv

@@ -55,9 +55,6 @@ msgdata,hsm_node_announcement_sig_reply,signature,secp256k1_ecdsa_signature,
 
 # Sign a withdrawal request
 msgtype,hsm_sign_withdrawal,7
-msgdata,hsm_sign_withdrawal,satoshi_out,amount_sat,
-msgdata,hsm_sign_withdrawal,change_out,amount_sat,
-msgdata,hsm_sign_withdrawal,change_keyindex,u32,
 msgdata,hsm_sign_withdrawal,num_outputs,u16,
 msgdata,hsm_sign_withdrawal,outputs,bitcoin_tx_output,num_outputs
 msgdata,hsm_sign_withdrawal,num_inputs,u16,

hsmd/hsmd.c

@@ -1576,26 +1576,18 @@ static struct io_plan *handle_sign_withdrawal_tx(struct io_conn *conn,
 						 struct client *c,
 						 const u8 *msg_in)
 {
-	struct amount_sat satoshi_out, change_out;
-	u32 change_keyindex;
 	struct utxo **utxos;
 	struct bitcoin_tx *tx;
-	struct pubkey changekey;
 	struct bitcoin_tx_output **outputs;
 	u32 nlocktime;
 
-	if (!fromwire_hsm_sign_withdrawal(tmpctx, msg_in, &satoshi_out,
-					  &change_out, &change_keyindex,
+	if (!fromwire_hsm_sign_withdrawal(tmpctx, msg_in,
 					  &outputs, &utxos, &nlocktime))
 		return bad_req(conn, c, msg_in);
 
-	if (!bip32_pubkey(&secretstuff.bip32, &changekey, change_keyindex))
-		return bad_req_fmt(conn, c, msg_in,
-				   "Failed to get key %u", change_keyindex);
-
 	tx = withdraw_tx(tmpctx, c->chainparams,
-			 cast_const2(const struct utxo **, utxos), outputs,
-			 &changekey, change_out, NULL, NULL, nlocktime);
+			 cast_const2(const struct utxo **, utxos),
+			 outputs, NULL, nlocktime);
 
 	sign_all_inputs(tx, utxos);
 

wallet/wallet.h

@@ -67,8 +67,6 @@ struct unreleased_tx {
 	/* The tx itself (unsigned initially) */
 	struct bitcoin_tx *tx;
 	struct bitcoin_txid txid;
-	/* Index of change output, or -1 if none. */
-	int change_outnum;
 };
 
 /* Possible states for tracked outputs in the database. Not sure yet

wallet/walletrpc.c

@@ -86,9 +86,6 @@ static struct command_result *broadcast_and_wait(struct command *cmd,
 	/* FIXME: hsm will sign almost anything, but it should really
 	 * fail cleanly (not abort!) and let us report the error here. */
 	u8 *msg = towire_hsm_sign_withdrawal(cmd,
-					     utx->wtx->amount,
-					     utx->wtx->change,
-					     utx->wtx->change_key_index,
 					     cast_const2(const struct bitcoin_tx_output **,
 							 utx->outputs),
 					     utx->wtx->utxos,
@@ -312,10 +309,8 @@ static struct command_result *json_prepare_tx(struct command *cmd,
 	 * Support only one output. */
 	if (destination) {
 		outputs = tal_arr(tmpctx, struct bitcoin_tx_output *, 1);
-		outputs[0] = tal(outputs, struct bitcoin_tx_output);
-		outputs[0]->script = tal_steal(outputs[0],
-					       cast_const(u8 *, destination));
-		outputs[0]->amount = (*utx)->wtx->amount;
+		outputs[0] = new_tx_output(outputs, (*utx)->wtx->amount,
+					   destination);
 		out_len = tal_count(outputs[0]->script);
 
 		goto create_tx;
@@ -357,11 +352,9 @@ static struct command_result *json_prepare_tx(struct command *cmd,
 					    "'%.*s' is a invalid satoshi amount",
 					    t[2].end - t[2].start, buffer + t[2].start);
 
+		outputs[i] = new_tx_output(outputs, *amount,
+					   cast_const(u8 *, destination));
 		out_len += tal_count(destination);
-		outputs[i] = tal(outputs, struct bitcoin_tx_output);
-		outputs[i]->amount = *amount;
-		outputs[i]->script = tal_steal(outputs[i],
-					       cast_const(u8 *, destination));
 
 		/* In fact, the maximum amount of bitcoin satoshi is 2.1e15.
 		 * It can't be equal to/bigger than 2^64.
@@ -387,8 +380,6 @@ static struct command_result *json_prepare_tx(struct command *cmd,
 	}
 
 create_tx:
-	(*utx)->outputs = tal_steal(*utx, outputs);
-
 	if (chosen_utxos)
 		result = wtx_from_utxos((*utx)->wtx, *feerate_per_kw,
 					out_len, maxheight,
@@ -405,19 +396,27 @@ create_tx:
 	if ((*utx)->wtx->all_funds)
 		outputs[0]->amount = (*utx)->wtx->amount;
 
+	/* Add the change as the last output */
 	if (!amount_sat_eq((*utx)->wtx->change, AMOUNT_SAT(0))) {
+		struct bitcoin_tx_output *change_output;
+
 		changekey = tal(tmpctx, struct pubkey);
 		if (!bip32_pubkey(cmd->ld->wallet->bip32_base, changekey,
 				  (*utx)->wtx->change_key_index))
 			return command_fail(cmd, LIGHTNINGD, "Keys generation failure");
-	} else
-		changekey = NULL;
+
+		change_output = new_tx_output(outputs, (*utx)->wtx->change,
+					      scriptpubkey_p2wpkh(tmpctx, changekey));
+		tal_arr_expand(&outputs, change_output);
+	}
+
+	(*utx)->outputs = tal_steal(*utx, outputs);
 	(*utx)->tx = withdraw_tx(*utx, chainparams,
-				 (*utx)->wtx->utxos, (*utx)->outputs,
-				 changekey, (*utx)->wtx->change,
+				 (*utx)->wtx->utxos,
+				 (*utx)->outputs,
 				 cmd->ld->wallet->bip32_base,
-				 &(*utx)->change_outnum,
 				 locktime);
+
 	bitcoin_txid((*utx)->tx, &(*utx)->txid);
 
 	return NULL;