[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
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
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 - htlc_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, 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. A periodic garbage collector warns about and evicts pending HTLCs whose resolution was never observed. 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. 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 circuit keys, a nil manager is a no-op, local sends are skipped, and a hook panic is absorbed by the guard.
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.
81315c4 to
c6480fe
Compare
|
Thanks for the feedback @carlaKC Following your suggestions, I totally dropped the dev RPC methods. Added the benchmark, which shows that the reputation subsystem adds an extra Ready for another round. |
|
@carlaKC: review reminder |
|
Will get to this early this week - it's on my list! |
erickcestari
left a comment
erickcestari
left a comment
There was a problem hiding this comment.
Sorry for late review 😅
I could learn a lot about how the proposed reputation model works.
It's looking really good. It's not my final review yet, but I'll get there soon.
| // reputationMgr is the optional read-only local reputation subsystem, | ||
| // non-nil only when the experimental reputation flag is set. | ||
| reputationMgr *reputation.Manager |
There was a problem hiding this comment.
It's nil only when the experimental routing.no-reputation flag is set.
| t.Fatalf("revenue after fail: got %d, want 0", rev) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
We could also test accountable HTLCs at manager_test.go to confirm that they can decrease the channel's reputation if held.
| // TestAccountableResolution drives accountable HTLCs through the full | |
| // forward/resolve path. Unlike unaccountable HTLCs, accountable ones are | |
| // charged the opportunity cost of the time they held the outgoing slot, so | |
| // they are the only way a channel's reputation can decrease. | |
| func TestAccountableResolution(t *testing.T) { | |
| t.Parallel() | |
| // The default resolution period is 90s, so resolving at 270s overruns | |
| // it by exactly 2x: opportunity cost = 2 * fee = 2000. | |
| const ( | |
| fee = 1000 | |
| fast = 30 * time.Second | |
| slow = 270 * time.Second | |
| ) | |
| tests := []struct { | |
| name string | |
| hold time.Duration | |
| settled bool | |
| wantRep int64 | |
| wantRev int64 | |
| }{{ | |
| // Settling within the resolution period costs nothing, so the | |
| // HTLC earns its full fee just like an unaccountable one. | |
| name: "settled fast earns fee", | |
| hold: fast, | |
| settled: true, | |
| wantRep: fee, | |
| wantRev: fee, | |
| }, { | |
| // fee - 2*fee = -fee: holding the slot for too long costs more | |
| // than the forward earned. | |
| name: "settled slow costs reputation", | |
| hold: slow, | |
| settled: true, | |
| wantRep: -fee, | |
| wantRev: fee, | |
| }, { | |
| // A fast failure has no opportunity cost, but earns nothing | |
| // either. | |
| name: "failed fast is neutral", | |
| hold: fast, | |
| settled: false, | |
| wantRep: 0, | |
| wantRev: 0, | |
| }, { | |
| // A slow failure is pure cost: the fee was never earned, so | |
| // only the opportunity cost applies. | |
| name: "failed slow costs reputation", | |
| hold: slow, | |
| settled: false, | |
| wantRep: -2 * fee, | |
| wantRev: 0, | |
| }} | |
| for _, test := range tests { | |
| t.Run(test.name, func(t *testing.T) { | |
| t.Parallel() | |
| m, clk := buildManager(t, 1_000_000) | |
| in, out := circuit(1, 0), circuit(2, 0) | |
| m.OnForward( | |
| in, out, 2000, 1000, fee, 200, testHeight, true, | |
| ) | |
| advance(clk, test.hold) | |
| if test.settled { | |
| m.OnSettle(in, out) | |
| } else { | |
| m.OnFail(in, out) | |
| } | |
| outChan := m.channels[2] | |
| if len(outChan.pendingHTLCs) != 0 { | |
| t.Fatalf("pending not cleared: %d", | |
| len(outChan.pendingHTLCs)) | |
| } | |
| rep, err := outChan.outgoingReputation.valueAt(m.now()) | |
| if err != nil { | |
| t.Fatalf("valueAt: %v", err) | |
| } | |
| if rep != test.wantRep { | |
| t.Fatalf("reputation: got %d, want %d", rep, | |
| test.wantRep) | |
| } | |
| rev, err := m.channels[1].incomingRevenue.valueAt( | |
| m.now(), | |
| ) | |
| if err != nil { | |
| t.Fatalf("valueAt: %v", err) | |
| } | |
| if rev != test.wantRev { | |
| t.Fatalf("revenue: got %d, want %d", rev, | |
| test.wantRev) | |
| } | |
| }) | |
| } | |
| } |
| // Scoring reputation/revenue on the offered fee would let a | ||
| // sender inflate or destroy reputation by over/under-paying; | ||
| // the advertised fee is what the node actually charges. | ||
| advertisedFee := destination.AdvertisedFee(packet.amount) |
There was a problem hiding this comment.
Should this include the inbound fee?
CheckHtlcForward treats the fee we charge as inFee + outFee (link.go:2519-2531), but AdvertisedFee returns just outFee. So we admit the HTLC against one number and score reputation on another, and on a node with inbound fees configured the score is off by the inbound component.
The spec's fees is "the fees that are charged by the local node to forward the HTLC", which I read as the total.
| // And reputation re-accrues from live traffic: after this second | ||
| // forward+settle Bob again logs a positive reputation gain, proving the | ||
| // reset subsystem rebuilt its state from scratch. | ||
| ht.AssertNodeLogContains(bob, "Reputation gained: outgoing=") |
There was a problem hiding this comment.
nit: The comment claims it proves the reset subsystem rebuilt its state, but that's not true, since "Reputation gained: outgoing=" line is written at the first step. Instead we could count the number of times this string appear in the log.
carlaKC
left a comment
carlaKC
left a comment
There was a problem hiding this comment.
Haven't reviewed the tests in great depth, thanks for addressing previous feeback!
| // window. It backs both outgoing-channel reputation and (via | ||
| // aggregatedWindowAverage) the incoming-revenue threshold. The running value |
There was a problem hiding this comment.
nit: don't include information about usage in docs?
Applies here and in other places.
| return &decayingAverage{ | ||
| value: 0, | ||
| lastUpdated: start, | ||
| windowSecs: windowSecs, |
There was a problem hiding this comment.
nit: can inline window.Seconds
| func (d *decayingAverage) valueAt(ts uint64) (int64, error) { | ||
| if ts < d.lastUpdated { | ||
| return 0, errBackwardsTime | ||
| } |
There was a problem hiding this comment.
nit: here and a few places, use a secs suffix if we're going to use time as uint64 so that we don't confuse units.
| ) | ||
|
|
||
| // TestDecayingAverageDecay verifies the decay e^(-elapsed/window): the value | ||
| // decays to 1000/e^0.5 at half a window and to 1000/e at a full window. |
There was a problem hiding this comment.
Comment says 1000/e at a full window here, but decayRateForWindow says 1/e
| if warmup < 1 { | ||
| warmup = 1 | ||
| } |
There was a problem hiding this comment.
This isn't in the specification.
Was also pointed out in the LDK PR. I'd very strongly suggest pointing LLMs to claude (not the other impl) so that we don't perpetuate bugs.
| if at < pending.addedAt { | ||
| return errBackwardsTime | ||
| } |
There was a problem hiding this comment.
In this error case, I think it's still better to delete the htlc from our state? Otherwise it gets "stuck" and we have to gc it.
| // Log a greppable summary when a resolution moves reputation. The | ||
| // phrasing is stable: integration tests match on it. | ||
| switch { | ||
| case effFee > 0: | ||
| log.Infof("Reputation gained: outgoing=%v eff_fee=%d "+ | ||
| "new_outgoing_reputation=%d", outScid, effFee, newRep) | ||
|
|
||
| case effFee < 0: | ||
| log.Infof("Reputation lost: outgoing=%v eff_fee=%d "+ | ||
| "new_outgoing_reputation=%d", outScid, effFee, newRep) | ||
|
|
||
| default: | ||
| log.Debugf("Reputation resolve in=%v out=%v settled=%v "+ | ||
| "eff_fee=0 (neutral)", incoming, outgoing, settled) | ||
| } |
There was a problem hiding this comment.
seems a bit more easily greppable if we just have "resputation change (amount) " rather than breaking this up?
| // time. Such an eviction should never happen in normal operation (every | ||
| // forwarded HTLC is resolved by the switch), so it is logged as a warning: it | ||
| // signals that the manager's in-flight view has diverged from the switch's. | ||
| func (m *Manager) gcStalePendings() { |
There was a problem hiding this comment.
This loop is a bit code smelly to me. Leaving a htlc in flight means that we're buggy, and this will just sweep up our bugs after us (while subtly degrading the system by having too much in flight).
| // proportional fee). It is the fee the node advertised for this link, | ||
| // as distinct from the (possibly larger) fee actually offered by the | ||
| // incoming HTLC. | ||
| AdvertisedFee(amtToForward lnwire.MilliSatoshi) lnwire.MilliSatoshi |
| // guardedReputationManager wraps a ReputationManager so that a panic in any of | ||
| // its hooks can never propagate into the switch's forwarding goroutine. The | ||
| // reputation subsystem is log-only and MUST NOT be able to degrade forwarding; | ||
| // if a hook panics we log it, permanently disable the subsystem (fail open), | ||
| // and continue forwarding unaffected. |
There was a problem hiding this comment.
This is also a bit code-smelly to me.
Is this standard practice for LND to add this type of gating to new features?
| package lnd | ||
|
|
||
| import ( | ||
| "github.com/lightningnetwork/lnd/htlcswitch" | ||
| "github.com/lightningnetwork/lnd/reputation" | ||
| ) | ||
|
|
||
| // reputationManagerAdapter bridges the reputation.Manager to the switch's | ||
| // read-only htlcswitch.ReputationManager seam. The manager's hook signatures | ||
| // already match the switch interface (both use models.CircuitKey and | ||
| // lnwire.MilliSatoshi), so this is a thin, explicit bridge that keeps the | ||
| // switch from importing the reputation package directly. | ||
| type reputationManagerAdapter struct { | ||
| *reputation.Manager | ||
| } | ||
|
|
||
| // Compile-time assertion that the adapter satisfies the switch's read-only | ||
| // reputation seam. | ||
| var _ htlcswitch.ReputationManager = (*reputationManagerAdapter)(nil) | ||
|
|
||
| // newReputationManagerAdapter wraps a reputation.Manager as an | ||
| // htlcswitch.ReputationManager. | ||
| func newReputationManagerAdapter( | ||
| m *reputation.Manager) htlcswitch.ReputationManager { | ||
|
|
||
| return &reputationManagerAdapter{Manager: m} | ||
| } |
There was a problem hiding this comment.
*reputation.Manager already satisfies htlcswitch.ReputationManager, so the adapter isn't needed.
| // OnForward observes a forwarded HTLC at the point the switch | ||
| // commits to forwarding it to the outgoing channel. advertisedFee is | ||
| // the fee the node advertised on the outgoing link for this forward | ||
| // (not the fee offered by the incoming HTLC), height is the switch's | ||
| // current best block height, and accountable is the outgoing | ||
| // accountable bit as this node would forward it. | ||
| OnForward(incoming, outgoing CircuitKey, incomingAmt, | ||
| outgoingAmt, advertisedFee lnwire.MilliSatoshi, | ||
| incomingCltv, height uint32, accountable bool) |
There was a problem hiding this comment.
We may want to document that the outgoing CircuitKey has a zero HtlcID at forward time. Or We could change the interface for outoing be only a ShortChannelID instead of CircuitKey.
| // OnForward observes a forwarded HTLC at the point the switch | |
| // commits to forwarding it to the outgoing channel. advertisedFee is | |
| // the fee the node advertised on the outgoing link for this forward | |
| // (not the fee offered by the incoming HTLC), height is the switch's | |
| // current best block height, and accountable is the outgoing | |
| // accountable bit as this node would forward it. | |
| OnForward(incoming, outgoing CircuitKey, incomingAmt, | |
| outgoingAmt, advertisedFee lnwire.MilliSatoshi, | |
| incomingCltv, height uint32, accountable bool) | |
| // OnForward observes a forwarded HTLC at the point the switch | |
| // commits to forwarding it to the outgoing channel. advertisedFee is | |
| // the fee the node advertised on the outgoing link for this forward | |
| // (not the fee offered by the incoming HTLC), height is the switch's | |
| // current best block height, and accountable is the outgoing | |
| // accountable bit as this node would forward it. | |
| // | |
| // NOTE: only outgoing.ChanID is populated here; outgoing.HtlcID is | |
| // always zero. The switch calls this before handing the packet to the | |
| // outgoing link, and the outgoing HTLC ID is only assigned once that | |
| // link adds the HTLC to its commitment, so the keystone does not exist | |
| // yet. Implementations must not key state on the full outgoing | |
| // CircuitKey at forward time: OnSettle/OnFail receive the real | |
| // keystone, so the two ends would not match. Key on the incoming | |
| // CircuitKey instead, which is stable across the whole lifecycle. | |
| OnForward(incoming, outgoing CircuitKey, incomingAmt, | |
| outgoingAmt, advertisedFee lnwire.MilliSatoshi, | |
| incomingCltv, height uint32, accountable bool) |
There was a problem hiding this comment.
Perhaps just use the outgoing channel (not the circuit key)? We don't have use for the ID anyway iirc
|
Another thing that's worth taking a look at here is how this will work with non-strict forwarding. I think we'll report one outgoing channel on add and resolve with a different one - worth confirming with a test that we can handle it gracefully! |
4 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