Skip to content

multi: bound peer message subscriptions - #11159

Closed
moscowchill wants to merge 2 commits into
lightningnetwork:masterfrom
moscowchill:fix/bound-custom-message-subscriptions
Closed

moscowchill wants to merge 2 commits into
lightningnetwork:masterfrom
moscowchill:fix/bound-custom-message-subscriptions

Conversation

@moscowchill

@moscowchill moscowchill commented Sep 1, 2026 •

Copy link
Copy Markdown

Change Description

Custom-message and onion-message RPC subscribers currently use an unbounded per-client queue. If a stream stops accepting responses while peer messages continue to arrive, its queue retains those messages without a memory limit.

This PR adds an opt-in bounded mode to the subscription server and uses it for both peer-message RPCs. Each client may retain 100 pending messages, which limits maximum-sized queued payloads to less than 6.3 MiB. A client that exceeds the limit is removed and its queued references are drained immediately; healthy clients continue receiving ordered updates without producer blocking.

Slow-consumer eviction is reported as gRPC ResourceExhausted once the stream can make transport progress. Existing users of subscribe.NewServer() keep their current queue behavior.

Steps to Test

go test ./subscribe -count=20
go test . -count=1
go test -race ./subscribe -count=1
go test -race . -run '^(TestSlowSubscriptionClientError|TestSubscribeCustomMessagesSlowClient)$' -count=10

Pull Request Checklist

Testing

  • Your PR passes all CI checks.
  • Tests covering the positive and negative error paths are included.
  • The bug fix contains a blocked-stream regression test.

Code Style and Documentation

  • The change is substantial and focused.
  • The change follows the code documentation and 80-column guidelines.
  • The commit follows the ideal Git commit structure.
  • New logging uses an appropriate subsystem and level.
  • No lncli command is added.
  • A release-note entry is included.

Custom-message and onion-message RPC clients currently use unbounded per-client queues. A stalled stream can retain peer-controlled messages without limit.

Add opt-in bounded queues, evict slow clients with ResourceExhausted, drain their backlog, and preserve legacy behavior for other subscription users.

Signed-off-by: moscowchill <gasgeverij@proton.me>
Signed-off-by: moscowchill <gasgeverij@proton.me>
@moscowchill
moscowchill marked this pull request as ready for review September 1, 2026 18:23
@github-actions github-actions Bot added the severity-critical Requires expert review - security/consensus critical label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

🔴 PR Severity: CRITICAL

file changes | 6 files | 421 lines changed

🔴 Critical (2 files)
  • rpcserver.go - core RPC server coordination logic
  • server.go - core server coordination logic
🟡 Medium (1 file)
  • subscribe/subscribe.go - internal pub/sub event dispatch, not otherwise categorized
🟢 Low (3 files)
  • docs/release-notes/release-notes-0.22.0.md - release notes only
  • rpcserver_test.go - test-only changes
  • subscribe/subscribe_test.go - test-only changes

Analysis

The PR modifies rpcserver.go and server.go, both explicitly listed as core server-coordination files requiring expert review. The changes there are modest in size (35 and 14 lines respectively), and the accompanying subscribe/subscribe.go rework (148 lines) backs a new subscription/notification path exercised by the RPC layer. File count and total non-test/non-generated line count stay well below the bump thresholds, and only one critical package (server coordination) is touched, so no severity bump applies beyond the base CRITICAL classification driven by rpcserver.go/server.go.


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

@saubyk

saubyk commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR, @moscowchill.

Our contribution guidelines note that PRs from new contributors aren't prioritized for review at the moment. With the backlog we have, that means closing this instead of leaving it in limbo.

If you hit an actual failure that motivated this change, an issue with the reproduction details (lnd version, backend, logs, expected vs. observed behavior) would be much more useful to us than the patch on its own. We'll triage it from there.

If you'd like to contribute going forward, reviewing open PRs and helping triage issues is the path we recommend. It's a stronger signal of understanding than a first patch, and it makes it a lot easier for us to prioritize your PRs later.

Thanks for understanding.

@saubyk saubyk closed this Sep 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

severity-critical Requires expert review - security/consensus critical

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants