Use all valid routes during blinded path construction #9334
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 (
|
MPins
changed the title
MPins
changed the title
MPins
changed the title
|
This seems like the simpler solution to implement. Since we’re already querying all routes in FindBlindedPaths, it makes sense to just return them. However, it might be worthwhile to introduce a hard constant cap during the pathfinding process to prevent the array size from growing exponentially. Currently, the algorithm explores all possible paths within the given constraints. Instead, we could stop after finding, say, 100 routes in total, and then apply filtering to select the MaxNumPaths in BuildBlindedPaymentPaths, as proposed in this PR. Probably the constraints of the Min/Max Hop number prevents these high numbers for routes but I think having a cap is kind of a sanity check. |
Before the proposed changes we are querying all routes, sorting the routes based on probability and them capping them based on |
I think we need to do way more when creating the path rather than creating the path and later checking the probability this does not seem efficient. I think we should build the path in the first place based on probability not just running a depth first search on all available paths. But I am not sure how difficult that might be because we went with the recusrive approach. |
I think we might compute the total probability and sort the path list during the creation of it (into |
|
yes let's keep it as is and focus only on the underlying issue. |
|
Hello @ziggie1984 gently reminder of review 😉 |
|
unit-test fail ! |
There was a problem hiding this comment.
Looking good I am missing 2 things here:
- Can you add some logging stats how many routes are find in total, then how many are filtered out because of the probability as for example debug information. I am very curious whether routes will fail often at the level where the final route and relay information is accumulated.
- I tend to introduce a max route variable for the blinded path finding in general, maybe a default of 50 routes, otherwise we look for everything which with a big number of blinded hops might be wasteful.
MPins
force-pushed
the
issue-9076
branch
2 times, most recently
from
41573d0 to
c1251d7
Compare
Hello Ziggie, I had addressed your comments, thank you! Ready for your next review! |
|
@ziggie1984 and @Abdulkbk , it is ready for your review. 🤝 |
There was a problem hiding this comment.
Looking good, I want to get @ellemouton opinion on the recursive abort criteria in case we have a maximum number of paths.
Missing release notes for LND 20.
routing/router.go
Outdated
| for _, route := range routes { | ||
| if len(bestRoutes) >= int(restrictions.MaxNumPaths) { | ||
| if len(allRoutes) >= maxRoutes { |
There was a problem hiding this comment.
that is not what I meant, my idea was to exit the recursion early when enough paths are found,
I was more thinking of restricting the recursion in processNodeForBlindedPath but I would like to get @ellemouton opinion on this and also this can probably done in another PR.
Currently our DFS exhausts all Paths which exist in the graph, so I was thinking we could abort the DFS after we already have a hopSet of let's say 100 ?
Imagine we have a minRouteLen of 2 and a maxRouteLen of 6, each node has more than 100 channels, when reaching the depth of 6, we could easily accumulate > 1000 routes which seems unreasonable.
But the above restrictions we do not need, because really makes not difference we already have all the paths in memory.
There was a problem hiding this comment.
You right, thanks for the explanation.
The point is that, in the current implementation, we first retrieve all the routes and then sort them to prioritize those with the highest success probability.
Instead, we should modify the process to evaluate the probability of each route during the iteration, keeping only the best ones.
There was a problem hiding this comment.
seems like this is removed but not in this commit, can you make sure this is removed also in this commit
|
Hello @ellemouton , @ziggie1984 and @Abdulkbk ready for the next round 😊 |
Yeah might still be worth having some hard bound.
this is no longer relevant yeah? But from context: yeah could be cool to stop once we have found enough but hard to do since then we dont have all probabilities to compare against... something to ponder on. |
routing/router.go
Outdated
| log.Debugf("Found %v routes, discarded %v low probability routes", | ||
| len(paths), len(paths)-len(routes)) | ||
|
|
||
| bestRoutes = append(bestRoutes, route.route) | ||
| // Return all routes. | ||
| allRoutes := make([]*route.Route, 0, len(routes)) | ||
| for _, route := range routes { | ||
| allRoutes = append(allRoutes, route.route) |
There was a problem hiding this comment.
i think we can now just all the probability stuff on the caller side. No need to do it here or to pass in probabilitySrc probabilitySource anymore. We can now just add r.server.defaultMC.GetProbability to the AddInvoiceConfig and do sorting there.
There was a problem hiding this comment.
the only reason it was here before was cause we wanted to adhere to MaxNumPaths here.
There was a problem hiding this comment.
Hello @ellemouton, just to make sure ... basically do you mean do the probability stuff into the invoicesrpc.AddInvoice ... if I understand correctly it is just for code organization, right?
MPins
force-pushed
the
issue-9076
branch
4 times, most recently
from
48d10d0 to
1b389e9
Compare
|
Hello @ellemouton, @ziggie1984 and @Abdulkbk ready for the next review! |
rpcserver.go
Outdated
| return r.server.chanRouter.FindBlindedPaths( | ||
| r.selfNode, amt, | ||
| r.server.defaultMC.GetProbability, | ||
| QueryBlindedPaths: func() ([][]blindedpath.BlindedHop, error) { |
There was a problem hiding this comment.
why do we still need this function and cannot just add the routes to the AddInvoice call, it can all be predetermined before triggering the invoice creation call.
There was a problem hiding this comment.
I would like to hear @ellemouton 's opinion as well 😊
There was a problem hiding this comment.
i vote for keeping logic in one area & having only one location that needs to worry about calling various things. Otherwise you need to repeat a bunch of set-up calls each time you call AddInvoice from a different call-site.
rpcserver.go
Outdated
| return r.server.chanRouter.FindBlindedPaths( | ||
| r.selfNode, amt, | ||
| r.server.defaultMC.GetProbability, | ||
| QueryBlindedPaths: func() ([][]blindedpath.BlindedHop, error) { |
There was a problem hiding this comment.
I would like to hear @ellemouton 's opinion as well 😊
|
Hello @ellemouton, this is a friendly reminder that we need your opinion to move forward! |
|
Thanks for the ping! I'll prio for tomorrow 🙏 |
rpcserver.go
Outdated
| return r.server.chanRouter.FindBlindedPaths( | ||
| r.selfNode, amt, | ||
| r.server.defaultMC.GetProbability, | ||
| QueryBlindedPaths: func() ([][]blindedpath.BlindedHop, error) { |
There was a problem hiding this comment.
i vote for keeping logic in one area & having only one location that needs to worry about calling various things. Otherwise you need to repeat a bunch of set-up calls each time you call AddInvoice from a different call-site.
Thank you for the review! Sorry for keeping everything in one commit — I initially thought it made sense since it was all related to the same bug. But I agree it made the review harder. I’ll split it up to make things easier to follow. Anyway, as I mentioned in reply to your comments, the bug is addressed because the cap for |
MPins
force-pushed
the
issue-9076
branch
2 times, most recently
from
0460004 to
c788928
Compare
MaxNumPaths restriction moved from FindBlindedPaths to BuildBlindedPaymentPaths this way we can interact through all possibly routes when creating a new blinded path. The FindBlindedPaths function was moved from router.go to pathfind.go, since the probability logic was moved to the function BlindedPathsToRoutes on addinvoice.go. Add the function BlindedPathsToRoutes which converts blinded paths into routess, filtering out low-probability routes based on a given ProbabilitySource, and returns the valid ones sorted by success probability.
|
Hello @ellemouton and @ziggie1984, I’ve reordered the commits as requested. But I wasn’t able to split the “multi” commit, as the changes are tightly coupled due to dependencies between them. |
There was a problem hiding this comment.
So I think this PR still needs some work.
I agree with Elle that the current PR is a mix of Refactor + Logic change which makes it hard to review So I propose a new commit structure.
Moreover I don't think we should remove the responsibility of the chanRouter in this code but open for discussion on this one.
|
|
||
| // probabilitySrc is a helper that returns the mission control success | ||
| // probability of a forward between two vertices. | ||
| probabilitySrc := func(from route.Vertex, to route.Vertex, |
There was a problem hiding this comment.
Nit: probabilitySrc => mockProbabilitySrc
| QueryBlindedRoutes: func(amt lnwire.MilliSatoshi) ( | ||
| []*route.Route, error) { | ||
|
|
||
| return r.server.chanRouter.FindBlindedPaths( |
There was a problem hiding this comment.
I think we should not remove the responsibility of the chanRouter here. given the description of it, it should be the source which handles all graph queries see description (questions regarding graph reachability:
Lines 317 to 323 in b068d79
There was a problem hiding this comment.
If we still one to remove from there I definitely agree with Elle, that the commit structure right now is very hard to review because we mix refactor with a code change. So depending on our decision of where we want to do the blinded path finding we should first move code between packages, and then make the functional change, otherwise it is difficult to understand what really changed.
If we decide to remove the responsibilty of the chanRouter the commit structure should look something like this:
- Move
func (r *ChannelRouter) FindBlindedPathsinto its own package and add an additonal func param for the graph we previouly got for the chanRouter + change the corresponding tests to call the right func. - Move the max num of paths out (still keeping the probability) where it is + adding new tests etc ...
made an example here: https://github.com/ziggie1984/lnd/tree/new-commit-structure
and I think we don't have to do further refactor in this PR, meaning that we do not need to pull out the probability stuff here.
There was a problem hiding this comment.
@ziggie1984 the path is already being created outside of router.go by findBlindedPaths → processNodeForBlindedPath in pathfind.go, so it might make sense to relocate this logic elsewhere. As @ellemouton previously mentioned, it's preferable to keep related logic in one place, minimizing the number of components that need to orchestrate multiple things.
With that in mind, I’d like to propose remodeling this PR — including the commit structure. Specifically:
1- create a commit to move the `MaxNumPaths` restriction from `FindBlindedPaths` to `BuildBlindedPaymentPaths`.
2- add one or more commits to reorganize the place of the logic accordingly.
3- if needed, add a commit for further refactoring/cleanup.
Does that sound good to both of you?
There was a problem hiding this comment.
So I would prefer laying out your idea how you wanna refactor the code, and only then do the functional change. Then it makes more sense because normally for me the flow should be:
You prepare the code (Refactor) so that the functional change becomes more clear. Otherwise why do the functional change and then refactor ? Somehow that would not make so much sense to me because the functional change was done beforehand, does it make sense ?
There was a problem hiding this comment.
@ziggie1984 I think the functional changes are quite clear when looking at the code directly. However, the refactoring introduces complexity that makes the flow harder to follow.
Since the refactoring was inspired by this comment from @ellemouton (#9334 (comment)) I think it would help to clarify the intent of each refactoring step in the commit messages after the functional change.
After reviewing everything, I’m now unsure whether the refactoring is the right move. While @ellemouton reasoning makes sense, we can see similar usage of the ProbabilityEstimator logic in the FindRoute → findPath flow, which suggests the current structure is already consistent with other parts of the codebase.
There was a problem hiding this comment.
As mentioned in my other comment, I am also hesitant ripping the logic out of the ChanRouter because it should be responsible of finding the paths. But I think we should have a design call on the way forward here.
| @@ -547,35 +547,59 @@ func genBlindedRouteData(rand *rand.Rand) *record.BlindedRouteData { | |||
| // an example mentioned in this spec document: | |||
| // https://github.com/lightning/bolts/blob/master/proposals/route-blinding.md | |||
| // This example does not use any dummy hops. | |||
| // Added an additional node (Dave), increasing the possible paths, which can | |||
| // then be filtered using the MaxNumPaths (maximum blinded paths) option. | |||
There was a problem hiding this comment.
I think Elle mentioned this in your other PR, I am in favour of always adding new unit-tests rather than trying to change old ones. With the new commit structure this should be easy to do. I don'T think we need to check the blinded path values in this new test but rather only check if the right amount of route is returned.
Fixes #9076
MaxNumPaths restriction moved from
FindBlindedPathstoBuildBlindedPaymentPathsthis way we can interact through all possibly routes.If the reviewers agreed to this approach we will have to modify the
TestFindBlindedPathsWithMCbecause it is considering that MaxNumPaths retriction is applied onFindBlindedPathsfunction.