routing+routerrpc: take max cltv limit into account within path finding #3595
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
wpaulino
requested review from
cfromknecht,
joostjager and
Roasbeef
as code owners
wpaulino
force-pushed
the
max-cltv-expiry-pathfinding
branch
from
46eaa24
to
ad5568f
Compare
joostjager
reviewed
Oct 11, 2019
With the introduction of the max CLTV limit parameter, nodes are able to reject HTLCs that exceed it. This should also be applied to path finding, otherwise HTLCs crafted by the same node that exceed it never left the switch. This wasn't a big deal since the previous max CLTV limit was ~5000 blocks. Once it was lowered to 1008, the issue became more apparent. Therefore, all of our path finding attempts now have a restriction of said limit in in order to properly carry out HTLCs to the network.
The previous limit of 1008 proved to be low, given that almost 50% of the network still advertises CLTV deltas of 144 blocks, possibly resulting in routes with many hops failing.
wpaulino
force-pushed
the
max-cltv-expiry-pathfinding
branch
from
ad5568f
to
4e1658a
Compare
Roasbeef
reviewed
Oct 11, 2019
joostjager
reviewed
Oct 14, 2019
wpaulino
requested review from
Roasbeef and
joostjager
and removed request for
cfromknecht
joostjager
approved these changes
Oct 14, 2019
| findErr error | ||
| route, err := r.FindRoute( | ||
| sourcePubKey, targetPubKey, amtMSat, restrictions, | ||
| destTlvRecords, finalCLTVDelta, |
There was a problem hiding this comment.
Can the FindRoute without cltv delta parameter now be removed?
There was a problem hiding this comment.
That's already done in the diff.
There was a problem hiding this comment.
Did you mean to ask why? If so, because the router's FindRoute already applies the default when the value is 0.
There was a problem hiding this comment.
I mean the finalExpiry ...uint16 (three dots)
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.
With the introduction of the max CLTV limit parameter, nodes are able to reject HTLCs that exceed it. This should also be applied to path finding, otherwise HTLCs crafted by the same node that exceed it never left the switch. This wasn't a big deal since the previous max CLTV limit was ~5000 blocks. Once it was lowered to 1008, the issue became more apparent. Therefore, all of our path finding attempts now have a restriction of said limit in in order to properly carry out HTLCs to the network.
Along the way, we also increase the default limit to 2016 blocks as the previous one of 1008 blocks proved to be low, given that almost 50% of the network still advertises CLTV deltas of 144 blocks, possibly resulting in routes with many hops failing.
Fixes #3590.