From 3b8c0a7397cc0694117dfe63ecb2bff0b6957a4b Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 28 Aug 2020 12:10:57 +0930 Subject: [PATCH] sendpsbt: just reserve (maybe bump) inputs on send, don't mark spent. Marking spent means if the transaction doesn't confirm for some reason, the user will need to force a rescan to find the funds. Now we have timed reservations, reserving for (an additional) 12 hours should be sufficient. We also take this opportunity (now we have our own callback path) to record the tx in the wallet only on success. Signed-off-by: Rusty Russell --- tests/test_closing.py | 2 +- tests/test_connection.py | 24 +++++++++++- wallet/wallet.c | 8 ++-- wallet/walletrpc.c | 80 +++++++++++++++++++++++++++++----------- 4 files changed, 87 insertions(+), 27 deletions(-) diff --git a/tests/test_closing.py b/tests/test_closing.py index 96d0f4075..3ae387b36 100644 --- a/tests/test_closing.py +++ b/tests/test_closing.py @@ -1543,7 +1543,7 @@ Make sure we show the address. channel_id = first_channel_id(l1, l2) # listfunds will show 1 output change, and channels. - assert len(l1.rpc.listfunds()['outputs']) == 1 + assert len([o for o in l1.rpc.listfunds()['outputs'] if not o['reserved']]) == 1 l1.stop() l2.rpc.close(l1.info['id'], unilateraltimeout=1) diff --git a/tests/test_connection.py b/tests/test_connection.py index 1bdd72ac6..a0551d2c4 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -703,6 +703,8 @@ def test_funding_change(node_factory, bitcoind): assert only_one(outputs)['value'] == 10000000 l1.rpc.fundchannel(l2.info['id'], 1000000) + bitcoind.generate_block(1, wait_for_mempool=1) + sync_blockheight(bitcoind, [l1]) outputs = {r['status']: r['value'] for r in l1.db_query( 'SELECT status, SUM(value) AS value FROM outputs GROUP BY status;')} @@ -734,10 +736,21 @@ def test_funding_all_too_much(node_factory): """ l1, l2 = node_factory.line_graph(2, fundchannel=False) - l1.fundwallet(2**24 + 10000) + addr, txid = l1.fundwallet(2**24 + 10000) l1.rpc.fundchannel(l2.info['id'], "all") - assert only_one(l1.rpc.listfunds()['outputs'])['status'] == 'unconfirmed' + # One reserved, confirmed output spent above, and one change. + outputs = l1.rpc.listfunds()['outputs'] + + spent = only_one([o for o in outputs if o['status'] == 'confirmed']) + + assert spent['txid'] == txid + assert spent['address'] == addr + assert spent['reserved'] is True + + pending = only_one([o for o in outputs if o['status'] != 'confirmed']) + assert pending['status'] == 'unconfirmed' + assert pending['reserved'] is False assert only_one(l1.rpc.listfunds()['channels'])['channel_total_sat'] == 2**24 - 1 @@ -877,6 +890,13 @@ def test_funding_by_utxos(node_factory, bitcoind): l1.rpc.connect(l3.info["id"], "localhost", l3.port) l1.rpc.fundchannel(l3.info["id"], int(0.007 * 10**8), utxos=[utxos[2]]) + # Fund another channel with already reserved utxos + with pytest.raises(RpcError, match=r"UTXO.*already reserved"): + l1.rpc.fundchannel(l3.info["id"], int(0.01 * 10**8), utxos=utxos) + + bitcoind.generate_block(1, wait_for_mempool=1) + sync_blockheight(bitcoind, [l1]) + # Fund another channel with already spent utxos with pytest.raises(RpcError, match=r"Already spent UTXO "): l1.rpc.fundchannel(l3.info["id"], int(0.01 * 10**8), utxos=utxos) diff --git a/wallet/wallet.c b/wallet/wallet.c index 15594ad82..dec065ff3 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -3208,13 +3208,15 @@ wallet_outpoint_spend(struct wallet *w, const tal_t *ctx, const u32 blockheight, int changes; if (outpointfilter_matches(w->owned_outpoints, txid, outnum)) { stmt = db_prepare_v2(w->db, SQL("UPDATE outputs " - "SET spend_height = ? " + "SET spend_height = ?, " + " status = ? " "WHERE prev_out_tx = ?" " AND prev_out_index = ?")); db_bind_int(stmt, 0, blockheight); - db_bind_sha256d(stmt, 1, &txid->shad); - db_bind_int(stmt, 2, outnum); + db_bind_int(stmt, 1, output_status_in_db(output_state_spent)); + db_bind_sha256d(stmt, 2, &txid->shad); + db_bind_int(stmt, 3, outnum); db_exec_prepared_v2(take(stmt)); diff --git a/wallet/walletrpc.c b/wallet/walletrpc.c index c4c611252..2b2db663c 100644 --- a/wallet/walletrpc.c +++ b/wallet/walletrpc.c @@ -1043,25 +1043,69 @@ static const struct json_command signpsbt_command = { AUTODATA(json_command, &signpsbt_command); +struct sending_psbt { + struct command *cmd; + struct utxo **utxos; + struct wally_tx *wtx; +}; + +static void sendpsbt_done(struct bitcoind *bitcoind UNUSED, + bool success, const char *msg, + struct sending_psbt *sending) +{ + struct lightningd *ld = sending->cmd->ld; + struct json_stream *response; + struct bitcoin_txid txid; + struct amount_sat change; + + if (!success) { + /* Unreserve the inputs again. */ + for (size_t i = 0; i < tal_count(sending->utxos); i++) { + wallet_unreserve_utxo(ld->wallet, + sending->utxos[i], + get_block_height(ld->topology)); + } + + was_pending(command_fail(sending->cmd, LIGHTNINGD, + "Error broadcasting transaction: %s." + " Unsent tx discarded %s", + msg, + type_to_string(tmpctx, struct wally_tx, + sending->wtx))); + return; + } + + wallet_transaction_add(ld->wallet, sending->wtx, 0, 0); + + /* Extract the change output and add it to the DB */ + wallet_extract_owned_outputs(ld->wallet, sending->wtx, NULL, &change); + + response = json_stream_success(sending->cmd); + wally_txid(sending->wtx, &txid); + json_add_hex_talarr(response, "tx", linearize_wtx(tmpctx, sending->wtx)); + json_add_txid(response, "txid", &txid); + was_pending(command_success(sending->cmd, response)); +} + static struct command_result *json_sendpsbt(struct command *cmd, const char *buffer, const jsmntok_t *obj UNNEEDED, const jsmntok_t *params) { struct command_result *res; + struct sending_psbt *sending; struct wally_psbt *psbt; - struct wally_tx *w_tx; - struct tx_broadcast *txb; - struct utxo **utxos; - struct bitcoin_txid txid; + struct lightningd *ld = cmd->ld; if (!param(cmd, buffer, params, p_req("psbt", param_psbt, &psbt), NULL)) return command_param_failed(); - w_tx = psbt_finalize(psbt, true); - if (!w_tx) + sending = tal(cmd, struct sending_psbt); + sending->cmd = cmd; + sending->wtx = tal_steal(sending, psbt_finalize(psbt, true)); + if (!sending->wtx) return command_fail(cmd, LIGHTNINGD, "PSBT not finalizeable %s", type_to_string(tmpctx, struct wally_psbt, @@ -1070,27 +1114,21 @@ static struct command_result *json_sendpsbt(struct command *cmd, /* We have to find/locate the utxos that are ours on this PSBT, * so that we know who to mark as used. */ - res = match_psbt_inputs_to_utxos(cmd, psbt, NULL, &utxos); + res = match_psbt_inputs_to_utxos(cmd, psbt, NULL, &sending->utxos); if (res) return res; - txb = tal(cmd, struct tx_broadcast); - txb->utxos = cast_const2(const struct utxo **, - tal_steal(txb, utxos)); - txb->wtx = tal_steal(txb, w_tx); - txb->cmd = cmd; - txb->expected_change = NULL; - - /* FIXME: Do this *after* successful broadcast! */ - wallet_transaction_add(cmd->ld->wallet, txb->wtx, 0, 0); - wally_txid(txb->wtx, &txid); - wallet_transaction_annotate(cmd->ld->wallet, &txid, - TX_UNKNOWN, 0); + for (size_t i = 0; i < tal_count(sending->utxos); i++) { + if (!wallet_reserve_utxo(ld->wallet, sending->utxos[i], + get_block_height(ld->topology))) + fatal("UTXO not reservable?"); + } /* Now broadcast the transaction */ bitcoind_sendrawtx(cmd->ld->topology->bitcoind, - tal_hex(tmpctx, linearize_wtx(tmpctx, w_tx)), - wallet_withdrawal_broadcast, txb); + tal_hex(tmpctx, + linearize_wtx(tmpctx, sending->wtx)), + sendpsbt_done, sending); return command_still_pending(cmd); }