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.

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

splice: Reestablish when commit or sig sends fail #6840

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

ddustin
Copy link
Collaborator

@ddustin ddustin commented Nov 3, 2023

Adds tests for when the connection fails during

  1. splice tx_signature
  2. splice commitment_signed

Fleshed out the reestablish flow for these two cases and implemented the fixes to make these reestablish flows work.

Fixes #6703

Changelog-Fixed: Implemented splicing restart logic for tx_signature and commitment_signed.

@ddustin ddustin force-pushed the ddustin/splice_reestablish branch 5 times, most recently from 95105a3 to da8aa04 Compare November 3, 2023 06:39
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like that test are failing

test_splice passed 1 out of the required 1 times. Success!

===End Flaky Test Report===
=========================== short test summary info ============================
FAILED tests/test_opening.py::test_rbf_reconnect_tx_construct - TimeoutError: Unable to find "[re.compile('dualopend daemon died before signed PSBT returned')]" in logs.
FAILED tests/test_splicing_disconnect.py::test_splice_disconnect_sig - TimeoutError: Unable to find "[re.compile('CHANNELD_AWAITING_SPLICE to CHANNELD_NORMAL')]" in logs.
===== 2 failed, 105 passed, 582 skipped, 33 warnings in 699.05s (0:11:39) ======

@ddustin ddustin force-pushed the ddustin/splice_reestablish branch 6 times, most recently from 497d677 to 166c3ae Compare November 3, 2023 20:10
@ddustin ddustin force-pushed the ddustin/splice_reestablish branch 7 times, most recently from 1e41866 to 50bcd89 Compare November 9, 2023 01:03
@ddustin ddustin added this to the v23.11 milestone Nov 10, 2023
vincenzopalazzo

This comment was marked as off-topic.

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to accept this since we're close to release and it only effects experimental. However, it's a giant blob of unreviewable changes, which is not nice :(

At least one thing is wrong, though.

channeld/channeld.c Show resolved Hide resolved
@vincenzopalazzo
Copy link
Collaborator

This is introducing breaking change so @nepet drop for the milestone, it is breaking VLS with #6870 so we should drop it from the current release IMHO too

@vincenzopalazzo vincenzopalazzo modified the milestones: v23.11, v24.02 Nov 15, 2023
@ddustin ddustin force-pushed the ddustin/splice_reestablish branch 4 times, most recently from 8fdab93 to c1ffe15 Compare November 16, 2023 19:26
@nepet nepet requested a review from ksedgwic November 17, 2023 10:01
Copy link
Collaborator

@ksedgwic ksedgwic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Investigated the "commitment mismatch" VLS was seeing with tests/test_splicing_disconnect.py::test_splice_disconnect_commit, looks like this is not a bug and we can easily handle in VLS: https://gitlab.com/lightning-signer/vls-hsmd/-/merge_requests/132#note_1654982750

This PR looks good from VLS

Adds tests for when the connection fails during
1) splice tx_signature
2) splice commitment_signed

Fleshed out the reestablish flow for these two cases and implemented the fixes to make these reestablish flows work.

Part of this work required changing commit process for splices: Now we send a single commit_part for the splice where previously we sent all commits, and accordingly, we no longer revoke in response.

Changelog-Fixed: Implemented splicing restart logic for tx_signature and commitment_signed. Splice commitments are reworked in a manner incompatible with the last version.
@nepet
Copy link
Collaborator

nepet commented Nov 19, 2023

Squashed fixup commit

@rustyrussell
Copy link
Contributor

Ack b54af9f

Copy link
Collaborator

@niftynei niftynei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bunch of nits, but nothing major. Nice job getting this turned around so quickly.

More tests would be nice??

Also echo what @rustyrussell said about more commits please. I need more commit inflation!!

ACK b54af9f

@@ -1923,6 +1923,7 @@ static bool test_channel_inflight_crud(struct lightningd *ld, const tal_t *ctx)
AMOUNT_MSAT(10),
AMOUNT_SAT(1111),
0,
false,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: probably worth testing this field works in the db tests (eg set to true once)

@@ -1256,6 +1257,7 @@ void wallet_inflight_add(struct wallet *w, struct channel_inflight *inflight)

db_bind_s64(stmt, inflight->funding->splice_amnt);
db_bind_int(stmt, inflight->i_am_initiator);
db_bind_int(stmt, inflight->force_sign_first);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we usually do a "type conversion" btw int + bool

db_bind_int(stmt, inflight->remote_tx_sigs ? 1 : 0);

struct wally_psbt *psbt;
s64 splice_amnt;
struct bitcoin_tx *last_tx;
/* last_sig is assumed valid if last_tx is set */
struct bitcoin_signature last_sig;
bool i_am_initiator;
bool force_sign_first;
bool is_locked;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove this?


inflight = last_inflight(peer);

if (inflight && (!inflight->last_tx || !inflight->remote_tx_sigs)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add comment from spec here?

bool skip_commitments,
enum tx_role our_role)
bool send_commitments,
bool recv_commitments,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe call this 'expect_rcv_commitments'?

del l1.daemon.opts['dev-disconnect']

# Should reconnect, and reestablish the splice.
l1.start()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could also just call connect again?

inv = l2.rpc.invoice(10**2, '3', 'no_3')
l1.rpc.pay(inv['bolt11'])

# Check that the splice doesn't generate a unilateral close transaction
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there another signal we can check for?

funds_result = l1.rpc.fundpsbt("109000sat", "slow", 166, excess_as_change=True)

result = l1.rpc.splice_init(chan_id, 100000, funds_result['psbt'])
print("l1 splice_update")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we get rid of the prints? 😁


l2.daemon.wait_for_log(r'dev_disconnect: \+WIRE_COMMITMENT_SIGNED')

print("Killing l2 without sending WIRE_COMMITMENT_SIGNED")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: assert we're in the state expected before killing + reconnecting


# Check that the splice doesn't generate a unilateral close transaction
time.sleep(5)
assert l1.db_query("SELECT count(*) as c FROM channeltxs;")[0]['c'] == 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one other test i would add tests the tx-abort pathway?

@nepet nepet merged commit a6a9e5b into ElementsProject:master Nov 20, 2023
38 checks passed
@ddustin ddustin mentioned this pull request Dec 25, 2023
71 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

splice: lightningd updates scid too early
6 participants