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

[gossiper] Ignore received ChannelAnnouncements for own channels #4899

Merged
merged 5 commits into from Feb 11, 2021

Conversation

halseth
Copy link
Contributor

@halseth halseth commented Jan 6, 2021

Fixes #4690

@halseth halseth force-pushed the ignore-local-ann-from-remote branch 2 times, most recently from 42be9da to 094b259 Compare January 6, 2021 11:31
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 👌

@@ -544,7 +544,7 @@ func (d *AuthenticatedGossiper) ProcessRemoteAnnouncement(msg lnwire.Message,
// entire channel announcement and update messages will be re-constructed and
// broadcast to the rest of the network.
func (d *AuthenticatedGossiper) ProcessLocalAnnouncement(msg lnwire.Message,
source *btcec.PublicKey, optionalFields ...OptionalMsgField) chan error {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@cfromknecht
Copy link
Contributor

hmm unit tests are unhappy

// To avoid inserting edges in the graph for our own channels that we
// have already closed, we ignore such channel announcements coming
// from the remote.
case *lnwire.ChannelAnnouncement:
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I guess this could happen after SCB recovery, the channel isn't yet closed, but we learn about it? I wonder why this didn't show up in any of the SCB itests.

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 think it can also happen just after you have closed a channel, then someone send you the announcement?

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.

That was meant to be a comment...

@halseth
Copy link
Contributor Author

halseth commented Jan 7, 2021

Uh, yeah need to fix the unit tests.

@halseth halseth force-pushed the ignore-local-ann-from-remote branch from 094b259 to fb77b2b Compare January 18, 2021 13:30
@Roasbeef Roasbeef added this to the 0.13.0 milestone Jan 21, 2021
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 added database Related to the database/storage of LND gossip P1 MUST be fixed or reviewed labels Jan 28, 2021
@Roasbeef Roasbeef added this to Reviewer approved in v0.13.0-beta Jan 28, 2021
@halseth halseth force-pushed the ignore-local-ann-from-remote branch 2 times, most recently from 359711d to d335992 Compare February 1, 2021 12:49
@Roasbeef
Copy link
Member

Needs a rebase!

It will be the same announcement, no need to distinguish.
To make it more clear what is local and remote messages, we change to
use `selfKey` only for local messages.
To avoid learning about our own channel we have already closed and
removed from our graph, we ignore all ChannelAnns for channels we are
involved in.
@halseth halseth merged commit b52e872 into lightningnetwork:master Feb 11, 2021
v0.13.0-beta automation moved this from Reviewer approved to Done Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix database Related to the database/storage of LND gossip P1 MUST be fixed or reviewed
Projects
No open projects
3 participants