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.

Author: niftynei

bitcoin/tx.c

@@ -14,6 +14,16 @@
 
 #define SEGREGATED_WITNESS_FLAG 0x1
 
+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,
 			  struct amount_sat amount)
 {

bitcoin/tx.h

@@ -43,6 +43,9 @@ struct bitcoin_tx_input {
 	u8 **witness;
 };
 
+struct bitcoin_tx_output *new_tx_output(const tal_t *ctx,
+					struct amount_sat amount,
+					const u8 *script);
 
 /* SHA256^2 the tx: simpler than sha256_tx */
 void bitcoin_txid(const struct bitcoin_tx *tx, 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,30 +14,36 @@ 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)
+			       const struct ext_key *bip32_base)
 {
 	struct bitcoin_tx *tx;
+	struct pubkey key;
+	u8 *script;
+	size_t i;
 
-	tx = tx_spending_utxos(ctx, chainparams, utxos, bip32_base,
-			       !amount_sat_eq(change, AMOUNT_SAT(0)),
-			       tal_count(outputs));
+	assert(tal_count(utxos) > 0 && tal_count(outputs) > 0);
+
+	tx = bitcoin_tx(ctx, chainparams,
+			tal_count(utxos),
+			tal_count(outputs));
+
+	/* Add our utxo's */
+	for (i = 0; i < tal_count(utxos); i++) {
+		if (utxos[i]->is_p2sh && bip32_base) {
+			bip32_pubkey(bip32_base, &key, utxos[i]->keyindex);
+			script = bitcoin_scriptsig_p2sh_p2wpkh(tmpctx, &key);
+		} else {
+			script = NULL;
+		}
+
+		bitcoin_tx_add_input(tx, &utxos[i]->txid, utxos[i]->outnum,
+				     BITCOIN_TX_DEFAULT_SEQUENCE,
+		 		     utxos[i]->amount, script);
+	}
 
 	bitcoin_tx_add_multi_outputs(tx, outputs);
+	permute_outputs(tx, NULL, (const void **)outputs);
 
-	if (!amount_sat_eq(change, AMOUNT_SAT(0))) {
-		const void *map[2];
-		map[0] = int2ptr(0);
-		map[1] = int2ptr(1);
-		bitcoin_tx_add_output(tx, scriptpubkey_p2wpkh(tmpctx, changekey),
-				      change);
-		permute_outputs(tx, NULL, map);
-		if (change_outnum)
-			*change_outnum = ptr2int(map[1]);
-	} else if (change_outnum)
-		*change_outnum = -1;
 	permute_inputs(tx, (const void **)utxos);
 	elements_tx_add_fee_output(tx);
 	assert(bitcoin_tx_check(tx));

common/withdraw_tx.h

@@ -21,18 +21,12 @@ 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.
  */
 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);
+			       const struct ext_key *bip32_base);
 
 #endif /* LIGHTNING_COMMON_WITHDRAW_TX_H */

hsmd/hsm_wire.csv

@@ -67,9 +67,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

@@ -1640,25 +1640,17 @@ 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;
 
-	if (!fromwire_hsm_sign_withdrawal(tmpctx, msg_in, &satoshi_out,
-					  &change_out, &change_keyindex,
+	if (!fromwire_hsm_sign_withdrawal(tmpctx, msg_in,
 					  &outputs, &utxos))
 		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);
+			 cast_const2(const struct utxo **, utxos),
+			 outputs, NULL);
 
 	sign_all_inputs(tx, utxos);
 

wallet/wallet.h

@@ -63,8 +63,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

@@ -84,9 +84,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);
@@ -293,10 +290,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;
@@ -338,11 +333,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.
@@ -368,8 +361,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,
@@ -386,18 +377,26 @@ static struct command_result *json_prepare_tx(struct command *cmd,
 	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, get_chainparams(cmd->ld),
-				 (*utx)->wtx->utxos, (*utx)->outputs,
-				 changekey, (*utx)->wtx->change,
-				 cmd->ld->wallet->bip32_base,
-				 &(*utx)->change_outnum);
+				 (*utx)->wtx->utxos,
+				 (*utx)->outputs,
+				 cmd->ld->wallet->bip32_base);
+
 	bitcoin_txid((*utx)->tx, &(*utx)->txid);
 
 	return NULL;