routing: improve BuildRoute robustness #6912
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joostjager
changed the title
joostjager
force-pushed
the
newroute-safety
branch
from
f7b172c to
121a5b5
Compare
fix min htlc newroute
Sanity check to prevent invalid routes from being constructed.
With newRoute being aware of min_htlc constraints, there is no more need to do a forward pass in BuildRoute. This also fixes potential edge cases where the forward pass gets to a payment amount that still violates htlc ranges. The downside of this simplification is that more value may be dropped off along the path as fees rather than paying extra to the final hop. However, as the primary intention of the BuildRoute `min_amt` flag is to allow for cheap probes, this change is likely to have no consequences.
joostjager
force-pushed
the
newroute-safety
branch
from
121a5b5 to
4f6cde9
Compare
joostjager
commented
Sep 14, 2022
| } | ||
|
|
||
| pathEdges = append(pathEdges, policy) | ||
| edges[i] = policy | ||
| } | ||
|
|
||
| // Build and return the final route. | ||
| return newRoute( |
There was a problem hiding this comment.
I am considering doing away with newRoute completely. BuildRoute already steps though the hops and could just as well build the final route structure on the fly. This would also uncover potential bugs in BuildRoute that are later smoothed over in the final newRoute pass.
For pathfinding this may be even more relevant. If pathfinding chooses a sub-optimal path because of a bug, newRoute may fix it without anyone noticing.
There was a problem hiding this comment.
Implemented alternative in #6920
|
@bitromortac: review reminder |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Applies the
min_htlcchannel policy innewRoute. Some routes cannot be reconstructed purely based on the final payment amount.Additionally a sanity check for
max_htlcis added.With these changes, if becomes possible to simplify
BuildRouteand remove the possibility of running into edge cases aroundmin_htlc.A follow-up to this is to let pathfinding consider channels that have a
min_htlcabove the running amount. This would for example allow a user to complete a 100 msat payment via a channel that has a 1000 msatmin_htlc. The difference is paid in fees.