Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI: restore dual-funding tests. #6273

Merged
merged 7 commits into from
May 29, 2023
Merged

CI: restore dual-funding tests. #6273

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2b4afe3
lightningd: fix DF crash from libwally update.
rustyrussell May 23, 2023
75620f7
pytest: update test_v2_fail_second for listpeers changes.
rustyrussell May 23, 2023
fdd0ce4
pytest: fix up DF tests after onchaind broadcast rework.
rustyrussell May 23, 2023
693bab8
CI: restore dual-funding tests.
rustyrussell May 23, 2023
f6d9516
pytest: slow down for bitcoind.
rustyrussell May 23, 2023
c351f31
dualopend: fix minor memleak report.
rustyrussell May 23, 2023
dd0c2a2
lightningd: fix incorrect reuse of dualopend, leading to dev_queryfee…
rustyrussell May 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
lightningd: fix incorrect reuse of dualopend, leading to dev_queryfee… 
…rates 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>
  • Loading branch information
@rustyrussell
rustyrussell committed May 25, 2023
commit dd0c2a28ec9d21ff1892ff0a63d6b7c4d78d925b
62 changes: 21 additions & 41 deletions lightningd/dual_open_control.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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]);
Expand Down Expand Up @@ -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,
Expand Down
3 changes: 0 additions & 3 deletions tests/test_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'])

Expand Down