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

routing+routerrpc: take max cltv limit into account within path finding #3595

Merged

Conversation

@wpaulino
Copy link
Collaborator

wpaulino commented 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.

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.

@wpaulino wpaulino force-pushed the wpaulino:max-cltv-expiry-pathfinding branch from 46eaa24 to ad5568f Oct 11, 2019
lnrpc/routerrpc/router_backend.go Outdated Show resolved Hide resolved
lnrpc/rpc.proto Outdated Show resolved Hide resolved
routing/pathfind_test.go Outdated Show resolved Hide resolved
lnrpc/routerrpc/router_backend.go Show resolved Hide resolved
wpaulino added 3 commits 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 wpaulino force-pushed the wpaulino:max-cltv-expiry-pathfinding branch from ad5568f to 4e1658a Oct 11, 2019
@wpaulino wpaulino requested a review from joostjager Oct 11, 2019
@wpaulino wpaulino added this to the 0.8.0 milestone Oct 11, 2019
lnrpc/rpc.proto Show resolved Hide resolved
lnrpc/routerrpc/router_backend.go Show resolved Hide resolved
lnrpc/rpc.proto Show resolved Hide resolved
@wpaulino wpaulino requested review from Roasbeef and joostjager and removed request for cfromknecht Oct 14, 2019
findErr error
route, err := r.FindRoute(
sourcePubKey, targetPubKey, amtMSat, restrictions,
destTlvRecords, finalCLTVDelta,

This comment has been minimized.

Copy link
@joostjager

joostjager Oct 14, 2019

Collaborator

Can the FindRoute without cltv delta parameter now be removed?

This comment has been minimized.

Copy link
@wpaulino

wpaulino Oct 14, 2019

Author Collaborator

That's already done in the diff.

This comment has been minimized.

Copy link
@wpaulino

wpaulino Oct 14, 2019

Author Collaborator

Did you mean to ask why? If so, because the router's FindRoute already applies the default when the value is 0.

This comment has been minimized.

Copy link
@joostjager

joostjager Oct 14, 2019

Collaborator

I mean the finalExpiry ...uint16 (three dots)

Copy link
Member

Roasbeef left a comment

LGTM 🧯

@Roasbeef Roasbeef merged commit faacd8d into lightningnetwork:master Oct 14, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
coverage/coveralls Coverage increased (+0.05%) to 63.248%
Details
@wpaulino wpaulino deleted the wpaulino:max-cltv-expiry-pathfinding branch Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.