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: non-strict path finding #3558

Merged
merged 3 commits into from
Oct 30, 2019

Conversation

joostjager
Copy link
Contributor

@joostjager joostjager commented Sep 30, 2019

This pr makes path finding (and build route) aware of non-strict forwarding. Before deciding on a path edge, it calculates a single unified policy for the complete set of channels between two nodes.

@joostjager joostjager added incomplete WIP PR, not fully finalized, but light review possible routing labels Sep 30, 2019
@joostjager joostjager force-pushed the non-strict-pathfinding branch 5 times, most recently from 6dd4409 to 1bcafde Compare September 30, 2019 19:51

// MaxMilliSatoshi is the maximum number of msats that can be expressed
// in this data type.
MaxMilliSatoshi = ^MilliSatoshi(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

should we cap this at 21M * 100M * 1000 msat instead? introduce in same commit as usage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ltc...?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the maximum amount of millisatoshis that could ever exist on the chain. Litecoin would have a different amount though.

routing/unified_policy.go Outdated Show resolved Hide resolved
routing/unified_policy.go Outdated Show resolved Hide resolved
u.feeProportionalMillionths = policy.FeeProportionalMillionths
}

// For htlc amount constraints, take the widest range.
Copy link
Contributor

Choose a reason for hiding this comment

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

are we not trying to take the narrowest range, as to be able to satisfy any of the links? maybe we are thinking about the widths differently

Copy link
Contributor Author

@joostjager joostjager Oct 3, 2019

Choose a reason for hiding this comment

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

I took a different approach where I don't squash all policies together. Instead, the optimal policy is returned based on the actual amount.

routing/unified_policy.go Outdated Show resolved Hide resolved
routing/unified_policy.go Outdated Show resolved Hide resolved
@joostjager joostjager removed the request for review from halseth October 1, 2019 17:27
@joostjager
Copy link
Contributor Author

Did a quick check how prevalent multiple (advertized) channels between a pair of nodes at the moment. About 7.5% of the node pairs have multiple channels. This doesn't say much though about how often multi-channel node connections are traversed during payments.

lncli describegraph | jq '.edges[] | .node1_pub + " " + .node2_pub' -r | sort | uniq -c | awk '{print $1}' | sort | uniq -c

nof channels node pair count
1 29093
2 1834
3 367
4 102
5 23
6 10
7 3
8 2
9 5
10 1
12 1
15 1
17 1
18 1
29 1
35 1
105 1
150 1

@joostjager
Copy link
Contributor Author

Results when filtered for channels between bos list nodes is 20%.

nof channels node pair count
1 3015
2 427
3 252
4 73
5 15
6 2
7 1
15 1
17 1

@joostjager joostjager added v0.9.0 and removed incomplete WIP PR, not fully finalized, but light review possible labels Oct 6, 2019
@joostjager joostjager added this to the 0.9 milestone Oct 8, 2019
@joostjager joostjager self-assigned this Oct 23, 2019
Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

Nice refactor on BuildRoute!


// MaxMilliSatoshi is the maximum number of msats that can be expressed
// in this data type.
MaxMilliSatoshi = ^MilliSatoshi(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's the maximum amount of millisatoshis that could ever exist on the chain. Litecoin would have a different amount though.

routing/unified_policies.go Show resolved Hide resolved
routing/unified_policies.go Show resolved Hide resolved
@wpaulino
Copy link
Contributor

Travis failed with:

--- FAIL: TestChannelLinkWaitForRevocation (10.99s)
    link_test.go:4615: expected RevokeAndAck, got *lnwire.CommitSig

Looks unrelated, but is it known?

@wpaulino wpaulino removed their request for review October 24, 2019 13:38
@joostjager
Copy link
Contributor Author

Travis failed with:

--- FAIL: TestChannelLinkWaitForRevocation (10.99s)
    link_test.go:4615: expected RevokeAndAck, got *lnwire.CommitSig

Looks unrelated, but is it known?

Looks unrelated indeed. Maybe caused by the log commit ticker... Could be fixed by #2927

In this commit we change path finding to no longer consider all channels
between a pair of nodes individually. We assume that nodes forward
non-strict and when we attempt a connection between two nodes, we don't
want to try multiple channels because their policies may not be identical.
Having distinct policies for channel to the same peer is against the
recommendation in the spec, but it happens in the wild. Especially since
we recently changed the default cltv delta value.

What this commit introduces is a unified policy. This can be looked upon
as the greatest common denominator of all policies and should maximize
the probability of getting the payment forwarded.
Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

Very clean and well tested!! LGTM ✅

routing/unified_policies.go Outdated Show resolved Hide resolved
@joostjager joostjager merged commit a8f077a into lightningnetwork:master Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants