-
Notifications
You must be signed in to change notification settings - Fork 268
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
Add support for using last_funding_locked tlv in channel_reestablish #3007
base: master
Are you sure you want to change the base?
Changes from 4 commits
c1ebbbc
1726b49
3facc4a
e8fadf4
c2ad77d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We will need a Can you turn this into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||||||||||||||||
|
@@ -775,10 +777,14 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with | |||||||||||||||
|
||||||||||||||||
case Event(c: CurrentFeerates.BitcoinCore, d: DATA_NORMAL) => handleCurrentFeerate(c, d) | ||||||||||||||||
|
||||||||||||||||
case Event(_: ChannelReady, _: DATA_NORMAL) => | ||||||||||||||||
// This happens on reconnection, because channel_ready is sent again if the channel hasn't been used yet, | ||||||||||||||||
// otherwise we cannot be sure that it was correctly received before disconnecting. | ||||||||||||||||
stay() | ||||||||||||||||
case Event(_: ChannelReady, d: DATA_NORMAL) => | ||||||||||||||||
// After a reconnection, if the channel hasn't been used yet, our peer cannot be sure we received their channel_ready | ||||||||||||||||
// so they will resend it. Their remote funding status must also be set to Locked if it wasn't already. | ||||||||||||||||
// NB: Their remote funding status will be stored when the commitment is next updated, or channel_ready will | ||||||||||||||||
// be sent again if a reconnection occurs first. | ||||||||||||||||
stay() using d.copy(commitments = d.commitments.copy(active = d.commitments.active.collect { | ||||||||||||||||
case c if c.fundingTxIndex == 0 => c.copy(remoteFundingStatus = RemoteFundingStatus.Locked) | ||||||||||||||||
})) | ||||||||||||||||
Comment on lines
+785
to
+787
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this point using
Suggested change
|
||||||||||||||||
|
||||||||||||||||
// Channels are publicly announced if both parties want it: we ignore this message if we don't want to announce the channel. | ||||||||||||||||
case Event(remoteAnnSigs: AnnouncementSignatures, d: DATA_NORMAL) if d.commitments.announceChannel => | ||||||||||||||||
|
@@ -1379,6 +1385,18 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with | |||||||||||||||
case Event(msg: SpliceLocked, d: DATA_NORMAL) => | ||||||||||||||||
d.commitments.updateRemoteFundingStatus(msg.fundingTxId, d.lastAnnouncedFundingTxId_opt) match { | ||||||||||||||||
case Right((commitments1, commitment)) => | ||||||||||||||||
// 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 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 !spliceLockedSentAfterReestablish.contains(ann.shortChannelId) && commitment.shortChannelId_opt.contains(ann.shortChannelId) => | ||||||||||||||||
spliceLockedSentAfterReestablish = Some(ann.shortChannelId) | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think this is necessary? Can There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes it can, this is harmless. I think we should simplify this once you've introduced the
|
||||||||||||||||
SpliceLocked(d.channelId, msg.fundingTxId) | ||||||||||||||||
} | ||||||||||||||||
// If the commitment is confirmed, we were waiting to receive the remote splice_locked before sending our announcement_signatures. | ||||||||||||||||
val localAnnSigs_opt = if (d.commitments.announceChannel) commitment.signAnnouncement(nodeParams, commitments1.params) else None | ||||||||||||||||
localAnnSigs_opt match { | ||||||||||||||||
|
@@ -1391,7 +1409,7 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with | |||||||||||||||
} | ||||||||||||||||
maybeEmitEventsPostSplice(d.aliases, d.commitments, commitments1, d.lastAnnouncement_opt) | ||||||||||||||||
maybeUpdateMaxHtlcAmount(d.channelUpdate.htlcMaximumMsat, commitments1) | ||||||||||||||||
stay() using d.copy(commitments = commitments1) storing() sending localAnnSigs_opt.toSeq | ||||||||||||||||
stay() using d.copy(commitments = commitments1) storing() sending spliceLocked_opt.toSeq ++ localAnnSigs_opt.toSeq | ||||||||||||||||
case Left(_) => stay() | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
|
@@ -2227,13 +2245,19 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with | |||||||||||||||
} | ||||||||||||||||
case _ => Set.empty | ||||||||||||||||
} | ||||||||||||||||
val lastFundingLockedTlvs: Set[ChannelReestablishTlv] = | ||||||||||||||||
if (d.commitments.params.remoteParams.initFeatures.hasFeature(Features.SplicePrototype) && d.commitments.params.localParams.initFeatures.hasFeature(Features.SplicePrototype)) { | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to gate this on It's only helpful to look at By the way, we are missing |
||||||||||||||||
d.commitments.lastLocalLocked_opt.map(c => ChannelReestablishTlv.MyCurrentFundingLockedTlv(c.fundingTxId)).toSet ++ | ||||||||||||||||
d.commitments.lastRemoteLocked_opt.map(c => ChannelReestablishTlv.YourLastFundingLockedTlv(c.fundingTxId)).toSet | ||||||||||||||||
} else Set.empty | ||||||||||||||||
|
||||||||||||||||
val channelReestablish = ChannelReestablish( | ||||||||||||||||
channelId = d.channelId, | ||||||||||||||||
nextLocalCommitmentNumber = d.commitments.localCommitIndex + 1, | ||||||||||||||||
nextRemoteRevocationNumber = d.commitments.remoteCommitIndex, | ||||||||||||||||
yourLastPerCommitmentSecret = PrivateKey(yourLastPerCommitmentSecret), | ||||||||||||||||
myCurrentPerCommitmentPoint = myCurrentPerCommitmentPoint, | ||||||||||||||||
tlvStream = TlvStream(rbfTlv) | ||||||||||||||||
tlvStream = TlvStream(rbfTlv ++ lastFundingLockedTlvs) | ||||||||||||||||
) | ||||||||||||||||
// we update local/remote connection-local global/local features, we don't persist it right now | ||||||||||||||||
val d1 = Helpers.updateFeatures(d, localInit, remoteInit) | ||||||||||||||||
|
@@ -2371,34 +2395,46 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with | |||||||||||||||
case None => d.spliceStatus | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
// re-send splice_locked (must come *after* potentially retransmitting tx_signatures) | ||||||||||||||||
// NB: there is a key difference between channel_ready and splice_confirmed: | ||||||||||||||||
// Prune previous funding transactions and RBF attempts if we already sent splice_locked for the last funding | ||||||||||||||||
// transaction confirmed by our counterparty; we either missed their splice_locked or it confirmed while disconnected. | ||||||||||||||||
val commitments1: Commitments = channelReestablish.myCurrentFundingLocked_opt.flatMap(myCurrentFundingLocked => | ||||||||||||||||
d.commitments.updateRemoteFundingStatus(myCurrentFundingLocked, d.lastAnnouncedFundingTxId_opt).toOption.map(_._1)) | ||||||||||||||||
.getOrElse(d.commitments) | ||||||||||||||||
Comment on lines
+2398
to
+2402
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find it dangerous to have multiple places where we update the commitments and rely on numbering variable names, it requires scanning the whole block of code to ensure we're using the right version...can you instead move this change below, where we do the
|
||||||||||||||||
|
||||||||||||||||
// 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 what we sent; | ||||||||||||||||
// 3) or (public channels only) we have not received their announcement_signatures for our latest locked commitment. | ||||||||||||||||
Comment on lines
+2405
to
+2407
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is quite confusing, because the second statement is contained in the first one: if the last
Suggested change
|
||||||||||||||||
// 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 | ||||||||||||||||
// - splice_confirmed: the commitment index can be updated as long as it is compatible with all splices, so | ||||||||||||||||
// we must keep sending our most recent splice_locked at each reconnection | ||||||||||||||||
val spliceLocked = d.commitments.active | ||||||||||||||||
.filter(c => c.fundingTxIndex > 0) // only consider splice txs | ||||||||||||||||
.collectFirst { case c if c.localFundingStatus.isInstanceOf[LocalFundingStatus.Locked] => | ||||||||||||||||
// - splice_locked: the commitment index can be updated as long as it is compatible with all splices | ||||||||||||||||
// We must send our most recent splice_locked until our counterparty receives it and, for a public | ||||||||||||||||
// channel, also sends their announcement signatures. | ||||||||||||||||
val spliceLocked = commitments1.lastLocalLocked_opt | ||||||||||||||||
.filter(_.fundingTxIndex > 0) // only consider splice txs | ||||||||||||||||
.collect { case c if !channelReestablish.yourLastFundingLocked_opt.contains(c.fundingTxId) || | ||||||||||||||||
(commitments1.announceChannel && d.lastAnnouncement_opt.forall(ann => !c.shortChannelId_opt.contains(ann.shortChannelId))) => | ||||||||||||||||
Comment on lines
+2415
to
+2416
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those conditions are quite hard to parse, it's helpful to decompose them with comments: val spliceLocked = commitments1.lastLocalLocked_opt match {
case None => None
// We only send splice_locked for splice transactions.
case Some(c) if c.fundingTxIndex == 0 => None
case Some(c) =>
// If our peer has not received our splice_locked, we retransmit it.
val notReceivedByRemote = !channelReestablish.yourLastFundingLocked_opt.contains(c.fundingTxId)
// If this is a public channel and we haven't announced the splice, we retransmit our splice_locked and
// will exchange announcement_signatures afterwards.
val notAnnouncedYet = commitments1.announceChannel && d.lastAnnouncement_opt.forall(ann => !c.shortChannelId_opt.contains(ann.shortChannelId))
if (notReceivedByRemote || notAnnouncedYet) {
log.debug("re-sending splice_locked for fundingTxId={}", c.fundingTxId)
spliceLockedSentAfterReestablish = c.shortChannelId_opt
Some(SpliceLocked(d.channelId, c.fundingTxId))
} else {
None
}
} |
||||||||||||||||
log.debug("re-sending splice_locked for fundingTxId={}", c.fundingTxId) | ||||||||||||||||
spliceLockedSentAfterReestablish = c.shortChannelId_opt | ||||||||||||||||
SpliceLocked(d.channelId, c.fundingTxId) | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
sendQueue = sendQueue ++ spliceLocked | ||||||||||||||||
|
||||||||||||||||
// we may need to retransmit updates and/or commit_sig and/or revocation | ||||||||||||||||
sendQueue = sendQueue ++ syncSuccess.retransmit | ||||||||||||||||
|
||||||||||||||||
// then we clean up unsigned updates | ||||||||||||||||
val commitments1 = d.commitments.discardUnsignedUpdates() | ||||||||||||||||
val commitments2 = commitments1.discardUnsignedUpdates() | ||||||||||||||||
|
||||||||||||||||
commitments1.remoteNextCommitInfo match { | ||||||||||||||||
commitments2.remoteNextCommitInfo match { | ||||||||||||||||
case Left(_) => | ||||||||||||||||
// we expect them to (re-)send the revocation immediately | ||||||||||||||||
startSingleTimer(RevocationTimeout.toString, RevocationTimeout(commitments1.remoteCommitIndex, peer), nodeParams.channelConf.revocationTimeout) | ||||||||||||||||
startSingleTimer(RevocationTimeout.toString, RevocationTimeout(commitments2.remoteCommitIndex, peer), nodeParams.channelConf.revocationTimeout) | ||||||||||||||||
case _ => () | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
// do I have something to sign? | ||||||||||||||||
if (commitments1.changes.localHasChanges) { | ||||||||||||||||
if (commitments2.changes.localHasChanges) { | ||||||||||||||||
self ! CMD_SIGN() | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
|
@@ -2446,11 +2482,11 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with | |||||||||||||||
|
||||||||||||||||
// We tell the peer that the channel is ready to process payments that may be queued. | ||||||||||||||||
if (!shutdownInProgress) { | ||||||||||||||||
val fundingTxIndex = commitments1.active.map(_.fundingTxIndex).min | ||||||||||||||||
val fundingTxIndex = commitments2.active.map(_.fundingTxIndex).min | ||||||||||||||||
peer ! ChannelReadyForPayments(self, remoteNodeId, d.channelId, fundingTxIndex) | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
goto(NORMAL) using d.copy(commitments = commitments1, spliceStatus = spliceStatus1) sending sendQueue | ||||||||||||||||
goto(NORMAL) using d.copy(commitments = commitments2, spliceStatus = spliceStatus1) sending sendQueue | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
case Event(c: CMD_ADD_HTLC, d: DATA_NORMAL) => handleAddDisconnected(c, d) | ||||||||||||||||
|
@@ -2869,6 +2905,7 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with | |||||||||||||||
sigStash = Nil | ||||||||||||||||
announcementSigsStash = Map.empty | ||||||||||||||||
announcementSigsSent = Set.empty | ||||||||||||||||
spliceLockedSentAfterReestablish = None | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
/* | ||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -144,8 +144,10 @@ trait CommonFundingHandlers extends CommonHandlers { | |||||||||||||||||||||||
// We need to periodically re-send channel updates, otherwise channel will be considered stale and get pruned by network. | ||||||||||||||||||||||||
context.system.scheduler.scheduleWithFixedDelay(initialDelay = REFRESH_CHANNEL_UPDATE_INTERVAL, delay = REFRESH_CHANNEL_UPDATE_INTERVAL, receiver = self, message = BroadcastChannelUpdate(PeriodicRefresh)) | ||||||||||||||||||||||||
val commitments1 = commitments.modify(_.remoteNextCommitInfo).setTo(Right(channelReady.nextPerCommitmentPoint)) | ||||||||||||||||||||||||
// Set the remote status for all initial funding commitments to Locked. If there are RBF attempts, only one will be confirmed locally. | ||||||||||||||||||||||||
val commitments2 = commitments1.copy(active = commitments.active.collect { case c if c.fundingTxIndex == 0 => c.copy(remoteFundingStatus = RemoteFundingStatus.Locked) }) | ||||||||||||||||||||||||
Comment on lines
146
to
+148
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
|
||||||||||||||||||||||||
peer ! ChannelReadyForPayments(self, remoteNodeId, commitments.channelId, fundingTxIndex = 0) | ||||||||||||||||||||||||
DATA_NORMAL(commitments1, aliases1, None, initialChannelUpdate, None, None, None, SpliceStatus.NoSplice) | ||||||||||||||||||||||||
DATA_NORMAL(commitments2, aliases1, None, initialChannelUpdate, None, None, None, SpliceStatus.NoSplice) | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
def delayEarlyAnnouncementSigs(remoteAnnSigs: AnnouncementSignatures): Unit = { | ||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this with the other
val
s at the beginning of the class definition?