pathfinding #192

Vib-UX · 2021-08-21T18:24:33Z

fixes #190

codecov-commenter · 2021-08-21T19:43:28Z

Codecov Report

Merging #192 (698cb5a) into main (eacb603) will decrease coverage by 0.90%.
The diff coverage is 45.05%.

❗ Current head 698cb5a differs from pull request most recent head 96146da. Consider uploading reports for the commit 96146da to get more accurate results

@@            Coverage Diff             @@
##             main     #192      +/-   ##
==========================================
- Coverage   93.37%   92.47%   -0.91%     
==========================================
  Files         182      183       +1     
  Lines        4757     4848      +91     
  Branches      561      581      +20     
==========================================
+ Hits         4442     4483      +41     
- Misses        174      214      +40     
- Partials      141      151      +10

Impacted Files	Coverage Δ
packages/graph/lib/PriorityQueue.ts	`34.00% <34.00%> (ø)`
packages/graph/lib/graph.ts	`73.01% <58.53%> (-26.99%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eacb603...96146da. Read the comment docs.

bmancini55

K its coming along nicely. Some nits surrounding coding style that we can go over. The biggest issues I see are:

The construction of the adjacency list is only from node1 -> node2. The graph is a symmetric directed graph meaning each channel is two edges: node1->node2 and node2->node1.
This would be facilitated by using the Node's channels property, which is a Map. If it helps you can create a secondary property on Node that returns a tuple containing the [Channel,ChannelSettings] for the Node's channel setting which would aid in point 1.
Finding the min distance should ideally use a heap, but we can get to optimizations later.

bmancini55 · 2021-09-08T19:42:19Z

packages/graph/lib/graph.ts

@@ -7,6 +8,8 @@ import { Node } from "./node";
 * Graph represents a
 */
 export class Graph {
+    public nodes_list: string[] = [];


Probably don't need nodes_list. Can easily iterate over a Map type:

const map = new Map(); map.set(1, 'a'); map.set(2, 'b'); map.set(3, 'c'); // iterate over key-values since Map is an iterator for(const [key, val] of map) { console.log(key, val); } // iterate over keys as an iterator for(const key of map.keys()) { console.log(key); } // iterate over vals as an iterator for(const val of map.values()) { console.log(val); }

Lastly you can covert any iterator into an Array with Array.from:

console.log(Array.from(map)); console.log(Array.from(map.keys())); console.log(Array.from(map.values()));

Sure thing will work with maps (;

bmancini55 · 2021-09-08T19:44:21Z

packages/graph/lib/graph.ts

@@ -7,6 +8,8 @@ import { Node } from "./node";
 * Graph represents a
 */
 export class Graph {
+    public nodes_list: string[] = [];
+    public adjacencyList = {};


Same for adjacency list. The Node type includes a Channels Map that contains all of the Channels the Node is a member of.

bmancini55 · 2021-09-08T19:46:01Z

packages/graph/lib/graph.ts

+        const str_id = dest.nodeId.toString("hex");
+
+        // Distances will store the base_fee required to traverse from the src
+        let distances = {},


Language nit: use maps over objects. Even though JS/TS allows object mutation, memory management is most effective when types remain static. As such the Set/Map types are better suited for that type of stuff.

Vib-UX · 2021-09-09T10:26:32Z

K its coming along nicely. Some nits surrounding coding style that we can go over. The biggest issues I see are:

The construction of the adjacency list is only from node1 -> node2. The graph is a symmetric directed graph meaning each channel is two edges: node1->node2 and node2->node1.

This would be facilitated by using the Node's channels property, which is a Map. If it helps you can create a secondary property on Node that returns a tuple containing the [Channel,ChannelSettings] for the Node's channel setting which would aid in point 1.

Finding the min distance should ideally use a heap, but we can get to optimizations later.

Yes, I made a mistake it should behave like an undirected graph. Nice Catch 😓

bmancini55 · 2021-09-09T14:05:00Z

Yes, I made a mistake it should behave like an undirected graph. Nice Catch sweat

Conceptually yes, channels are similar to undirected/symmetric directed graph. So both directions or every node pair where a channel exists are theoretically possible and need to be considered. For routing purposes that symmetry may break when:

A channel only has settings on one side (meaning it only received a channel_update from one of the nodes)
A node may disable it's side of the channel in the update message
The min/max settings may prevent an HTLC from being routeable.

bmancini55 · 2021-09-09T14:09:09Z

Also just for edification, node1/node2 in the channel are just the lexicographical ordering of the identifiers. When I first starting learning this stuff I thought node1 was the opener or had some larger meaning. But it's just the node with the lower ordered nodeId.

Vib-UX · 2021-09-09T16:54:46Z

Also just for edification, node1/node2 in the channel are just the lexicographical ordering of the identifiers. When I first starting learning this stuff I thought node1 was the opener or had some larger meaning. But it's just the node with the lower ordered nodeId.

That's really insightful 💡 , I actually thought that node1 is probably the one who opened the channel.

Vib-UX · 2021-09-10T05:02:28Z

K its coming along nicely. Some nits surrounding coding style that we can go over. The biggest issues I see are:

The construction of the adjacency list is only from node1 -> node2. The graph is a symmetric directed graph meaning each channel is two edges: node1->node2 and node2->node1.

This would be facilitated by using the Node's channels property, which is a Map. If it helps you can create a secondary property on Node that returns a tuple containing the [Channel,ChannelSettings] for the Node's channel setting which would aid in point 1.

Finding the min distance should ideally use a heap, but we can get to optimizations later.

For Heap optimization, I found this package related to priority queues we can import that and workaround with it or should I build it from scratch?

https://www.npmjs.com/package/priorityqueuejs

bmancini55 · 2021-09-10T14:30:06Z

For Heap optimization, I found this package related to priority queues we can import that and workaround with it or should I build it from scratch?
* https://www.npmjs.com/package/priorityqueuejs

Scratch, we try to avoid external packages where possible.

We'll most likely put it in the Core package but feel free to add it to this project first so we don't have to deal with deps issues while prototyping.

Vib-UX · 2021-09-10T14:33:35Z

For Heap optimization, I found this package related to priority queues we can import that and workaround with it or should I build it from scratch?
* https://www.npmjs.com/package/priorityqueuejs
Scratch, we try to avoid external packages where possible.

We'll most likely put it in the Core package but feel free to add it to this project first so we don't have to deal with deps issues while prototyping.

Sure 🤝

Randy808 · 2021-09-14T19:24:39Z

packages/graph/lib/graph.ts

+        // from dest --> src and return route from src --> dest
+
+        // Convert it to corresponding node_id
+        const str_id = dest.nodeId.toString("hex");


Consider calling this variable dest_str_id or dest_id

bmancini55

min-heap looks good with a couple minors. If you want to split that into a separate PR and add tests we can merge it in.

I still need to review dijkstras using the heap.

bmancini55 · 2021-09-15T19:31:56Z

packages/graph/lib/PriorityQueue.ts

+        this._queue.push(value);
+        let pos = this._queue.length - 1;
+
+        while (pos !== 0 && this._compare(this._queue[this._parentOf(pos)], this._queue[pos])) {


micro optimization but cache the call to _parentOf since we call it 3 times. will require refactor to a do/while loop.

bmancini55 · 2021-09-15T19:44:53Z

packages/graph/lib/PriorityQueue.ts

+    }
+
+    private _compare(item1: T, item2: T) {
+        if (this._comparator) {


I think we can make the _comparator compulsory in the constructor, then we dont' need this check and can drop this whole function.

bmancini55 · 2021-09-15T19:53:36Z

packages/graph/lib/PriorityQueue.ts

+        this._queue = [];
+    }
+
+    public contains(value: T, comparator?: (item: T) => boolean) {


Do we need this operation? If not then lets drop it since its extra code

Vib-UX · 2021-09-18T01:41:01Z

min-heap looks good with a couple minors. If you want to split that into a separate PR and add tests we can merge it in.

I still need to review dijkstras using the heap.

separate PR would make it look neat

Work in Progress

288d11f

Vib-UX changed the title ~~Work in Progress~~ pathfinding Aug 21, 2021

Channel--->edge type/error

fa1d77d

Vib-UX added 4 commits August 22, 2021 20:39

Sample Testing Pathfinding pass

51f9117

npm-run-format

c043568

Channel Capacity

613a78d

Rev Dijkstra

edeb8f0

bmancini55 reviewed Sep 8, 2021

View reviewed changes

Vib-UX added 2 commits September 10, 2021 06:49

Objects ---> Maps

b092620

Adjacency List ---> Maps

71b275a

Vib-UX added 3 commits September 11, 2021 05:37

Heap Optimization

112b5c3

Pq Scratch

5623cb2

Strict Maps

0daa0bc

Vib-UX requested a review from bmancini55 September 11, 2021 13:48

Randy808 reviewed Sep 14, 2021

View reviewed changes

bmancini55 reviewed Sep 15, 2021

View reviewed changes

refactor

96146da

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pathfinding #192

pathfinding #192

Vib-UX commented Aug 21, 2021

codecov-commenter commented Aug 21, 2021 •

edited

Loading

bmancini55 left a comment •

edited

Loading

bmancini55 Sep 8, 2021 •

edited

Loading

Vib-UX Sep 9, 2021

bmancini55 Sep 8, 2021

bmancini55 Sep 8, 2021 •

edited

Loading

Vib-UX commented Sep 9, 2021

bmancini55 commented Sep 9, 2021

bmancini55 commented Sep 9, 2021

Vib-UX commented Sep 9, 2021

Vib-UX commented Sep 10, 2021

bmancini55 commented Sep 10, 2021

Vib-UX commented Sep 10, 2021

Randy808 Sep 14, 2021

bmancini55 left a comment

bmancini55 Sep 15, 2021

bmancini55 Sep 15, 2021 •

edited

Loading

bmancini55 Sep 15, 2021

Vib-UX commented Sep 18, 2021

pathfinding #192

Are you sure you want to change the base?

pathfinding #192

Conversation

Vib-UX commented Aug 21, 2021

codecov-commenter commented Aug 21, 2021 • edited Loading

Codecov Report

bmancini55 left a comment • edited Loading

Choose a reason for hiding this comment

bmancini55 Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Vib-UX Sep 9, 2021

Choose a reason for hiding this comment

bmancini55 Sep 8, 2021

Choose a reason for hiding this comment

bmancini55 Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Vib-UX commented Sep 9, 2021

bmancini55 commented Sep 9, 2021

bmancini55 commented Sep 9, 2021

Vib-UX commented Sep 9, 2021

Vib-UX commented Sep 10, 2021

bmancini55 commented Sep 10, 2021

Vib-UX commented Sep 10, 2021

Randy808 Sep 14, 2021

Choose a reason for hiding this comment

bmancini55 left a comment

Choose a reason for hiding this comment

bmancini55 Sep 15, 2021

Choose a reason for hiding this comment

bmancini55 Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

bmancini55 Sep 15, 2021

Choose a reason for hiding this comment

Vib-UX commented Sep 18, 2021

codecov-commenter commented Aug 21, 2021 •

edited

Loading

bmancini55 left a comment •

edited

Loading

bmancini55 Sep 8, 2021 •

edited

Loading

bmancini55 Sep 8, 2021 •

edited

Loading

bmancini55 Sep 15, 2021 •

edited

Loading