[1/?] Local reputation: subsystem core, read only

[GeorgeTsagk]

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:

• we record HTLC add/settle/fail times
• we don't affect HTLC forwarding at all: an HTLC rejection due to insufficient reputation is a no-op
• we log individual HTLC "mock" decision, "would this HTLC make it into protected slots?"

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

• Break up commits into smaller self-explanatory ones
• Add better itest coverage (probably a debug API to verify reputation numbers as well)
• [ ] (?) Handle cold start (historical traffic read) for 2nd part
• [x] (?) Properly handle in-flight HTLCs when restarting for 2nd part

[github-actions]

PR Severity: CRITICAL

Automated classification | 21 non-test files | ~3,427 lines changed (excluding tests/generated)

CRITICAL (4 files)

• htlcswitch/interfaces.go - htlcswitch package; HTLC forwarding/payment routing state machine
• htlcswitch/reputation_guard.go - htlcswitch package; new reputation gating for HTLC forwarding
• htlcswitch/switch.go - htlcswitch package; core switch integration
• server.go - Core server coordination

MEDIUM (16 files)

• lncfg/routing.go - lncfg/* routing config
• log.go - top-level logger registration
• reputation_adapter.go - adapter wiring reputation manager into server
• reputation/buckets.go - new reputation package (uncategorized)
• reputation/channel.go - new reputation package
• reputation/channels.go - new reputation package
• reputation/clock.go - new reputation package
• reputation/config.go - new reputation package
• reputation/decaying_average.go - new reputation package
• reputation/decision.go - new reputation package
• reputation/htlc.go - new reputation package
• reputation/log.go - new reputation package
• reputation/manager.go - new reputation package
• reputation/manager_startup.go - new reputation package
• reputation/revenue.go - new reputation package
• reputation/store.go - new reputation package

LOW (13 files -- excluded from counts)

• htlcswitch/reputation_hooks_test.go, reputation/*_test.go -- test files
• itest/list_on_test.go, itest/lnd_reputation_test.go -- integration tests
• lntest/harness_assertion.go -- test harness
• reputation/DESIGN.md -- documentation

Analysis

This PR introduces a new channel reputation system for HTLC jamming mitigation. The critical classification is driven by direct modifications to htlcswitch -- one of lnd's most sensitive packages governing HTLC forwarding and the payment routing state machine -- and to server.go (core server coordination).

Key concerns warranting careful review:

• htlcswitch/switch.go: Integration of reputation gating into the HTLC forwarding path. Any regression could cause incorrect HTLC accept/reject decisions, affecting payment reliability.
• htlcswitch/reputation_guard.go (new file, 82 lines): New guard logic sitting in the critical forwarding path.
• htlcswitch/interfaces.go: Interface additions that all implementors must satisfy; watch for subtle behavioral changes.
• reputation/manager.go (650 lines): Large new manager with in-memory state, startup logic, and a backing store -- persistence correctness and concurrency safety should be verified.

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 severity-override-{critical,high,medium,low} label.
<!-- pr-severity-bot -->

[carlaKC]

Chatted to @GeorgeTsagk about strategies to break up this PR up and lighten review burden on the LND team!

PR Breakdown

I 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*

• Decaying averages
• Tracking peers reputation
• Ability to add/remove HTLCs to this system

2. Connect to switch

• Report HTLCs to reputation manager (including on restart)
• Set experimental field based on accountable and reputation signal
• Log reputation decision for HTLCS
• No persistence, no loading historical forwards
• Minimal tracking of current HTLC set (if required to update reputation)
• Log HTLC reputation data

3. Restarts and in-flight

• Persist revenue and reputation for peers
• Perform "best effort" load from historical forwards if DB values are missing

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*

• General bucket slot tracking
• Bucket state management
• Benchmarks for performance (this is a place we've identified we need to be careful!)

5. Utilize buckets

• Add decision making for bucket + reputation
• Persistence of bucket data for in-flight HTLCs
• Connect manager to bucket system

Other RPCs/snapshots can be added after that, but if the majority of folks aren't running LND with dev server then I think the value of surfacing this information in separate APIs is minimal. Perhaps could think about adding to more mainstream APIs (like listchannels), but that decision doesn't need to happen now IMO.

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!

[GeorgeTsagk]

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:

• Decaying averages & reputation
• HTLC switch read-only hooks (feeding HTLCs to the system)
• Individual HTLC mock-decision log (no buckets) i.e "if this HTLC was being forwarded in isolation, could it be protected"

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.

[carlaKC]

So I believe the next step here is to strip some things away from this PR and only keep 1 & 2

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.

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.

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 ❣️

[GeorgeTsagk]

Ok marking this as ready for review, it now adds:

• reputation subsystem and related math (+tests)
• hooks into htlcswitch -> feeding HTLC traffic data into the system
• devRPC methods to help expose internal values
• basic e2e itest

[carlaKC]

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

[GeorgeTsagk]

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!

Checked, we call the reputation hook right after the strict pick takes place. So it's the real pick from the get go:

lnd/htlcswitch/switch.go

Lines 3038 to 3068 in 85bd467

	packet.outgoingChanID = destination.ShortChanID()
	// Feed the (read-only) reputation manager this forward. This only
	// observes the event to update internal reputation state; it never
	// affects the forwarding decision (log-only).
	if s.cfg.ReputationManager != nil {
	// Use the fee the node ADVERTISED on the outgoing link for this
	// forward (base fee + proportional over the outgoing amount),
	// not the (possibly larger) fee offered by the incoming HTLC.
	// 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)
	// Derive the outgoing accountable bit exactly as the outgoing
	// link does: only accountable if we received it accountable AND
	// this node forwards the experimental accountability signal. A
	// node running --protocol.no-experimental-accountability drops
	// the bit, so it must not penalise a peer never told the HTLC
	// was accountable.
	outgoingAccountable := htlcAccountable(htlc) &&
	s.shouldFwdExpAccountability()
	s.cfg.ReputationManager.OnForward(
	CircuitKey{
	ChanID: packet.incomingChanID,
	HtlcID: packet.incomingHTLCID,
	},
	packet.outgoingChanID, packet.incomingAmount,
	packet.amount, advertisedFee, packet.incomingTimeout,
	s.BestHeight(), outgoingAccountable,

Edit: will add a test, won't hurt

[GeorgeTsagk]

Thanks for the feedback @erickcestari and @carlaKC. I have addressed your comments (some threads still open for discussion, will resolve async as we figure things out).

[litbot-9000]

@carlaKC: review reminder
@erickcestari: review reminder

[elnosh]

Apologies for a late review. I am, of course, not that deeply familiar with LND codebase so these are just some general observations more directed towards the jamming side implementation in the reputation package as I don't have that much context on all the htlcswitch inner workings.

Just to clear a doubt - the README says "It holds no persisted state, so reputation resets on restart and re-accrues from live forwarding traffic." but this comment says that persistence will be added. I assume there will be follow-up PR to add persistence of at least the reputation and revenue values? otherwise it would be very fragile to lose all this historical behavior after a restart.

One other high-level-ish note is if there should be any fallback handling for nodes that set fees to 0. I just looked at the graph and there a few policies with both base and proportional fees to 0 so there could be a fallback "sane default" fee policy for such channels but I don't think it should be that much of a deal.

Last thing, I think capturing "will this node have reputation for this sole HTLC" is the more useful piece of information but the current impl will effectively only measure that (?) if no one flips the accountable signal then there will be no accountable HTLCs contributing to a total in-flight risk.

[elnosh]

small note - I agree with this behavior and think we should probably change the spec to account for it

[elnosh]

seems there is no way to remove channels from the manager. Perhaps not that big of a deal right now but could be good to add way to remove closed channels from the manager.

[GeorgeTsagk]

Agreed, though not urgent while this is in-memory and log-only. The follow-up persistence PR has to deal with channel lifecycle anyway, so I'd handle removal of closed channels there.

[erickcestari]

OnFail reports outgoing scid 0 whenever the outgoing link rejects an add, so the manager never releases the pending HTLC it recorded on the forward.

memoryMailBox.FailAdd builds a fresh fail packet and doesn't copy outgoingChanID (mailbox.go:736), and the keystone was never set because the link only assigns it after AddHTLC succeeds (link.go:1668). So both sources in handlePacketFail (switch.go:3219-3221, here) are empty, resolveHTLC misses on m.channels[0] (er) and returns nil.

I've made this test at reputation_hooks_test.go that should pass after adding the outgoingChanID value on the htlcPacket struct that memoryMailBox.FailAdd creates.

// TestSwitchReputationLocalFailAddResolvesPending drives the real
// memoryMailBox.FailAdd path end to end and asserts the reputation manager is
// told to resolve the HTLC against the same outgoing channel it was told to
// track it under. Matching keys are what let the manager release the pending
// HTLC it recorded on the forward.
//
// The add is delivered to the outgoing link but never committed, so no circuit
// keystone is written. FailAdd is then invoked exactly as the link does when
// channel.AddHTLC rejects the HTLC, or as the mailbox does when the delivery
// deadline expires. On that path the fail packet is the only remaining record
// of which channel the switch chose, so if FailAdd drops outgoingChanID the
// manager sees the zero scid, matches nothing, and leaks the pending forever.
func TestSwitchReputationLocalFailAddResolvesPending(t *testing.T) {
	t.Parallel()

	repMgr := &mockReputationManager{}
	s, aliceLink, bobLink := newReputationTestSwitch(t, repMgr)

	preimage, err := genPreimage()
	require.NoError(t, err, "unable to generate preimage")
	rhash := sha256.Sum256(preimage[:])

	addPkt := &htlcPacket{
		incomingChanID: aliceLink.ShortChanID(),
		incomingHTLCID: 0,
		outgoingChanID: bobLink.ShortChanID(),
		obfuscator:     NewMockObfuscator(),
		htlc: &lnwire.UpdateAddHTLC{
			PaymentHash: rhash,
			Amount:      1,
		},
	}
	require.NoError(t, s.ForwardPackets(nil, addPkt))

	// Take the add off the outgoing link's mailbox without completing the
	// circuit: the keystone is only written once the link has added the
	// HTLC to its commitment.
	var delivered *htlcPacket
	select {
	case delivered = <-bobLink.packets:

	case <-time.After(time.Second):
		t.Fatal("add was not propagated to destination")
	}

	forwards, _, _ := repMgr.snapshot()
	require.Len(t, forwards, 1)
	require.Equal(t, bobLink.ShortChanID(), forwards[0].out,
		"forward must be tracked against the outgoing channel")

	// The outgoing link rejects the add. This is the real FailAdd, which
	// builds the fail packet and hands it back to the switch.
	bobLink.mailBox.FailAdd(delivered)

	select {
	case pkt := <-aliceLink.packets:
		require.NoError(t, aliceLink.deleteCircuit(pkt))

	case <-time.After(time.Second):
		t.Fatal("fail was not propagated upstream")
	}

	// Both keys must match the forward, otherwise the manager cannot find
	// the pending HTLC it recorded and never releases it.
	_, _, fails := repMgr.snapshot()
	require.Len(t, fails, 1)
	require.Equal(t, forwards[0].in, fails[0].in,
		"resolve must report the same htlc as the forward")
	require.Equal(t, forwards[0].out, fails[0].out,
		"resolve must report the same channel as the forward")
}

[erickcestari]

I've just noticed that this is the same issue as @elnosh commented.

[GeorgeTsagk]

Fixed it a different way after elnosh flagged the same leak: instead of stamping outgoingChanID onto the FailAdd packet, resolutions now identify the HTLC by its incoming circuit key alone and the manager recovers the outgoing channel from an index recorded at forward time. That covers every path that lacks the scid without touching the mailbox. Added a mailbox FailAdd test along the lines of yours.

[GeorgeTsagk]

@elnosh on the review notes: yes, a follow-up PR will persist the reputation and revenue values, including decaying them correctly across downtime. On zero-fee policies I'd leave it as is, such channels simply never accrue reputation, which fails on the strict side. And right, until peers set the accountable bit the in-flight term is zero and both verdicts coincide, which is why both are logged.

[elnosh]

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.

[elnosh]

just to note that we (in LDK) decided to clamp on this rather than error. Specially on cases where the HTLC succeeded.

[litbot-9000]

@carlaKC: review reminder
@erickcestari: review reminder
@GeorgeTsagk, remember to re-request review from reviewers when ready

[litbot-9000]

@carlaKC: review reminder
@erickcestari: review reminder
@GeorgeTsagk, remember to re-request review from reviewers when ready

[litbot-9000]

@carlaKC: review reminder
@erickcestari: review reminder
@GeorgeTsagk, remember to re-request review from reviewers when ready

[erickcestari]

LGTM!

The best of my knowledge everything looks good to me. Nice work!