From 0bd0de54faac5501a74cd0a4d0d8dafaf7f44b0d Mon Sep 17 00:00:00 2001 From: niftynei Date: Sat, 6 Jun 2020 15:05:14 -0500 Subject: [PATCH] psbt: have withdraw_tx use psbt's to create signed txs this will allow us to add inputs that aren't ours to a tx that we sign and finalize --- hsmd/hsm_wire.csv | 7 ++-- hsmd/hsmd.c | 102 +++++++++++++++++---------------------------- wallet/walletrpc.c | 37 ++++++++++------ 3 files changed, 66 insertions(+), 80 deletions(-) diff --git a/hsmd/hsm_wire.csv b/hsmd/hsm_wire.csv index 5f2a6056b..194d9a926 100644 --- a/hsmd/hsm_wire.csv +++ b/hsmd/hsm_wire.csv @@ -54,15 +54,14 @@ msgtype,hsm_node_announcement_sig_reply,106 msgdata,hsm_node_announcement_sig_reply,signature,secp256k1_ecdsa_signature, # Sign a withdrawal request +#include msgtype,hsm_sign_withdrawal,7 -msgdata,hsm_sign_withdrawal,num_outputs,u16, -msgdata,hsm_sign_withdrawal,outputs,bitcoin_tx_output,num_outputs msgdata,hsm_sign_withdrawal,num_inputs,u16, msgdata,hsm_sign_withdrawal,inputs,utxo,num_inputs -msgdata,hsm_sign_withdrawal,nlocktime,u32, +msgdata,hsm_sign_withdrawal,psbt,wally_psbt, msgtype,hsm_sign_withdrawal_reply,107 -msgdata,hsm_sign_withdrawal_reply,tx,bitcoin_tx, +msgdata,hsm_sign_withdrawal_reply,psbt,wally_psbt, # Sign an invoice msgtype,hsm_sign_invoice,8 diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 856a619f4..b4e04705a 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -1512,87 +1512,63 @@ static void hsm_key_for_utxo(struct privkey *privkey, struct pubkey *pubkey, } } -static void sign_input(struct bitcoin_tx *tx, struct utxo *in, - struct pubkey *inkey, - struct bitcoin_signature *sig, - int index) +/* Find our inputs by the pubkey associated with the inputs, and + * add a partial sig for each */ +static void sign_our_inputs(struct utxo **utxos, struct wally_psbt *psbt) { - struct privkey inprivkey; - u8 *subscript, *wscript, *script; - - /* Figure out keys to spend this. */ - hsm_key_for_utxo(&inprivkey, inkey, in); - - /* It's either a p2wpkh or p2sh (we support that so people from - * the last bitcoin era can put funds into the wallet) */ - wscript = p2wpkh_scriptcode(tmpctx, inkey); - if (in->is_p2sh) { - /* For P2SH-wrapped Segwit, the (implied) redeemScript - * is defined in BIP141 */ - subscript = bitcoin_redeem_p2sh_p2wpkh(tmpctx, inkey); - script = bitcoin_scriptsig_p2sh_p2wpkh(tx, inkey); - bitcoin_tx_input_set_script(tx, index, script); - } else { - /* Pure segwit uses an empty inputScript; NULL has - * tal_count() == 0, so it works great here. */ - subscript = NULL; - bitcoin_tx_input_set_script(tx, index, NULL); - } - /* This is the core crypto magic. */ - sign_tx_input(tx, index, subscript, wscript, &inprivkey, inkey, - SIGHASH_ALL, sig); - - /* The witness is [sig] [key] */ - bitcoin_tx_input_set_witness( - tx, index, take(bitcoin_witness_p2wpkh(tx, sig, inkey))); -} - -/* This completes the tx by filling in the input scripts with signatures. */ -static void sign_all_inputs(struct bitcoin_tx *tx, struct utxo **utxos) -{ - /*~ Deep in my mind there's a continuous battle: should arrays be - * named as singular or plural? Is consistency the sign of a weak - * mind? - * - * ZmnSCPxj answers thusly: One must make peace with the fact, that - * the array itself is singular, yet its contents are plural. Do you - * name the array, or do you name its contents? Is the array itself - * the thing and the whole of the thing, or is it its contents that - * define what it is? - * - *... I'm not sure that helps! */ - assert(tx->wtx->num_inputs == tal_count(utxos)); for (size_t i = 0; i < tal_count(utxos); i++) { - struct pubkey inkey; - struct bitcoin_signature sig; + struct utxo *utxo = utxos[i]; + for (size_t j = 0; j < psbt->num_inputs; j++) { + struct privkey privkey; + struct pubkey pubkey; - sign_input(tx, utxos[i], &inkey, &sig, i); + if (!wally_tx_input_spends(&psbt->tx->inputs[j], + &utxo->txid, utxo->outnum)) + continue; + + hsm_key_for_utxo(&privkey, &pubkey, utxo); + + /* This line is basically the entire reason we have + * to iterate through to match the psbt input + * to the UTXO -- otherwise we would just + * call wally_sign_psbt for every utxo privkey + * and be done with it. We can't do that though + * because any UTXO that's derived from channel_info + * requires the HSM to find the pubkey, and we + * skip doing that until now as a bit of a reduction + * of complexity in the calling code */ + psbt_input_add_pubkey(psbt, j, &pubkey); + + if (wally_sign_psbt(psbt, privkey.secret.data, + sizeof(privkey.secret.data)) != WALLY_OK) + status_broken("Received wally_err attempting to " + "sign utxo with key %s. PSBT: %s", + type_to_string(tmpctx, struct pubkey, + &pubkey), + type_to_string(tmpctx, struct wally_psbt, + psbt)); + + } } } -/*~ lightningd asks us to sign a withdrawal or funding as above but in theory +/*~ lightningd asks us to sign a withdrawal; same as above but in theory * we can do more to check the previous case is valid. */ static struct io_plan *handle_sign_withdrawal_tx(struct io_conn *conn, struct client *c, const u8 *msg_in) { struct utxo **utxos; - struct bitcoin_tx *tx; - struct bitcoin_tx_output **outputs; - u32 nlocktime; + struct wally_psbt *psbt; if (!fromwire_hsm_sign_withdrawal(tmpctx, msg_in, - &outputs, &utxos, &nlocktime)) + &utxos, &psbt)) return bad_req(conn, c, msg_in); - tx = withdraw_tx(tmpctx, c->chainparams, - cast_const2(const struct utxo **, utxos), - outputs, NULL, nlocktime); - - sign_all_inputs(tx, utxos); + sign_our_inputs(utxos, psbt); return req_reply(conn, c, - take(towire_hsm_sign_withdrawal_reply(NULL, tx))); + take(towire_hsm_sign_withdrawal_reply(NULL, psbt))); } /*~ Lightning invoices, defined by BOLT 11, are signed. This has been diff --git a/wallet/walletrpc.c b/wallet/walletrpc.c index 9d7e4dcbb..0df56f861 100644 --- a/wallet/walletrpc.c +++ b/wallet/walletrpc.c @@ -80,16 +80,13 @@ static void wallet_withdrawal_broadcast(struct bitcoind *bitcoind UNUSED, static struct command_result *broadcast_and_wait(struct command *cmd, struct unreleased_tx *utx) { - struct bitcoin_tx *signed_tx; + struct wally_psbt *signed_psbt; + struct wally_tx *signed_wtx; struct bitcoin_txid signed_txid; /* 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, - cast_const2(const struct bitcoin_tx_output **, - utx->outputs), - utx->wtx->utxos, - utx->tx->wtx->locktime); + u8 *msg = towire_hsm_sign_withdrawal(cmd, utx->wtx->utxos, utx->tx->psbt); if (!wire_sync_write(cmd->ld->hsm_fd, take(msg))) fatal("Could not write sign_withdrawal to HSM: %s", @@ -97,25 +94,39 @@ static struct command_result *broadcast_and_wait(struct command *cmd, msg = wire_sync_read(cmd, cmd->ld->hsm_fd); - if (!fromwire_hsm_sign_withdrawal_reply(utx, msg, &signed_tx)) + if (!fromwire_hsm_sign_withdrawal_reply(utx, msg, &signed_psbt)) fatal("HSM gave bad sign_withdrawal_reply %s", tal_hex(tmpctx, msg)); - signed_tx->chainparams = utx->tx->chainparams; + + signed_wtx = psbt_finalize(signed_psbt, true); + + if (!signed_wtx) { + /* Have the utx persist past this command */ + tal_steal(cmd->ld->wallet, utx); + add_unreleased_tx(cmd->ld->wallet, utx); + return command_fail(cmd, LIGHTNINGD, + "PSBT is not finalized %s", + type_to_string(tmpctx, + struct wally_psbt, + signed_psbt)); + } /* Sanity check */ - bitcoin_txid(signed_tx, &signed_txid); + wally_txid(signed_wtx, &signed_txid); if (!bitcoin_txid_eq(&signed_txid, &utx->txid)) fatal("HSM changed txid: unsigned %s, signed %s", tal_hex(tmpctx, linearize_tx(tmpctx, utx->tx)), - tal_hex(tmpctx, linearize_tx(tmpctx, signed_tx))); + tal_hex(tmpctx, linearize_wtx(tmpctx, signed_wtx))); /* Replace unsigned tx by signed tx. */ - tal_free(utx->tx); - utx->tx = signed_tx; + wally_tx_free(utx->tx->wtx); + utx->tx->wtx = tal_steal(utx->tx, signed_wtx); + tal_free(utx->tx->psbt); + utx->tx->psbt = tal_steal(utx->tx, signed_psbt); /* Now broadcast the transaction */ bitcoind_sendrawtx(cmd->ld->topology->bitcoind, - tal_hex(tmpctx, linearize_tx(tmpctx, signed_tx)), + tal_hex(tmpctx, linearize_tx(tmpctx, utx->tx)), wallet_withdrawal_broadcast, utx); return command_still_pending(cmd);