AuxTrafficShaper.PaymentBandwidth uses HTLC view
#9687
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
GeorgeTsagk
force-pushed
the
aux-trff-shpr-htlcview
branch
from
0834db3 to
cabcc01
Compare
|
@guggero Realized that we're using this method to access the htlc view data Line 2697 in cabcc01 which primarily copies non-pointer values, and uses a Copy method on the only field that needs it (custom records)
I think this means we won't have to create an |
GeorgeTsagk
force-pushed
the
aux-trff-shpr-htlcview
branch
from
cabcc01 to
129adde
Compare
We add a public method for the lightning channel to expose the latest HtlcView. This is used in a follow up commit by the channel link.
In order to get more precise bandwidth reports, we also need to provide this method with the latest htlc view. Since aux data is committed to in the channel commitment, some uncommited HTLCs may not be accounted for, so we need to manually provide them via the HTLC view.
GeorgeTsagk
force-pushed
the
aux-trff-shpr-htlcview
branch
from
129adde to
d0ef248
Compare
| lc.RLock() | ||
| defer lc.RUnlock() | ||
|
|
||
| return newAuxHtlcView(lc.fetchHTLCView( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| // the HTLCs when creating the view. | ||
| lc.RLock() | ||
| defer lc.RUnlock() | ||
|
|
There was a problem hiding this comment.
I think it's better to not wrap the method, also need some docs explaining the indices used here.
htlcView := lc.fetchHTLCView(theirLogIndex, ourLogIndex)There was a problem hiding this comment.
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
Since the mutation happens before we call |
Description
This PR adds an extra argument to the
PaymentBandwidthhook, which helps with computing a more precise aux htlc view, ultimately leading to more precise bandwidth results.We expose the HtlcView from the lighting channel for the link to use when checking whether an HTLC can be added to the channel.