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

discovery: use token bucket based rate limiting to throttle gossip #5006

Merged

Conversation

wpaulino
Copy link
Contributor

The recently added gossip throttling was shown to be too aggressive, especially with our auto channel enable/disable signaling. We switch to a token bucket based system instead as it's based on time, rather than a block height which isn't constantly updated at a given rate.

Along the way, we also revert the commits that exposed a config flag that was only necessary for our integration tests with the previous gossip throttling logic.

discovery/gossiper.go Show resolved Hide resolved
discovery/gossiper.go Show resolved Hide resolved
The recently added gossip throttling was shown to be too aggressive,
especially with our auto channel enable/disable signaling. We switch to
a token bucket based system instead as it's based on time, rather than a
block height which isn't constantly updated at a given rate.
@wpaulino wpaulino force-pushed the new-rate-limit-chan-updates branch from 30ed562 to 83a0d03 Compare February 11, 2021 00:21
Comment on lines +29 to +37
// DefaultMaxChannelUpdateBurst is the default maximum number of updates
// for a specific channel and direction that we'll accept over an
// interval.
DefaultMaxChannelUpdateBurst = 10

// DefaultChannelUpdateInterval is the default interval we'll use to
// determine how often we should allow a new update for a specific
// channel and direction.
DefaultChannelUpdateInterval = time.Minute
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These values were somewhat arbitrarily chosen in case reviewers have any thoughts/suggestions.

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 🥃

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 🎉 default parameters seem reasonable, we can tune further if anything pops up during the rc phase.

@cfromknecht cfromknecht merged commit 1ee5eb9 into lightningnetwork:master Feb 11, 2021
@wpaulino wpaulino deleted the new-rate-limit-chan-updates branch February 11, 2021 02:06
@cfromknecht cfromknecht mentioned this pull request Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants