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

askrene-getroutes: fix the value of the amount and delay #8066

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Lagrang3
Copy link
Collaborator

@Lagrang3 Lagrang3 commented Feb 7, 2025

In the previous implementation getroutes would provide for every i-th hop the value of the amount and delay which correspond to the (i-1)-th HTLC. This commit changes that, instead the i-th hop contains the value of the amount and delay of the i-th HTLC.

Changelog-Changed: askrene: getroutes outputs contains for each hop the correct amount and delay values.

Previously proposed as #7639

@Lagrang3 Lagrang3 requested a review from cdecker as a code owner February 7, 2025 19:33
@Lagrang3 Lagrang3 added this to the v25.02 milestone Feb 8, 2025
@Lagrang3 Lagrang3 force-pushed the askrene-fixup branch 2 times, most recently from 66ed456 to 2d4acee Compare February 8, 2025 11:00
In the previous implementation getroutes would provide for every i-th
hop the value of the amount and delay which correspond to the (i-1)-th
HTLC. This commit changes that, instead the i-th hop contains the value
of the amount and delay of the i-th HTLC.

Changelog-Changed: askrene: getroutes outputs contains for each hop the correct amount and delay values.

Signed-off-by: Lagrang3 <[email protected]>
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

I appreciate this is better for how we use the routes, but we can't break the API.

"amount_msat": 1250013,
"delay": 6
"amount_msat": 1250000,
"delay": 0
}
]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we were to make this change, what is the fee for this route? What is the total delay? You can no longer tell? Information has been lost.

We can't change the API anyway, but we could do:

  1. Add "node_id" with the prev node id (to make that explicit)
  2. Add "amount_msat_for_next" and "delay_for_next" which would be these fields.
  3. Make the documentation describe these fields more clearly.

Copy link
Collaborator Author

@Lagrang3 Lagrang3 Feb 11, 2025

Choose a reason for hiding this comment

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

Agree, we could add those fields instead of changing the API.
Or we could add for each route (not each hop) a total_delay and total_amount_msat, values.

Or maybe leave getroutes as it is and create a new rpc, for instance askrene-routes, just any different name,
that gives us precisely the quantities that we need to feed to the sphinx_path API.

For example:

deliver 1000 to C, final_cltv = 10
A -> B -> C
base fee = 1
delay delta = 6
routes: [
    {total_amount: 1002,
     final_amount: 1000,
     total_delay: 22,
     final_delay: 10,
     final_node: C,
     path: [ {node: A, channel: AB, amount: 1001, delay: 16},
                  {node: B, channel: BC, amount: 1000, delay: 10}] }
]

To build the onions:

for each route:
    sp = new sphinx_path()
    for each hop in route['path']:
        payload = onion_nonfinal_hop(hop['channel'], hop['amount'], hop['delay']+blockheight)
        sp.add_hop(hop['node'], payload)
    sp.add_final_hop(route['final_node'], route['final_amount'], ...)

@Lagrang3 Lagrang3 modified the milestones: v25.02, v25.05 Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants