pathfinding: capacity-dependent apriori model probability #6857

bitromortac · 2022-08-24T13:06:40Z

Change Description

Adds a limit for the pathfinding probability, namely that if the amount reaches the capacity of a channel, the success probability should decrease drastically. Fixes #5988 for the current pathfinding system.

To achieve this, we multiply the previous probability with a factor to take capacity into account:

P *= 1 - 0.5 / [1 + exp(-(amount - cutoffFactor*capacity)/(smearingFactor*capacity))]
graph for cap=1Msat, cutoff=0.75*cap, smearing=0.1*cap

This function has the effect that for small amounts we don't alter the current behavior, but for large amounts the probability is reduced. We still consider low-capacity channels and don't throw them away like a hard capacity reduction would do.

This PR introduces building blocks for an alternative description of a probability, see #6815.

Performance considerations:

I investigated the runtime for the capacity fetching and computation. This is interesting if we want to remove the capacity as a parameter of Estimator.getPairProbability and insert a routingGraph as a dependency instead, but this seems to add quite some latency.

without extra calculation (without last commit):
[DBG] CRTR: Pathfinding perf metrics: nodes=119, edges=10133, time=108.88252ms
with extra calculation (last commit):
[DBG] CRTR: Pathfinding perf metrics: nodes=250, edges=18004, time=13.57969395s

Questions:

The cutoff and scaling parameters need discussion, also whether we want to expose them.
Capacity computation should be given another thought.

Todo:

fix itests

lnrpc/routerrpc/router_backend.go

bitromortac · 2022-09-07T10:31:11Z

Added a few more unit test cases, fixed a failing itest and made only the apriori hop probability capacity dependent.

routing/probability_estimator.go

joostjager

Relatively compact pr with a large potential impact. No major comments. Mainly that it might be possible to optimize the commit structure for fewer rewrites. Hold off the addition of the capacity parameter until everything is in place. And I think some commits are more readable when squashed.

routing/probability.go

routing/missioncontrol.go

routing/probability_estimator.go

routing/pathfind.go

routing/unified_policies.go

lnrpc/routerrpc/router_server.go

lnrpc/routerrpc/router_backend.go

routing/pathfind_test.go

routing/probability.go

routing/pathfind_test.go

bitromortac · 2022-09-15T09:50:35Z

Relatively compact pr with a large potential impact. No major comments. Mainly that it might be possible to optimize the commit structure for fewer rewrites. Hold off the addition of the capacity parameter until everything is in place. And I think some commits are more readable when squashed.

Thank you for the quick review! I put the capacity calculation into getPolicy. Commits are now reordered to have less rewrites, some were squashed.

routing/graph.go

joostjager

Nice improvement with the commit structure 👍

routing/probability.go

routing/unified_policies.go

routing/graph.go

lnrpc/routerrpc/router_backend.go

lnrpc/routerrpc/router_server.go

lnrpc/routerrpc/router_backend.go

routing/pathfind_test.go

routing/probability_estimator_test.go

routing/unified_policies.go

routing/probability.go

routing/graph.go

routing/probability_estimator_test.go

rpcserver.go

positiveblue

LGTM @bitromortac

Thank you very much for taking the time of rewriting the commits in an easy way, this is my first time digging into this part of the code and it was pretty easy to follow 🥇

positiveblue · 2022-10-21T13:39:06Z

routing/unified_policies.go

@@ -223,6 +224,18 @@ func (u *unifiedPolicy) getPolicyNetwork(
 			continue
 		}

+		// Track the maximal capacity for usable channels. If we don't
+		// know the capacity, we fall back to MaxHTLC.
+		capMsat := lnwire.NewMSatFromSatoshis(edge.capacity)


q: when do we have no idea about the capacity of a channel? Unannounced channels from invoices with hop hints?

this is the case for hop hint channels, yes, as well as for neutrino nodes, which don't have the capacity for channels, as they would have to query each UTXO in the graph

If a neutrino node is running w/ the default "assume chan valid", then they won't have this information.

routing/unified_policies.go

positiveblue · 2022-10-21T14:46:05Z

routing/missioncontrol_test.go

+
+	// We relax the accuracy for the probability check because of the
+	// capacity cutoff factor.
+	require.InDelta(


routing/probability_estimator.go

lnrpc/routerrpc/router_backend.go

routing/graph.go

joostjager · 2022-10-24T07:14:43Z

routing/graph.go

+func (g *CachedGraph) FetchAmountPairCapacity(nodeFrom, nodeTo route.Vertex,
+	amount lnwire.MilliSatoshi) (btcutil.Amount, error) {
+
+	// For the local node we assume no information on the channel capacity.


Why is this necessary? And is it the right point to do this check? From a distance it looks like a pair capacity could still be returned for local channels.

Right, I removed it from the function, for this we already have the local probability calculation and it is a layer violation.

joostjager · 2022-10-24T07:17:55Z

routing/graph.go

+
+	// We may not have all policies available to describe the hop between
+	// the nodes (in the case of hop hints), which is why we return 0 in
+	// this case.


I think this comment requires slightly more explanation. Why is it necessary to return 0 in case of hop hints? Maybe the explanation belongs in a different layer, because isn't CachedGraph purely about caching the graph without taking into account higher level usage patterns that might involve policies obtained from other sources?

Agree, it is not the right place to have it here. I added a fixup commit with a solution how this could be handled closer to the rpc level, please have a look. I'm not sure if this is the best approach of returning errors here.

yyforyongyu · 2022-11-22T04:16:58Z

routing/unified_edges_test.go

+	unifierFilled.addPolicy(fromNode, &p1, c1)
+	unifierFilled.addPolicy(fromNode, &p2, c2)
+
+	tests := []struct {


yyforyongyu · 2022-11-22T04:20:03Z

go.mod

@@ -50,9 +50,10 @@ require (
 	go.etcd.io/etcd/client/pkg/v3 v3.5.0
 	go.etcd.io/etcd/client/v3 v3.5.0
 	golang.org/x/crypto v0.0.0-20210921155107-089bfa567519
-	golang.org/x/net v0.0.0-20211015210444-4f30a5c0130f
+	golang.org/x/exp v0.0.0-20221111094246-ab4555d3164f
+	golang.org/x/net v0.1.0


nit: updating go modules should be in a dedicated commit.

yyforyongyu · 2022-11-22T04:21:34Z

lntypes/comparison.go

+import "golang.org/x/exp/constraints"
+
+// Number defines a type constraint for numbers.
+type Number interface {


yyforyongyu · 2022-11-22T04:23:57Z

routing/unified_edges.go

+		// know the capacity, we fall back to MaxHTLC.
+		capMsat := lnwire.NewMSatFromSatoshis(edge.capacity)
+		if capMsat == 0 && edge.policy.MessageFlags.HasMaxHtlc() {
+			capMsat = edge.policy.MaxHTLC


Let's add a debug log here to show the fallback?

I think we would only like to do this if we are in assumeChannelValid=false (normal) mode, otherwise this could become very spammy - every channel would fall back to MaxHTLC. I could add a Trace statement here. Alternatively we have to pass in assumeChannelValid, which would perhaps be too much just for logging?

yeah I think this only happens in neutrino. Using a Trace sounds good.

yyforyongyu · 2022-11-22T04:46:27Z

routing/router.go

@@ -1809,7 +1808,7 @@ func (r *ChannelRouter) FindRoute(source, target route.Vertex,
 		}),
 	)

-	return route, nil
+	return route, probability, nil


I think instead of returning probability here, we can instead add a field to route.Route to keep the code clean and tight. Plus I think probability belongs to the struct Route anyway.

I am not sure about that. Routes can exist without a probability. They are a foundational data structure in lightning, where as probability is just a field that a specific pathfinding implementation adds to it. It also isn't translated to data on the wire.

Routes can exist without a probability.

hmmm what do you mean? Just to be clear I'm not suggesting that we need to save probability to disk, nor do we need to send it over the wire. It can be added as an ephemeral field on Route so we don't need to return numerous values while some callsites don't use them. If the definition of existence is whether it's defined in specs, I think we have many fundamental structs in channeldb that has non-existent fields.

On the other hand, I think Route always has a probability in lnd's context, even when users use !use_mc flag we still have a probability of 1.

Understand both points, would be grateful for more input.

yyforyongyu · 2022-11-25T06:13:03Z

routing/unified_edges.go

+		// know the capacity, we fall back to MaxHTLC.
+		capMsat := lnwire.NewMSatFromSatoshis(edge.capacity)
+		if capMsat == 0 && edge.policy.MessageFlags.HasMaxHtlc() {
+			capMsat = edge.policy.MaxHTLC


yeah I think this only happens in neutrino. Using a Trace sounds good.

lnrpc/routerrpc/router_backend.go

yyforyongyu · 2022-11-25T06:20:49Z

routing/probability_estimator.go

@@ -10,6 +10,43 @@ import (
 	"github.com/lightningnetwork/lnd/routing/route"
 )

+const (
+	// capacityCutoffFraction and capacitySmearingFraction define how


cmd/lncli/cmd_mission_control.go

yyforyongyu · 2022-11-25T06:34:48Z

lnrpc/routerrpc/router_backend.go

@@ -103,7 +103,7 @@ type MissionControl interface {
 	// GetProbability is expected to return the success probability of a
 	// payment from fromNode to toNode.
 	GetProbability(fromNode, toNode route.Vertex,
-		amt lnwire.MilliSatoshi) float64
+		amt lnwire.MilliSatoshi, capacity btcutil.Amount) float64


This is probably an edgy case, but what if the channel capacity is actually zero?

Good question, is it even possible to have a channel with zero capacity, or do you refer to an error in capacity validation? In that case the capacity factor would not be active (it would be 1), so no change compared to the current situation. Either the channel has a maxHTLCMsat set, then it must be less than or equal to the capacity per spec. In that scenario it would be caught by the amtInRange check. Otherwise the amtInRange check currently doesn't prohibit sending to a zero sat channel:

lnd/routing/unified_policies.go

Line 104 in e23c5dc

if u.capacity > 0 &&

In the worst-case scenario we would send to that route and would fail, avoiding the channel next time.

lightninglabs-deploy · 2022-12-02T07:05:20Z

@Roasbeef: review reminder
@bitromortac, remember to re-request review from reviewers when ready

yyforyongyu

LGTM🎉 Still think it's better to put probability inside Route, but it's a non-blocker. Again great work!

Roasbeef · 2022-12-10T00:42:03Z

I think this is ready to land after a rebase!

We encapsulate the capacity inside a unifiedPolicyEdge for later usage. The meaning of "policy" has changed now, which will be refactored in the next commmit.

This commit refactors the semantics of unified policies to unified edges. The main changes are the following renamings: * unifiedPolicies -> nodeEdgeUnifier * unifiedPolicy -> edgeUnifier * unifiedPolicyEdge -> unifiedEdge Comments and shortened variable names are changed to reflect the new semantics.

The test for unified edges is refactored into a table-driven test. It accomodates already a unifier per test for later expansion.

This commit adds experimental support for generic type constraints.

This commit adds the maximal capacity between two nodes to the unified edge data. We use MaxHTLC as a replacement if the channel capacity is not available. In tests we use larger maxHTLC values to be able to convert to a non-zero sat capacity.

Extends the pathfinder with a capacity argument for later usage. In tests, the inserted testCapacity has no effect, but will be used later to estimate reduced probabilities from it.

The returned probability can then be used in QueryRoutes to not having to reconstruct the probability.

FetchPairCapacity is used by the following endpoints to introduce the capacity for probability calculations: * QueryProbability * QueryRoutes

We multiply the apriori probability with a factor to take capacity into account: P *= 1 - 1 / [1 + exp(-(amount - cutoff)/smearing)] The factor is a function value between 1 (small amount) and 0 (high amount). The zero limit may not be reached exactly depending on the smearing and cutoff combination. The function is a logistic function mirrored about the y-axis. The cutoff determines the amount at which a significant reduction in probability takes place and the smearing parameter defines how smooth the transition from 1 to 0 is. Both, the cutoff and smearing parameters are defined in terms of fixed fractions of the capacity.

We deprecate `QueryProbability`, as it displays the same information as `QueryMissionControl` less the probability. `QueryRoutes` still contains the total probability of a route.

Changes the docstring of QueryProbability to reflect changes that were introduced in lightningnetwork#6857.

bitromortac added the path finding label Aug 24, 2022

joostjager reviewed Aug 24, 2022

View reviewed changes

lnrpc/routerrpc/router_backend.go Outdated Show resolved Hide resolved

bitromortac force-pushed the 2208-apriori-capacity branch from 5791f71 to 4d90dd8 Compare August 26, 2022 09:38

bitromortac mentioned this pull request Aug 26, 2022

pathfinding: probability for bimodal distribution #6815

Merged

bitromortac force-pushed the 2208-apriori-capacity branch from 4d90dd8 to 39ad143 Compare September 7, 2022 10:27

joostjager reviewed Sep 7, 2022

View reviewed changes

routing/probability_estimator.go Show resolved Hide resolved

bitromortac force-pushed the 2208-apriori-capacity branch from 39ad143 to 9a7b003 Compare September 12, 2022 13:26

bitromortac requested a review from joostjager September 12, 2022 13:27

joostjager reviewed Sep 12, 2022

View reviewed changes

bitromortac force-pushed the 2208-apriori-capacity branch 2 times, most recently from b9c3d90 to 0d33c19 Compare September 15, 2022 09:48

bitromortac commented Sep 15, 2022

View reviewed changes

routing/graph.go Outdated Show resolved Hide resolved

bitromortac requested a review from joostjager September 15, 2022 09:58

joostjager reviewed Sep 15, 2022

View reviewed changes

bitromortac force-pushed the 2208-apriori-capacity branch from 0d33c19 to 05e2552 Compare September 30, 2022 10:26

bitromortac requested a review from joostjager October 4, 2022 09:21

joostjager reviewed Oct 5, 2022

View reviewed changes

routing/unified_policies.go Outdated Show resolved Hide resolved

routing/probability.go Outdated Show resolved Hide resolved

routing/graph.go Outdated Show resolved Hide resolved

routing/probability_estimator_test.go Outdated Show resolved Hide resolved

joostjager reviewed Oct 5, 2022

View reviewed changes

rpcserver.go Outdated Show resolved Hide resolved

saubyk added the mission control label Oct 13, 2022

saubyk requested review from Roasbeef, yyforyongyu and positiveblue and removed request for Roasbeef October 13, 2022 16:36

bitromortac force-pushed the 2208-apriori-capacity branch from 05e2552 to 60460ab Compare October 17, 2022 14:57

positiveblue approved these changes Oct 23, 2022

View reviewed changes

joostjager reviewed Oct 24, 2022

View reviewed changes

bitromortac force-pushed the 2208-apriori-capacity branch from 60460ab to ca541fe Compare October 26, 2022 13:55

bitromortac requested a review from joostjager October 26, 2022 14:05

bitromortac force-pushed the 2208-apriori-capacity branch from 3a6c985 to 58d4deb Compare November 17, 2022 16:17

bitromortac requested review from yyforyongyu and Roasbeef November 17, 2022 17:04

yyforyongyu reviewed Nov 22, 2022

View reviewed changes

bitromortac force-pushed the 2208-apriori-capacity branch from 58d4deb to 734eca5 Compare November 24, 2022 15:56

yyforyongyu reviewed Nov 25, 2022

View reviewed changes

bitromortac force-pushed the 2208-apriori-capacity branch 2 times, most recently from aba7ea7 to 51c7bb6 Compare December 5, 2022 15:02

bitromortac requested a review from yyforyongyu December 5, 2022 15:03

yyforyongyu approved these changes Dec 7, 2022

View reviewed changes

bitromortac added 11 commits December 12, 2022 13:19

routing: return *unifiedPolicyEdge in getPolicy

7d29ab9

We encapsulate the capacity inside a unifiedPolicyEdge for later usage. The meaning of "policy" has changed now, which will be refactored in the next commmit.

routing: refactor unified edges test

ce6cade

The test for unified edges is refactored into a table-driven test. It accomodates already a unifier per test for later expansion.

mod: add golang.org/x/exp

e96d48e

This commit adds experimental support for generic type constraints.

lntypes+routing: add generic Min/Max functions

99273cc

routing: implement capacity in getEdge

2b6308a

This commit adds the maximal capacity between two nodes to the unified edge data. We use MaxHTLC as a replacement if the channel capacity is not available. In tests we use larger maxHTLC values to be able to convert to a non-zero sat capacity.

routing: use capacity in pathfinding

516e3a8

Extends the pathfinder with a capacity argument for later usage. In tests, the inserted testCapacity has no effect, but will be used later to estimate reduced probabilities from it.

router: return probability from findPath

66ffc64

The returned probability can then be used in QueryRoutes to not having to reconstruct the probability.

routing+routerrpc: add capacity in rpcs

454c115

FetchPairCapacity is used by the following endpoints to introduce the capacity for probability calculations: * QueryProbability * QueryRoutes

routerrpc: mark QueryProbability deprecated

2d7fda2

We deprecate `QueryProbability`, as it displays the same information as `QueryMissionControl` less the probability. `QueryRoutes` still contains the total probability of a route.

bitromortac force-pushed the 2208-apriori-capacity branch from 51c7bb6 to 2d7fda2 Compare December 12, 2022 13:02

Roasbeef merged commit d468391 into lightningnetwork:master Dec 13, 2022

bitromortac added a commit to bitromortac/lnd that referenced this pull request Jan 11, 2023

routerrpc: update QueryProbability documentation

045e5cf

Changes the docstring of QueryProbability to reflect changes that were introduced in lightningnetwork#6857.

bitromortac mentioned this pull request Jan 11, 2023

routerrpc: update QueryProbability documentation #7310

Merged

joostjager mentioned this pull request Jan 26, 2023

routing: inbound fees send support #6934

Merged

bitromortac deleted the 2208-apriori-capacity branch April 17, 2024 12:45

pathfinding: capacity-dependent apriori model probability #6857

pathfinding: capacity-dependent apriori model probability #6857

Uh oh!

Conversation

bitromortac commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Description

Performance considerations:

Questions:

Uh oh!

Uh oh!

bitromortac commented Sep 7, 2022

Uh oh!

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bitromortac commented Sep 15, 2022

Uh oh!

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

positiveblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bitromortac commented Aug 24, 2022 •

edited

Loading

joostjager Nov 22, 2022 •

edited

Loading

bitromortac Dec 5, 2022 •

edited

Loading