Skip to content

AuxTrafficShaper.PaymentBandwidth uses HTLC view #9687

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
guggero merged 4 commits into from
Apr 17, 2025
Merged

AuxTrafficShaper.PaymentBandwidth uses HTLC view #9687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b96d57e
lnwallet: add AuxHtlcView
GeorgeTsagk Apr 14, 2025
a279058
lnwallet: replace HtlcView with AuxHtlcView in existing code
GeorgeTsagk Apr 14, 2025
53254c7
lnwallet: add FetchLatestHTLCView to LightningChannel
GeorgeTsagk Apr 8, 2025
d0ef248
htlcswitch: add HtlcView as PaymentBandwidth argument
GeorgeTsagk Apr 8, 2025
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
5 changes: 3 additions & 2 deletions htlcswitch/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/lightningnetwork/lnd/graph/db/models"
"github.com/lightningnetwork/lnd/invoices"
"github.com/lightningnetwork/lnd/lntypes"
"github.com/lightningnetwork/lnd/lnwallet"
"github.com/lightningnetwork/lnd/lnwallet/chainfee"
"github.com/lightningnetwork/lnd/lnwire"
"github.com/lightningnetwork/lnd/record"
Expand Down Expand Up @@ -516,8 +517,8 @@ type AuxTrafficShaper interface {
// is a custom channel that should be handled by the traffic shaper, the
// ShouldHandleTraffic method should be called first.
PaymentBandwidth(htlcBlob, commitmentBlob fn.Option[tlv.Blob],
linkBandwidth,
htlcAmt lnwire.MilliSatoshi) (lnwire.MilliSatoshi, error)
linkBandwidth, htlcAmt lnwire.MilliSatoshi,
htlcView lnwallet.AuxHtlcView) (lnwire.MilliSatoshi, error)

// IsCustomHTLC returns true if the HTLC carries the set of relevant
// custom records to put it under the purview of the traffic shaper,
Expand Down
1 change: 1 addition & 0 deletions htlcswitch/link.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3505,6 +3505,7 @@ func (l *channelLink) AuxBandwidth(amount lnwire.MilliSatoshi,
commitmentBlob := l.CommitmentCustomBlob()
auxBandwidth, err := ts.PaymentBandwidth(
htlcBlob, commitmentBlob, l.Bandwidth(), amount,
l.channel.FetchLatestAuxHTLCView(),
)
if err != nil {
return fn.Err[OptionalBandwidth](fmt.Errorf("failed to get "+
Expand Down
25 changes: 13 additions & 12 deletions lnwallet/aux_leaf_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ type CommitDiffAuxInput struct {
// UnfilteredView is the unfiltered, original HTLC view of the channel.
// Unfiltered in this context means that the view contains all HTLCs,
// including the canceled ones.
UnfilteredView *HtlcView
UnfilteredView AuxHtlcView

// WhoseCommit denotes whose commitment transaction we are computing the
// diff for.
Expand Down Expand Up @@ -177,9 +177,8 @@ type AuxLeafStore interface {
// correspond to the passed aux blob, and an existing channel
// commitment.
FetchLeavesFromCommit(chanState AuxChanState,
commit channeldb.ChannelCommitment,
keyRing CommitmentKeyRing, whoseCommit lntypes.ChannelParty,
) fn.Result[CommitDiffAuxResult]
commit channeldb.ChannelCommitment, keyRing CommitmentKeyRing,
whoseCommit lntypes.ChannelParty) fn.Result[CommitDiffAuxResult]

// FetchLeavesFromRevocation attempts to fetch the auxiliary leaves
// from a channel revocation that stores balance + blob information.
Expand All @@ -206,7 +205,7 @@ func auxLeavesFromView(leafStore AuxLeafStore, chanState *channeldb.OpenChannel,
return leafStore.FetchLeavesFromView(CommitDiffAuxInput{
ChannelState: NewAuxChanState(chanState),
PrevBlob: blob,
UnfilteredView: originalView,
UnfilteredView: newAuxHtlcView(originalView),
WhoseCommit: whoseCommit,
OurBalance: ourBalance,
TheirBalance: theirBalance,
Expand All @@ -227,13 +226,15 @@ func updateAuxBlob(leafStore AuxLeafStore, chanState *channeldb.OpenChannel,
return fn.MapOptionZ(
prevBlob, func(blob tlv.Blob) fn.Result[fn.Option[tlv.Blob]] {
return leafStore.ApplyHtlcView(CommitDiffAuxInput{
ChannelState: NewAuxChanState(chanState),
PrevBlob: blob,
UnfilteredView: nextViewUnfiltered,
WhoseCommit: whoseCommit,
OurBalance: ourBalance,
TheirBalance: theirBalance,
KeyRing: keyRing,
ChannelState: NewAuxChanState(chanState),
PrevBlob: blob,
UnfilteredView: newAuxHtlcView(
nextViewUnfiltered,
),
WhoseCommit: whoseCommit,
OurBalance: ourBalance,
TheirBalance: theirBalance,
KeyRing: keyRing,
})
},
)
Expand Down
35 changes: 35 additions & 0 deletions lnwallet/aux_signer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"github.com/lightningnetwork/lnd/fn/v2"
"github.com/lightningnetwork/lnd/input"
"github.com/lightningnetwork/lnd/lntypes"
"github.com/lightningnetwork/lnd/lnwallet/chainfee"
"github.com/lightningnetwork/lnd/lnwire"
"github.com/lightningnetwork/lnd/tlv"
)
Expand All @@ -13,6 +14,37 @@ import (
// signatures within the custom data for an existing HTLC.
var htlcCustomSigType tlv.TlvType65543

// AuxHtlcView is a struct that contains a safe copy of an HTLC view that can
// be used by aux components.
type AuxHtlcView struct {
// NextHeight is the height of the commitment transaction that will be
// created using this view.
NextHeight uint64

// Updates is a Dual of the Local and Remote HTLCs.
Updates lntypes.Dual[[]AuxHtlcDescriptor]

// FeePerKw is the fee rate in sat/kw of the commitment transaction.
FeePerKw chainfee.SatPerKWeight
}

// newAuxHtlcView creates a new safe copy of the HTLC view that can be used by
// aux components.
//
// NOTE: This function should only be called while holding the channel's read
// lock, since the underlying local/remote payment descriptors are accessed
// directly.
func newAuxHtlcView(v *HtlcView) AuxHtlcView {
return AuxHtlcView{
NextHeight: v.NextHeight,
Updates: lntypes.Dual[[]AuxHtlcDescriptor]{
Local: fn.Map(v.Updates.Local, newAuxHtlcDescriptor),
Remote: fn.Map(v.Updates.Remote, newAuxHtlcDescriptor),
},
FeePerKw: v.FeePerKw,
}
}

// AuxHtlcDescriptor is a struct that contains the information needed to sign or
// verify an HTLC for custom channels.
type AuxHtlcDescriptor struct {
Expand Down Expand Up @@ -99,6 +131,9 @@ func (a *AuxHtlcDescriptor) RemoveHeight(

// newAuxHtlcDescriptor creates a new AuxHtlcDescriptor from a payment
// descriptor.
//
// NOTE: This function should only be called while holding the channel's read
// lock, since the underlying payment descriptors are accessed directly.
func newAuxHtlcDescriptor(p *paymentDescriptor) AuxHtlcDescriptor {
return AuxHtlcDescriptor{
ChanID: p.ChanID,
Expand Down
14 changes: 14 additions & 0 deletions lnwallet/channel.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2704,6 +2704,20 @@ func (v *HtlcView) AuxTheirUpdates() []AuxHtlcDescriptor {
return fn.Map(v.Updates.Remote, newAuxHtlcDescriptor)
}

// FetchLatestAuxHTLCView returns the latest HTLC view of the lightning channel
// as a safe copy that can be used outside the wallet code in concurrent access.
func (lc *LightningChannel) FetchLatestAuxHTLCView() AuxHtlcView {
// This read lock is important, because we access both the local and
// remote log indexes as well as the underlying payment descriptors of
// the HTLCs when creating the view.
lc.RLock()
defer lc.RUnlock()

Copy link
Member

@yyforyongyu yyforyongyu Apr 15, 2025

Choose a reason for hiding this comment

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

I think it's better to not wrap the method, also need some docs explaining the indices used here.

htlcView := lc.fetchHTLCView(theirLogIndex, ourLogIndex)

Copy link
Collaborator Author

@GeorgeTsagk GeorgeTsagk Apr 15, 2025

Choose a reason for hiding this comment

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

This part

return newAuxHtlcView(lc.fetchHTLCView(
		lc.updateLogs.Remote.logIndex, lc.updateLogs.Local.logIndex,
	))

is important as the wrapper newAuxHtlcView provides us with a safe copy of the data as an AuxHtlcView struct.

Sure can add more docs w.r.t the indices used, but let's first see if we agree on previous comment

return newAuxHtlcView(lc.fetchHTLCView(
Copy link
Member

@yyforyongyu yyforyongyu Apr 15, 2025

Choose a reason for hiding this comment

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

I'm not sure how PaymentBandwidth is implemented in tapd, but I have a question re the logIndex used here - we rarely use logIndex from both the update logs to get a set of entries, since technically they can stay out of sync and there's no direct relationship between Remove.logIndex and Local.logIndex. So I'd imagine there's a filter in PaymentBandwidth that handles looking at logs from a specific chain?

For instance, in availableBalance, we'd grab the remote-acked index from the local commitment tip, then use it to get a list of HTLC updates, which are then used to calculate the bandwidth. This bandwidth is then used in multiple places, to decide whether we can forward an HTLC or not, fees, etc.

Copy link
Collaborator Author

@GeorgeTsagk GeorgeTsagk Apr 15, 2025

Choose a reason for hiding this comment

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

we rarely use logIndex from both the update logs to get a set of entries, since technically they can stay out of sync and there's no direct relationship between Remove.logIndex and Local.logIndex.

The reason I picked these values is because I want the greatest value at the time of calling, in order to include all HTLCs in the view

So I'd imagine there's a filter in PaymentBandwidth that handles looking at logs from a specific chain?

Yes, the wrapper newAuxHtlcView helps here, as we create a safe copy of the HtlcView (which is wrapped as a struct called AuxHtlcView).

On the tapd side we parse both the logs of the local and remote chains, and mutate the external state accordingly.

For instance, in availableBalance, we'd grab the remote-acked index from the local commitment tip, then use it to get a list of HTLC updates, which are then used to calculate the bandwidth. This bandwidth is then used in multiple places, to decide whether we can forward an HTLC or not, fees, etc.

I'm not sure in which cases the remote acked index and the updateLogs.Local.logIndex misalign, but this shouldn't really matter for PaymentBandwidth as we really care about the local balance. I believe the only issue with picking an "outdated" value is that the remote balance may not be totally accurate.

Copy link
Member

@yyforyongyu yyforyongyu Apr 16, 2025

Choose a reason for hiding this comment

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

I'm not sure in which cases the remote acked index and the updateLogs.Local.logIndex misalign

If we are sending lots of HTLCs these two can likely be different all the time. We use the remote acked index to filter out the logs we received in updateLogs.Remote when deciding the local balance, as it's not safe to include updates if they are not acked by the remote. It seems to me that you wanna replicate the logic used in availableBalance with more customized settings on the tapd side?

On the tapd side we parse both the logs of the local and remote chains, and mutate the external state accordingly.

Cool yeah then returning as much info as needed does make sense, tho I'm not sure how you gonna filter updates there since AuxHtlcDescriptor doesn't have the log index info.

Copy link
Collaborator Author

@GeorgeTsagk GeorgeTsagk Apr 16, 2025

Choose a reason for hiding this comment

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

Yeah we use the logIndex only on the LND side to make sure all HTLCs are included, then most of the filtering in tapd side happens around the addHeight of the HTLC, see https://github.com/lightninglabs/taproot-assets/blob/47dd3046a068f567b6b8c2baf2e1f9436633dd8a/tapchannel/commitment.go#L348

I believe the logIndex is not useful outside of the LND channel state machine, so that's why we didn't include it in the AuxHtlcDescriptor

lc.updateLogs.Remote.logIndex, lc.updateLogs.Local.logIndex,
))
}

// fetchHTLCView returns all the candidate HTLC updates which should be
// considered for inclusion within a commitment based on the passed HTLC log
// indexes.
Expand Down
4 changes: 3 additions & 1 deletion routing/bandwidth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/go-errors/errors"
"github.com/lightningnetwork/lnd/fn/v2"
"github.com/lightningnetwork/lnd/htlcswitch"
"github.com/lightningnetwork/lnd/lnwallet"
"github.com/lightningnetwork/lnd/lnwire"
"github.com/lightningnetwork/lnd/tlv"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -150,7 +151,8 @@ func (*mockTrafficShaper) ShouldHandleTraffic(_ lnwire.ShortChannelID,
// is a custom channel that should be handled by the traffic shaper, the
// HandleTraffic method should be called first.
func (*mockTrafficShaper) PaymentBandwidth(_, _ fn.Option[tlv.Blob],
linkBandwidth, _ lnwire.MilliSatoshi) (lnwire.MilliSatoshi, error) {
linkBandwidth, _ lnwire.MilliSatoshi,
_ lnwallet.AuxHtlcView) (lnwire.MilliSatoshi, error) {

return linkBandwidth, nil
}
Expand Down
Loading