Add local reputation subsystem (read-only)#10919
Draft
GeorgeTsagk wants to merge 8 commits into
Draft
Conversation
github-actions
Bot
added
the
severity-critical
PR Severity: CRITICAL
CRITICAL (4 files)
MEDIUM (16 files)
LOW (13 files -- excluded from counts)
AnalysisThis PR introduces a new channel reputation system for HTLC jamming mitigation. The critical classification is driven by direct modifications to Key concerns warranting careful review:
Both severity-bump thresholds are exceeded (21 non-test files, ~3,427 non-test lines), but the base severity was already CRITICAL. To override, add a |
GeorgeTsagk
force-pushed
the
local-reputation-subsystem
branch
2 times, most recently
from
1be208a to
5bdb907
Compare
github-actions
Bot
added
severity-critical
GeorgeTsagk
force-pushed
the
local-reputation-subsystem
branch
3 times, most recently
from
615d701 to
516c694
Compare
23 tasks
Add a self-contained, log-only subsystem implementing the local resource conservation scheme from the lightning/bolts proposal (outgoing reputation + HTLC accountability), modelled on rust-lightning's resource_manager. The subsystem observes forwarded HTLCs and computes, per channel: an outgoing-channel reputation (decaying average of effective fees), an incoming-channel revenue threshold, a sufficiency verdict, and a general/congestion/protected resource-bucket assignment. It is strictly observational: it never fails, delays, or re-prioritises an HTLC and never touches the wire. Reputation is built only from live traffic (no historical read); decaying-average state is persisted so it survives restarts, while in-flight HTLC tracking is discarded on restart (a documented log-only trade-off). Arithmetic mirrors the LDK reference so the two agree numerically. When a channel needs every slot in the general bucket (small channels whose per-channel allocation meets or exceeds the bucket's total slot count), the slots are now assigned deterministically rather than drawn from the ChaCha20 keystream, which could otherwise fail to converge within the attempt budget and make slot assignment intermittently error.
Add three nil-guarded hooks at the switch's circuit-matching layer (handlePacketAdd/handlePacketSettle/handlePacketFail) that feed forwarded HTLC add/settle/fail events to an optional ReputationManager. The switch defines the interface; it never imports the reputation package, keeping the dependency a black box that can only call observation methods. The hooks pass copies of scalar values (circuit keys, amounts, cltv, the accountable bit) — no pointers into switch state — so they cannot mutate forwarding. A guardedReputationManager wraps the manager in a recover() boundary: because the hooks run on the forwarding goroutine, a panic in the (log-only) subsystem must never take down HTLC forwarding, so on a panic the guard logs, disables the subsystem, and forwarding continues unaffected.
Construct the local reputation manager in the server when the experimental routing.reputation flag is set (default off), and wire it into the switch via a small adapter that bridges the switch's ReputationManager interface to the reputation package. When the flag is off the manager is nil and the hooks are no-ops with zero overhead, so the switch is byte-for-byte unchanged. This also registers the REPM subsystem logger with the root logger; without it the log-only subsystem would emit nothing in a running node.
Add an integration test that runs a forwarding node with the reputation subsystem enabled through a successful forward, a downstream-failed forward, and a warm restart. It asserts non-interference (payments behave exactly as without the subsystem) and, via a new HarnessTest.AssertNodeLogContains helper, that the node actually computes reputation by matching the "Reputation gained" log line after a forward. After the warm restart it asserts the node keeps forwarding successfully; post-restart reputation state is verified in a later commit via the FetchReputation read-out (a post-restart log re-assertion would be tautological, since lnd's log rotator appends to the same file and the pre-restart line survives).
Previously the local reputation subsystem discarded all in-flight HTLCs on
restart: Manager.ReplayInFlight was implemented and unit-tested but never
called, so the pending set started empty and resolutions of HTLCs that
spanned the restart were tolerated as no-ops. That under-counts in-flight
risk and bucket occupancy, and (once enforcement lands) is an attack surface
where a restart wipes a peer's accountability.
Wire it up. The switch circuit map retains only the four cheap fields per
open circuit (incoming/outgoing keys + amounts); the two missing fields are
recovered from the live incoming-channel commitment HTLC:
- IncomingCltv: HTLC.RefundTimeout.
- Accountable: the experimental accountable custom record (TLV 106823,
value 7), which is persisted in the incoming update_add_htlc custom
records and round-trips through OpenChannel ExtraData, so it IS
recoverable after a restart (no guessing / no default needed).
Add a read-only ActiveCircuits() snapshot accessor to the circuit map (and a
Switch passthrough) to enumerate open keystoned circuits. In server.go, after
the switch starts, join those circuits (filtered to real forwards:
Incoming.ChanID != hop.Source and keystone set) with FetchAllOpenChannels()
ActiveHtlcs() by (incoming chan id, incoming htlc id) and feed the result to
ReplayInFlight, guarded by s.reputationMgr != nil. Circuits whose incoming
HTLC is no longer live (resolved/closed concurrently with startup) are
skipped rather than replayed with bogus metadata.
The join logic lives behind small seams (circuitSource/openChannelSource) in
reputation_adapter.go and is unit-tested with fakes. DESIGN.md §6/§9 updated:
in-flight HTLCs are now reconstructed (not discarded) and ReplayInFlight is
wired (no longer parked).
Add Snapshot(), a pure read that returns a copy of per-channel computed reputation state under the manager lock, mirroring the snapshot-under-lock pattern used by the persistence flush. Per channel it exposes the scid, outgoing reputation, incoming revenue threshold, current in-flight risk, the at-rest sufficiency verdict, pending-HTLC count, and the slot/liquidity occupancy of the general/congestion/protected buckets. Decayed values are computed via new non-mutating peekAt helpers on the decaying and aggregated-window averages, so a snapshot never advances the live state. Unit-tested for the forward+settle lifecycle, at-rest emptiness, and non-mutation.
Expose the local reputation subsystem's computed state through a new unary, read-only FetchReputation method on the devrpc (dev/debug) sub-server. The handler calls Manager.Snapshot() and marshals the per-channel state into the new ChannelReputation / BucketOccupancy proto messages. The reputation manager is wired into the devrpc Config through PopulateDependencies. It is nil unless the experimental --routing.reputation flag is set; the handler is nil-safe and returns a clear "reputation subsystem disabled" error in that case. The method requires a read-only offchain macaroon permission, and an lncli "fetchreputation" command and a harness RPC wrapper are added to drive it. Generated proto stubs regenerated via 'make rpc' (docker-based gen_protos).
After the successful Alice->Bob->Carol forward settles, the local reputation itest now calls the new devrpc FetchReputation RPC on Bob and asserts the exact computed values instead of only matching a log line: the outgoing (Bob->Carol) channel's reputation increased by exactly the forwarding fee Bob earned (read as ground truth from his forwarding history), the incoming (Alice->Bob) channel's revenue moved by the same fee, and all bucket occupancy and pending state returned to zero once the HTLC resolved. Forwarding events flush on a periodic ticker, so the read of Bob's forwarding history is wrapped in wait.NoError and polled until exactly one event is reported before the fee is read, instead of asserting the length once and racing the flush. After the warm restart the test fetches the reputation snapshot again and asserts the persisted values reloaded unchanged (the outgoing reputation is still positive and both values match their pre-restart figures up to integer rounding over the decay window). This replaces the previous, tautological post-restart 'Reputation gained' log re-assertion (lnd's log rotator appends to the same file, so the pre-restart line survives a restart regardless of whether any recomputation happened). The existing 'Reputation gained' log assertion before the restart is kept as the synchronization point that the resolution has been observed.
GeorgeTsagk
force-pushed
the
local-reputation-subsystem
branch
from
516c694 to
8813fe2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a subsystem that implements local reputation as proposed here.
You can read more about channel jamming mitigationa here.
The current goal is to only record and calculate revenue/reputation averages and implement the unprotected/protected buckets in a log-only mode, meaning that:
This PR aims to be non-invasive to existing HTLC forwarding code paths. A reviewer treating the reputation subsystem as a black-box should be confident that by recording HTLC events via the reputation subsystem we're not interrupting any other operation.
Checklist for undrafting