Update pathfind.go to allow sending to self

[ZapUser77]

This commit allows payments to be sent to one's self, in order to balance channels. It has added loop prevention mechanisms as well.
It also ensures that payments to one's self only comes in channels with sufficient balance.

[joostjager]

An alternative implementation could be to temporarily add a second "virtual" self' node to the graph and use the normal routing algorithm to route from self to self'. I think it will make it easier to verify the correctness of the algorithm.

In any case, there are definitely some unit tests needed to cover the new functionality.

[ZapUser77]

First time using GitHub. I'm assuming the reviewers run the unit tests, correct?

[joostjager]

As a developer you should always run unit tests yourself for code that you change. pathfind_test.go in this case.

But what I meant was not just to run the tests, but also extend them to cover the new route to self scenario.

Also something to think about is how to indicate the desired channels to and from self. Without that, is this functionality useful for rebalancing?

[ZapUser77]

I have ran tests myself, sending to myself. Works great. It had some bugs, but I fixed those. I'll have to poke around and see what you're expecting as the format of results you're expecting in pathfind_test.go.

"Also something to think about is how to indicate the desired channels to and from self. Without that, is this functionality useful for rebalancing?"

That's exactly what I'm working on now. Trying to figure out how to pass arguments from the CLI to the actual functions. Once the arguments get to find-path, its easy.

[ZapUser77]

Figured out how to run the tests, after submitting. The route to self works in practice, but doesn't work in the test case.
I think has something to do with the test sending nil as bandwidth hints.

[joostjager]

Also note that #1321 will change the routing algorithm to backwards and has the findPath test refactored. Depending on merge order, this may require changes.

[joostjager]

I still think it is difficult to verify the correctness of your modifications. You add:

• Two code blocks for loop detection (normally not necessary for Dijkstra)
• Rely on integer overflow behaviour
• Using distance 0, infinity and infinity-1 for the source node to signal states
• Flipping edge bandwidth

I expect that adding a second virtual self node (earlier comment) results in higher quality code, because the core of the algorithm remains unchanged and also is in line with descriptions of the Dijkstra algorithm. You can probably add the (reverse) edges back to self to the additionalEdges map that is already available.

[ZapUser77]

Wouldn't the 'virtual self' have to have all the same channel ID's as the 'self' for proper route creation?
Then you run into "How do you avoid trying to route through the virtual node when not sending to self". Would you just create/delete the virtual node on demand? How would that be affected by a node with massive number of channels? Or frequent balancing (my end goal is self-balancing channels that are triggered when funds go in/out of a channel causing it to be unbalanced)?

One of the blocks for 'loop detection' doesn't just check for loops, it's useful by always ignoring edges to nodes you just came from. The other I had to add, because, somehow, a loop was created between two nodes directly connected to me. [All three nodes are connected by zero fee channels, if that makes a difference.]

I'll do some research on adding the 'virtual self' to the channel graph. I could probably manipulate the data flow to ignore all paths -> vSelf if destination != vSelf. Then, whenever a channel is changed, it'd have to affect the vSelf is well, if is left on the map.

[joostjager]

Yes, it will have the same channel ids, but that is also what you want isn't it? Use any of that set to leave self and return via another one from the set.

If you are not sending to self, you just don't add the virtual self. Nothing different happens at all compared to the current algorithm. So indeed, only when routing to self, you create the virtual self.

Massive number of channels is no problem, it is just an additional edge for every channel, O(n).

I don't see why frequent balancing would be different with either of the algorithms.

Dijkstra doesn't work with cycles. Three nodes with zero fee channels form a cycle. But normally the time lock delta adds some weight to the edge too, so it shouldn't happen. I think that case needs further investigation on what happened exactly. Checking for loops shouldn't be necessary.

I would not try to add virtual self to the global channel graph, that sounds like a scary change. I'd just do it locally inside findPath using additionalEdges. It should be quite simple and clean.

[ZapUser77]

I'm stuck:
How do I reference the channel policy of the node on the other end of the channel?
Can't assign the virtual edges the policy from self, it needs to be a copy of the policy to self. Wouldn't I have to pull that from the graph somehow? Something like "outEdge.ChannelID.outEdge.node.feeBaseMSat"?

                    edgeInfo *channeldb.ChannelEdgeInfo,
                    outEdge, _ *channeldb.ChannelEdgePolicy) error {

                    // If the remote balance is > amount add to vSelf edge to additional
                    // edges, else ignore.  The edge policy must be from the point of view
                    // of the non-self nodes.
                    edgeBandwidth, ok := bandwidthHints[edgeInfo.ChannelID]

                    if lnwire.NewMSatFromSatoshis(edgeInfo.Capacity) - edgeBandwidth > amt{

                            tovSelfEdge := &channeldb.ChannelEdgePolicy{
                                    Node:                                        vSelf,
                                    ChannelID:                                 outEdge.ChannelID,
      ***                          FeeBaseMSat:                            outEdge.node.FeeBaseMSat,
      ***                          FeeProportionalMillionths:       .FeeProportionalMillionths,
       ***                         TimeLockDelta:                         .TimeLockDelta,
                            }

[joostjager]

If you run LightningNode.ForEachChannel on your self node, it will return both the outgoing edge policies as well as the incoming edge policies of the remote node. Those incoming ones can be copied with self replaced by vself. Only when the search is done, the modified incoming edge policies need to be reverted to the original. Maybe by filling a lookup table when the vself edges are created.

[ZapUser77]

"If you run LightningNode.ForEachChannel on your self node," that's exactly what I'm doing. I do not understand the syntax to reference the channel policy data of the remote node. I just need the syntax. The policies of self never need to change, or be copied. vSelf doesn't need outgoing channel policies as AdditionalEdges only needs the channel policy leading to vSelf.
Would "outEdge.ChannelID.outEdge.node.feeBaseMSat" be the FeeBaseMSat for the distant node policy for the channel back to self?

                    err := sourceNode.ForEachChannel(tx, func(tx *bolt.Tx,
                    edgeInfo *channeldb.ChannelEdgeInfo,
                    outEdge, _ *channeldb.ChannelEdgePolicy) error {

                    // If the remote balance is > amount add to vSelf edge to additional
                    // edges, else ignore.  The edge policy must be from the point of view
                    // of the non-self nodes.
                    edgeBandwidth, ok := bandwidthHints[edgeInfo.ChannelID]

                    if lnwire.NewMSatFromSatoshis(edgeInfo.Capacity) - edgeBandwidth > amt{

                            tovSelfEdge := &channeldb.ChannelEdgePolicy{
                                    Node:                      vSelf,
                                    ChannelID:                 outEdge.ChannelID,
                                    FeeBaseMSat:               outEdge.ChannelID.outEdge.node.FeeBaseMSat,
                                    FeeProportionalMillionths: outEdge.ChannelID.outEdge.node.FeeProportionalMillionths,
                                    TimeLockDelta:             outEdge.ChannelID.outEdge.node.TimeLockDelta,
                            }

                    additionalEdges := map[Vertex][]*channeldb.ChannelEdgePolicy{
                            NewVertex(outEdge.node.pubKeyBtes): {tempEdge},
                    }
            }

[joostjager]

You have to replace that _ in the call to ForEachChannel to inEdge

[ZapUser77]

I'm almost done, just need to make it more efficient. Considering I undid everything I posted in this PR, and started over, how do I post with a single commit, just the changes in the new file. [Using GitHub only, I strongly dislike command line.]
Or do I just delete this PR, and make an entirely new one?

[joostjager]

Yes, I can imagine, the approach with virtual self is quite different. Hope you agree that it is better and/or more readable and that I didn't overlook something.

You can remove your old branch, rename your new branch to the old one and force push that to github. But I don't know how to do it without cmd line. It is probably useful to invest a little into command line knowledge anyway, as the project likes to have a clean commit history which requires rebasing.

[ZapUser77]

There: I've rewritten the 'route to self' functionality, as well as unit tests.
NOTE: It will only find paths of exactly 3 hops, due to a how the pathfinding algo works.
To find longer paths would require more complex logic: Essentially, at each hop along the path, it'd have to temporarily set the distance of all nodes not in the existing path to infinity, then set them back to what they were afterwards.
3-hop paths should cover most situations.
When 1321 goes into affect, the "vSelf" additional edge algo will have to be changed. Easy fix.

P.S. Sorry about all the random lines edited. My compiler auto-corrects blank space formatting inconsistencies.

[joostjager]

That loop checking should really not be necessary with vself. I think what is missing, and what you were planning to add anyway, is to constraint the outgoing edges from self and the incoming edges of vself. If you'd do that (explicitly specify from where to where to rebalance), the problem will solve itself and you also don't have the limitation of 3 hops.

[ZapUser77]

There we go. Squashed down into one.

Even if you constrain the edges, there is still a limit on the hops can that can be routed through.
Lets say A-B and A-C and B-C... the first iteration of processing edges, processes all the ones directly connected to A, setting the distance of B and C. Once you get to B or C, you can't search go to the other one, because A-C is always less than A-B-C. Adding a constraint only adds 1 hop per constrained side to the max hops.

There's no way to remove that limitation, without writing a completely new pathfinding algo that finds "all paths", then sorts by shortest.

The A-B-A check isn't necessary if you have constraints, however I want constraints to be optional, and still work.

P.S. Sorry for the open/closing... That wasn't me... that was github doing it without my permission as I reset my master to the same as the origin.

[joostjager]

So you have self node A and nodes B and C, all fully connected. After transformation you'll get a virtual self A'.

Edges: A -> B, A -> C, B -> C, C -> B, B -> A', C -> A'

You are looking for a route from A to A'. If you go A -> B -> C -> A' and find that A -> C -> A' is actually better, why wouldn't you want to use it then? If it rebalances, why not? SHorter, cheaper, better.

[ZapUser77]

A-C-A... doesn't rebalance anything. You are receiving in the same pipe you sent it out of. Unless you had multiple channels between A<->C. Then instead of blocking the going back down the node, you'd just block backtracking down the same channel ID.

It's not just looking for a route from A to A'. You must ignore all routes that would use the same channel twice.

[joostjager]

Yes, via the same channel it is not a rebalance of course. Constraints would solve it, but if you don't want to add them for now, you could do it like this inside the bestNode.ForEachChannel callback:

if outEdge.Node == vSelf && firstChannelOfBestPathToNode(bestNode) == outEdge.Channel { return nil }

So don't process the edge that you used to the first hop.

I am almost sure it will work and will get you a nice flexible any-length path solution.

[ZapUser77]

One way to remote the hop-count limit: Do pathfinding from both ends and meet in the middle.
Basically: Whenever your 'process edge' encounters a node with a shorter distance to the source, it checks to see if it's already in it's existing path. If not, then it looks at the reverse path from that new node, and adds it's reverse path weight to the current path to create a path to A'.

"if outEdge.Node == vSelf && channelInBestPathToNode(bestNode, outEdge) { return nil }"
Interesting, I'll research this and see if it yields the desired results.

Pretty sure it doesn't release the hop-count limit.
The algo sets the distance to each node directly connected.
A-100-B
A-100-C
B-200-C

When it looks from B, it sees that A-B-C distance is > current known best distance, and ignores the B-C route.

[ZapUser77]

"undefined: channelInBestPathToNode" Searched and GitHub says this doesn't exist in LND.

[joostjager]

You are right indeed. My suggestion of skipping edges messes up the algorithm.

But constraining to a specific in- and outgoing channel, I think it is going to work isn't it? You probably want it in the end anyway, because what is the use of an uncontrolled "rebalance"? It could just as well be an "unbalance".

[joostjager]

Yes, that was a method name that I made up. Pseudo-code to explain the idea.

[joostjager]

If you want the uncontrolled variant, you could also loop over all channels and perform a search to vself for every one of them. Suppose self has channels A, B, C and D. If you perform the search for A, then only add A as an outgoing edge from self and only add B, C and D as incoming edges to vself. Then repeat for all channels.

It gives you max. 4 routes of which the shortest can be selected.

But the question remains, what is the use case of uncontrolled movement of funds?

[ZapUser77]

I want both constraints to be optional: You can specify one, or the other, neither, or both. Maximum flexibility.

My idea of "searching from both ends" fixes the issue, with maximum flexibility. But it adds a great deal of complexity that will take time to sort out.

Lol... Or I whenever a node encounters another node that has a shorter path, it calls up a recursive "find path" with the source as the current node, having all existing path inside ignored edges/nodes for the new call. No matter how far away, or recursive it gets... It won't ever traverse back down the way it came!
Completely eliminating the 3-hop max.

There's no logical reason for "Neither" constraint. There are multiple uses for a single constraint: You just want the cheapest way to balance a single channel. And you need to have the option of balancing in either direction.

[joostjager]

Okay, if neither is not logical, the problem is fixed isn't it? You specify a set of outgoing channels. That set will be the only edges going out of self and the inverse of the set will be the only incoming edges of vself.

If you want to balance a single channel, then depending in balance direction, you'd either have that channel as the one and only edge from self or to vself. All the other channels edges will be added at the other end of the search graph.

[ZapUser77]

There are 4 variants: Neither, Both, Incoming only, Outgoing Only.
"Neither" doesn't have anything logical for, or against. The other three are needed.

Even with "Both" You still run into a hop limitation due to Dijksktra's. It would have to be a recursive algo that appends the route to the outer loop.

I need to think about that some more... make sure the it doesn't get stuck in infinite recursion.
Got it: It only calls the recursion if Target = Vself, and when the recursion is called, target is always Self (not vSelf), thus preventing infinite recursion.

[joostjager]

Both is possible, but not if the in and out sets overlap? Is there a good use case for overlapping in and out sets?

[ZapUser77]

I don't know what you mean by "in and out sets".

[joostjager]

In set is the set of channels that the payment may originate from. Out set is the set of channels through any of which the payment may be received

[ZapUser77]

Yes, when sending yourself the set must overlap at some point. You have to be able to create a loop, and exit the loop without becoming infinite.
Do you have a method for direct communication, twitter, wire or something similar, so we don't spam this commit with posts?

[joostjager]

Yes, good idea. lnd slack (Joost Jager)