discovery: unify rate.Limiter across all gossip peers by Roasbeef · Pull Request #9607 · lightningnetwork/lnd

Roasbeef · 2025-03-17T21:43:14Z

In this commit, we revamp the old message based rate limiting. First, we
move to meter by bytes/s instead of messages/s. The old logic had an
error in that it limited groups of message replies, instead of each
message. With this new approach, we'll use the newly added
SerializedSize method to implement fine grained bandwidth metering.

We need to pick two values, the burst rate, and the msg bytes rate. The
burst rate is the max amt that can be sent in a given period of time. We
need to set this above 65 KB, or the max msg limit, otherwise no
messages can be sent. The bucket starts with this many tokens (bytes).
As those are depleted, the amount of tokens is refilled at the msg
bytes rate.

As conservative values, we've chosen 200 KB as the burst rate, and 100
KB/s as the limit.

Depends on #9623

coderabbitai · 2025-03-17T21:43:22Z

Important

Review skipped

Auto reviews are limited to specific labels.

🏷️ Labels to auto review (1)

llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai plan to trigger planning for file edits and PR creation.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

Roasbeef · 2025-03-18T02:52:27Z

 		defer returnSema()

 		for _, msg := range newUpdatestoSend {
+			if err := g.maybeAddMsgDelay(); err != nil {


Looking at the diff again, I think ti'll just move this call into sendToPeerSync.

morehouse · 2025-03-19T11:06:40Z

Concept ACK.

This makes sense -- the rate limit is there to throttle outgoing bandwidth, so it should apply globally rather than per-peer and per-incoming-request.

Roasbeef · 2025-03-20T22:51:50Z

Pushed up an updated variant where we'll use rate.Limiter to implement a global outbound bandwidth rate limiter. We'll likely need to tune the defaults somewhat.

yyforyongyu

Looks like the changes got rebased out😂

Roasbeef · 2025-03-21T01:37:53Z

Looks like the changes got rebased out😂

LOL, not sure what happened there...

FIxed!

yyforyongyu

Looking good - I also ran the benchmark tests and it doesn't seem to impact the performance which is nice,

On master,

=== RUN   TestLightningNetworkDaemon/tranche00/117-of-275/btcd/async_payments_benchmark
    lnd_payment_test.go:757: 	Benchmark info: Elapsed time:  40.501505292s
    lnd_payment_test.go:758: 	Benchmark info: TPS:  11.925482683119037

=== RUN   TestLightningNetworkDaemon/tranche00/225-of-275/btcd/remote_signer-funding_async_payments
    lnd_payment_test.go:757: 	Benchmark info: Elapsed time:  42.424433709s
    lnd_payment_test.go:758: 	Benchmark info: TPS:  11.384948666917278


=== RUN   TestLightningNetworkDaemon/tranche00/226-of-275/btcd/remote_signer-funding_async_payments_taproot
    lnd_payment_test.go:757: 	Benchmark info: Elapsed time:  42.670583291s
    lnd_payment_test.go:758: 	Benchmark info: TPS:  11.319273437302027

This branch,

=== RUN   TestLightningNetworkDaemon/tranche00/117-of-274/btcd/async_payments_benchmark
    lnd_payment_test.go:757:    Benchmark info: Elapsed time:  43.210928459s
    lnd_payment_test.go:758:    Benchmark info: TPS:  11.177727885627055

=== RUN   TestLightningNetworkDaemon/tranche00/224-of-274/btcd/remote_signer-funding_async_payments
    lnd_payment_test.go:757:    Benchmark info: Elapsed time:  43.428684625s
    lnd_payment_test.go:758:    Benchmark info: TPS:  11.121681537689446

=== RUN   TestLightningNetworkDaemon/tranche00/225-of-274/btcd/remote_signer-funding_async_payments_taproot
    lnd_payment_test.go:757:    Benchmark info: Elapsed time:  46.923330958s
    lnd_payment_test.go:758:    Benchmark info: TPS:  10.293386896005364

Think once the CI is fixed we are good to go.

yyforyongyu · 2025-03-21T07:33:21Z

+
+// TestMaybeAddMsgDelay tests that the SyncManager.maybeAddMsgDelay method
+// correctly computes delays based on message size.
+func TestMaybeAddMsgDelay(t *testing.T) {


yyforyongyu · 2025-03-21T07:34:56Z

 	// DefaultMaxUndelayedQueryReplies specifies how many gossip queries we
 	// will respond to immediately before starting to delay responses.
-	DefaultMaxUndelayedQueryReplies = 10
+	DefaultMaxUndelayedQueryReplies = 50


This can be removed now

Done.

Part of me wonders if we should keep a base line msg/s second limit in addition to the bandwidth limit.

I like the current approach as it deals with network resources directly - what would be a use case for limiting by num of msgs?

Just thinking about a degenerate case where someone is having us send super small messages, so while we're under the total limit, we're doing more work overall.

I think this only matters for messages we send out not ones that they send us, so the size of our sent messages is less under their control (still under their control somewhat since they are querying).

I think it'd be wise to also limit the number of messages so that they don't potentially sit in memory in the case of routing dumps, but this should be addressed by the existing syncer semaphore.

Roasbeef · 2025-03-21T19:46:33Z

I also ran the benchmark tests and it doesn't seem to impact the performance which is nice,

That's expected, since this should only affect gossip syncers. We don't rate limit our own commit sig or HTLC messages.

What do we think about the set of defaults? Is 100 KB/s reasonable for most nodes?

Roasbeef · 2025-03-21T23:03:19Z

Fixed a bug (was returning early from the send loop).

Also modified the structure to allow us to cancel a rate limit reservation if a peer exits. This way we release those tokens/bytes back to the bucket.

yyforyongyu

Close! Some typos and my main comment is whether we should add a simple validation for the user configs, to make sure we won't be blocked on the delay forever.

Also modified the structure to allow us to cancel a rate limit reservation if a peer exits. This way we release those tokens/bytes back to the bucket.

If the peer reconnects I think a new rate limiter will be created with a bucket full of tokens.

yyforyongyu · 2025-03-24T11:07:27Z

+func TestDeriveRateLimitReservation(t *testing.T) {
+	// Define standard test parameters for rate limiting.
+	bytesPerSec := uint64(1000) // 1000 bytes/second
+	bytesBurst := uint64(100)   // 100 bytes burst capacity


Looks like the burst is smaller than the rate?

I think this is ok for the test, as we're just making assertions based on the delay value, rather than having the test sleep. In the future I think something like synctest would allow us to actually sleep here to assert messages are eventually sent.

yyforyongyu · 2025-03-24T11:18:35Z

 	// DefaultMaxUndelayedQueryReplies specifies how many gossip queries we
 	// will respond to immediately before starting to delay responses.
-	DefaultMaxUndelayedQueryReplies = 10
+	DefaultMaxUndelayedQueryReplies = 50


I like the current approach as it deals with network resources directly - what would be a use case for limiting by num of msgs?

Roasbeef · 2025-03-24T18:07:38Z

If the peer reconnects I think a new rate limiter will be created with a bucket full of tokens.

So there's a global bucket. The change I made handles the case where a peer sleeps to send 65 KB, but then disconnects before they actually send (or disconnects mid sleep). We'll now release that reservation back into the available pool.

Crypt-iQ

Concept ACK. Reviewed the code and I think this is a good change. I think this needs to be tested under real load during RC to figure out better values. My only concern is that depending on the values, it might be possible that an attacker can get honest peers rate-limited by asking for a graph dump since the limiter is shared. So I think good values here will go a long way. Alternatively, maybe we could note down all the scenarios where peers will request routing dumps and then evaluate if all of those cases are necessary?

Roasbeef · 2025-03-24T20:09:20Z

My only concern is that depending on the values, it might be possible that an attacker can get honest peers rate-limited by asking for a graph dump since the limiter is shared

FWIW, we still have that global semaphore to limit the number of outstanding gossip filter requests. Moving to a per peer limiter is possible, then then an attacker can just spin up multiple peers to get around it.

The original motivation for this change was to move to a global rate limiting value, as in the wild for certain degenerate cases, all the peers were below the rate limit, but in aggregate (hundreds of peers) bandwidth utilization was very high.

ziggie1984

LGTM, really easy to review change 👌

ziggie1984 · 2025-03-25T01:02:43Z

+		bytesPerSecond = DefaultMsgBytesPerSecond
+	}
+
+	bytesBurst := cfg.AllotedMsgBytesBurst


Should we do some boundary checks for these constants ?

I added a check in the config validation. We need this value to be above 65 KB.

ziggie1984 · 2025-03-25T01:06:10Z

+	peer lnpeer.Peer, nodeID route.Vertex, msgs ...lnwire.Message) error {
+
+	for _, msg := range msgs {
+		if err := m.maybeRateLimitMsg(ctx, nodeID, msg); err != nil {


I wonder if we should skip rate-limiting when sync=true, imo we should not really rate-limit if we require a peer synchronization.

IIUC sync=true here just means we should block until the message has been fully sent out.

ziggie1984 · 2025-03-25T01:08:57Z

 }
+
+// TestSizeableMessage is a test implementation of lnwire.SizeableMessage.
+type TestSizeableMessage struct {


why did you not use a mock ?

Just needed some simpler behavior so I didn't reach for it 🤷

ziggie1984 · 2025-03-25T01:14:24Z

+	})
+
+	// Test that message size affects delay.
+	t.Run("larger messages have longer delays", func(t *testing.T) {


Nit: seems like we are testing the rateLimiter here, I think we might not really need this testcase ?

In a sense that's true, but at least we're able to bind the current implementation to a basic property based assumption.

A larger test would need to actually sleep to make sure we're able to eventually send messages. Once we can use synctest, we can add something like that.

ziggie1984 · 2025-03-25T01:34:46Z

 ; The number of confirmations required before processing channel announcements.
 ; gossip.announcement-conf=6

+; The allotted bandwdith rate expressed in bytes/second that will be alloacted


Nit: alloacted => allocated, bandwdith => bandwidth

In this commit, we revamp the old message based rate limiting. First, we move to meter by bytes/s instead of messages/s. The old logic had an error in that it limited groups of message replies, instead of each message. With this new approach, we'll use the newly added SerializedSize method to implement fine grained bandwidth metering. We need to pick two values, the burst rate, and the msg bytes rate. The burst rate is the max amt that can be sent in a given period of time. We need to set this above 65 KB, or the max msg limit, otherwise no messages can be sent. The bucket starts with this many tokens (bytes). As those are depleted, the amount of tokens is refilled at the msg bytes rate. As conservative values, we've chosen 200 KB as the burst rate, and 100 KB/s as the limit.

We go with the defaults of if no values are set.

Roasbeef commented Mar 18, 2025

View reviewed changes

Roasbeef force-pushed the unified-gossip-limiter branch from 4d7b24d to 7662eaa Compare March 20, 2025 22:49

Roasbeef changed the base branch from master to size-msg-test-msg March 20, 2025 22:50

Roasbeef marked this pull request as ready for review March 20, 2025 22:50

Roasbeef requested review from Crypt-iQ and yyforyongyu March 20, 2025 22:50

Roasbeef force-pushed the unified-gossip-limiter branch from 7662eaa to ceb4ea0 Compare March 20, 2025 23:05

Roasbeef force-pushed the size-msg-test-msg branch from a383ea9 to 9eaec1f Compare March 20, 2025 23:06

yyforyongyu reviewed Mar 21, 2025

View reviewed changes

Roasbeef force-pushed the size-msg-test-msg branch from 9eaec1f to 05a6b68 Compare March 21, 2025 01:35

Roasbeef force-pushed the unified-gossip-limiter branch from ceb4ea0 to dae6da6 Compare March 21, 2025 01:37

yyforyongyu reviewed Mar 21, 2025

View reviewed changes

Roasbeef changed the base branch from size-msg-test-msg to master March 21, 2025 19:19

Roasbeef force-pushed the unified-gossip-limiter branch 2 times, most recently from acfda36 to 2bb575f Compare March 21, 2025 19:20

Roasbeef force-pushed the unified-gossip-limiter branch 2 times, most recently from 1aa2f65 to c4f23b5 Compare March 21, 2025 23:02

Roasbeef force-pushed the unified-gossip-limiter branch from c4f23b5 to 6171441 Compare March 21, 2025 23:30

yyforyongyu reviewed Mar 24, 2025

View reviewed changes

Roasbeef force-pushed the unified-gossip-limiter branch 2 times, most recently from e6bb1b0 to 1166f2c Compare March 24, 2025 19:24

Crypt-iQ reviewed Mar 24, 2025

View reviewed changes

ziggie1984 approved these changes Mar 25, 2025

View reviewed changes

Roasbeef added 3 commits March 24, 2025 19:21

multi: add new config options to tune gossip msg allocated bandwidth

c7ed5d6

We go with the defaults of if no values are set.

docs/release-notes: add release notes for gossip rate limit

8c3c53f

Roasbeef force-pushed the unified-gossip-limiter branch from 1166f2c to 8c3c53f Compare March 25, 2025 02:22

Roasbeef merged commit 1cebfed into lightningnetwork:master Mar 25, 2025

Uh oh!

Conversation

Roasbeef commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morehouse commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Roasbeef commented Mar 20, 2025

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Roasbeef commented Mar 21, 2025

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Roasbeef commented Mar 21, 2025

Uh oh!

Roasbeef commented Mar 21, 2025

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Roasbeef Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Roasbeef commented Mar 24, 2025

Uh oh!

Crypt-iQ left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Roasbeef commented Mar 24, 2025

Uh oh!

ziggie1984 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Roasbeef commented Mar 17, 2025 •

edited

Loading

coderabbitai Bot commented Mar 17, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

morehouse commented Mar 19, 2025 •

edited

Loading

Roasbeef Mar 24, 2025 •

edited

Loading

Crypt-iQ left a comment •

edited

Loading