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

Retransmit NodeAnnouncement on regular intervals #2084

Merged
merged 8 commits into from Sep 17, 2019

Conversation

halseth
Copy link
Contributor

@halseth halseth commented Oct 23, 2018

This PR makes the gossiper retransmit our NodeAnnouncement every 24 hrs, to avoid other nodes considering it being a zombie. This is similar to what is done for channels.

Fixes #2005.

@Roasbeef Roasbeef added p2p Code related to the peer-to-peer behaviour P3 might get fixed, nice to have needs review PR needs review by regular contributors labels Oct 24, 2018
Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

Nice commit structure!

discovery/gossiper.go Outdated Show resolved Hide resolved
discovery/gossiper_test.go Outdated Show resolved Hide resolved
discovery/gossiper_test.go Outdated Show resolved Hide resolved
discovery/gossiper_test.go Outdated Show resolved Hide resolved
discovery/gossiper_test.go Outdated Show resolved Hide resolved
discovery/gossiper.go Outdated Show resolved Hide resolved
server.go Outdated
ProofMatureDelta: 0,
TrickleDelay: time.Millisecond * time.Duration(cfg.TrickleDelay),
RetransmitDelay: time.Minute * 30,
RebroadcastInterval: time.Hour * 24,
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason this is the default? It could be made less spammy IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could reasonably be made longer, but don't think one update every 24 hrs is the biggest spam problem atm.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure but given that we only consider things stale after 2 weeks it could be set a bit higher.

@wpaulino wpaulino added the first pass review done PR has had first pass of review, needs more tho label Oct 27, 2018
@halseth halseth force-pushed the node-announcement-stale branch 3 times, most recently from 8821aad to 89edb0d Compare October 30, 2018 09:36
@halseth halseth force-pushed the node-announcement-stale branch 2 times, most recently from f8ab1a7 to ce72601 Compare November 9, 2018 12:09
@wpaulino
Copy link
Contributor

wpaulino commented Sep 3, 2019

Needs a rebase!

@halseth
Copy link
Contributor Author

halseth commented Sep 4, 2019

Rebased.

discovery/gossiper_test.go Outdated Show resolved Hide resolved
discovery/gossiper.go Outdated Show resolved Hide resolved
discovery/gossiper_test.go Outdated Show resolved Hide resolved
discovery/gossiper_test.go Outdated Show resolved Hide resolved
@wpaulino wpaulino removed the request for review from Roasbeef September 4, 2019 22:03
discovery/gossiper.go Show resolved Hide resolved
discovery/gossiper_test.go Show resolved Hide resolved
discovery/gossiper_test.go Outdated Show resolved Hide resolved
discovery/gossiper.go Outdated Show resolved Hide resolved
err)
}

timestamp := time.Unix(int64(currentNodeAnn.Timestamp), 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just something to note, the timestamp of our node announcement is updated to time.Now every time the server is started.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because of this, if a node restarts before it reaches a full day of uptime, it'll never rebroadcast its node announcement.

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 that's good enough for now. I think maybe there was an issue/PR already for transmitting the node announcement if it changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Relevant:
#1120
#325
#274

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Won't the announcement be rebroadcast when the time us updated at startup? From that issue it seems so.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that's the case. We only seem to send when opening a channel or when running with --nat.

discovery/gossiper_test.go Outdated Show resolved Hide resolved
@halseth halseth force-pushed the node-announcement-stale branch 4 times, most recently from 506d249 to 4dc3b2c Compare September 10, 2019 12:39
discovery/gossiper.go Outdated Show resolved Hide resolved
err)
}

timestamp := time.Unix(int64(currentNodeAnn.Timestamp), 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Because of this, if a node restarts before it reaches a full day of uptime, it'll never rebroadcast its node announcement.

discovery/gossiper_test.go Outdated 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 🔥

Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

LGTM, though I still think the node announcement timestamp issue should be addressed in order to be consistent, likely as a separate PR.

@halseth
Copy link
Contributor Author

halseth commented Sep 17, 2019

LGTM, though I still think the node announcement timestamp issue should be addressed in order to be consistent, likely as a separate PR.

I updated and reopened #274. Let me know if this is what you had in mind.

@wpaulino
Copy link
Contributor

I updated and reopened #274. Let me know if this is what you had in mind.

SGTM

@wpaulino wpaulino merged commit 9e4c4c5 into lightningnetwork:master Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first pass review done PR has had first pass of review, needs more tho needs review PR needs review by regular contributors p2p Code related to the peer-to-peer behaviour P3 might get fixed, nice to have
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send out updated announcements periodically
5 participants