From 16656a85cfc2f3938eafaba4ebb9d14e451a6dd6 Mon Sep 17 00:00:00 2001 From: lisa neigut Date: Mon, 16 Sep 2019 19:08:05 -0500 Subject: [PATCH] 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 | 10 ++++++++++ bitcoin/tx.h | 4 ++++ common/withdraw_tx.c | 31 ++++--------------------------- common/withdraw_tx.h | 7 +------ hsmd/hsm_wire.csv | 3 --- hsmd/hsmd.c | 14 +++----------- wallet/wallet.h | 2 -- wallet/walletrpc.c | 35 +++++++++++++++++------------------ 8 files changed, 39 insertions(+), 67 deletions(-) diff --git a/bitcoin/tx.c b/bitcoin/tx.c index a9f67f6fe..d7e839417 100644 --- a/bitcoin/tx.c +++ b/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) { diff --git a/bitcoin/tx.h b/bitcoin/tx.h index 31f81b020..4c7209081 100644 --- a/bitcoin/tx.h +++ b/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); diff --git a/common/withdraw_tx.c b/common/withdraw_tx.c index 46fdddcee..b52aac517 100644 --- a/common/withdraw_tx.c +++ b/common/withdraw_tx.c @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -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); diff --git a/common/withdraw_tx.h b/common/withdraw_tx.h index 163f1b475..cf5d67af1 100644 --- a/common/withdraw_tx.h +++ b/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 */ diff --git a/hsmd/hsm_wire.csv b/hsmd/hsm_wire.csv index 32b78ac3d..5f2a6056b 100644 --- a/hsmd/hsm_wire.csv +++ b/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, diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 264a6dd6a..856a619f4 100644 --- a/hsmd/hsmd.c +++ b/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); diff --git a/wallet/wallet.h b/wallet/wallet.h index 88dda612c..0f0d132e0 100644 --- a/wallet/wallet.h +++ b/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 diff --git a/wallet/walletrpc.c b/wallet/walletrpc.c index ca3650b77..068c3d44b 100644 --- a/wallet/walletrpc.c +++ b/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;