peer: bound peer-controlled resource growth - #11090
Conversation
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (1 file)
🟢 Low (3 files)
AnalysisThe substantive change is entirely in To override, add a |
yyforyongyu
added
backport-v0.20.x-branch
85a430b to
bbada64
Compare
|
|
||
| // pongReplyLimiter and pingFloodLimiter enforce the two-tier inbound | ||
| // ping policy for this connection. | ||
| pongReplyLimiter *rate.Limiter |
There was a problem hiding this comment.
just name it pongLimiter and pingLimiter?
| // outgoingMsg interface box, and list element, which we round up here. | ||
| // Charging this overhead makes cheap-message floods visible to the byte | ||
| // budget instead of treating shared Pong payloads as free. | ||
| queuedMsgOverhead = 128 |
There was a problem hiding this comment.
all these consts are flying around, i wonder if we could turn them into a struct or config
| } | ||
| popMsg(front) | ||
| case msg := <-p.outgoingQueue: | ||
| if msg.priority { |
There was a problem hiding this comment.
i wonder if we should stop modifying this loop? feels like a layer violation as the abstraction is broken? the queuing layer should not be aware of the ping/pong context?
bbada64 to
1fef25c
Compare
Bound Pong replies with separate reply and flood token buckets. Count every decoded Ping before generic routing. Disconnect peers that exhaust the flood budget through a stable sentinel error.
Exercise exact production burst, refill, and Pong-size boundaries. Retain deterministic cadence and reply-suppression coverage without relying on wall-clock scheduling.
Use a mock message router that would consume an oversized Ping. Prove flood accounting disconnects first and preserves the sentinel error for operator diagnostics.
Collect count, retained-memory, and fixed-overhead thresholds in one private policy. Keep wire-specific cost estimation with that policy. Leave the generic queue independent of message semantics.
Calculate retained-memory charges when messages enter the peer queue. Carry them as immutable metadata. Keep generic priority queue accounting independent of wire types.
Route queueHandler through the bounded queue abstraction and disconnect when either the count or retained-memory budget is exceeded. Use one nil-gated send case for empty and non-empty states, and keep servicing producers during teardown so disconnect cannot deadlock.
Exercise count and retained-memory overflow, backlog draining, and teardown servicing. Use declarative workloads with fresh retained onion payloads so the byte-bound regression matches production ownership.
Document the ping reply rate and outgoing queue bounds in both supported patch release notes so operators know that peer-controlled resource exhaustion is mitigated.
1fef25c to
c8b6718
Compare
2 participants
Change Description
Bound the resources a peer can consume through inbound pings and queued
outgoing messages.
Inbound ping handling now has two limits:
that reply budget are accepted without a Pong.
This flood check also covers pings whose requested Pong size would otherwise
be ignored.
The per-peer outgoing backlog is also capped at 10,000 messages and 16 MiB of
charged memory. Queue accounting covers fixed overhead and the high-risk
dynamic payloads without serializing messages on the hot path. An overflowing
peer is disconnected while the queue remains serviced until teardown, avoiding
deadlocks with synchronous senders.
Release notes are included for 0.21.3 and 0.20.4.
Open Question
The reply budget deliberately deviates from BOLT 1's requirement to answer
valid pings. A peer using those pings for liveness may close the connection when
its Pong is skipped, so the 1-10 ping/second band can relocate the disconnect
decision to the remote peer and make lnd appear unresponsive. Should the two
tiers remain, or should they be collapsed into the flood disconnect threshold?
This draft implements both tiers so reviewers can evaluate the concrete
behavior.
Steps to Test
The queue-accounting and teardown-drain regression tests were also mutation
tested by removing their respective implementation lines and confirming each
test failed.
Pull Request Checklist
Testing
Code Style and Documentation