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: add cltv limit #2640

Merged
merged 6 commits into from Mar 27, 2019
Merged

routing: add cltv limit #2640

merged 6 commits into from Mar 27, 2019

Conversation

@joostjager
Copy link
Collaborator

@joostjager joostjager commented Feb 13, 2019

This PR adds a maximum cltv limit for payments, similar to the currently existing fee limit.

@joostjager joostjager requested a review from halseth Feb 13, 2019
@joostjager joostjager force-pushed the joostjager:cltv-limit branch from 532ffa0 to 8d284a5 Feb 13, 2019
@Roasbeef Roasbeef added this to the 0.6 milestone Feb 13, 2019
@joostjager joostjager requested a review from wpaulino Feb 13, 2019
Copy link
Collaborator

@wpaulino wpaulino left a comment

Somewhat unrelated -- I noticed that we check for the fee limit when constructing the route from the path. Is there a reason that check is still needed since we already do it within path finding?

routing/pathfind.go Outdated Show resolved Hide resolved
routing/pathfind_test.go Outdated Show resolved Hide resolved
cmd/lncli/commands.go Show resolved Hide resolved
lnrpc/rpc.proto Show resolved Hide resolved
routing/pathfind_test.go Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:cltv-limit branch from 8d284a5 to e8a215a Feb 14, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Feb 14, 2019

Somewhat unrelated -- I noticed that we check for the fee limit when constructing the route from the path. Is there a reason that check is still needed since we already do it within path finding?

I don't think there is. It is a sanity check, but I think it can be removed. I didn't bother adding another one for cltvLimit.

@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Feb 14, 2019

@wpaulino PTAL

@wpaulino
Copy link
Collaborator

@wpaulino wpaulino commented Feb 14, 2019

I don't think there is. It is a sanity check, but I think it can be removed. I didn't bother adding another one for cltvLimit.

Gotcha, would be nice to remove it in a follow-up PR then.

@joostjager joostjager force-pushed the joostjager:cltv-limit branch from e8a215a to 04e5c8d Feb 14, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Feb 14, 2019

I don't think there is. It is a sanity check, but I think it can be removed. I didn't bother adding another one for cltvLimit.

Gotcha, would be nice to remove it in a follow-up PR then.

Yes, I can do that.

@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Feb 14, 2019

@wpaulino ptal

Copy link
Collaborator

@wpaulino wpaulino left a comment

LGTM 🌵

routing/pathfind.go Show resolved Hide resolved
routing/router.go Show resolved Hide resolved
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Feb 21, 2019

@halseth comments replied to, ptal

@joostjager joostjager force-pushed the joostjager:cltv-limit branch from 04e5c8d to db62003 Mar 5, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Mar 5, 2019

Rebased

@joostjager joostjager force-pushed the joostjager:cltv-limit branch 2 times, most recently from 4738b27 to d3ef006 Mar 6, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Mar 6, 2019

I don't think there is. It is a sanity check, but I think it can be removed. I didn't bother adding another one for cltvLimit.

Gotcha, would be nice to remove it in a follow-up PR then.

It is happening in #2497

@joostjager joostjager force-pushed the joostjager:cltv-limit branch from d3ef006 to daba5e2 Mar 15, 2019
@wpaulino
Copy link
Collaborator

@wpaulino wpaulino commented Mar 15, 2019

Needs a rebase.

@@ -2836,6 +2837,11 @@ func extractPaymentIntent(rpcPayReq *rpcPaymentRequest) (rpcPaymentIntent, error
payIntent.outgoingChannelID = &rpcPayReq.OutgoingChanId
}

// Take cltv limit from request if set.
if rpcPayReq.CltvLimit != 0 {

This comment has been minimized.

@halseth

halseth Mar 19, 2019
Collaborator

This check wouldn't be needed if we used 0 to indicate no limit.

This comment has been minimized.

@joostjager

joostjager Mar 19, 2019
Author Collaborator

It is the same discussion as above. The conversion is done as early as possible to prevent having to deal with the magic value further down the call stack.

cmd/lncli/commands.go Outdated Show resolved Hide resolved
routing/pathfind.go Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:cltv-limit branch from daba5e2 to 8d6d7f1 Mar 19, 2019
This condition may be caused by a bug somewhere else in the system.
Expose it here as a warn log line.
@joostjager joostjager force-pushed the joostjager:cltv-limit branch from 8d6d7f1 to 12dc634 Mar 19, 2019
@joostjager joostjager force-pushed the joostjager:cltv-limit branch from 12dc634 to ec0d241 Mar 19, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Mar 19, 2019

ptal

Copy link
Collaborator

@halseth halseth left a comment

LGTM 🕺

@halseth halseth merged commit 4d8100c into lightningnetwork:master Mar 27, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0003%) to 60.953%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants