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.

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

Add support for using last_funding_locked tlv in channel_reestablish #3007

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add support for using last_funding_locked tlv in channel_reestablish #3007

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c1ebbbc
Add support for using last_funding_locked tlv in channel_reestablish
remyers Feb 4, 2025
1726b49
Fix accessors for last_funding_locked tlv
remyers Feb 18, 2025
3facc4a
Fix to set the remote funding status to Locked after receiving channe…
remyers Feb 19, 2025
e8fadf4
Add var to prevent a splice_locked loop after channel_reestablish
remyers Feb 24, 2025
c2ad77d
Add Lightning message codec tests for new tlvs
remyers Feb 25, 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
11 changes: 8 additions & 3 deletions eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,8 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with
var announcementSigsStash = Map.empty[RealShortChannelId, AnnouncementSignatures]
// we record the announcement_signatures messages we already sent to avoid unnecessary retransmission
var announcementSigsSent = Set.empty[RealShortChannelId]
// we keep track of the splice_locked we sent after channel_reestablish to avoid sending it again
private var spliceLockedSentAfterReestablish: Option[RealShortChannelId] = None
Comment on lines +225 to +226
Copy link
Member

Choose a reason for hiding this comment

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

We will need a Map of the splice_locked messages we sent to keep track of our local nonces for taproot, so this is a good opportunity to do it right now, otherwise we'll need to change this code anyway...

Can you turn this into private var spliceLockedSent = Map.empty[RealShortChannelId, SpliceLocked]? And implement a mecanism to prune the oldest RealShortChannelIds when we exceed 10 elements, like we do for announcement signatures?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't we use TxId as the key instead of RealShortChannelId because we might have sent SpliceLocked for a zero conf channel?

Copy link
Member

Choose a reason for hiding this comment

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

Right, that's probably what we need here!


private def trimAnnouncementSigsStashIfNeeded(): Unit = {
if (announcementSigsStash.size >= 10) {
Expand Down Expand Up @@ -1386,12 +1388,13 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with
// If we already have a signed channel announcement for this commitment, then we are receiving splice_locked
// again after a reconnection and must retransmit our splice_locked and new announcement_signatures. Nodes
// retransmit splice_locked after a reconnection when they have received splice_locked but NOT matching signatures
// before the last disconnect. If announcement_signatures have been sent since a reconnect, then do not
// before the last disconnect. If a matching splice_locked has already been sent since reconnecting, then do not
// retransmit splice_locked to avoid a loop.
// NB: It is important both nodes retransmit splice_locked after reconnecting to ensure new Taproot nonces
// are exchanged for channel announcements.
val spliceLocked_opt = d.lastAnnouncement_opt.collect {
case ann if announcementSigsSent.isEmpty && commitment.shortChannelId_opt.contains(ann.shortChannelId) =>
case ann if !spliceLockedSentAfterReestablish.contains(ann.shortChannelId) && commitment.shortChannelId_opt.contains(ann.shortChannelId) =>
spliceLockedSentAfterReestablish = Some(ann.shortChannelId)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think this is necessary? Can splice_locked be sent again if it has already been sent? I thought it was safer to set spliceLockedSentAfterReestablish but we could take it out and the loop after channel_reestablish will still not occur.

Copy link
Member

Choose a reason for hiding this comment

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

Can splice_locked be sent again if it has already been sent?

Yes it can, this is harmless. I think we should simplify this once you've introduced the spliceLockedSent map as requested in my other comment to do the following when we receive splice_locked:

  • check the spliceLockedSent map: if it contains this scid, we don't need to send anything (we may have already re-sent splice_locked on reestablish)
  • otherwise, check if the local funding status is Locked, and if it is, retransmit our splice_locked
  • that means we may unnecessarily retransmit (if their channel_reestablish said that they already received it), but it's harmless, simplifies the logic (no need to look at the announcement at all), and shouldn't happen often

SpliceLocked(d.channelId, msg.fundingTxId)
}
// If the commitment is confirmed, we were waiting to receive the remote splice_locked before sending our announcement_signatures.
Expand Down Expand Up @@ -2400,7 +2403,7 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with

// Retransmit splice_locked (must come *after* potentially retransmitting tx_signatures):
// 1) If they did not receive our last splice_locked;
// 2) or the last splice_locked they received is different from our what we sent;
// 2) or the last splice_locked they received is different from what we sent;
// 3) or (public channels only) we have not received their announcement_signatures for our latest locked commitment.
// NB: there is a key difference between channel_ready and splice_locked:
// - channel_ready: a non-zero commitment index implies that both sides have seen the channel_ready
Expand All @@ -2412,6 +2415,7 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with
.collect { case c if !channelReestablish.yourLastFundingLocked_opt.contains(c.fundingTxId) ||
(commitments1.announceChannel && d.lastAnnouncement_opt.forall(ann => !c.shortChannelId_opt.contains(ann.shortChannelId))) =>
log.debug("re-sending splice_locked for fundingTxId={}", c.fundingTxId)
spliceLockedSentAfterReestablish = c.shortChannelId_opt
SpliceLocked(d.channelId, c.fundingTxId)
}
sendQueue = sendQueue ++ spliceLocked
Expand Down Expand Up @@ -2901,6 +2905,7 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with
sigStash = Nil
announcementSigsStash = Map.empty
announcementSigsSent = Set.empty
spliceLockedSentAfterReestablish = None
}

/*
Expand Down