Clarify find_route Documentation#4421
Clarify find_route Documentation#4421StephenChi-hi wants to merge 3 commits intolightningdevkit:mainfrom
Conversation
|
👋 I see @tankyleo was un-assigned. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4421 +/- ##
==========================================
- Coverage 86.09% 86.06% -0.03%
==========================================
Files 156 156
Lines 103623 103623
Branches 103623 103623
==========================================
- Hits 89211 89187 -24
- Misses 11895 11919 +24
Partials 2517 2517
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lightning/src/routing/router.rs
Outdated
| /// However, the enabled/disabled bit on such channels as well as the `htlc_minimum_msat` / | ||
| /// `htlc_maximum_msat` *are* checked as they may change based on the receiving node. | ||
| /// | ||
| /// Routes are constructed by searching the provided [`NetworkGraph`] for paths from the payer |
There was a problem hiding this comment.
This reads as a second introductory sentence, maybe it should simply replace the first sentence instead?
lightning/src/routing/router.rs
Outdated
| /// | ||
| /// Routes are constructed by searching the provided [`NetworkGraph`] for paths from the payer | ||
| /// to the payee that satisfy the constraints in [`RouteParameters`]. Fees are accumulated | ||
| /// backwards from the payee to the payer, and CLTV expiry deltas are aggregated hop-by-hop. |
There was a problem hiding this comment.
"accumulated backwards" doesn't really tell me what this means or why its important if I don't know LN super well. Maybe its worth just leaving out or tweaking, though I'm not quite sure how to do so, maybe something like this?
| /// backwards from the payee to the payer, and CLTV expiry deltas are aggregated hop-by-hop. | |
| /// by totaling fees at each hop from the payee to the payer, and CLTV expiry deltas are aggregated hop-by-hop. |
lightning/src/routing/router.rs
Outdated
| /// backwards from the payee to the payer, and CLTV expiry deltas are aggregated hop-by-hop. | ||
| /// | ||
| /// Channel selection and path ranking are influenced by the provided [`ScoreLookUp`] | ||
| /// implementation, which may apply penalties based on historical reliability, |
There was a problem hiding this comment.
Mention how the penalties are included.
There was a problem hiding this comment.
Thanks for the feedback @TheBlueMatt . I will do that
|
🔔 1st Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tankyleo! This PR has been waiting for your review. |
…routing behavior, and error handling
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Please feel free to squash the commits.
| /// How we handle the payee depends on what they provided: | ||
| /// - Bolt11 invoices: Include features via [`RouteParameters::payment_params`] to enable MPP. | ||
| /// Without features, MPP only works if the payee is already known in the network graph. | ||
| /// - Bolt12 invoices: May include blinded paths for privacy. We unblind these in the returned |
There was a problem hiding this comment.
No? blinded paths cannot be unblinded.
| /// higher penalties are naturally less likely to be selected during the search. | ||
| /// 4. Path construction: We build multiple candidate paths while staying within the fee limit | ||
| /// and respecting all liquidity and feature constraints. | ||
| /// 5. Value provisioning: We collect paths until the total value is roughly 3x the requested |
There was a problem hiding this comment.
Not sure this is useful to describe unless we want to get into a lot of details about why we do this and how.
| /// Panics if first_hops contains channels without `short_channel_id`s; | ||
| /// [`ChannelManager::list_usable_channels`] will never include such channels. | ||
| /// Scoring and Channel Selection | ||
| /// The scorer you provide influences which paths we select during the actual pathfinding. The |
There was a problem hiding this comment.
This was already stated above.
| /// [`ChannelManager::list_usable_channels`] will never include such channels. | ||
| /// Scoring and Channel Selection | ||
| /// The scorer you provide influences which paths we select during the actual pathfinding. The | ||
| /// scorer assigns penalties to channels based on factors like: |
There was a problem hiding this comment.
It can, but in the context here the scorer doesn't do anything, its an interface to a scorer.
3 participants
hi, in this pr i worked on improving the documentation for the
find_routefunction insiderouter.rs.i explained better how the route is built using the
NetworkGraphandRouteParameters, and how fees are added up along the path. i also described how the cltv expiry is combined through the hops, and howScoreLookUpcan affect which path is chosen.i added more detail about how
first_hopsis used to pick outbound channels, and also listed the possible errors the function can return.i just wanted to make it easier for anyone reading the code to understand how route finding actually works and what to expect when calling this function.
this is part of my first open source contributions, so if anything should be adjusted or written better please let me know. i really appreciate the effort that has gone into this project already.