Test channel reestablish during splice lifecycle #4079
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @jkczyz! This PR has been waiting for your review. |
wpaulino
force-pushed
the
test-splice-reestablish
branch
from
aa5ab88 to
e841721
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4079 +/- ##
==========================================
+ Coverage 88.63% 88.72% +0.09%
==========================================
Files 180 180
Lines 135243 135528 +285
Branches 135243 135528 +285
==========================================
+ Hits 119873 120252 +379
+ Misses 12597 12506 -91
+ Partials 2773 2770 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Needs rebase :( |
wpaulino
force-pushed
the
test-splice-reestablish
branch
from
e841721 to
9588403
Compare
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
wpaulino
force-pushed
the
test-splice-reestablish
branch
from
9588403 to
02acd71
Compare
lightning/src/ln/channel.rs
Outdated
| .interactive_tx_signing_session | ||
| .as_ref() | ||
| .map(|signing_session| signing_session.unsigned_tx().compute_txid()); | ||
| self.pending_splice.is_some() && their_next_funding_txid == our_next_funding_txid |
There was a problem hiding this comment.
This is much stricter than the spec update proposed here - the spec update is simple (did the messages contain the fields) but this is checking that indirectly and in a complicated way. I'm not entirely sure why we need to bother with this, in any case, a redundant channel_ready isn't really a big deal and this is a bunch of annoying code.
There was a problem hiding this comment.
The spec does note that they must be "for a splice transaction". I found a way to simplify this though, we can just check for existence of pending_splice on reestablish.
There was a problem hiding this comment.
Latest push looks good modulo addressing @TheBlueMatt's comments.
wpaulino
force-pushed
the
test-splice-reestablish
branch
from
02acd71 to
5d2e474
Compare
We may produce duplicate `FundingTransactionReadyForSigning` events if the user has processed an initial event but has not yet called back with `funding_transaction_signed` and a peer reconnection occurs. If the user also handles the duplicate events, any duplicate calls to `funding_transaction_signed` after an initial successful one would return an error. This doesn't make sense, as the API should remain idempotent, so we return early on any duplicate calls.
We only want to produce `tx_signatures` once we know that the monitor update (either the initial one for a dual-funded channel, or a `RenegotiatedFunding` one for a splice) has been persisted. If we haven't received the counterparty's `commitment_signed` yet, then the monitor update hasn't been created, leading us to pass the `!awaiting_monitor_update` condition and produce a holder `tx_signatures` message.
If nodes have started a splice, this means they have both sent and received `channel_ready` already: in that case, it's unnecessary to retransmit `channel_ready` on reconnection.
Otherwise, we won't ever be able to resume a pending negotiation after a reconnection via `channel_reestablish`. Along the way, we also merge `should_reset_pending_splice_state` into `PendingFunding::can_abandon_state` to simplify the logic around when we're able to reset specific parts of the pending splice state.
We'll use this in the next commit to test the resend logic for `announcement_signatures` when reestablishing a channel that had a pending splice become locked.
This test captures all the new spec requirements introduced for a splice to the channel reestablish flow.
wpaulino
force-pushed
the
test-splice-reestablish
branch
from
5d2e474 to
5594f9b
Compare
4 participants
This also includes a couple small bug fixes found along the way.