From 71a4a2e31cd5fb748c7e717026c67a56a4290c1a Mon Sep 17 00:00:00 2001 From: niftynei Date: Tue, 11 May 2021 11:58:00 -0500 Subject: [PATCH] df: rework closing logic Trying to put all the disconnect logic into the same path was a dumb idea. If you asked to reconnect but passed in an 'unsaved' channel, we would not call the 'reconnect' code. Instead, we make a differentiation between "unsaved" channels (ones that we haven't received commitment tx for) and handle the disconnect for these separate from where we want to do a reconnect. --- lightningd/connect_control.c | 8 +- lightningd/dual_open_control.c | 118 ++++++++------------ lightningd/dual_open_control.h | 7 +- lightningd/peer_control.c | 5 +- lightningd/test/run-invoice-select-inchan.c | 7 +- tests/test_connection.py | 5 +- wallet/db_postgres_sqlgen.c | 2 +- wallet/db_sqlite3_sqlgen.c | 2 +- wallet/statements_gettextgen.po | 6 +- wallet/test/run-wallet.c | 7 +- 10 files changed, 66 insertions(+), 101 deletions(-) diff --git a/lightningd/connect_control.c b/lightningd/connect_control.c index a3698cc6a..adbb43408 100644 --- a/lightningd/connect_control.c +++ b/lightningd/connect_control.c @@ -297,11 +297,9 @@ static void peer_please_disconnect(struct lightningd *ld, const u8 *msg) channel_cleanup_commands(c, "Reconnected"); channel_fail_reconnect(c, "Reconnected"); } - else { - /* v2 has unsaved channels, not uncommitted_chans */ - c = unsaved_channel_by_id(ld, &id); - if (c) - channel_close_conn(c, "Reconnected"); + else if ((c = unsaved_channel_by_id(ld, &id))) { + log_info(c->log, "Killing opening daemon: Reconnected"); + channel_unsaved_close_conn(c, "Reconnected"); } } diff --git a/lightningd/dual_open_control.c b/lightningd/dual_open_control.c index c6e03d8f2..e36d65f43 100644 --- a/lightningd/dual_open_control.c +++ b/lightningd/dual_open_control.c @@ -52,63 +52,46 @@ static void channel_disconnect(struct channel *channel, notify_disconnect(channel->peer->ld, &channel->peer->id); - if (channel_unsaved(channel)) { - log_debug(channel->log, "%s", - "Unsaved peer failed." - " Disconnecting and deleting channel."); - delete_channel(channel); - return; - } - - if (reconnect) + if (!reconnect) + channel_set_owner(channel, NULL); + else channel_fail_reconnect(channel, "%s: %s", channel->owner ? channel->owner->name : "dualopend-dead", desc); - else - channel_set_owner(channel, NULL); } -void channel_close_conn(struct channel *channel, const char *why) +void channel_unsaved_close_conn(struct channel *channel, const char *why) { - /* Close dualopend */ - if (channel->owner) { - log_info(channel->log, "Killing dualopend: %s", why); + /* Gotta be unsaved */ + assert(channel_unsaved(channel)); + log_info(channel->log, "Unsaved peer failed." + " Disconnecting and deleting channel. Reason: %s", + why); - subd_release_channel(channel->owner, channel); - channel->owner = NULL; - } + notify_disconnect(channel->peer->ld, &channel->peer->id); + channel_cleanup_commands(channel, why); - channel_disconnect(channel, LOG_INFORM, false, why); + channel_set_owner(channel, NULL); + delete_channel(channel); } -void channel_close_reconn(struct channel *channel, const char *why) -{ - /* Close the daemon */ - if (channel->owner) { - log_info(channel->log, "Killing %s: %s", - channel->owner->name, why); - - subd_release_channel(channel->owner, channel); - channel->owner = NULL; - } - - channel_disconnect(channel, LOG_INFORM, true, why); -} - -static void channel_err_broken_reconn(struct channel *channel, - const char *fmt, ...) +static void channel_saved_err_broken_reconn(struct channel *channel, + const char *fmt, ...) { va_list ap; const char *errmsg; + /* We only reconnect to 'saved' channel peers */ + assert(!channel_unsaved(channel)); + va_start(ap, fmt); errmsg = tal_vfmt(tmpctx, fmt, ap); va_end(ap); log_broken(channel->log, "%s", errmsg); - channel_close_reconn(channel, errmsg); + channel_disconnect(channel, LOG_INFORM, true, errmsg); } static void channel_err_broken(struct channel *channel, @@ -121,8 +104,11 @@ static void channel_err_broken(struct channel *channel, errmsg = tal_vfmt(tmpctx, fmt, ap); va_end(ap); - log_broken(channel->log, "%s", errmsg); - channel_close_conn(channel, errmsg); + if (channel_unsaved(channel)) { + log_broken(channel->log, "%s", errmsg); + channel_unsaved_close_conn(channel, errmsg); + } else + channel_disconnect(channel, LOG_BROKEN, false, errmsg); } void json_add_unsaved_channel(struct json_stream *response, @@ -520,14 +506,8 @@ static void rbf_channel_hook_cb(struct rbf_channel_payload *payload STEALS) tal_steal(tmpctx, payload); - if (!dualopend) { - channel_close_conn(channel, tal_fmt(tmpctx, - "Lost conn to node %s" - " awaiting rbf_channel callback", - type_to_string(tmpctx, struct node_id, - &channel->peer->id))); + if (!dualopend) return; - } tal_del_destructor2(dualopend, rbf_channel_remove_dualopend, payload); @@ -663,16 +643,9 @@ openchannel2_hook_cb(struct openchannel2_payload *payload STEALS) struct channel *channel = payload->channel; u8 *msg; - /* Our daemon died, we fail and try to reconnect */ - if (!dualopend) { - channel_close_conn(channel, - tal_fmt(tmpctx, "Lost conn to node %s" - " awaiting callback openchannel2", - type_to_string(tmpctx, - struct node_id, - &channel->peer->id))); + /* Our daemon died! */ + if (!dualopend) return; - } /* Free payload regardless of what happens next */ tal_steal(tmpctx, payload); @@ -969,9 +942,9 @@ openchannel2_sign_hook_cb(struct openchannel2_psbt_payload *payload STEALS) send_msg: /* Peer's gone away, let's try reconnecting */ if (!payload->dualopend) { - channel_err_broken_reconn(channel, "%s: dualopend daemon died" - " before signed PSBT returned", - channel->owner->name); + channel_saved_err_broken_reconn(channel, + "dualopend daemon died" + " before signed PSBT returned"); return; } tal_del_destructor2(payload->dualopend, @@ -1499,15 +1472,17 @@ static void handle_peer_locked(struct subd *dualopend, const u8 *msg) struct pubkey remote_per_commit; struct channel *channel = dualopend->channel; - if (!fromwire_dualopend_peer_locked(msg, &remote_per_commit)) + if (!fromwire_dualopend_peer_locked(msg, &remote_per_commit)) { channel_internal_error(channel, "Bad WIRE_DUALOPEND_PEER_LOCKED: %s", tal_hex(msg, msg)); + return; + } - /* Updates channel with the next per-commit point etc */ + /* Updates channel with the next per-commit point etc, calls + * channel_internal_error on failure */ if (!channel_on_funding_locked(channel, &remote_per_commit)) - channel_internal_error(channel, - "Got funding_locked twice"); + return; /* Remember that we got the lock-in */ wallet_channel_save(dualopend->ld->wallet, channel); @@ -2572,7 +2547,7 @@ static void handle_commit_received(struct subd *dualopend, if (channel->state == DUALOPEND_OPEN_INIT) { if (peer_active_channel(channel->peer)) { - channel_err_broken_reconn(channel, + channel_saved_err_broken_reconn(channel, "Already have active" " channel with %s", type_to_string(tmpctx, @@ -2627,13 +2602,12 @@ static void handle_commit_received(struct subd *dualopend, funding_ours, feerate_funding, psbt))) { - channel_err_broken_reconn(channel, - "wallet_update_channel failed" - " (chan %s)", - type_to_string( - tmpctx, - struct channel_id, - &channel->cid)); + channel_internal_error(channel, + "wallet_update_channel failed" + " (chan %s)", + type_to_string(tmpctx, + struct channel_id, + &channel->cid)); channel->open_attempt = tal_free(channel->open_attempt); return; @@ -2852,9 +2826,9 @@ static void start_fresh_dualopend(struct peer *peer, take(&hsmfd), NULL); if (!channel->owner) { - channel_err_broken_reconn(channel, - "Running lightning_dualopend: %s", - strerror(errno)); + channel_internal_error(channel, + "Running lightningd_dualopend: %s", + strerror(errno)); return; } diff --git a/lightningd/dual_open_control.h b/lightningd/dual_open_control.h index 8c82ce21a..46d27c2f3 100644 --- a/lightningd/dual_open_control.h +++ b/lightningd/dual_open_control.h @@ -20,11 +20,8 @@ void dualopen_tell_depth(struct subd *dualopend, const struct bitcoin_txid *txid, u32 depth); -void channel_close_conn(struct channel *channel, - const char *why); - -void channel_close_reconn(struct channel *channel, - const char *why); +/* Close connection to an unsaved channel */ +void channel_unsaved_close_conn(struct channel *channel, const char *why); void json_add_unsaved_channel(struct json_stream *response, const struct channel *channel); diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index a9d91097f..24110899f 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -1562,7 +1562,8 @@ static struct command_result *json_close(struct command *cmd, return command_success(cmd, json_stream_success(cmd)); } if ((channel = peer_unsaved_channel(peer))) { - channel_close_conn(channel, "close command called"); + channel_unsaved_close_conn(channel, + "close command called"); return command_success(cmd, json_stream_success(cmd)); } return command_fail(cmd, LIGHTNINGD, @@ -1867,7 +1868,7 @@ static struct command_result *json_disconnect(struct command *cmd, } channel = peer_unsaved_channel(peer); if (channel) { - channel_close_conn(channel, "disconnect command"); + channel_unsaved_close_conn(channel, "disconnect command"); return command_success(cmd, json_stream_success(cmd)); } if (!peer->uncommitted_channel) { diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c index 87a701116..b81b083ac 100644 --- a/lightningd/test/run-invoice-select-inchan.c +++ b/lightningd/test/run-invoice-select-inchan.c @@ -59,10 +59,6 @@ const char *channel_change_state_reason_str(enum state_change reason UNNEEDED) /* Generated stub for channel_cleanup_commands */ void channel_cleanup_commands(struct channel *channel UNNEEDED, const char *why UNNEEDED) { fprintf(stderr, "channel_cleanup_commands called!\n"); abort(); } -/* Generated stub for channel_close_conn */ -void channel_close_conn(struct channel *channel UNNEEDED, - const char *why UNNEEDED) -{ fprintf(stderr, "channel_close_conn called!\n"); abort(); } /* Generated stub for channel_fail_forget */ void channel_fail_forget(struct channel *channel UNNEEDED, const char *fmt UNNEEDED, ...) { fprintf(stderr, "channel_fail_forget called!\n"); abort(); } @@ -115,6 +111,9 @@ bool channel_tell_depth(struct lightningd *ld UNNEEDED, const struct bitcoin_txid *txid UNNEEDED, u32 depth UNNEEDED) { fprintf(stderr, "channel_tell_depth called!\n"); abort(); } +/* Generated stub for channel_unsaved_close_conn */ +void channel_unsaved_close_conn(struct channel *channel UNNEEDED, const char *why UNNEEDED) +{ fprintf(stderr, "channel_unsaved_close_conn called!\n"); abort(); } /* Generated stub for command_fail */ struct command_result *command_fail(struct command *cmd UNNEEDED, errcode_t code UNNEEDED, const char *fmt UNNEEDED, ...) diff --git a/tests/test_connection.py b/tests/test_connection.py index bab011286..8f0f04746 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -481,10 +481,7 @@ def test_reconnect_openingd(node_factory): l1.rpc.connect(l2.info['id'], 'localhost', l2.port) # We should get a message about reconnecting. - if l2.config('experimental-dual-fund'): - l2.daemon.wait_for_log('Killing dualopend: Reconnected') - else: - l2.daemon.wait_for_log('Killing opening daemon: Reconnected') + l2.daemon.wait_for_log('Killing opening daemon: Reconnected') l2.daemon.wait_for_log('Handed peer, entering loop') # Should work fine. diff --git a/wallet/db_postgres_sqlgen.c b/wallet/db_postgres_sqlgen.c index c9e2f8126..a65d479ff 100644 --- a/wallet/db_postgres_sqlgen.c +++ b/wallet/db_postgres_sqlgen.c @@ -1888,4 +1888,4 @@ struct db_query db_postgres_queries[] = { #endif /* LIGHTNINGD_WALLET_GEN_DB_POSTGRES */ -// SHA256STAMP:4404a89dadab8225901d024fd293d4b47b57ed4c6b5e7f00cf1fc9df0c345d57 +// SHA256STAMP:74b99da984e5e1872a7b3de32d3fc00efd7538e95e0509c6a839097522ea8a94 diff --git a/wallet/db_sqlite3_sqlgen.c b/wallet/db_sqlite3_sqlgen.c index 256d3326b..e07ca3ac3 100644 --- a/wallet/db_sqlite3_sqlgen.c +++ b/wallet/db_sqlite3_sqlgen.c @@ -1888,4 +1888,4 @@ struct db_query db_sqlite3_queries[] = { #endif /* LIGHTNINGD_WALLET_GEN_DB_SQLITE3 */ -// SHA256STAMP:4404a89dadab8225901d024fd293d4b47b57ed4c6b5e7f00cf1fc9df0c345d57 +// SHA256STAMP:74b99da984e5e1872a7b3de32d3fc00efd7538e95e0509c6a839097522ea8a94 diff --git a/wallet/statements_gettextgen.po b/wallet/statements_gettextgen.po index 33769f9ba..1c249dd51 100644 --- a/wallet/statements_gettextgen.po +++ b/wallet/statements_gettextgen.po @@ -1238,11 +1238,11 @@ msgstr "" msgid "not a valid SQL statement" msgstr "" -#: wallet/test/run-wallet.c:1445 +#: wallet/test/run-wallet.c:1444 msgid "SELECT COUNT(1) FROM channel_funding_inflights WHERE channel_id = ?;" msgstr "" -#: wallet/test/run-wallet.c:1643 +#: wallet/test/run-wallet.c:1642 msgid "INSERT INTO channels (id) VALUES (1);" msgstr "" -# SHA256STAMP:be8710aae042274c3b689ec333ee2d50d4116c3ea4d6b97338eaf88050137967 +# SHA256STAMP:6f39707798a473b25ddf2706f514421533231682d47b0141df500193e9c27fd2 diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index a6c720a84..955d84175 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -60,16 +60,15 @@ void broadcast_tx(struct chain_topology *topo UNNEEDED, bool success UNNEEDED, const char *err)) { fprintf(stderr, "broadcast_tx called!\n"); abort(); } -/* Generated stub for channel_close_conn */ -void channel_close_conn(struct channel *channel UNNEEDED, - const char *why UNNEEDED) -{ fprintf(stderr, "channel_close_conn called!\n"); abort(); } /* Generated stub for channel_tell_depth */ bool channel_tell_depth(struct lightningd *ld UNNEEDED, struct channel *channel UNNEEDED, const struct bitcoin_txid *txid UNNEEDED, u32 depth UNNEEDED) { fprintf(stderr, "channel_tell_depth called!\n"); abort(); } +/* Generated stub for channel_unsaved_close_conn */ +void channel_unsaved_close_conn(struct channel *channel UNNEEDED, const char *why UNNEEDED) +{ fprintf(stderr, "channel_unsaved_close_conn called!\n"); abort(); } /* Generated stub for command_fail */ struct command_result *command_fail(struct command *cmd UNNEEDED, errcode_t code UNNEEDED, const char *fmt UNNEEDED, ...)