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: local balance check before path finding #3749

Merged

Conversation

joostjager
Copy link
Collaborator

@joostjager joostjager commented Nov 21, 2019

Fixes #1680

@joostjager joostjager removed request for Roasbeef and halseth Nov 21, 2019
@joostjager joostjager force-pushed the extended-routing-failures branch 2 times, most recently from 962616c to 7939a9d Compare Nov 21, 2019
Copy link
Collaborator

@cfromknecht cfromknecht left a comment

nice commit structure, only question i have is whether or not to also begin exposing the tlv failures as a specific rpc destination failure rather than no route found. it's not super in line with this pr, so may be best in a follow up

routing/pathfind.go Outdated Show resolved Hide resolved
routing/payment_session.go Outdated Show resolved Hide resolved
routing/payment_lifecycle.go Show resolved Hide resolved
@joostjager joostjager force-pushed the extended-routing-failures branch 2 times, most recently from 05ffb0f to bbd7cd5 Compare Nov 26, 2019
@joostjager joostjager requested a review from cfromknecht Nov 26, 2019
@joostjager joostjager self-assigned this Nov 26, 2019
@joostjager joostjager added this to WIP in v0.9.0-beta via automation Nov 26, 2019
@joostjager joostjager added this to the 0.9.0 milestone Nov 26, 2019
@joostjager joostjager requested a review from guggero Nov 26, 2019
channeldb/payments.go Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the extended-routing-failures branch from bbd7cd5 to a57595b Compare Nov 26, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Nov 26, 2019

Todo: add test

@joostjager joostjager force-pushed the extended-routing-failures branch from a57595b to 7f7cc1e Compare Nov 27, 2019
@joostjager joostjager moved this from WIP to Needs Review in v0.9.0-beta Nov 27, 2019
Copy link
Collaborator

@guggero guggero left a comment

Great work, the changes were very easy to follow.
This fix should really improve the UX of lnd, especially for beginners.
There are only two small things that should be fixed IMO, the rest is non-blocking.

routing/pathfind_test.go Show resolved Hide resolved
routing/errors.go Outdated Show resolved Hide resolved
channeldb/payments.go Outdated Show resolved Hide resolved
routing/pathfind.go Show resolved Hide resolved
@joostjager joostjager requested a review from guggero Nov 27, 2019
@joostjager joostjager force-pushed the extended-routing-failures branch from 7f7cc1e to 32900ec Compare Nov 27, 2019
Copy link
Collaborator

@guggero guggero left a comment

LGTM 💯

Copy link
Collaborator

@cfromknecht cfromknecht left a comment

LGTM 👍

// ErrTargetNotInNetwork is returned when the target of a path-finding
// or payment attempt isn't known to be within the current version of
// the channel graph.
ErrTargetNotInNetwork
ErrTargetNotInNetwork errorCode = iota
Copy link
Collaborator

@cfromknecht cfromknecht Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preexisting, but errorCode should be exported if ErrTargetNotInNetwork is also exported for godocs

Copy link
Collaborator

@cfromknecht cfromknecht Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be made into a trivial follow up

v0.9.0-beta automation moved this from Needs Review to Approved Dec 3, 2019
@cfromknecht
Copy link
Collaborator

@cfromknecht cfromknecht commented Dec 3, 2019

two of the async benchmarks failed, perhaps an indicator that this introduces a performance regression?

@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Dec 3, 2019

two of the async benchmarks failed, perhaps an indicator that this introduces a performance regression?

Manually checked the timing of a queryroutes call on testnet, didn't see a difference with master. Restarted itests

@joostjager joostjager moved this from Approved to Needs Review in v0.9.0-beta Dec 3, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Dec 3, 2019

Yes, does seem to be failing consistently. Will check it out.

joostjager added 2 commits Dec 4, 2019
A unified policy differs between local channels and other channels on
the network. There is more information available for local channels and
this is used in the unified policy.

Previously we used the pathfinding source pubkey to determine whether to
apply the local channel logic or not. If queryroutes is executed with a
source node that isn't the self node, this wouldn't work.
@joostjager joostjager force-pushed the extended-routing-failures branch from 32900ec to 9ae014e Compare Dec 4, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Dec 4, 2019

Turned out to be a deadlock with getting the source node. Fixed now

@joostjager joostjager moved this from Needs Review to Approved in v0.9.0-beta Dec 4, 2019
@joostjager joostjager merged commit 883f9e5 into lightningnetwork:master Dec 4, 2019
1 of 2 checks passed
v0.9.0-beta automation moved this from Approved to Done Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v0.9.0-beta
  
Done
Development

Successfully merging this pull request may close these issues.

3 participants