From dd0c2a28ec9d21ff1892ff0a63d6b7c4d78d925b Mon Sep 17 00:00:00 2001 From: Rusty Russell <rusty@rustcorp.com.au> Date: Thu, 25 May 2023 11:06:27 +0930 Subject: [PATCH] lightningd: fix incorrect reuse of dualopend, leading to dev_queryfeerates race CI hit this issue where it would get a tx_abort in fundchannel. This happens when the dualopend we use to query the feerates has not exited yet (it waits for the tx_abort reply), and we mistakenly reuse it. With multi-channel support, this is wrong: just run another one and it all Just Works. This means we need to rework our dual_open_control.c logic, since it would previously create an unsaved channel then not clean up if something went wrong. Most people will never try to negotiate opening multiple channels to the same peer at the same time (vs. having an established channel and opening a new one), so this case is a bit weird. ``` rates = l1.rpc.dev_queryrates(l2.info['id'], amount, amount) # l1 leases a channel from l2 l1.rpc.fundchannel(l2.info['id'], amount, request_amt=amount, feerate='{}perkw'.format(feerate), > compact_lease=rates['compact_lease']) tests/test_opening.py:1611: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ contrib/pyln-client/pyln/client/lightning.py:833: in fundchannel return self.call("fundchannel", payload) contrib/pyln-testing/pyln/testing/utils.py:721: in call res = LightningRpc.call(self, method, payload, cmdprefix, filter) _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <pyln.testing.utils.PrettyPrintingLightningRpc object at 0x7f6cbcd97950> method = 'fundchannel' payload = {'amount': 500000, 'announce': True, 'compact_lease': '029a00640064000000644c4b40', 'feerate': '2000perkw', ...} cmdprefix = None, filter = None def call(self, method, payload=None, cmdprefix=None, filter=None): """Generic call API: you can set cmdprefix here, or set self.cmdprefix ... if not isinstance(resp, dict): raise ValueError("Malformed response, response is not a dictionary %s." % resp) elif "error" in resp: > raise RpcError(method, payload, resp['error']) E pyln.client.lightning.RpcError: RPC call failed: method: fundchannel, payload: {'id': '022d223620a359a47ff7f7ac447c85c46c923da53389221a0054c11c1e3ca31d59', 'amount': 500000, 'feerate': '2000perkw', 'announce': True, 'request_amt': 500000, 'compact_lease': '029a00640064000000644c4b40'}, error: {'code': -1, 'message': 'Abort requested', 'data': {'id': '022d223620a359a47ff7f7ac447c85c46c923da53389221a0054c11c1e3ca31d59', 'method': 'openchannel_init'}} ``` Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> --- lightningd/dual_open_control.c | 62 ++++++++++++---------------------- tests/test_connection.py | 3 -- 2 files changed, 21 insertions(+), 44 deletions(-) diff --git a/lightningd/dual_open_control.c b/lightningd/dual_open_control.c index 9151276de106..4708578cbcac 100644 --- a/lightningd/dual_open_control.c +++ b/lightningd/dual_open_control.c @@ -2842,24 +2842,6 @@ static struct command_result *json_openchannel_init(struct command *cmd, return command_fail(cmd, FUNDING_UNKNOWN_PEER, "Unknown peer"); } - channel = peer_any_unsaved_channel(peer, NULL); - if (!channel) { - channel = new_unsaved_channel(peer, - peer->ld->config.fee_base, - peer->ld->config.fee_per_satoshi); - - /* We derive initial channel_id *now*, so we can tell it to - * connectd. */ - derive_tmp_channel_id(&channel->cid, - &channel->local_basepoints.revocation); - } - - if (channel->open_attempt - || !list_empty(&channel->inflights)) - return command_fail(cmd, FUNDING_STATE_INVALID, - "Channel funding in-progress. %s", - channel_state_name(channel)); - if (!feature_negotiated(cmd->ld->our_features, peer->their_features, OPT_DUAL_FUND)) { @@ -2898,6 +2880,20 @@ static struct command_result *json_openchannel_init(struct command *cmd, type_to_string(tmpctx, struct wally_psbt, psbt)); + if (socketpair(AF_LOCAL, SOCK_STREAM, 0, fds) != 0) { + return command_fail(cmd, FUND_MAX_EXCEEDED, + "Failed to create socketpair: %s", + strerror(errno)); + } + + /* Now we can't fail, create channel */ + channel = new_unsaved_channel(peer, + peer->ld->config.fee_base, + peer->ld->config.fee_per_satoshi); + /* We derive initial channel_id *now*, so we can tell it to connectd. */ + derive_tmp_channel_id(&channel->cid, + &channel->local_basepoints.revocation); + /* Get a new open_attempt going */ channel->opener = LOCAL; channel->open_attempt = oa = new_channel_open_attempt(channel); @@ -2943,12 +2939,6 @@ static struct command_result *json_openchannel_init(struct command *cmd, false, rates); - if (socketpair(AF_LOCAL, SOCK_STREAM, 0, fds) != 0) { - return command_fail(cmd, FUND_MAX_EXCEEDED, - "Failed to create socketpair: %s", - strerror(errno)); - } - /* Start dualopend! */ if (!peer_start_dualopend(peer, new_peer_fd(cmd, fds[0]), channel)) { close(fds[1]); @@ -3424,24 +3414,14 @@ static struct command_result *json_queryrates(struct command *cmd, peer->connected == PEER_DISCONNECTED ? "not connected" : "still connecting"); - /* FIXME: This is wrong: we should always create a new channel? */ - channel = peer_any_unsaved_channel(peer, NULL); - if (!channel) { - channel = new_unsaved_channel(peer, - peer->ld->config.fee_base, - peer->ld->config.fee_per_satoshi); + channel = new_unsaved_channel(peer, + peer->ld->config.fee_base, + peer->ld->config.fee_per_satoshi); - /* We derive initial channel_id *now*, so we can tell it to - * connectd. */ - derive_tmp_channel_id(&channel->cid, - &channel->local_basepoints.revocation); - } - - if (channel->open_attempt - || !list_empty(&channel->inflights)) - return command_fail(cmd, FUNDING_STATE_INVALID, - "Channel funding in-progress. %s", - channel_state_name(channel)); + /* We derive initial channel_id *now*, so we can tell it to + * connectd. */ + derive_tmp_channel_id(&channel->cid, + &channel->local_basepoints.revocation); if (!feature_negotiated(cmd->ld->our_features, peer->their_features, diff --git a/tests/test_connection.py b/tests/test_connection.py index d2aa4167cc7d..d39e6c815470 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -1460,9 +1460,6 @@ def test_funding_v2_corners(node_factory, bitcoind): l1.rpc.openchannel_init(l2.info['id'], amount + 1, psbt) start = l1.rpc.openchannel_init(l2.info['id'], amount, psbt) - with pytest.raises(RpcError, match=r'Channel funding in-progress. DUALOPEND_OPEN_INIT'): - l1.rpc.fundchannel(l2.info['id'], amount) - # We can abort a channel l1.rpc.openchannel_abort(start['channel_id'])