DynComms [1/n]: Implement Quiescence Protocol #8270
Conversation
ProofOfKeags
force-pushed
the
feature/stfu
branch
3 times, most recently
from
d1aa5e5
to
9a6d107
Compare
ProofOfKeags
force-pushed
the
feature/stfu
branch
from
bc147f4
to
7a78e3d
Compare
There was a problem hiding this comment.
Pretty straight forward diff! Missing some context from the other PR, and also still need to catch up w/ the latest state of the spec.
The main thing I think we need to zoom in on re unit tests is: the assumption that if we don't ACK a new settle/fail from the mailbox, then upon reconnection, all those items are retransmitted once again. If this is the case, then we can just force a disconnection after the stfu cycle is complete (see comment there about needing to send a special internal error to make that happen).
One other question I have is: is it the expected flow that a disconnect restores the channel lifecycle back to "active"? Or do we really want another protocol level message here so we can go back to normal w/o needing to re-create the peer connection?
|
|
||
| // Initiator is a byte that identifies whether we are the initiator of | ||
| // this process. | ||
| Initiator bool |
There was a problem hiding this comment.
Would assume given the existing connection context both sides already know how's the initiator when things are being sent?
There was a problem hiding this comment.
If you look at the spec, it is included as a means of determining who holds the session.
htlcswitch/quiescer.go
Outdated
| if q.received { | ||
| return false, | ||
| fmt.Errorf( | ||
| "stfu already received for channel %s", |
There was a problem hiding this comment.
If we promote these to either an error variable or a simple error struct, then unit tests can use errors.As and errors.Is, etc.
The caller/driver of the state machine would then also be able to use a switch to handle the error case.
| @@ -5340,6 +5340,39 @@ func (lc *LightningChannel) NumLocalUpdatesPendingOnRemote() uint64 { | |||
| return lc.localUpdateLog.logIndex - lastRemoteCommit.ourMessageIndex | |||
| } | |||
|
|
|||
| // NumLocalUpdatesPendingOnLocal returns the number of local updates that still | |||
| // need to be applied to the local commitment tx. | |||
| func (lc *LightningChannel) NumLocalUpdatesPendingOnLocal() uint64 { | |||
There was a problem hiding this comment.
Isn't it enough to simply know if we owe a commitment or if they do? OweCommitment and NeedCommitment seem to cover those cases. Still getting through the diff however.
There was a problem hiding this comment.
Unfortunately, no. See this. There is still some instability in spec discussion and so the details of this PR aren't ultra high priority, but in the interest of not wasting time I've opted to implement it to the best of my understanding knowing full well that we may wish to throw some of this away.
I would prefer that this code change wasn't needed, however, as written, the best interpretation I have of the spec requires this.
htlcswitch/link.go
Outdated
| if finished { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
I would've thought we'd have some logic here to signal to the switch we're not eligible to forward. Maybe that's later on, still getting through the diff.
There was a problem hiding this comment.
This is handled implicitly in the EligibleToForward query.
| @@ -638,7 +638,8 @@ func (l *channelLink) EligibleToForward() bool { | |||
| return l.channel.RemoteNextRevocation() != nil && | |||
| l.ShortChanID() != hop.Source && | |||
| l.isReestablished() && | |||
| !l.IsDraining(Outgoing) | |||
| !l.IsDraining(Outgoing) && | |||
| l.quiescer.canSendUpdates() | |||
There was a problem hiding this comment.
👍
Seems like IsDraining could just call canSendUpdates within the impl? FWIW, missing some partial context from that other PR.
There was a problem hiding this comment.
There is some slight nuance here where adds are a subset of updates. Intuitively, "draining" means we can't send adds, but we can send removes (fulfill/fail) and fee updates. canSendUpdates refers to all channel updates. If we change IsDraining (now IsFlushing) to CanSendAdds then there are some ways we can consolidate this. I generally try and name things to maximize future readers' ability to understand.
| @@ -1652,6 +1653,17 @@ func (l *channelLink) handleDownstreamUpdateAdd(pkt *htlcPacket) error { | |||
| ) | |||
| } | |||
|
|
|||
| // If the channel is quiescent then we issue a temporary channel failure | |||
| // and bounce it. | |||
| if !l.quiescer.canSendUpdates() { | |||
There was a problem hiding this comment.
Same above re folding into IsDraining.
There was a problem hiding this comment.
I wonder also if we can fashion things s.t underneath everything uses the quiescer, but depending on the input signal/event, it may or may not send/expect the stfu message.
There was a problem hiding this comment.
I think your other idea of having a more flexible TrafficControl interface is the better route. Quiescence is all or nothing. Flushing is about preventing adds only. If you want to consolidate we should choose a flexible core and then we can add some convenience language over the top of it.
| @@ -1738,6 +1750,14 @@ func (l *channelLink) handleDownstreamPkt(pkt *htlcPacket) { | |||
| _ = l.handleDownstreamUpdateAdd(pkt) | |||
|
|
|||
| case *lnwire.UpdateFulfillHTLC: | |||
| if !l.quiescer.canSendUpdates() { | |||
There was a problem hiding this comment.
Hmm, this is one of those "never should happen" scenarios right? I guess can happen given a slim concurrency window where we mark state as flushing, but the items are already in the mailbox.
Def need to test this out more, but my understanding is that these are un-ack'd, so they'll sit in the mailbox, to eventually be retransmitted once the connection recycles.
There was a problem hiding this comment.
Yes, but I believe every subsystem needs to be responsible for ensuring its own consistency so it is included for completeness. As we tease these systems apart a bit better, I think we can get more clear about the caller's responsibilities.
This code is a living system and I'd rather have some redundant checks than have someone change something else and break a fundamental guarantee in a place they didn't touch.
htlcswitch/link.go
Outdated
| @@ -2035,6 +2049,20 @@ func (l *channelLink) handleUpstreamMsg(msg lnwire.Message) { | |||
| "assigning index: %v", msg.PaymentHash[:], index) | |||
|
|
|||
| case *lnwire.UpdateFulfillHTLC: | |||
| if !l.quiescer.canRecvUpdates() { | |||
| l.fail( | |||
There was a problem hiding this comment.
Hmm, I think we'd still want them to be able to send settle+fail message, I might be misunderstanding the current spec draft though, will double check there.
There was a problem hiding this comment.
From a game theoretic POV, maybe. Implementing the spec as-written, though, requires that I treat this scenario as a protocol violation.
ProofOfKeags
force-pushed
the
feature/stfu
branch
from
7a78e3d
to
6da6398
Compare
|
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe implementation of the Quiescence ( Changes
Assessment against linked issues
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
ProofOfKeags
force-pushed
the
feature/stfu
branch
2 times, most recently
from
5e41a47
to
408ba29
Compare
ProofOfKeags
force-pushed
the
feature/stfu
branch
15 times, most recently
from
70613b5
to
29e5c42
Compare
In this commit we defer processRemoteAdds using a new mechanism on the quiescer where we capture a closure that needs to be run. We do this because we need to avoid the scenario where we send back immediate resolutions to the newly added HTLCs when quiescent as it is a protocol violation. It is not enough for us to simply defer sending the messages since the purpose of quiescence itself is to have well-defined and agreed upon channel state. If, for whatever reason, the node (or connection) is restarted between when these hooks are captured and when they are ultimately run, they will be resolved by the resolveFwdPkgs logic when the link comes back up. In a future commit we will explicitly call the quiescer's resume method when it is OK for htlc traffic to commence.
ProofOfKeags
force-pushed
the
feature/stfu
branch
from
a62d087
to
01e2833
Compare
NOTE: This PR is part of a series implementing Dynamic Commitments. This PR does not directly implement any Dynamic Commitments specific logic but quiescence is a protocol gadget that is a prerequisite for Dynamic Commitments.
Change Description
This change implements the behavior described in the Quiescence Specification. It allows us to respond to our peer's request to quiesce the channel as well as implementing some
ChannelUpdateHandleroperations that allow us to initiate the process ourselves.Some commits towards the end of the series have been included to allow us to initiate quiescence via RPC for the purposes of integration and interop testing. These commits should be removed before this PR is considered ready to merge. They will ultimately be replaced by RPCs that initiate the Dynamic Commitments protocol itself which will implicitly initiate quiescence as part of its process.
NOTE: This PR does NOT include a mechanism for timing out a quiescence session. This means that if we have an intentionally or unintentionally uncooperative peer, the channel will remain quiesced indefinitely. This is not desirable and will either be addressed in later commits in this PR or into a subsequent PR. However, this PR is submitted without it as it is "complete" in its own right.
Steps to Test
Steps for reviewers to follow to test the change.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.