multi: resend shutdown on reestablish #8464
Conversation
ellemouton
requested review from
ProofOfKeags,
Crypt-iQ,
Roasbeef and
yyforyongyu
as code owners
|
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. |
ellemouton
force-pushed
the
resend-shutdown-2
branch
from
535d7a8
to
e1de181
Compare
|
Approach ACK 👍 |
ellemouton
force-pushed
the
resend-shutdown-2
branch
3 times, most recently
from
0b404d0
to
6f3c0b0
Compare
|
PTAL @Crypt-iQ & @ProofOfKeags 🙏 |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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.
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.
renamed to "PreviouslySentShutdown" 👍
peer/brontide.go
Outdated
There was a problem hiding this comment.
So this all works but I have absolutely no idea how we are going to patch @Roasbeef's coop close v2 PR into this.
ellemouton
force-pushed
the
resend-shutdown-2
branch
from
6f3c0b0
to
0b51620
Compare
There was a problem hiding this comment.
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.
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
ellemouton
force-pushed
the
resend-shutdown-2
branch
from
0b51620
to
17cf1ce
Compare
|
@ProofOfKeags: review reminder |
|
|
||
| // ShutdownInfo contains various info about the shutdown initiation of a | ||
| // channel. | ||
| type ShutdownInfo struct { |
There was a problem hiding this comment.
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.
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.
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.
ellemouton
force-pushed
the
resend-shutdown-2
branch
from
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
shutdownissent but before fee negotiation starts, then
shutdownis correctlyre-sent and the coop closure continues. This includes ensuring that
the delivery address sent in the first
shutdownis the one that isused in the final co-op close tx.
The issue
The issue is that we only mark a channel as
ChanStatusCoopBroadcastedat the time of actually doing the broadcast. This means that if there is a
re-connect between the
shutdownmessage 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
shutdownthen we MUST resend it again.Fix overview
This adds a
ShutdownInfotype 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
ShutdownInfotype along with encode/decode methods for it. Thenmethods to persist & fetch
Shutdownmessage on startup so that it knows to immediately block outgoinghtlcs & to queue the shutdown message.
behaviour
Fixes #8397