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

Jump to bottom

Fix warning messages #6840

Merged
merged 2 commits into from
Oct 26, 2022
Merged

Fix warning messages #6840

merged 2 commits into from
Oct 26, 2022

Conversation

positiveblue
Copy link
Contributor

Stop casting warnings to errors before passing it to the service handling the message. BOLT1 talks about error and warning messages so I created a new interface called ErrorLike.

Warning messages for pending channels are now handled by the funding manager. Until now they were simply ignored.

Currently, the only action triggered by a warning messages sent by our peers is logging.

Fixes #6801

@positiveblue positiveblue mentioned this pull request Aug 17, 2022
Closed
5 tasks
@Roasbeef Roasbeef added this to the v0.16.0 milestone Aug 17, 2022
@positiveblue positiveblue force-pushed the fix-warning-messages branch 2 times, most recently from 5570b13 to a7fb4bd Compare August 19, 2022 15:55
Crypt-iQ
Crypt-iQ requested changes Aug 22, 2022
View reviewed changes
lnwire/error.go Outdated
@@ -47,6 +47,25 @@ func (e FundingError) Error() string {
return e.String()
}

// ErrorLike is an interface implemented by Error and Warning messages as
// described in BOLT1.
type ErrorLike interface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really see the benefit of this - I dislike changing the peer/brontide code more than it has to be changed. I think it is better if Warning does not embed Error and either:

  • handleError is duplicated with a handleWarning function
  • handleError handles both a Warning or an Error without an interface, and is renamed to handleErrorOrWarning or something similar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose option A 👍

@saubyk saubyk removed this from the v0.16.0 milestone Aug 24, 2022
@saubyk
Copy link
Collaborator

saubyk commented Aug 24, 2022

Cleared the milestone for the pr as the issue (#6801) is tagged now

@positiveblue positiveblue force-pushed the fix-warning-messages branch 2 times, most recently from 6e90ecf to 09f0103 Compare August 24, 2022 20:35
@positiveblue positiveblue requested a review from Crypt-iQ August 24, 2022 22:59
Crypt-iQ
Crypt-iQ reviewed Sep 1, 2022
View reviewed changes
Copy link
Collaborator

@Crypt-iQ Crypt-iQ left a comment

Choose a reason for hiding this comment

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

i think the link test should be added back and that a fundingmanager test should be added

lnwire/lnwire.go
@@ -194,6 +208,18 @@ func WriteElement(w *bytes.Buffer, element interface{}) error {
if _, err := w.Write(e[:]); err != nil {
return err
}

case WarningData:
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this called anywhere?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can actually also just fall through to the case below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, and the method will be removed soon* (YY comment). However, I added in case someone is using lnwire.WriteElement outside the lnd code base for completeness.

Roasbeef
Roasbeef reviewed Sep 30, 2022
View reviewed changes
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.

IIRC, you had a test that you used to repro the behavior in the first place? If we modified SendCustomMessage to allow sending messages below the custom message range w/ a build tag (for itests) then we can test this out in the wild to ensure that sending a warning doesn't lead to a force close. This should help catch any future regressions in the future.

lnwire/lnwire.go
@@ -194,6 +208,18 @@ func WriteElement(w *bytes.Buffer, element interface{}) error {
if _, err := w.Write(e[:]); err != nil {
return err
}

case WarningData:
Copy link
Member

Choose a reason for hiding this comment

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

I think we can actually also just fall through to the case below.

peer/brontide.go
@@ -1775,7 +1807,7 @@ func messageSummary(msg lnwire.Message) string {
msg.ChanID, msg.ID, msg.FailureCode)

case *lnwire.Warning:
return fmt.Sprintf("%v", msg.Error.Error())
return fmt.Sprintf("%v", msg.Warning())
Copy link
Member

Choose a reason for hiding this comment

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

If we keep the existing Error method, then we can pass it around as a normal error. Not sure if we 100% want that behavior though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would leave it like this so we do not have any error in the future using the Error interface for warnings.

@positiveblue positiveblue force-pushed the fix-warning-messages branch 3 times, most recently from 359d6b8 to 7a1b788 Compare October 13, 2022 12:01
@Crypt-iQ Crypt-iQ mentioned this pull request Oct 19, 2022
Closed
5 tasks
@lightninglabs-deploy
Copy link

@Roasbeef: review reminder
@Crypt-iQ: review reminder

Crypt-iQ
Crypt-iQ approved these changes Oct 24, 2022
View reviewed changes
Copy link
Collaborator

@Crypt-iQ Crypt-iQ left a comment

Choose a reason for hiding this comment

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

needs rebase

Roasbeef
Roasbeef approved these changes Oct 26, 2022
View reviewed changes
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 ❄️

@guggero
Copy link
Collaborator

guggero commented Oct 26, 2022

@positiveblue the new linter rules seem to cause a few new complaints.

@positiveblue positiveblue force-pushed the fix-warning-messages branch 2 times, most recently from 06d769f to a48b0a0 Compare October 26, 2022 14:39
@positiveblue
multi: stop casting peer warning messages as errors
4d4d8e4 
Split the logic for processing `error` and `warning` messages from our
peers.
@guggero guggero merged commit 359a563 into lightningnetwork:master Oct 26, 2022
@Roasbeef Roasbeef mentioned this pull request Jan 5, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Roasbeef Roasbeef Roasbeef approved these changes

@Crypt-iQ Crypt-iQ Crypt-iQ approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

lnd force-closes in response to warning messages
6 participants
@positiveblue @saubyk @lightninglabs-deploy @guggero @Roasbeef @Crypt-iQ