onionmessage: bound per-peer rate limiter registry

[erickcestari]

The PeerRateLimiter kept a token bucket per peer and never removed any
entry. With the channel-presence gate disabled via --onion-msg-relay-all
any connecting pubkey that sends an onion message allocates a bucket,
and buckets are retained across disconnects, so a peer reconnecting
under a stream of fresh identities could grow the map without bound and
exhaust memory.

Bound the registry by evicting buckets that have refilled to their full
burst once it grows past a high-water mark. A full bucket holds the same
tokens as a freshly-created one, so recreating it lazily on the peer's
next message preserves the limiter's behaviour, while a bucket that
still owes tokens is retained and the per-peer rate stays a ceiling
across reconnects.

Since only a full bucket is evictable, AllowN debits a newly created
bucket before publishing it. Otherwise the sweep that its own insertion
triggers reclaims it while still full and discards the debit, handing
the peer a fresh burst on every message.

[github-actions]

🟡 PR Severity: MEDIUM

Classification | 3 files | 184 additions / 22 deletions

🟡 Medium (1 file)

• onionmessage/ratelimit.go - Not a categorized critical/high package; falls under "other Go files not categorized above"

🟢 Low (2 files)

• docs/release-notes/release-notes-0.22.0.md - Release notes
• onionmessage/ratelimit_test.go - Test-only change

Analysis

This PR modifies rate-limiting logic in onionmessage/ratelimit.go, which is not one of the explicitly enumerated CRITICAL or HIGH severity packages (e.g. it is not lnwire/*, peer/*, htlcswitch/*, etc.). Onion message rate limiting is peripheral logic rather than core wallet, channel, HTLC, or peer-transport code, so it defaults to the "other Go files not categorized above" MEDIUM bucket.

No severity bump applies: excluding the test file, only 2 files are in scope (ratelimit.go and the release notes) with ~119 non-test lines changed in ratelimit.go, well under both the >20-file and >500-line bump thresholds. No critical packages are touched.

No override label is present.

---

_{To override, add a severity-override-{critical,high,medium,low} label.}

[TechLateef]

This is a great fix! I was looking through evictFullBuckets and I think there might be a small TOCTOU race around the double-check that could cause numPeers to leak.

If a sweep happens at the same time a worker processes a message for the same peer, I think the sequence would be:

• The sweeper removes the full bucket with LoadAndDelete.
• A worker calls AllowN, doesn't find the peer, creates a new bucket, and increments numPeers.
• The sweeper then hits the double-check, decides the bucket shouldn't have been deleted, and restores the old bucket, overwriting the worker's newly created one. Since it "undoes" the deletion, it also skips decrementing numPeers.

In that case, the map size stays the same, but numPeers ends up +1. If this happens repeatedly, numPeers could eventually drift past maxPeers, causing every new peer to trigger an O(N) sweep.

Would it make sense to just delete unconditionally here and avoid the double-check altogether? That seems like it would keep the map and numPeers in sync, but I may be missing something.

[erickcestari]

Good catch! I've updated to delete unconditionally (and added a race condition test for regression) which might reset a bucket, however it is bounded to only 1 message, and it only happens when evicting, so it's bounded.

[Roasbeef]

Thanks for this fix!

Spotted a bug in the logic that would eliminate rate limiting all together once maxPeers was reached, see the comment inline.

[Roasbeef]

So if we just added a new bucket/peer, then the token bucket is full. The call to evictFullBuckets below will then evict that bucket. The next message sent by the peer will then do the same thing, meaning they can use a full burst each time. We should do the sweep only at the end here to ensure the new message removes tokens from the bucket before we do the eviction attempt.

[Roasbeef]

To talk thru a trivia case here, assume maxPeers is 1, then a new peer connects.

[erickcestari]

I've updated the code to avoid that. So now it first debit the bucket and then sweep it. I've also added more tests.