From 0fd87b85dac92f5da54b93a75c4dfa3283308346 Mon Sep 17 00:00:00 2001 From: ZmnSCPxj jxPCSnmZ Date: Thu, 8 Oct 2020 16:01:09 +0800 Subject: [PATCH] openingd/: Fail `fundchannel_start` if we already are, or will become, the fundee. Fixes: #4108 Changelog-Fixed: Network: Fixed a race condition when us and a peer attempt to make channels to each other at nearly the same time. --- lightningd/opening_common.c | 2 ++ lightningd/opening_common.h | 5 ++++ lightningd/opening_control.c | 49 +++++++++++++++++++++++++++++++++++- tests/test_connection.py | 1 - 4 files changed, 55 insertions(+), 2 deletions(-) diff --git a/lightningd/opening_common.c b/lightningd/opening_common.c index 12086ab37..78dd693ed 100644 --- a/lightningd/opening_common.c +++ b/lightningd/opening_common.c @@ -55,6 +55,8 @@ new_uncommitted_channel(struct peer *peer) uc->peer->uncommitted_channel = uc; tal_add_destructor(uc, destroy_uncommitted_channel); + uc->got_offer = false; + return uc; } diff --git a/lightningd/opening_common.h b/lightningd/opening_common.h index 6d789f51b..313c6b6c3 100644 --- a/lightningd/opening_common.h +++ b/lightningd/opening_common.h @@ -46,6 +46,11 @@ struct uncommitted_channel { /* Public key for funding tx. */ struct pubkey local_funding_pubkey; + /* If true, we are already in fundee-mode and any future + * `fundchannel_start` on our end should fail. + */ + bool got_offer; + /* These are *not* filled in by new_uncommitted_channel: */ /* Minimum funding depth (if opener == REMOTE). */ diff --git a/lightningd/opening_control.c b/lightningd/opening_control.c index f7676b8e1..080ab63fb 100644 --- a/lightningd/opening_control.c +++ b/lightningd/opening_control.c @@ -104,6 +104,11 @@ wallet_commit_channel(struct lightningd *ld, bool option_static_remotekey; bool option_anchor_outputs; + /* We cannot both be the fundee *and* have a `fundchannel_start` + * command running! + */ + assert(!(uc->got_offer && uc->fc)); + /* Get a key to use for closing outputs from this tx */ final_key_idx = wallet_get_newindex(ld); if (final_key_idx == -1) { @@ -538,6 +543,14 @@ failed: tal_free(uc); } +/* These two functions used to be a single function, pre-declaring + * here in order to reduce review load since the original function + * is not really changed at all in its operation, just need access + * to the cleanup part in a separate piece of code. + */ +static void +opening_funder_failed_cancel_commands(struct uncommitted_channel *uc, + const char *desc); static void opening_funder_failed(struct subd *openingd, const u8 *msg, struct uncommitted_channel *uc) { @@ -554,6 +567,16 @@ static void opening_funder_failed(struct subd *openingd, const u8 *msg, return; } + opening_funder_failed_cancel_commands(uc, desc); +} +static void +opening_funder_failed_cancel_commands(struct uncommitted_channel *uc, + const char *desc) +{ + /* If no funding command(s) pending, do nothing. */ + if (!uc->fc) + return; + /* Tell anyone who was trying to cancel */ for (size_t i = 0; i < tal_count(uc->fc->cancels); i++) { struct json_stream *response; @@ -568,12 +591,17 @@ static void opening_funder_failed(struct subd *openingd, const u8 *msg, was_pending(command_fail(uc->fc->cmd, LIGHTNINGD, "%s", desc)); /* Clear uc->fc, so we can try again, and so we don't fail twice - * if they close. */ + * if they close. + * This code is also used in the case where we turn out to already + * be the fundee, in which case we should not have any `fc` at all, + * so we definitely should clear this. + */ uc->fc = tal_free(uc->fc); } struct openchannel_hook_payload { struct subd *openingd; + struct uncommitted_channel* uc; struct amount_sat funding_satoshis; struct amount_msat push_msat; struct amount_sat dust_limit_satoshis; @@ -631,6 +659,7 @@ openchannel_hook_final(struct openchannel_hook_payload *payload STEALS) struct subd *openingd = payload->openingd; const u8 *our_upfront_shutdown_script = payload->our_upfront_shutdown_script; const char *errmsg = payload->errmsg; + struct uncommitted_channel* uc = payload->uc; /* We want to free this, whatever happens. */ tal_steal(tmpctx, payload); @@ -641,6 +670,16 @@ openchannel_hook_final(struct openchannel_hook_payload *payload STEALS) tal_del_destructor2(openingd, openchannel_payload_remove_openingd, payload); + if (!errmsg) { + /* Plugins accepted the offer, cancel any of our + * funder-side commands. */ + opening_funder_failed_cancel_commands(uc, + "Have in-progress " + "`open_channel` from " + "peer"); + uc->got_offer = true; + } + subd_send_msg(openingd, take(towire_openingd_got_offer_reply(NULL, errmsg, our_upfront_shutdown_script))); @@ -739,6 +778,7 @@ static void opening_got_offer(struct subd *openingd, payload = tal(openingd, struct openchannel_hook_payload); payload->openingd = openingd; + payload->uc = uc; payload->our_upfront_shutdown_script = NULL; payload->errmsg = NULL; if (!fromwire_openingd_got_offer(payload, msg, @@ -1089,6 +1129,13 @@ static struct command_result *json_fund_channel_start(struct command *cmd, return command_fail(cmd, LIGHTNINGD, "Already funding channel"); } + if (peer->uncommitted_channel->got_offer) { + return command_fail(cmd, LIGHTNINGD, + "Have in-progress " + "`open_channel` from " + "peer"); + } + /* BOLT #2: * - if both nodes advertised `option_support_large_channel`: * - MAY set `funding_satoshis` greater than or equal to 2^24 satoshi. diff --git a/tests/test_connection.py b/tests/test_connection.py index de56d74af..79cf67638 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -2761,7 +2761,6 @@ def test_channel_opener(node_factory): assert(l2.rpc.listpeers()['peers'][0]['channels'][0]['closer'] == 'remote') -@pytest.mark.xfail(strict=True) def test_fundchannel_start_alternate(node_factory, executor): ''' Test to see what happens if two nodes start channeling to each other alternately.