Skip to content

Update channel_reestablish for splicing #3886

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Jun 24, 2025

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.

To this end, the spec updates the channel_reestablish logic to support splicing.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jun 24, 2025

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@jkczyz jkczyz assigned jkczyz and unassigned jkczyz Jun 26, 2025
@jkczyz jkczyz force-pushed the 2025-06-channel-reestablish branch 2 times, most recently from 06c0dfc to f000b76 Compare June 27, 2025 17:04
@jkczyz jkczyz marked this pull request as ready for review June 27, 2025 17:12
@jkczyz
Copy link
Contributor Author

jkczyz commented Jun 27, 2025

@wpaulino Ready for review now.

Copy link

codecov bot commented Jun 27, 2025

Codecov Report

Attention: Patch coverage is 60.06826% with 117 lines in your changes missing coverage. Please review.

Project coverage is 89.51%. Comparing base (82519a2) to head (8004c6f).
Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/channel.rs 45.49% 106 Missing and 9 partials ⚠️
lightning/src/ln/channelmanager.rs 92.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3886      +/-   ##
==========================================
+ Coverage   88.83%   89.51%   +0.68%     
==========================================
  Files         166      166              
  Lines      119259   128842    +9583     
  Branches   119259   128842    +9583     
==========================================
+ Hits       105943   115339    +9396     
- Misses      10988    11101     +113     
- Partials     2328     2402      +74     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ldk-reviews-bot
Copy link

🔔 2nd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 3rd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 4th Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@jkczyz jkczyz self-assigned this Jul 3, 2025
@jkczyz jkczyz moved this to Goal: Merge in Weekly Goals Jul 3, 2025
@ldk-reviews-bot
Copy link

🔔 5th Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 6th Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@jkczyz jkczyz force-pushed the 2025-06-channel-reestablish branch from f000b76 to e2ea3bf Compare July 7, 2025 21:02
@jkczyz
Copy link
Contributor Author

jkczyz commented Jul 7, 2025

Rebased.

Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

We may need to double check our incoming channel_ready handling to make sure we can handle receiving one long after channel_ready was already exchanged due to the new logic surrounding your_last_funding_locked_txid.

@@ -10128,10 +10135,52 @@ where
}
}

#[cfg(splicing)]
fn maybe_get_your_last_funding_locked_txid(&self, features: &InitFeatures) -> Option<Txid> {
if !features.supports_splicing() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the TLVs are optional, I don't think we really need to check this and can always set them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm.. if the implementation supports splicing but didn't set the feature bit, won't they understand the TLV?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. This method is only for an outgoing channel_reestablish so they'll just ignore the TLV if they don't support splicing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, dropped the part needing init features.

@ldk-reviews-bot
Copy link

👋 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 jkczyz force-pushed the 2025-06-channel-reestablish branch from e2ea3bf to 69be701 Compare July 9, 2025 00:17
Copy link
Contributor Author

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

We may need to double check our incoming channel_ready handling to make sure we can handle receiving one long after channel_ready was already exchanged due to the new logic surrounding your_last_funding_locked_txid.

ACK. Also need to address https://github.com/lightningdevkit/rust-lightning/pull/3736/files#r2133028859.

@@ -10128,10 +10135,52 @@ where
}
}

#[cfg(splicing)]
fn maybe_get_your_last_funding_locked_txid(&self, features: &InitFeatures) -> Option<Txid> {
if !features.supports_splicing() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm.. if the implementation supports splicing but didn't set the feature bit, won't they understand the TLV?

@jkczyz jkczyz force-pushed the 2025-06-channel-reestablish branch from 69be701 to f7b9785 Compare July 9, 2025 21:47
jkczyz added 3 commits July 9, 2025 16:49
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.
@jkczyz jkczyz force-pushed the 2025-06-channel-reestablish branch from f7b9785 to b0291f4 Compare July 9, 2025 21:49
@jkczyz
Copy link
Contributor Author

jkczyz commented Jul 9, 2025

Also need to address https://github.com/lightningdevkit/rust-lightning/pull/3736/files#r2133028859.

Latest push adds some commits to address this.

@jkczyz jkczyz requested a review from TheBlueMatt July 9, 2025 22:20
@jkczyz
Copy link
Contributor Author

jkczyz commented Jul 9, 2025

We may need to double check our incoming channel_ready handling to make sure we can handle receiving one long after channel_ready was already exchanged due to the new logic surrounding your_last_funding_locked_txid.

Looks like we have some logic below. I think the catch-all case still holds? We should only receive a channel_ready if we didn't set your_last_funding_locked_txid.

// If we reconnected before sending our `channel_ready` they may still resend theirs.
ChannelState::ChannelReady(_) => check_reconnection = true,
_ => return Err(ChannelError::close("Peer sent a channel_ready at a strange time".to_owned())),
}
if check_reconnection {
// They probably disconnected/reconnected and re-sent the channel_ready, which is
// required, or they're sending a fresh SCID alias.
let expected_point =
if self.context.cur_counterparty_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 1 {
// If they haven't ever sent an updated point, the point they send should match
// the current one.
self.context.counterparty_cur_commitment_point
} else if self.context.cur_counterparty_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 2 {
// If we've advanced the commitment number once, the second commitment point is
// at `counterparty_prev_commitment_point`, which is not yet revoked.
debug_assert!(self.context.counterparty_prev_commitment_point.is_some());
self.context.counterparty_prev_commitment_point
} else {
// If they have sent updated points, channel_ready is always supposed to match
// their "first" point, which we re-derive here.
Some(PublicKey::from_secret_key(&self.context.secp_ctx, &SecretKey::from_slice(
&self.context.commitment_secrets.get_secret(INITIAL_COMMITMENT_NUMBER - 1).expect("We should have all prev secrets available")
).expect("We already advanced, so previous secret keys should have been validated already")))
};
if expected_point != Some(msg.next_per_commitment_point) {
return Err(ChannelError::close("Peer sent a reconnect channel_ready with a different point".to_owned()));
}
return Ok(None);
}

// Clear the interactive transaction constructor
self.interactive_tx_constructor.take();
self.interactive_tx_signing_session = Some(signing_session);
self.context.channel_state.set_interactive_signing();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not set the flag above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are other error conditions, which could prevent us from assigning interactive_tx_signing_session to Some.

})
let signature = self.get_initial_counterparty_commitment_signature(funding, logger);
if let Some(signature) = signature {
log_info!(logger, "Generated commitment_signed for peer for channel {}", &self.channel_id());
Copy link
Contributor

Choose a reason for hiding this comment

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

Debug/trace instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is maintaining the existing logging level. Should we reduce it?

@@ -5524,38 +5525,35 @@ where
#[rustfmt::skip]
fn get_initial_commitment_signed<L: Deref>(
&mut self, funding: &FundingScope, logger: &L
) -> Result<msgs::CommitmentSigned, ChannelError>
) -> Option<msgs::CommitmentSigned>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also rename the method to be v2 specific

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a commit renaming this and another similarly named method to make them consistent.

.ok_or_else(|| ChannelError::Close(
(
"Failed to get signatures for new commitment_signed".to_owned(),
ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) },
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be a ProcessingError instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we do the same in funding_tx_constructed? It was already using HolderForceClosed.

if !self.context.channel_state.is_their_tx_signatures_sent()
&& !session.has_received_commitment_signed()
{
return next_local_commitment_number + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

A comment here would be nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the spec requirements. Let me know if there is something more specific to our implementation that we should say. I can't quite recall the exact reason behind this.

// `splice_locked` it has sent:
// - MUST retransmit `splice_locked`.
let sent_splice_txid = self
.maybe_get_my_current_funding_locked(their_features)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it enough to just check pending_splice.sent_funding_txid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC, we may have already promoted the splice.

.or_else(|| {
msg.your_last_funding_locked_txid
.and_then(|last_funding_txid| {
funding_txid.filter(|funding_txid| last_funding_txid != *funding_txid)
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is handling the case where we sent channel_ready on txid A, they processed it, but now we have a channel_ready for txid B that we need to send? Basically the following case in splicing but for channel_ready:

let splice_locked = msg
	// A receiving node:
	//   - if `your_last_funding_locked` is set and it does not match the most recent
	//     `splice_locked` it has sent:
	//     - MUST retransmit `splice_locked`.
	.your_last_funding_locked_txid
	.and_then(|last_funding_txid| {
		sent_splice_txid.filter(|sent_splice_txid| last_funding_txid != *sent_splice_txid)
	})

I don't think this is possible in LDK today, we seem to close the channel once the funding becomes unconfirmed after having sent channel_ready (see do_best_block_updated). I guess it can happen once we support dual funding RBF? One RBF confirms to one node, but another confirms to the other, and now we need to recover via this mechanism.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think you're right. Though your_last_funding_locked_txid is the one they said we sent them in channel_ready. So for dual funding RBF it would need to be a re-org? i.e., we initially sent channel_ready for your_last_funding_locked_txid but while disconnected there was a re-org and another RBF candidate confirmed.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Didn't get all the way through but a few comments.

@@ -10128,10 +10135,52 @@ where
}
}

#[cfg(splicing)]
fn maybe_get_your_last_funding_locked_txid(&self, features: &InitFeatures) -> Option<Txid> {
if !features.supports_splicing() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

// A node:
// - if `option_splice` was negotiated and `your_last_funding_locked` is not
// set in the `channel_reestablish` it received:
// - MUST retransmit `channel_ready`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Honestly I'm not sure what the harm is in duplicatively sending channel_ready/splice_locked? We could cut down on a lot of the code here if we just always sent it, and it seems like that should be fine, does the spec allow for it/require clients ignore extra retransmissions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Immediately after that part of the spec, it states:

  - otherwise:
    - MUST NOT retransmit `channel_ready`, but MAY send `channel_ready` with
      a different `short_channel_id` `alias` field.
  - upon reconnection:
    - MUST ignore any redundant `channel_ready` it receives.

So it seems we may be able to just always re-transmit channel_ready. I don't see anything explicit about splice_locked. FWIW, I broached the subject about cleaning up the language in the spec recently: https://github.com/lightning/bolts/pull/1160/files#r2167803850

@@ -8401,94 +8401,215 @@ where
let is_awaiting_remote_revoke = self.context.channel_state.is_awaiting_remote_revoke();
let next_counterparty_commitment_number = INITIAL_COMMITMENT_NUMBER - self.context.cur_counterparty_commitment_transaction_number + if is_awaiting_remote_revoke { 1 } else { 0 };

let channel_ready = if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.holder_commitment_point.transaction_number() == 1 {
let splicing_negotiated = their_features.supports_splicing();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We haven't "negtotiated" splicing unless we also set the bit, which is controlled by our config, so we need to check that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will we want a config option? Or should we always send init with the feature bit set?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I guess maybe not? Might be worth just calling provided_init_features(config) and checking if its set cause we may decide something else later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, though seems like we should also check their_features given this would otherwise affect channel_ready behavior when they don't support splicing but we do.


if let Some(session) = &self.interactive_tx_signing_session {
if !self.context.channel_state.is_their_tx_signatures_sent()
&& !session.has_received_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.

Gah, so now we have a commitment number for which we only have a commitment tx on one funding context but not the others? Bleh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm... @wpaulino and I were walking through this the other day, but I can't recall exactly why self.holder_commitment_point.transaction_number() would have advanced here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It feels wrong? Like, we should get a new commitment_signed for the new funding at the same index as the current commitment_signed we have for the existing funding, but its also possible the spec wants a new index, that just sucks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the original requirements are:

The sending node:
  - MUST set `next_commitment_number` to the commitment number of the
  next `commitment_signed` it expects to receive.

Which in our code we had represented as:

INITIAL_COMMITMENT_NUMBER - self.holder_commitment_point.transaction_number()

IIUC, we are subtracting from INITIAL_COMMITMENT_NUMBER because transaction_number is initialized by INITIAL_COMMITMENT_NUMBER (counting down) but next_commitment_number is suppose to count up from 0, according to the spec:

`next_commitment_number`: A commitment number is a 48-bit
incrementing counter for each commitment transaction; counters
are independent for each peer in the channel and start at 0.
They're only explicitly relayed to the other node in the case of
re-establishment, otherwise they are implicit.

Then the splicing spec added the last two lines of the following:

  - 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.

So I think the intention of adding 1 here was to get the previous commitment number (i.e., the one for thecommitment_signed we had sent)... are we assuming was it advanced then? This is where I'm a little foggy. But if that's suppose to be the intention, shouldn't we subtract given next_commitment_number is suppose to be counting up?

jkczyz and others added 15 commits July 11, 2025 10:40
When splicing is negotiated, channel_ready must be retransmitted when
your_last_funding_locked is not set. Further, the current logic for
retransmitting channel_ready is only applicable when splicing is not
negotiated.
The ChannelState::NegotiatingFunding assertion check in
ChannelContext::get_initial_commitment_signed will fail when
implementing splicing's channel_reestablish logic. In order to support
it and channel establishment, enter ChannelState::FundingNegotiated
prior to calling the method and update the assertion accordingly.
When ChannelContext::get_initial_commitment_signed is called for V2
channel establishment, any errors should result in closing the channel.
However, in the future, when this is used for splicing it should abort
instead of closing the channel. Move the error construction to the call
sites in anticipation of this.
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.
The channel_reestablish protocol supports retransmitting splice_locked
messages as needed. Add support for doing such when handling
channel_reestablish messages.
The splicing spec updates channel_establishment logic to retransmit
channel_ready or splice_locked for announced channels. Specifically:

- if `my_current_funding_locked` is included:
  - if `announce_channel` is set for this channel:
    - if it has not received `announcement_signatures` for that transaction:
      - MUST retransmit `channel_ready` or `splice_locked` after exchanging `channel_reestablish`.
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.
The channel_reestablish protocol supports retransmitting channel_ready
messages as needed. Add support for doing such when handling
channel_reestablish messages.
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.
Copy link
Contributor Author

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Need to dig into the spec for a couple comments still. Wanted to respond to everything else in the meanwhile.

@@ -8401,94 +8401,215 @@ where
let is_awaiting_remote_revoke = self.context.channel_state.is_awaiting_remote_revoke();
let next_counterparty_commitment_number = INITIAL_COMMITMENT_NUMBER - self.context.cur_counterparty_commitment_transaction_number + if is_awaiting_remote_revoke { 1 } else { 0 };

let channel_ready = if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.holder_commitment_point.transaction_number() == 1 {
let splicing_negotiated = their_features.supports_splicing();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will we want a config option? Or should we always send init with the feature bit set?

// Clear the interactive transaction constructor
self.interactive_tx_constructor.take();
self.interactive_tx_signing_session = Some(signing_session);
self.context.channel_state.set_interactive_signing();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are other error conditions, which could prevent us from assigning interactive_tx_signing_session to Some.

})
let signature = self.get_initial_counterparty_commitment_signature(funding, logger);
if let Some(signature) = signature {
log_info!(logger, "Generated commitment_signed for peer for channel {}", &self.channel_id());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is maintaining the existing logging level. Should we reduce it?

.ok_or_else(|| ChannelError::Close(
(
"Failed to get signatures for new commitment_signed".to_owned(),
ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we do the same in funding_tx_constructed? It was already using HolderForceClosed.


if let Some(session) = &self.interactive_tx_signing_session {
if !self.context.channel_state.is_their_tx_signatures_sent()
&& !session.has_received_commitment_signed()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm... @wpaulino and I were walking through this the other day, but I can't recall exactly why self.holder_commitment_point.transaction_number() would have advanced here.

if !self.context.channel_state.is_their_tx_signatures_sent()
&& !session.has_received_commitment_signed()
{
return next_local_commitment_number + 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the spec requirements. Let me know if there is something more specific to our implementation that we should say. I can't quite recall the exact reason behind this.

// `splice_locked` it has sent:
// - MUST retransmit `splice_locked`.
let sent_splice_txid = self
.maybe_get_my_current_funding_locked(their_features)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC, we may have already promoted the splice.

@@ -10128,10 +10135,52 @@ where
}
}

#[cfg(splicing)]
fn maybe_get_your_last_funding_locked_txid(&self, features: &InitFeatures) -> Option<Txid> {
if !features.supports_splicing() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, dropped the part needing init features.

@@ -5524,38 +5525,35 @@ where
#[rustfmt::skip]
fn get_initial_commitment_signed<L: Deref>(
&mut self, funding: &FundingScope, logger: &L
) -> Result<msgs::CommitmentSigned, ChannelError>
) -> Option<msgs::CommitmentSigned>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a commit renaming this and another similarly named method to make them consistent.

@jkczyz jkczyz force-pushed the 2025-06-channel-reestablish branch from b0291f4 to 8004c6f Compare July 11, 2025 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Goal: Merge
Development

Successfully merging this pull request may close these issues.

4 participants