Repository navigation
[1/?] Local reputation: subsystem core, read only - #10919
GeorgeTsagk wants to merge 6 commits into
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
304508e to
81315c4
Compare
GeorgeTsagk
changed the title
|
Ok marking this as ready for review, it now adds:
|
GeorgeTsagk
added
routing
channel jamming
carlaKC
left a comment
carlaKC
left a comment
There was a problem hiding this comment.
Primarily reviewed the first commit, haven't looked at the tests yet.
High level thoughts:
- I think it's worth spending a bit more time thinking about how this interacts with the switch, and whether a queue is the right call here.
- There are a few places where this can be better aligned with how LND does things, both major things like using existing interfaces and shorter comments
- I am concerned by pointing a LLM at the LDK pr, it puts us at risk of propagating bugs and makes the process of improving the spec by having to implement it weaker
- Snapshot and dev rpc are pretty low value IMO, would far rather see benchmarking
0ebdac9 to
babef31
Compare
elnosh
left a comment
elnosh
left a comment
There was a problem hiding this comment.
changes addressing previous review look good to me.
On zero-fee policies I'd leave it as is, such channels simply never accrue reputation, which fails on the strict side.
in this purely log phase, either seems fine. But noting that if a node sets its fees to 0 across all its channels, it means nothing in protected bucket is "protected". Everything is a function of the fees so outgoing_channel_reputation - in_flight_risk >= incoming_revenue_threshold is true. Meaning, everything is accepted.
|
@carlaKC: review reminder |
2 similar comments
|
@carlaKC: review reminder |
|
@carlaKC: review reminder |
babef31 to
a79ffec
Compare
erickcestari
left a comment
erickcestari
left a comment
There was a problem hiding this comment.
LGTM!
The best of my knowledge everything looks good to me. Nice work!
elnosh
left a comment
elnosh
left a comment
There was a problem hiding this comment.
LGTM.
given my context, mostly focused on the reputation manager and how it does all the reputation/revenue tracking based on the HTLCs.
0fc3dbd to
1e348d3
Compare
carlaKC
left a comment
carlaKC
left a comment
There was a problem hiding this comment.
Small q and one more thing from me.
I'm working on some spec updates to get to the latest and greatest. Will push those today for comparison (a few changes came up during the LDK impl which we believed warranted changes).
|
Latest and greatest is up! lightning/bolts#1280 (commits) |
1e348d3 to
9ef3e81
Compare
Add the numeric primitives underlying local reputation scoring, following the "Decaying Average" and "Revenue Threshold Aggregation" sections of BOLT lightningnetwork#1280, plus a package README describing the subsystem: - saturatedI64: int64 arithmetic that clamps rather than wraps, so the long-window fee accumulators never silently flip sign. - decayingAverage: a value decaying as e^(-elapsed/window) per the spec's decay_rate. - aggregatedWindowAverage: a decaying average over several windows with the spec's exponential warm-up factor.
Add the per-channel reputation state and the BOLT lightningnetwork#1280 scoring rules built on the decaying-average primitives: - Config: the tunable parameters (resolution period, revenue window, reputation multiplier, revenue window count) with the spec defaults. - effectiveFee/opportunityCost/inFlightRisk: an HTLC's contribution to reputation and its worst-case in-flight risk. - channelReputation: the per-channel outgoing reputation, incoming revenue threshold and pending HTLCs, plus the sufficiency inequality outgoing_reputation - risk >= revenue_threshold.
Add the Manager that ties the scoring together behind the OnForward/OnSettle/ OnFail hooks. The hooks run synchronously under a single lock: OnForward records the pending HTLC and computes (and logs) the reputation decision, both for the HTLC in isolation and against the risk already in flight on its outgoing channel, while OnSettle/OnFail resolve it and update the outgoing reputation and incoming revenue averages. The subsystem is log-only and holds no persisted state, so reputation re-accrues from live traffic after a restart. Every resolution drops its own pending HTLC, so a pending that outlives the worst case time it could be held for means a resolution was never reported to us. A periodic check warns about those and deliberately leaves them in place rather than sweeping them away, so the underlying bug stays visible. Includes unit tests and benchmarks for the per-forward hook cost.
Feed forwarded HTLCs to the reputation subsystem through a read-only seam on the switch. The switch calls OnForward/OnSettle/OnFail at the circuit layer behind a nil check, so the subsystem is skipped entirely when disabled. The manager is wrapped in a panic boundary before being handed to the switch: a bug in the (log-only) subsystem can never take down HTLC forwarding. Only the outgoing channel is reported to the subsystem, not an outgoing circuit key: at forward time the switch has not yet handed the packet to the outgoing link, so no outgoing HTLC ID exists yet. The subsystem is enabled by default and can be disabled with the new routing.no-reputation flag. Includes unit tests for the switch seam: each hook fires once with the right keys, a nil manager is a no-op, local sends are skipped, a hook panic is absorbed by the guard, and a non-strict forward reports the channel the HTLC actually went out on for both the add and its resolution.
Add an integration test asserting that a forwarding node running the log-only reputation subsystem forwards, fails and restarts exactly as it would without it, while emitting the expected reputation log lines.
9ef3e81 to
e4675bb
Compare
6 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