-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
multi: resend shutdown on reestablish #8464
multi: resend shutdown on reestablish #8464
Conversation
Important Auto Review SkippedAuto reviews are limited to the following labels: llm-review. Please add one of these labels to enable auto reviews. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the 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 (
|
@Crypt-iQ & @ProofOfKeags - pinging here for an Approach ACK. |
535d7a8
to
e1de181
Compare
Approach ACK 👍 |
0b404d0
to
6f3c0b0
Compare
PTAL @Crypt-iQ & @ProofOfKeags 🙏 |
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.
Overall I think this is good. My main concern is how we square some of the choices made here with the v2 close so I've summoned @Roasbeef for that. I personally think this has priority even if we can't square it completely so I don't know that even if there are design conflicts that we would change how this works.
A lot of the problems here are related to the fact that we use the brontide to manage the integration of channel subcomponents which we shouldn't really do but that's way way way beyond the scope of what we need to address in this PR.
script := []byte{1, 3, 4, 5} | ||
|
||
// Create a ShutdownInfo struct. | ||
shutdownInfo := NewShutdownInfo(script, locallyInitiated) |
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.
The fact that this works implies we need to fix our type system. I figured we'd have to newtype wrap the byte array or use a constructor that lifts it into the DeliveryAddress. The test overall seems fine but I don't think you should be able to construct a DeliveryAddress that looks like this.
Fixing it is better done in a diff PR though.
htlcswitch/link.go
Outdated
// ShutdownMsg is an optional value that is set if, at the time of | ||
// the link being started, persisted shutdown info was found for the | ||
// channel. This value being set means that we previously sent a | ||
// Shutdown message to our peer, and so we should do so again on | ||
// re-establish and should not allow anymore HTLC adds on the outgoing | ||
// direction of the link. | ||
ShutdownMsg fn.Option[lnwire.Shutdown] |
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.
If we go this route, where we're willing to let the link be shutdown aware, we probably ought to have a Shutdown method on the link too and then we can ditch all of the lifecycle hooks and stuff. When I was working on #8167, @Roasbeef was averse to doing this type of thing. I happen to side with you here that shutdown is ultimately part of the link's lifecycle so this should be OK but I do think we need an explicit ACK from @Roasbeef
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.
However, the choice to make this a static field does have implications in regards to the coop close v2, since Shutdown may be transmitted multiple times in an attempt to reset protocol state back to the beginning of shutdown negotiations.
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.
is ultimately part of the link's lifecycle
Yeah. I think the Shutdown message is very much apart of the link lifecycle. The co-op close messages are not & so the link does not need to be aware of those at all but the Shutdown message gives the link an indication that it should wind down. Especially since the link is expected to perform a certain set of actions once the shutdown message is sent .
does have implications in regards to the coop close v2,
shouldnt be a problem for 2 reasons: 1) even if we do transmit Shutdown multiple times - it MUST be the exact same message (with identical contents) and 2) I think any co-op close stuff (and reset of negotiations etc) wont be handled in the link.
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.
Yeah I think you're right here. As a side note this lends some validity to the idea that some messages may be processed by multiple subsystems too in #8434
it MUST be the exact same message
I wonder what happens if it's not.. 🤔
I think any co-op close stuff (and reset of negotiations etc) wont be handled in the link.
Definitely.
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.
I wonder what happens if it's not..
It might give the other side reason to just force close since to them we are doing something fishy. Not sure if any impl actually does bork out here though
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.
Ah - I just realised what could happen:
- we send Shutdown to the peer with delivery address A
- they persist that delivery address
- we re-send shutdown with a new delivery address B
- remote side swallows & ignores the message since they have already received a shutdown from us which was meant to be identical to the first one so they dont even bother parsing it fully.
- The 2 nodes start fee negotiation. But this will fail as soon as the remote side gets a signature from us that doesnt make sense given the transaction they have constructed based on delivery address A
- remote side does force close instead
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.
nit: Since this is in the config, I might also suggest we call this something else besides ShutdownMsg
. Though the comment on the struct row is sufficiently descriptive I wonder if we would benefit from referring to this as "QueuedShutdown", "PendingShutdown", or "PreexistingShutdown"
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.
renamed to "PreviouslySentShutdown" 👍
peer/brontide.go
Outdated
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.
So this all works but I have absolutely no idea how we are going to patch @Roasbeef's coop close v2 PR into this.
6f3c0b0
to
0b51620
Compare
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.
Thanks for the initial review @ProofOfKeags 🙏
Will contact @Roasbeef to see what he says re the simple co-op close stuff
htlcswitch/link.go
Outdated
// ShutdownMsg is an optional value that is set if, at the time of | ||
// the link being started, persisted shutdown info was found for the | ||
// channel. This value being set means that we previously sent a | ||
// Shutdown message to our peer, and so we should do so again on | ||
// re-establish and should not allow anymore HTLC adds on the outgoing | ||
// direction of the link. | ||
ShutdownMsg fn.Option[lnwire.Shutdown] |
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.
I wonder what happens if it's not..
It might give the other side reason to just force close since to them we are doing something fishy. Not sure if any impl actually does bork out here though
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.
one nit
0b51620
to
17cf1ce
Compare
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.
LGTM 🎣
@ProofOfKeags: review reminder |
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.
A couple nits wrt naming, but you can take them or leave them at your discretion. Otherwise, ship it.
|
||
// ShutdownInfo contains various info about the shutdown initiation of a | ||
// channel. | ||
type ShutdownInfo struct { |
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.
nit: I'm generally not a fan of things that end in "Info". Looking at this structure it seems to contain the Shutdown parameters, state, or metadata, but I'm not sure which of these is most appropriate. Ultimately if you think we should leave it as info, that's fine but just figured I'd mention it since in my experience "Info" is usually a stand-in for a more appropriate, more descriptive name.
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.
cool yeah gonna leave this one I think just cause in this case I cant really think of a better term & feel that Info is pretty appropriate because it contains a mixture of things we need to put in the shutdown message as well as other things about the shutdown that we should remember.
htlcswitch/link.go
Outdated
// ShutdownMsg is an optional value that is set if, at the time of | ||
// the link being started, persisted shutdown info was found for the | ||
// channel. This value being set means that we previously sent a | ||
// Shutdown message to our peer, and so we should do so again on | ||
// re-establish and should not allow anymore HTLC adds on the outgoing | ||
// direction of the link. | ||
ShutdownMsg fn.Option[lnwire.Shutdown] |
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.
nit: Since this is in the config, I might also suggest we call this something else besides ShutdownMsg
. Though the comment on the struct row is sufficiently descriptive I wonder if we would benefit from referring to this as "QueuedShutdown", "PendingShutdown", or "PreexistingShutdown"
In this commit, the `ChannelUpdateHandler`'s `EnableAdds` and `DisableAdds` methods are adjusted to return booleans instead of errors. This is done becuase currently, any error returned by these methods is treated by just logging the error since today all it means is that the proposed update has already been done. And so all we do today is log the error. But in future, if these methods are updated to return actual errors that need to be handled, then we might forget to handle them correctly at the various call sights. So we instead change the signature of the function to just return a boolean. In future, if we do need to return any error, we will have to go inspect every call sight in any case to fix compliation & then we can be sure we are handling the errors correctly.
This commit moves calls to link.DisableAdds to outside link.OnCommitOnce call backs. This is done so that if a user requests shutdown, then we can immediately block any new outgoing HTLCs. It's only the sending of the Shutdown message that needs to wait until after any pending CommitSig message is sent.
This commit adds an itest to demonstrate that the following bug exists: If channel Shutdown is initiated but then a re-connect is done before the shutdown is complete, then the initiating node currently does not properly resend the Shutdown message as required by the spec. This will be fixed in an upcoming commit.
ShutdownInfo contains any info about a previous Shutdown message that we have sent. This commit adds this type along with read and write methods for it in the channel db. The existence of the ShutdownInfo on disk represents the fact that we have previously sent the Shutdown message and hence that we should resend it on re-establish.
In this commit, we add the ability to start a link in shutdown mode. This means that we immediately disable any new HTLC adds in the outgoing direction and that we queue up a Shutdown message after the next CommitSig message is sent (or immediately if no CommitSig message is owed).
In this commit, we start persisting shutdown info when we send the Shutdown message. When starting up a link, we also first check if we have previously persisted Shutdown info and if we have, we start the link in shutdown mode meaning that it will not accept any new outgoing HTLC additions and it will queue the shutdown message after any pending CommitSig has been sent.
17cf1ce
to
2fe8520
Compare
~ Replaces #8447 -> opening new PR since this approach is different & so comments wont all transfer. ~
Summary
In this PR we ensure that if a re-establish happens after
shutdown
issent but before fee negotiation starts, then
shutdown
is correctlyre-sent and the coop closure continues. This includes ensuring that
the delivery address sent in the first
shutdown
is the one that isused in the final co-op close tx.
The issue
The issue is that we only mark a channel as
ChanStatusCoopBroadcasted
at the time of actually doing the broadcast. This means that if there is a
re-connect between the
shutdown
message being sent and the coopclose tx being finalised then we will forget that we were in the middle of a
shutdown and will continue with normal operation on restart. This is not
compliant with the spec which says that on re-connect, if we previously
sent a
shutdown
then we MUST resend it again.Fix overview
This adds a
ShutdownInfo
type which we persist when we send a shutdown message.The existence of this on re-start means we should start the link in "shutdown mode" meaning
that we immediately disable HTLC adds in the outgoing direction & queue a shutdown message
once any pending CommitSig has been sent.
PR flow
ShutdownInfo
type along with encode/decode methods for it. Thenmethods to persist & fetch
Shutdown
message on startup so that it knows to immediately block outgoinghtlcs & to queue the shutdown message.
behaviour
Fixes #8397