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

Changes from 1 commit
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
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