Probing for more reliable route fee estimation

[hieblmi]

This PR implements #7916

Overview

In this PR we extend the implementation of the graph basedEstimateRouteFee RPC with a payment probe based approach to obtain a more reliable routing fee estimate.

Notes on implementation

• The new fee estimation API now optionally accepts an invoice from the target node and a probe payment timeout.
• Probing distinguishes two cases, (1) probing to a public destination key specified in the invoice or (2), probing a private invoice destination reachable through public hop hints in the invoice.
• If multiple hop hints are provided then, for each of the public ones, a probe is dispatched and each of the fee estimates is returned. Private hints are skipped.
• lncli adds support to use the graph-based and probe-based fee estimation:

graph-based:

lncli estimateroutefee --dest 0266a18ed969ef95c8a5aa314b443b2b3b8d91ed1d9f8e95476f5f4647efdec079 --amt 100000

probe-based:

lncli estimateroutefee reg_zane estimateroutefee --pay_req lnbcrt1m1pj... --timeout 60s

[JssDWt]

There is currently a problem with prepayment probing in combination with some lightning service providers. Basically prepayment probes are not compatible with the current LSP protocols. Since nodes behind a LSP can be a significant portion of destinations, I think it's good to keep the following considerations in mind. In below context, assume the payee is a lightning node behind a LSP.

In the current state I generally recommend against sending prepayment probes when a LSP is involved. Or if you do really want to probe, probe up to the LSP node, and manually add the fees for the ultimate hop. These fees can be taken from the route hint in the invoice. I guess the question is: how do you determine whether a LSP is involved? Let's outline the protocols and their caveats.

Present: Breez LSP

• Prepayment probes with a random payment hash will, in some situations around just-in-time channels, fail with unknown_next_peer returned by the LSP, even though the actual payment with the real payment hash would succeed.
• There is an option to re-hash the payment hash prefixed with probing-01, like here. When used with the current Breez LSP, the Breez LSP will return incorrect_or_unknown_payment_details in the same situations as where the previous bullet would return unknown_next_peer.

Soon: LSPS2

Very soon Breez and multiple other LSPs will implement the LSPS2 protocol. This is a protocol for just-in-time channels. This initial version of the protocol is incompatible with prepayment probes. There are no payment hash tricks here.

These nodes should have feature bit 729 (option_supports_lsps) set in their node announcement. Recommendation is to send the prepayment probe to the node in the route hint, rather than the destination node, if this feature bit is set for the route hint node and the destination node is only reachable through the route hint node (quite a mouthful, sorry about that).

Medium term: LSPS4

The LSP spec group is currently in the process of designing a LSP protocol LSPS4 for just-in-time channels. When LSPS4 is fully implemented, prepayment probes won't be much of an issue. This is not going to be implemented soon, however.

Other thoughts

Prepayment probes don't work together with MPP very well. At least there is no indication whether you were able to deliver the full amount for the specified fee, even if all payment parts return with incorrect_or_unknown_payment_details. Point is the parts do not all arrive at the same time, like they would in an actual payment. The previous parts locking up liquidity in the route on an actual payment may very well change the path chosen for next parts.

[alexbosworth]

normally these are called payment_request when it's a bolt 11 payreq input

[hieblmi]

Thanks for the detailed rundown on LSP mechanics @JssDWt.

The primary focus of this PR is to improve the currently active graph based method of estimating routing fees. It is unreliable because the gossip announcements might not accurately reflect the network topology or fees at the time when a payment occurs. So in this first step the desired outcome is to improve the fee estimation for payments to publicly known nodes.

Follow-up PRs will focus on MPP/AMP as noted in the linked issue #7916. This seems to have gotten possible since the release of AMP by invalidating the last shard of a AMP, see #4219 (comment).

I think LSP specific probing protocols will also land with a separate PR.

[saubyk]

assigned @bitromortac for approach ack

[bitromortac]

Nice, I think the api looks good, except that we could return an lnrpc.PaymentFailureReason to find out more about why a probe failed. Also added a suggestion for how to handle the streaming responses differently.

Another important point is backwards compatibility, I think we need to support the old ux for some time, as the new api now has a time component. So I'd suggest to keep the old code with the pubkey/amount input and only use the new estimation if an invoice is provided. This can then be removed after some time when the rpc fields are removed.

[Roasbeef]

Prepayment probes don't work together with MPP very well. At least there is no indication whether you were able to deliver the full amount for the specified fee,

AMP solves that. You can send the N-1 shards, then make the Nth shared fail by giving it an invalid secret share.

[Roasbeef]

@JssDWt

These nodes should have feature bit 729 (option_supports_lsps) set in their node announcement. Recommendation is to send the prepayment probe to the node in the route hint, rather than the destination node, if this feature bit is set for the route hint node and the destination node is only reachable through the route hint node (quite a mouthful, sorry about that).

How will that work if a probing hash is used, and also only a single shard is sent that may be below the actual payment amount? The LSP just decides on a value to use for channel opening? What if the sender doesn't retry again, the LSP just eats the chain fees cost? What about if the user already has a channel open?

[JssDWt]

@Roasbeef

Assuming you're asking about probing all the way to the destination node, and not up until the LSP. Let me answer these one by one.

How will that work if a probing hash is used, and also only a single shard is sent that may be below the actual payment amount?

In that case the single shard is failed back by the LSP after a timeperiod similar to the MPP timeout. The recipient's invoice amount is the threshold for letting htlcs through and trigger channel open.

The LSP just decides on a value to use for channel opening?

The recipient communicates the intended amount to receive to the LSP before creating the invoice. The invoice then contains a route hint with a scid specifically for this payment/channel order.

What about if the user already has a channel open?

In that case there are no issues. The destination works like any other lightning node. Only if the recipient ordered a cchannel for that payment will this LSPS2 spec be used.

[Roasbeef]

@hieblmi is this ready to leave draft?

[hieblmi]

@hieblmi is this ready to leave draft?

@Roasbeef I will move to 'ready' once I have incorporated the hop hint fee estimation and refactor suggestions. Will focus on it.

[bitromortac]

Great tests/itests! I think the functionality added looks solid, the cltv calculation may need to be checked (see comment).

The issue with the zero amount invoices made me think if accepting an invoice to the api is really the best choice? There is some flow where a wallet gets a zero-amount invoice and the user fills in the amount. A probe would then be done with that amount. Right now we fail that use-case. Also there will be other invoice formats (e.g. bolt 12) or use cases that we may need to support here. So the question is whether we should not just extend the proto with hop hints, final cltv, node features and a bool to indicate if we want to probe or not, leaving the api backwards compatible. The api user could decode the invoice before using this endpoint with https://lightning.engineering/api-docs/api/lnd/lightning/decode-pay-req. I think it wouldn't be a big change to this PR but could add more value (also the current graph-based approach would become better by being able to take hop hints into account)? The lncli command could stay the same with using the invoice for convenience. Would appreciate feedback for that @ all.

[bitromortac]

I'm not sure the current version of this is optimal (with the advice given). Should we consider to panic here?

[alexbosworth]

The issue with the zero amount invoices made me think if accepting an invoice to the api is really the best choice? There is some flow where a wallet gets a zero-amount invoice and the user fills in the amount. A probe would then be done with that amount. Right now we fail that use-case.

Failing isn't good, but it could also just have a separate amount field to mirror the send payment API, which would match the design pattern of probe with the arguments you want to use for a payment before you actually pay.

I can see the argument for generic probing fields but thinking about the history of this feature what motivated it from a user perspective is that doing this yourself is too hard, even though you can actually do everything yourself with the existing APIs. This API adds zero functionality you couldn't achieve some other way with other APIs and you can already break things down into components if you want to do that. So given that motivating logic, making it easier with just the payment request would be easier and therefore correct for this API.

[saubyk]

This API adds zero functionality you couldn't achieve some other way with other APIs and you can already break things down into components if you want to do that. So given that motivating logic, making it easier with just the payment request would be easier and therefore correct for this API.

+1
Improving usability by just providing a payment request to probe is one of the core reasons for this rpc change. If we are removing the payment request from the arguments that will be counter-productive to the usability by adding additional steps to decode.

Also there will be other invoice formats (e.g. bolt 12) or use cases that we may need to support here

Keeping the API future compatible with bolt12 is a valid concern. Can that be handled internally by checking the format first for the invoice type? I believe a determination can be made my looking at the invoice string if it's bolt11 or 12

[hieblmi]

Thanks for the review @bitromortac!

I get your concern about being flexible with adaptations to the estimation if new requirements like bolt12 come in, without having to come up with a new interface, but yes that reduces the ease to use this API. One benefit of providing all params in the proto that you mentioned would be that they could be filled into the graph based estimation, which currently only allows for pubKey and amount. But for the ease of use of this API we could definitely parse the params based on the invoice format.

[ProofOfKeags]

My biggest observation is that the isLSP heuristic function should be cleaned up from an organization perspective. I expect that over time that code will need to be serviced over and over again as the industry evolves, and having the different heuristics tightly organized is going to be key to ensure that the evolution can happen in a way that doesn't introduce subtle bugs as well as being rapidly iterated on as we come up with different ways to assess something that is fuzzy.

Otherwise I'm a big fan of the PR overall 😁

[ProofOfKeags]

Separate: Consider making the return value a struct (LspHeuristics)

[ProofOfKeags]

This seems an odd choice as far as composability goes, shouldn't we pass in the lnrpc.RoutingPolicy? rather than making this function a constructor and a setter?

[hieblmi]

yes totally, I didn't take a closer look when I was migrating this method. Now simplified it to a minimum I think.

[ProofOfKeags]

I'd like to see some of this broken up, into the separate checks. Right now all of the different heuristics are mixed together in the same loop which makes it hard to figure out what is being looked at. Instead, I'd recommend having a "node id homogeneity check", a "max fee selector", a "max cltv selector", etc. and then have the overall isLSP function be a list of these heuristics bundled. Especially with early exit, reasoning about for loops larger than a screen height is difficult, and when someone returns to these heuristics months from now they'll be grateful that things are neatly organized into their constituent pieces.

[hieblmi]

Thank you for this suggestion @ProofOfKeags, that makes sense to keep this part extendable and readable. Would you mind giving me a hint in regards to the implementation? I thought to have an interface heuristicCheck that requires a method check which receives the hophints to be evaluated and a pointer to the result heuristics. Implemented is this interface then by the selectors you suggested. These checks are then passed into the isLsp method, something like this:

type heuristicCheck interface {
	check([][]zpay32.HopHint, lnwire.MilliSatoshi, *lspHeuristics)
}

type nodeIDHeuristic struct{}

func (nodeIDHeuristic) check(routeHints [][]zpay32.HopHint,
	amt lnwire.MilliSatoshi, result *lspHeuristics) {

}

type maxCltvSelector struct{}

func (maxCltvSelector) check(routeHints [][]zpay32.HopHint,
	amt lnwire.MilliSatoshi, result *lspHeuristics) {

}

type maxFeeSelector struct{}

func (maxFeeSelector) check(routeHints [][]zpay32.HopHint,
	amt lnwire.MilliSatoshi, result *lspHeuristics) {

}

func isLSP(routeHints [][]zpay32.HopHint,
	amt lnwire.MilliSatoshi, checks heuristicCheck...) *lspHeuristics {

}

[ProofOfKeags]

I don't think there's a ton of value to doing a golang interface for heuristicCheck but what I was saying is that we have three distinct things I was able to see. I'll provide signatures

// note it is possible not all of these individual heuristics require all of these arguments, and
// the arguments should be trimmed if unused.

func HasHomogenousNodeIDs(routeHints [][]zpay32.HopHint, amt lnwire.MilliSatoshi) bool { ... }

func FindMaxCLTV(routeHints [][]zpay32.HopHint, amt lnwire.MilliSatoshi) uint32 { ... }

func FindMaxFee(routeHints [][]zpay32.HopHint, amt lnwire.MilliSatoshi) lnwire.MilliSatoshi { ... }

func isLSP(routeHints [][]zpay32.HopHint, amt lnwire.MilliSatoshi) *lspHeuristics {
    var heuristics lspHeuristics
    heuristics.sameNodeID = HasHomogenousNodeIDs(routeHints, amt)
    heuristics.maxCLTV = FindMaxCLTV(routeHints, amt)
    heuristics.maxFee = FindMaxFee(routeHints, amt)

    // maybe we do some further computation on these results together here in the future or something

    return &heuristics
}

This way we can look at isLSP and be like "Oh 🤔, that's what we are measuring to make our determinations", and each of those heuristics only measures one thing, making it easy to follow and verify it works correctly as designed. This way, if we want to add a new check, it's simply a matter of implementing a new standalone function for it, and then adding it to the bundle for isLSP, rather than trying to splice it into a large for-loop.

Hopefully this sheds light on the approach I'm going for? Lmk if you foresee issues here, but I think it will make the code substantially nicer to look at 😁

[hieblmi]

Thanks that makes total sense. I am glad I asked as I was taking your "isLsp is a list of these heuristics" too literal by passing in a slice of these checks in my code example.

[ProofOfKeags]

🫡

[hieblmi]

Once again thanks for the thorough review @ProofOfKeags + @bitromortac, I've addressed all your comments.
Most notably I restructured isLsp to only check what its name implies, and added prepareLspRouteHints to handle the extraction of adjustedRouteHints which allow probing the LSP. I've also made the requested changes to tests and itests.

This PR is ready for another review round.

[hieblmi]

nit: put in var block

[bitromortac]

I have some more nits and the itests needs an update to test real probing to the receiver which needs to take the bits of the destination node into account (read from the invoice as discussed).

[bitromortac]

LGTM 🎉. There's only one remark left concerning the api docs.

[bitromortac]

nit: we should be more accurate here, suggestion:
The probing payment loop is aborted if this timeout is reached. Note that the probing process itself can take longer than the timeout if the HTLC becomes delayed or stuck. Canceling the context of this call will not cancel the payment loop, the duration is only controlled by the timeout parameter.

[hieblmi]

Once again thank you @bitromortac for this thorough review journey. The handling of route hints by lnd has been especially insightful for me.