Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions htlcswitch/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,13 @@ type ChannelLink interface {
// policy to govern if it an incoming HTLC should be forwarded or not.
UpdateForwardingPolicy(models.ForwardingPolicy)

// AdvertisedFee returns the fee this link's current forwarding policy
// charges to forward the given outgoing amount (base fee plus the
// 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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👌


// CheckHtlcForward should return a nil error if the passed HTLC details
// satisfy the current forwarding policy fo the target link. Otherwise,
// a LinkError with a valid protocol failure message should be returned
Expand Down Expand Up @@ -515,6 +522,29 @@ type htlcNotifier interface {
info channeldb.FinalHtlcInfo)
}

// ReputationManager is the read-only seam through which the switch feeds HTLC
// forwarding lifecycle events to the (optional) local reputation subsystem.
// It is a black box that only observes events to update internal reputation
// state; it never affects forwarding decisions or the wire (log-only). When no
// reputation manager is configured this is nil and the hooks are skipped.
type ReputationManager interface {
// 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)
Comment on lines +531 to +539

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps just use the outgoing channel (not the circuit key)? We don't have use for the ID anyway iirc


// OnSettle observes the successful resolution of a forwarded HTLC.
OnSettle(incoming, outgoing CircuitKey)

// OnFail observes the failed resolution of a forwarded HTLC.
OnFail(incoming, outgoing CircuitKey)
}

// AuxHtlcModifier is an interface that allows the sender to modify the outgoing
// HTLC of a payment by changing the amount or the wire message tlv records.
type AuxHtlcModifier interface {
Expand Down
14 changes: 14 additions & 0 deletions htlcswitch/link.go
Original file line number Diff line number Diff line change
Expand Up @@ -2482,6 +2482,20 @@ func (l *channelLink) UpdateForwardingPolicy(
l.cfg.FwrdingPolicy = newPolicy
}

// AdvertisedFee returns the fee this link's current forwarding policy charges
// to forward the given outgoing amount (base fee plus the proportional fee).
//
// NOTE: Part of the ChannelLink interface.
func (l *channelLink) AdvertisedFee(
amtToForward lnwire.MilliSatoshi) lnwire.MilliSatoshi {

l.RLock()
policy := l.cfg.FwrdingPolicy
l.RUnlock()

return ExpectedFee(policy, amtToForward)
}

// CheckHtlcForward should return a nil error if the passed HTLC details
// satisfy the current forwarding policy fo the target link. Otherwise,
// a LinkError with a valid protocol failure message should be returned
Expand Down
10 changes: 10 additions & 0 deletions htlcswitch/mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -738,6 +738,10 @@ type mockChannelLink struct {

checkHtlcForwardResult *LinkError

// advertisedFee is the fee returned by AdvertisedFee, letting tests
// control the outgoing link's advertised forwarding fee.
advertisedFee lnwire.MilliSatoshi

failAliasUpdate func(sid lnwire.ShortChannelID,
incoming bool) *lnwire.ChannelUpdate1

Expand Down Expand Up @@ -847,6 +851,12 @@ func (f *mockChannelLink) HandleChannelUpdate(lnwire.Message) {

func (f *mockChannelLink) UpdateForwardingPolicy(_ models.ForwardingPolicy) {
}

func (f *mockChannelLink) AdvertisedFee(
_ lnwire.MilliSatoshi) lnwire.MilliSatoshi {

return f.advertisedFee
}
func (f *mockChannelLink) CheckHtlcForward([32]byte, lnwire.MilliSatoshi,
lnwire.MilliSatoshi, uint32, uint32, models.InboundFee, uint32,
lnwire.ShortChannelID, lnwire.CustomRecords) *LinkError {
Expand Down
82 changes: 82 additions & 0 deletions htlcswitch/reputation_guard.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package htlcswitch

import (
"sync/atomic"

"github.com/lightningnetwork/lnd/lnwire"
)

// 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.
Comment on lines +9 to +13

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also a bit code-smelly to me.

Is this standard practice for LND to add this type of gating to new features?

//
// The hooks run synchronously on the switch's forwarding goroutine, so this
// boundary keeps a subsystem bug — a nil deref, an arithmetic panic — from
// taking down the node's HTLC forwarding.
type guardedReputationManager struct {
inner ReputationManager
disabled atomic.Bool
}

// NewGuardedReputationManager wraps the given ReputationManager with a panic
// boundary. It returns nil when inner is nil, so the switch's existing nil
// check still short-circuits a disabled subsystem with zero overhead.
func NewGuardedReputationManager(inner ReputationManager) ReputationManager {
if inner == nil {
return nil
}

return &guardedReputationManager{inner: inner}
}

// OnForward forwards the observation to the wrapped manager behind a panic
// boundary.
func (g *guardedReputationManager) OnForward(incoming, outgoing CircuitKey,
incomingAmt, outgoingAmt, advertisedFee lnwire.MilliSatoshi,
incomingCltv, height uint32, accountable bool) {

if g.disabled.Load() {
return
}
defer g.recoverHook("OnForward")

g.inner.OnForward(
incoming, outgoing, incomingAmt, outgoingAmt, advertisedFee,
incomingCltv, height, accountable,
)
}

// OnSettle forwards the observation to the wrapped manager behind a panic
// boundary.
func (g *guardedReputationManager) OnSettle(incoming, outgoing CircuitKey) {
if g.disabled.Load() {
return
}
defer g.recoverHook("OnSettle")

g.inner.OnSettle(incoming, outgoing)
}

// OnFail forwards the observation to the wrapped manager behind a panic
// boundary.
func (g *guardedReputationManager) OnFail(incoming, outgoing CircuitKey) {
if g.disabled.Load() {
return
}
defer g.recoverHook("OnFail")

g.inner.OnFail(incoming, outgoing)
}

// recoverHook recovers from a panic in a reputation hook, logging it and
// permanently disabling the subsystem so a deterministic bug cannot panic on
// every forwarded HTLC. Forwarding is never affected.
func (g *guardedReputationManager) recoverHook(method string) {
if r := recover(); r != nil {
log.Errorf("Reputation %s hook panicked; disabling reputation "+
"subsystem (forwarding is unaffected): %v", method, r)
g.disabled.Store(true)
}
}
Loading