Skip to content
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.

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

[feature]: Change EstimateRouteFee rpc to improve fee estimation functionality #7916

Closed
saubyk opened this issue Aug 24, 2023 · 5 comments · Fixed by #8136
Closed

[feature]: Change EstimateRouteFee rpc to improve fee estimation functionality #7916

saubyk opened this issue Aug 24, 2023 · 5 comments · Fixed by #8136
Assignees
Labels
enhancement Improvements to existing features / behaviour P1 MUST be fixed or reviewed payments Related to invoices/payments routing
Milestone

Comments

@saubyk
Copy link
Collaborator

saubyk commented Aug 24, 2023

EstimateRouteFee rpc can be changed to improve fee estimation for paying specific invoices.
Following changes should be made:

  1. Add invoice string as a parameter to estimate the fee for
  2. Add a timeout parameter which the user can set to control how much time the payment should take

The rpc should return:

  • destination reachable (true/false)
  • estimated fee for the route

If the rpc is not able to complete the probe within the timeout period specified by the user, it should return destination reachable as false.

Long term changes (not in the current scope of this ticket) which can be considered for future revisions of this rpc:

@saubyk saubyk added enhancement Improvements to existing features / behaviour routing payments Related to invoices/payments P1 MUST be fixed or reviewed labels Aug 24, 2023
@saubyk saubyk added this to the v0.18.0 milestone Aug 24, 2023
@saubyk saubyk added this to lnd v0.18 Aug 24, 2023
@saubyk saubyk moved this to 📋 Backlog in lnd v0.18 Aug 24, 2023
@hieblmi hieblmi self-assigned this Sep 5, 2023
@hieblmi
Copy link
Collaborator

hieblmi commented Oct 26, 2023

Hi @saubyk, I have some questions.

  1. Should the probing functionality only pertain to EstimateRouteFee or should the router offer a separate API to probe a target, wich can then be called from EstimateRouteFee but also from other endpoints.

  2. If probing is indicated then should the probe payment be sent on the route that the current API estimated from graph data or should the SendPayment Api figure out a route? I have to check if SendPayment calculates the route in the same way as EstimateRouteFee currently.

  3. How about handling liquidity issues, say if the invoice's payment amount can't be forwarded on a route then would that also be considered "reachable=false" or should this be an extra error condition? Maybe we can return more expressive errors depending on the probing issue?

@saubyk
Copy link
Collaborator Author

saubyk commented Oct 27, 2023

  1. Should the probing functionality only pertain to EstimateRouteFee or should the router offer a separate API to probe a target, wich can then be called from EstimateRouteFee but also from other endpoints.

I think it makes sense to use the same EstimateRouteFee api, which is not functional right now

  1. If probing is indicated then should the probe payment be sent on the route that the current API estimated from graph data or should the SendPayment Api figure out a route? I have to check if SendPayment calculates the route in the same way as EstimateRouteFee currently.

My expectation here is that the route is calculated the same way regardless, if it is different we should let SendPayment api decide, as the idea is to emulate the actual payment

  1. How about handling liquidity issues, say if the invoice's payment amount can't be forwarded on a route then would that also be considered "reachable=false" or should this be an extra error condition? Maybe we can return more expressive errors depending on the probing issue?

In this scenario, "reachable=false" is a correct outcome, but there should be a way to distinguish the reason for failure. As a successful probe is also expected to fail eventually, but the reason should indicate that payment failed because the payment hash was invalid, indicating that it can reach the destination. In the scenario you laid out, the payment cannot reach the destination with the given route

@alexbosworth
Copy link
Contributor

I guess there is an issue because it's unlikely you'll succeed on a single route

This implies that the API would be streaming, unless the caller is expected to repeat the call and let mission control take care of the path updating

Another thing to think about is what happens if the attempt takes a long time, do you move on or block

@saubyk
Copy link
Collaborator Author

saubyk commented Oct 30, 2023

I guess there is an issue because it's unlikely you'll succeed on a single route

This implies that the API would be streaming, unless the caller is expected to repeat the call and let mission control take care of the path updating

My assumption is, if the SendPayment API is used, it should try multiple routes until successful (or in this case, a failure with an invalid hash error).

Another thing to think about is what happens if the attempt takes a long time, do you move on or block

There is an expectation to include a new timeout param, which will enable the user to control how much time they want to allow for the payment to succeed in. If the payment takes more time, it's considered a failure

@hieblmi
Copy link
Collaborator

hieblmi commented Oct 31, 2023

Yes agree, I think we could solve all these issues by just calling SendPaymentV2 which tracks payments over multiple tries and also considers a user-specified timeout for each of those tries.

I have now a version of this working locally, the EstimateRouteFee calls SendPayment in a goroutine and monitors timeout and payment settlement failures due to invalid hash.

Question: Should we deprecate the current fields of a RouteFeeRequest and ignore them in the EstimateRouteFee call entirely?

bytes dest = 1;

@saubyk saubyk linked a pull request Oct 31, 2023 that will close this issue
@saubyk saubyk moved this from 📋 Backlog to 🏗 In progress in lnd v0.18 Oct 31, 2023
@hieblmi hieblmi pinned this issue Jan 17, 2024
@Roasbeef Roasbeef moved this from 🏗 In progress to 👀 In review in lnd v0.18 Feb 14, 2024
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in lnd v0.18 Mar 5, 2024
@saubyk saubyk unpinned this issue Mar 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to existing features / behaviour P1 MUST be fixed or reviewed payments Related to invoices/payments routing
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants