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

htlcswitch: send an error before marking a channel w/ local data loss #7312

Closed
wants to merge 1 commit into from
Closed

htlcswitch: send an error before marking a channel w/ local data loss #7312

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 18 additions & 0 deletions htlcswitch/link.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1049,6 +1049,13 @@ func (l *channelLink) htlcManager() {
"data loss: %v", err)
}

// Before we fail the link, we'll send an error
// to the other party so they know we can't
// continue.
//
// TODO(roasbeef): use reliable sender here?
l.sendFatalError()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be before the call to MarkDataLoss? I don't believe MarkBorked is called in peer/brontide.go for this since PermanentFailure will be false. It also seems like an Error is already sent since ErrRecoveryError is used in the call to l.fail(...) below. Is the issue here that the p2p connection may go down after MarkDataLoss, but before we eventually call ShouldSendToPeer in handleLinkFailure?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I actually had this initially, but the rationale was that by the time we send the Error they might have up already, which would cause some of the logic below to short circuit.

Is the issue here that the p2p connection may go down after MarkDataLoss, but before we eventually call ShouldSendToPeer in handleLinkFailure?

My understanding going into this change was that other implementations started to mostly ignore what we send for chan reest, even if it's all very invalid. So we now need to send the Error as that's the only thing they'll force close off of.

IIUC, on restart, we'll also repeat this loop all over again, w/ the MarkDataLoss then being a noop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tracing the pipeline a bit more, you're right that we'll eventually call handleLinkFailure in, which'll then fail since it's sending out ErrRecoveryError.


// We determined the commit chains were not possible to
// sync. We cautiously fail the channel, but don't
// force close.
Expand Down Expand Up @@ -3452,3 +3459,14 @@ func (l *channelLink) fail(linkErr LinkFailureError,
l.failed = true
l.cfg.OnChannelFailure(l.ChanID(), l.ShortChanID(), linkErr)
}

// sendFatalError sends an Error message to the remote peer, in order to prompt
// either a force close, or to signal that the channel can no longer continue.
func (l *channelLink) sendFatalError() {
// For fatal errors, we'll set sync=true so we ensure that this is sent
// out before we go to mark the channel as borked.
l.cfg.Peer.SendMessage(true, &lnwire.Error{
ChanID: l.ChanID(),
Data: []byte("fatal error"),
})
}