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

route blinding: use all valid routes during path construction iteration instead of the currently capped set #9076

Open
ellemouton opened this issue Sep 9, 2024 · 7 comments · May be fixed by #9334
Assignees
Labels
blinded paths bug Unintended code behaviour

Comments

@ellemouton
Copy link
Collaborator

Today, if we are constructing set of blinded payment paths to add to an invoice, we:

  1. Select up to MaxNumPaths routes to our own node that are considered blinded path candidates.
  2. This set is then passed to blindedpath.BuildBlindedPaymentPaths which attempts to use each path to build a valid blinded path
  3. If blindedpath.BuildBlindedPaymentPaths finds that it cannot use one of the paths it was given, it skips it and moves on to the next one

This order of operations means that we may run out of paths in blindedpath.BuildBlindedPaymentPaths and we wont go back and try some of the paths that we initially found by ChannelRouter's findBlindedPaths .

What we instead want is for blindedpath.BuildBlindedPaymentPaths to be able to continue with the iteration through possibly all of the routes found in findBlindedPaths until it reaches the max specified by MaxNumPaths.

@ellemouton ellemouton added enhancement Improvements to existing features / behaviour blinded paths bug Unintended code behaviour and removed enhancement Improvements to existing features / behaviour labels Sep 9, 2024
@ziggie1984
Copy link
Collaborator

Good idea, so you mean we should immediately when having the blinded path check if we can build the whole route before exiting the route search, so we make sure the returned routes are also possible to use ?

@ellemouton
Copy link
Collaborator Author

@ziggie1984 - yeah, or we make a way for the logic that is doing the iterating and building to query like NextCandidateRoute or something like that. Or actually, perhaps should return all possible routes and then do the MaxPath cap later on instead of as early as it is currently done

@MPins
Copy link
Contributor

MPins commented Oct 12, 2024

Do you mean that it might return less than MaxNumPaths routes without exhausting all valid routes? Maybe I can help to fix it!

@ellemouton
Copy link
Collaborator Author

@MPins - im saying it currently does the following:

  1. Chooses MaxNumPaths candidate paths
  2. Passes that to BuildBlindedPaths which might drop some of the paths returned cause they turn out not to be valid.

This is not ideal cause we then we can easily run out of paths to try.

Instead, the MaxNumPaths restriction should be applied later on instead. So when selecting candidates, we should not limit at that point.

@MPins
Copy link
Contributor

MPins commented Oct 14, 2024

Got it. If nobody had started working on it, I can give a try.

@saubyk
Copy link
Collaborator

saubyk commented Oct 14, 2024

This is for my education...why do we have a limit capped by MaxNumPaths?

@ziggie1984
Copy link
Collaborator

ziggie1984 commented Oct 14, 2024

otherwise the invoice would become super huge, there might be a huge number of paths to a particular node. Moreover we select the paths with the most probability at first so further paths have an even lower chance to succeed in making the payment so it makes sense to limit this number from this pov as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blinded paths bug Unintended code behaviour
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants