Update channel_reestablish for splicing
#3886
Conversation
|
👋 Thanks for assigning @wpaulino as a reviewer! |
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
jkczyz
force-pushed
the
2025-06-channel-reestablish
branch
2 times, most recently
from
06c0dfc to
f000b76
Compare
|
@wpaulino Ready for review now. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3886 +/- ##
==========================================
- Coverage 88.61% 88.59% -0.03%
==========================================
Files 174 174
Lines 127640 127733 +93
Branches 127640 127733 +93
==========================================
+ Hits 113113 113166 +53
- Misses 12046 12077 +31
- Partials 2481 2490 +9
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:
|
|
🔔 2nd Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @wpaulino! This PR has been waiting for your review. |
jkczyz
force-pushed
the
2025-06-channel-reestablish
branch
from
f000b76 to
e2ea3bf
Compare
|
Rebased. |
lightning/src/ln/channel.rs
Outdated
| @@ -10128,10 +10135,52 @@ where | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(splicing)] | |||
| fn maybe_get_your_last_funding_locked_txid(&self, features: &InitFeatures) -> Option<Txid> { | |||
| if !features.supports_splicing() { | |||
There was a problem hiding this comment.
Since the TLVs are optional, I don't think we really need to check this and can always set them
There was a problem hiding this comment.
Hmm.. if the implementation supports splicing but didn't set the feature bit, won't they understand the TLV?
There was a problem hiding this comment.
Yes. This method is only for an outgoing channel_reestablish so they'll just ignore the TLV if they don't support splicing.
There was a problem hiding this comment.
Ah, I was overloading "supporting" here. What I meant was that the other implementation supported splicing in the sense they have code written. But for whatever reason they chose not to set the feature bit in their init message. In that sense, they might not ignore the TLV?
I guess in our implementation here we don't really do any checks around this but should always do the right thing (i.e., never send splice_locked) since in the above scenario as we'd never have any FundingScope where is_splice is true.
Though, it doesn't seem like we check their feature bits when handling any splice-related messages. Do we care if they tell us they don't support splicing but send us those messages anyway? At very least it seems we should check if they support splicing before attempting to initiate a splice.
There was a problem hiding this comment.
Do we care if they tell us they don't support splicing but send us those messages anyway?
I don't think so, them sending the message implies they should support it?
At very least it seems we should check if they support splicing before attempting to initiate a splice.
Yeah we should always check they support a feature before we attempt to use it. The channel_reestablish TLVs are a bit different as they're not a feature on their own, and are odd so they can be ignored if not supported.
There was a problem hiding this comment.
They also have a perfectly valid meaning outside of a splicing context. I agree, I see no reason to not just always send them (even without the cfg).
There was a problem hiding this comment.
Alright, dropped the part needing init features.
|
👋 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. |
jkczyz
force-pushed
the
2025-06-channel-reestablish
branch
from
e2ea3bf to
69be701
Compare
There was a problem hiding this comment.
We may need to double check our incoming
channel_readyhandling to make sure we can handle receiving one long afterchannel_readywas already exchanged due to the new logic surroundingyour_last_funding_locked_txid.
ACK. Also need to address https://github.com/lightningdevkit/rust-lightning/pull/3736/files#r2133028859.
lightning/src/ln/channel.rs
Outdated
| @@ -10128,10 +10135,52 @@ where | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(splicing)] | |||
| fn maybe_get_your_last_funding_locked_txid(&self, features: &InitFeatures) -> Option<Txid> { | |||
| if !features.supports_splicing() { | |||
There was a problem hiding this comment.
Hmm.. if the implementation supports splicing but didn't set the feature bit, won't they understand the TLV?
jkczyz
force-pushed
the
2025-06-channel-reestablish
branch
2 times, most recently
from
f7b9785 to
b0291f4
Compare
Latest push adds some commits to address this. |
Looks like we have some logic below. I think the catch-all case still holds? We should only receive a rust-lightning/lightning/src/ln/channel.rs Lines 6491 to 6519 in 42085b9 |
jkczyz
force-pushed
the
2025-06-channel-reestablish
branch
from
b0291f4 to
8004c6f
Compare
jkczyz
force-pushed
the
2025-06-channel-reestablish
branch
from
8004c6f to
5a6eb89
Compare
|
Re-ordered and pulled some commits into #3982 (which is now merged) to help unblock testing. Still need to address some comments. |
jkczyz
force-pushed
the
2025-06-channel-reestablish
branch
from
5a6eb89 to
08065aa
Compare
|
Rebased to resolve merge conflicts. Will revisit to see what comments need to be addressed. |
jkczyz
force-pushed
the
2025-06-channel-reestablish
branch
from
08065aa to
2f88511
Compare
|
Squashed fixups for now. Working on updating this for the most recent spec changes. |
jkczyz
force-pushed
the
2025-06-channel-reestablish
branch
2 times, most recently
from
4e76094 to
f5c9271
Compare
|
@wpaulino @TheBlueMatt Let me know if you want me to squash and get rid of the commits that are reverted now that the spec has been simplified. |
|
Feel free to squash, simplify, and rebase IMO |
While splicing is not yet fully supported, checking if the feature has been negotiated is needed for changes to the channel_reestablish logic.
The splicing spec extends the channel_reestablish message with two more TLVs indicating which funding txid the sender has sent/received either explicitly via splice_locked or implicitly via channel_ready. This allows peers to detect if a splice_locked was lost during disconnection and must be retransmitted. This commit updates channel_reestablish with the TLVs. Subsequent commits will implement the spec requirements.
The previous commit extended the channel_reestablish message with your_last_funding_locked_txid and my_current_funding_locked_txid for use as described there. This commit sets those fields to the funding txid most recently sent/received accordingly.
The splicing spec updates the logic pertaining to next_funding_txid when
handling a channel_reestablish message. Specifically:
A receiving node:
- if `next_funding_txid` is set:
- if `next_funding_txid` matches the latest interactive funding transaction
or the current channel funding transaction:
- if `next_commitment_number` is equal to the commitment number of the
`commitment_signed` message it sent for this funding transaction:
- MUST retransmit its `commitment_signed` for that funding transaction.
- if it has already received `commitment_signed` and it should sign first,
as specified in the [`tx_signatures` requirements](#the-tx_signatures-message):
- MUST send its `tx_signatures` for that funding transaction.
- if it has already received `tx_signatures` for that funding transaction:
- MUST send its `tx_signatures` for that funding transaction.
- if it also sets `next_funding_txid` in its own `channel_reestablish`, but the
values don't match:
- MUST send an `error` and fail the channel.
- otherwise:
- MUST send `tx_abort` to let the sending node know that they can forget
this funding transaction.
This commit updates FundedChannel::channel_reestablish accordingly.
Co-authored-by: Wilmer Paulino <wilmer@wilmerpaulino.com>
Co-authored-by: Jeffrey Czyz <jkczyz@gmail.com>
The splicing spec updates the logic pertaining to next_commitment_number
when sending a channel_reestablish message. Specifically:
The sending node:
- if it has sent `commitment_signed` for an interactive transaction construction but
it has not received `tx_signatures`:
- MUST set `next_funding_txid` to the txid of that interactive transaction.
- if it has not received `commitment_signed` for that interactive transaction:
- MUST set `next_commitment_number` to the commitment number of the `commitment_signed` it sent.
When a splice transaction is promoted (i.e., when splice_locked has been exchanged), announcement_signatures must be sent. However, if we try to send a channel_announcement before they are received, then the signatures will be incorrect. To avoid this, clear the counterparty's announcement_signatures upon promoting a FundingScope.
When handling a counterparties channel_reestablish, the spec dictates that a splice_locked may be implied by my_current_funding_locked. Compare that against any pending splices and handle an implicit splice_locked message when applicable.
During channel reestablishment, announcement_signatures may need to be retransmitted. The splicing spec allows doing so without retransmitting splice_locked first, which could normally trigger retransmitting announcement_signatures. Instead, my_current_funding_locked lets the sender request retransmitting it.
The previous commit allowed requesting retransmission of announcement_signatures during channel reestablishment. This commit handles such requests.
jkczyz
force-pushed
the
2025-06-channel-reestablish
branch
from
f5c9271 to
719ade4
Compare
Done. |
| @@ -1215,6 +1215,7 @@ pub(super) struct ReestablishResponses { | |||
| pub shutdown_msg: Option<msgs::Shutdown>, | |||
| pub tx_signatures: Option<msgs::TxSignatures>, | |||
| pub tx_abort: Option<msgs::TxAbort>, | |||
| pub implicit_splice_locked: Option<msgs::SpliceLocked>, | |||
There was a problem hiding this comment.
Bleh, let's please note rely on ChannelManager to pipe channel.rs logic around in a loop. We need to be exposing Channel-internal to ChannelManager less, not more.
There was a problem hiding this comment.
May just need to rename this as there is some update logic that needs to be executed in ChannelManager::internal_splice_locked. Will re-visit to see if there's a better alternative.
| } else { | ||
| session.holder_tx_signatures().clone() | ||
| } | ||
| if !session.has_received_commitment_signed() { |
There was a problem hiding this comment.
Isn't this unreachable per the comment in maybe_get_next_funding_txid - "Since we have a signing_session, this implies we've sent an initial commitment_signed"? Or do we switch to interactive signing once we send our CS, before our peer sends their CS (and should we?).
There was a problem hiding this comment.
Or do we switch to interactive signing once we send our CS, before our peer sends their CS (and should we?).
Based on offline discussion with @wpaulino this statement is true.
| // - MUST retransmit its `commitment_signed` for that funding transaction. | ||
| if msg.next_local_commitment_number == next_counterparty_commitment_number { | ||
| // `next_counterparty_commitment_number` is guaranteed to always be the | ||
| // commitment number of the `commitment_signed` message we sent for this |
There was a problem hiding this comment.
This seems wrong re: #4014.
There was a problem hiding this comment.
@wpaulino said he will take a look when reviewing. Seems we'll need to follow-up in the spec meeting to potentially add a retransmit flag to retransmit commitment_signed.
There was a problem hiding this comment.
Could either of you comment on the spec PR as to what you had in mind with retransmit_flags? Might be worth having t-bast think on it before the spec meeting.
| // - if it has already received `tx_signatures` for that funding transaction: | ||
| // - MUST send its `tx_signatures` for that funding transaction. | ||
| if (session.has_received_commitment_signed() && session.holder_sends_tx_signatures_first()) | ||
| || self.context.channel_state.is_their_tx_signatures_sent() |
There was a problem hiding this comment.
Don't we need to check has_received_commitment_signed() even in the case where they already sent their tx signatures? Or can the state machine not advance in that case?
There was a problem hiding this comment.
Based on offline discussion self.context.channel_state.is_their_tx_signatures_sent() implies session.has_received_commitment_signed().
| // on reestablish and tell our peer to just forget about it. | ||
| // Our peer is doing something strange, but it doesn't warrant closing the channel. | ||
| (None, None, Some(msgs::TxAbort { | ||
| // The `next_funding_txid` does not match the latest interactive funding |
There was a problem hiding this comment.
This is the else block for where maybe_get_next_funding_txid returned None, not where the txid doesn't match, so the comment is confusing. But I'm also a bit less clear on the second case of maybe_get_next_funding_txid - if we have an interactive signing session, but they did send their tx signatures (and we received them), we'll end up here, but it seems its much too late to TxAbort.
There was a problem hiding this comment.
From offline discussion we'll drop the else block and use of maybe_get_next_funding_txid in the if block. Instead, we'll get the latter from the signing session.
| }) | ||
| } else if msg.next_local_commitment_number == next_counterparty_commitment_number - 1 { | ||
| // We've made an update so we must have exchanged `tx_signatures`, implying that |
There was a problem hiding this comment.
This might indicate we're sending our CS for the interactive signing session, right?
There was a problem hiding this comment.
Related to #3886 (comment), so will depend on how that is resolved.
There was a problem hiding this comment.
Discussed recent review comments offline. Left some notes about what was decided. @TheBlueMatt @wpaulino Feel free to expand on anything there that isn't clear. I'll address anything that can be resolved and not blocked on discussion at the spec meeting.
| } else { | ||
| session.holder_tx_signatures().clone() | ||
| } | ||
| if !session.has_received_commitment_signed() { |
There was a problem hiding this comment.
Or do we switch to interactive signing once we send our CS, before our peer sends their CS (and should we?).
Based on offline discussion with @wpaulino this statement is true.
| // - MUST retransmit its `commitment_signed` for that funding transaction. | ||
| if msg.next_local_commitment_number == next_counterparty_commitment_number { | ||
| // `next_counterparty_commitment_number` is guaranteed to always be the | ||
| // commitment number of the `commitment_signed` message we sent for this |
There was a problem hiding this comment.
@wpaulino said he will take a look when reviewing. Seems we'll need to follow-up in the spec meeting to potentially add a retransmit flag to retransmit commitment_signed.
| // - if it has already received `tx_signatures` for that funding transaction: | ||
| // - MUST send its `tx_signatures` for that funding transaction. | ||
| if (session.has_received_commitment_signed() && session.holder_sends_tx_signatures_first()) | ||
| || self.context.channel_state.is_their_tx_signatures_sent() |
There was a problem hiding this comment.
Based on offline discussion self.context.channel_state.is_their_tx_signatures_sent() implies session.has_received_commitment_signed().
| // on reestablish and tell our peer to just forget about it. | ||
| // Our peer is doing something strange, but it doesn't warrant closing the channel. | ||
| (None, None, Some(msgs::TxAbort { | ||
| // The `next_funding_txid` does not match the latest interactive funding |
There was a problem hiding this comment.
From offline discussion we'll drop the else block and use of maybe_get_next_funding_txid in the if block. Instead, we'll get the latter from the signing session.
| }) | ||
| } else if msg.next_local_commitment_number == next_counterparty_commitment_number - 1 { | ||
| // We've made an update so we must have exchanged `tx_signatures`, implying that |
There was a problem hiding this comment.
Related to #3886 (comment), so will depend on how that is resolved.
| @@ -1215,6 +1215,7 @@ pub(super) struct ReestablishResponses { | |||
| pub shutdown_msg: Option<msgs::Shutdown>, | |||
| pub tx_signatures: Option<msgs::TxSignatures>, | |||
| pub tx_abort: Option<msgs::TxAbort>, | |||
| pub implicit_splice_locked: Option<msgs::SpliceLocked>, | |||
There was a problem hiding this comment.
May just need to rename this as there is some update logic that needs to be executed in ChannelManager::internal_splice_locked. Will re-visit to see if there's a better alternative.
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
The splicing spec extends the
channel_reestablishmessage with two more TLVs indicating which funding txid the sender has sent/received either explicitly viasplice_lockedor implicitly viachannel_ready. This allows peers to detect if asplice_lockedwas lost during disconnection and must be retransmitted.To this end, the spec updates the
channel_reestablishlogic to support splicing.