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.

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: log fixes #4107

Merged
merged 3 commits into from
Mar 26, 2020
Merged

Conversation

Crypt-iQ
Copy link
Collaborator

This PR fixes the error log intro'd in #3143 when pipelining settles to the switch, and makes logging less spammy and more informative. The PR breakdown is as follows:

  • First commit changes the fallback in NextLocalHtlcIndex to RemoteCommitment since LocalCommitment lags behind for the LocalHtlcIndex field. Without this bug fix, open circuits could get prematurely trimmed, resulting in more error logs. A test case is included to check that the fix works.
  • Second commit returns ErrLinkShuttingDown so that the link logs when it's getting shut down. Also changes the order of the ackDownStreamPackets and check-if-link-quit call for consistency since the packets should be ACKed before going down.
  • Final commit changes the switch to only log an error if a update_fail_htlc comes in and closeCircuit returns ErrUnknownCircuit. Rationale being that only settles should hit this code path, anything else is a result of a link flap.

Closes #3656

@Crypt-iQ Crypt-iQ added logging Related to the logging / debug output functionality htlcswitch v0.10 labels Mar 22, 2020
@Crypt-iQ Crypt-iQ added this to the 0.10.0 milestone Mar 22, 2020
htlcswitch/link_test.go Outdated Show resolved Hide resolved
htlcswitch/link_test.go Outdated Show resolved Hide resolved
htlcswitch/link_test.go Show resolved Hide resolved
htlcswitch/link.go Show resolved Hide resolved
htlcswitch/switch.go Show resolved Hide resolved
@Crypt-iQ Crypt-iQ requested a review from cfromknecht March 24, 2020 18:59
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Great series of bug fixes! The diff looks pretty good to me, only comments pertain to populating the commit messages of each of the commits as bit more, as the PR body won't be committed to our git history.

channeldb/channel.go Show resolved Hide resolved
htlcswitch/link.go Show resolved Hide resolved
Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

LGTM barring roasbeef's comments, could also use a squash 👍

htlcswitch/switch.go Show resolved Hide resolved
This commit changes the fallback in NextLocalHtlcIndex to
RemoteCommitment since the LocalHtlcIndex field lags behind
on the LocalCommitment. Without this bug fix, open circuits
would get prematurely trimmed, resulting in more erroneous
logs. A test case is included to check that the fix works.
This commit modifies updateCommitTx to error with ErrLinkShuttingDown
when we try to send a ContractUpdate on the htlcUpdates chan and the
link has closed the quit chan. It also changes the order of the call
to ackDownStreamPackets and contract update call for consistency since
the packets should be acknowledged before the link goes down.
This commit changes the switch to only log an error if update_fail_htlc
comes in and closeCircuit returns ErrUnknownCircuit. Rationale
being that only settles should hit this code path, anything else
is a result of a link flap and should be treated as an error.
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🦋

@Roasbeef Roasbeef merged commit 31de326 into lightningnetwork:master Mar 26, 2020
@Crypt-iQ Crypt-iQ deleted the switch_err_0220 branch March 27, 2020 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
htlcswitch logging Related to the logging / debug output functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unfounded [ERR] HSWC: Unable to find target channel for HTLC settle/fail
3 participants