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.

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

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

Update channel_reestablish for splicing #3886

Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
bd294b3
Add features for splicing
jkczyz Jun 19, 2025
99affd3
Add funding_locked_txid TLVs to channel_reestablish
jkczyz Jun 5, 2025
fa4c9c9
Set funding_locked_txid TLVs in channel_reestablish
jkczyz Jun 8, 2025
2c790d0
f - don't check splice feature bit
jkczyz Jul 11, 2025
5789c8f
Update channel_reestablish logic for channel_ready
jkczyz Jun 19, 2025
ab5ccd1
Enter FundingNegotiated state after constructing funding tx
jkczyz Jul 9, 2025
cb31e89
Return an Option from ChannelContext::get_initial_commitment_signed
jkczyz Jul 9, 2025
cc4152f
f - TODO(splicing): Support async signing
jkczyz Jul 11, 2025
4a0e343
Update next_funding_txid logic for channel_reestablish
jkczyz Jun 23, 2025
5123cc8
f - clean up spec requirement
jkczyz Jul 11, 2025
5619b37
Use consistent initial commitment_signed naming
jkczyz Jul 11, 2025
feffea4
Update next_commitment_number logic for channel_reestablish
jkczyz Jun 23, 2025
1047dcd
f - add comment with spec requirement
jkczyz Jul 11, 2025
71ffb50
Send splice_locked on channel_reestablish
jkczyz Jun 20, 2025
b647280
Retransmit channel_ready / splice_locked awaiting announcement_sigs
jkczyz Jun 25, 2025
a907398
Clear announcement_sigs on FundingScope promotion
jkczyz Jun 25, 2025
15eef7b
Send channel_ready on channel_reestablish
jkczyz Jun 27, 2025
8004c6f
Handle implicit splice_locked during channel_reestablish
jkczyz Jun 27, 2025
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
Next Next commit
Update next_commitment_number logic for channel_reestablish 
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.
  • Loading branch information
@jkczyz
jkczyz committed Jul 11, 2025
commit feffea4356793b6b92ba72c669bd43a1ac67dda0
17 changes: 16 additions & 1 deletion lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10141,6 +10141,21 @@ where
self.sign_channel_announcement(node_signer, announcement).ok()
}

fn get_next_local_commitment_number(&self) -> u64 {
let next_local_commitment_number =
INITIAL_COMMITMENT_NUMBER - self.holder_commitment_point.transaction_number();

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jkczyz yeah I think we just assumed this was still decrementing from INITIAL_COMMITMENT_NUMBER as opposed to counting from 0, so we just need to subtract 1 instead.

We subtract 1 because holder_commitment_point always points to the next commitment number we expect an update for, and splicing requires that we send/receive an initial commitment_signed for the current commitment number. However, we're not taking this into account when sending/receiving that initial commitment_signed, so that needs to be fixed separately.

{
return next_local_commitment_number + 1;
}
}

next_local_commitment_number
}

#[rustfmt::skip]
fn maybe_get_next_funding_txid(&self) -> Option<Txid> {
// If we've sent `commtiment_signed` for an interactively constructed transaction
Expand Down Expand Up @@ -10229,7 +10244,7 @@ where

// next_local_commitment_number is the next commitment_signed number we expect to
// receive (indicating if they need to resend one that we missed).
next_local_commitment_number: INITIAL_COMMITMENT_NUMBER - self.holder_commitment_point.transaction_number(),
next_local_commitment_number: self.get_next_local_commitment_number(),
// We have to set next_remote_commitment_number to the next revoke_and_ack we expect to
// receive, however we track it by the next commitment number for a remote transaction
// (which is one further, as they always revoke previous commitment transaction, not
Expand Down