[1/?] Local reputation: subsystem core, read only#10919
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 |
1be208a to
5bdb907
Compare
github-actions
Bot
added
severity-critical
615d701 to
516c694
Compare
516c694 to
8813fe2
Compare
|
Chatted to @GeorgeTsagk about strategies to break up this PR up and lighten review burden on the LND team! PR BreakdownI was talking to claude about this, and produced this plan, but zero promises because I haven't even read it - just an artifact from this discussion! (commits marked with * are dead code for the sake of incremental steps, could be squashed if that's not okay) 1. Implement reputation tracking*
2. Connect to switch
3. Restarts and in-flight
Once we get to this point, we get a very rudimentary "would this HTLC in isolation be able to enter the protected bucket (if needed)" sanity check. It doesn't take into account that there may be other HTLCs in flight, or whether we'll actually need to use protected resources, but this is a very valuable sanity check that we can't otherwise obtain with the data that's currently surfaced in LND (because we don't have historical failed forwards). 4. Implement bucketing logic*
5. Utilize buckets
Other RPCs/snapshots can be added after that, but if the majority of folks aren't running LND with Review@elnosh and I are happy to review here! We'll be able to provide strong reviews on the jamming work, since it's our focus. I should be able to provide reasonable review on the switch interactions, though my view of this system is of course a few years stale! |
|
Thanks @carlaKC for writing the summary. So I believe the next step here is to strip some things away from this PR and only keep 1 & 2:
This should leave us with a more minimal & lean diff, leaving out any noisy parts related to restarts/persistence and cold start. Another comment on this strategy: if we ever deploy 1&2, then reputation systems in the wild will already start recording values from forwarding, at that point I don't think it would make sense to ship historical-read as a follow-up update to this system, we are practically doing a slow-bootstrap already. |
Yeah SGTM! If we're okay with a bit of temporarily dead code, I think it makes sense to do 1 / 2 as separate PRs for the sake of small incremental steps. That's a question of project preferences, so depends on how LND prefers to do things nowadays.
Indeed! We do need 6 months data to get realreal values, so perhaps for (3) we could just focus on persistence, because we won't get far if we lose all our data every time we restart. Just 2x fields per channel, so not too bad! @erickcestari also agreed to help out with review ❣️ |
8813fe2 to
d061d4a
Compare
github-actions
Bot
added
severity-critical
cbfa4d6 to
304508e
Compare
Add the log-only local reputation subsystem: per-channel decaying-average outgoing reputation and aggregated-window incoming revenue, in-flight HTLC risk tracking, and the per-HTLC isolation decision. A single worker goroutine owns the channel-state maps lock-free, draining a bounded event queue fed by the non-blocking OnForward/OnSettle/OnFail hooks. Also add Manager.Snapshot(), a read-only view of the computed per-channel state (scid, outgoing reputation, incoming revenue, in-flight risk, pending count and the at-rest sufficiency verdict). Because the maps are owned solely by the worker, Snapshot dispatches a snapshot request through the worker's event queue and blocks on a reply channel rather than reading the maps directly, keeping the single-owner, lock-free invariant race-free. Decayed values are computed via non-mutating peekAt helpers so a snapshot never advances the live state.
Unit tests for the reputation subsystem: the decaying-average and aggregated-window arithmetic, config validation, the in-flight risk and effective-fee matrix, the forward/settle/fail lifecycle on the worker, and the read-only Snapshot() path (including its worker-request race-safety and non-mutating peekAt helpers).
Wire the log-only reputation subsystem into the forwarding path. The switch grows read-only OnForward/OnSettle/OnFail hooks (guarded so a hook panic can never take down forwarding), the server constructs and starts the manager behind the experimental --routing.reputation flag, and an adapter feeds the best block height into the in-flight risk calculation off the hot path. The subsystem is enabled by default; it can be disabled with the opt-out routing.no-reputation flag.
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 proto message. 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).
Add seam tests for the switch reputation hooks and the panic guard, and an end-to-end integration test that runs a forwarding node with --routing.reputation. The itest asserts the log-only invariant (a successful forward, a failed forward and a restart never affect routing) and reads the exact computed state back over the new devrpc FetchReputation RPC: on a fresh channel a single fast settle earns the outgoing channel exactly the advertised forwarding fee as reputation and moves the incoming channel's revenue positive. After a restart the snapshot is empty (no persistence) and re-accrues from live traffic, confirming the documented self-bootstrapping reset.
Document the experimental log-only local reputation subsystem and its read-only FetchReputation devrpc introspection RPC (and lncli command).
304508e to
81315c4
Compare
GeorgeTsagk
changed the title
|
Ok marking this as ready for review, it now adds:
|
GeorgeTsagk
added
routing
channel jamming
2 participants
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 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
[ ] (?) Handle cold start (historical traffic read)for 2nd part[x] (?) Properly handle in-flight HTLCs when restartingfor 2nd part