Support onion message pathfinding by valentinewallace · Pull Request #1669 · lightningdevkit/rust-lightning

valentinewallace · 2022-08-15T18:34:24Z

We could reuse router::get_route, but it's better to start from scratch
because get_route includes a lot of payment-specific computations that impact
performance.

Partially addresses #1607

Used in upcoming commit(s) to test onion message pathfinding

We *could* reuse router::get_route, but it's better to start from scratch because get_route includes a lot of payment-specific computations that impact performance.

TheBlueMatt

The Dijkstras implementation could be cleaned up a lot...

TheBlueMatt · 2022-08-16T19:45:08Z

+}
+
+// Heavily adapted from https://github.com/samueltardieu/pathfinding/blob/master/src/directed/dijkstra.rs
+// TODO: how2credit the repo (is that necessary?)?


Hmmmmm, good question. In general, the MIT and Apache licenses both require attribution, including of downstream projects. However, the Apache license only requires it if there is a file called "NOTICE" or any "copyright, patent, trademark, and attribution notices", which then must be provided downstream, and the MIT license only requires that "the above copyright notice be included", but the original repo doesn't actually include the MIT license anywhere, nor does it include any relevant notices as far as I can see, so there is no relevant "above copyright notice" to include, aside from the first paragraph of the MIT license, which we of course include as LICENSE-MIT. Thus, I think we can reasonably argue that a simple comment above this code indicating that it is adapted from (link) which is code Copyright Samuel Tardieu should suffice, which complies with the Apache "You must cause any modified files to carry prominent noticesstating that You changed the files" requirement.

#1724 ended up not adapting this crate's implementation anymore (shoutout to Wikpedia)

valentinewallace · 2022-08-17T17:49:01Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions

TheBlueMatt · 2022-08-17T20:52:33Z

I believe the upstream dijkstras you pulled from is actually buggy, anyway, so the generic-ness isn't worth much :p

tnull · 2022-08-18T05:20:19Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions

Ah, didn't realize keeping it generic had that intention. Well, it's not unreadable and reusing test vectors may be worth it.
If we however also see a way of refactoring the payment pathfinding to reuse the same generic implementation, we'd should definitely consider it, IMO.

valentinewallace · 2022-08-18T16:34:17Z

I believe the upstream dijkstras you pulled from is actually buggy, anyway, so the generic-ness isn't worth much :p

Lol I might've introduced bugs in my quest to eliminate all vecs from the original, to be fair

valentinewallace · 2022-08-18T16:45:29Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions
Ah, didn't realize keeping it generic had that intention. Well, it's not unreadable and reusing test vectors may be worth it. If we however also see a way of refactoring the payment pathfinding to reuse the same generic implementation, we'd should definitely consider it, IMO.

I did wonder if there were other cases where we'd want a generic implementation. I have no idea whether that'd be realistic with our payment pathfinding though lol, we'd need a way to "decrement available liquidity" for hops in paths that were already found, at least, but maybe that's easy? 🤔

TheBlueMatt · 2022-08-18T18:50:47Z

Oops, sorry lol didn't mean to close.

I did wonder if there were other cases where we'd want a generic implementation. I have no idea whether that'd be realistic with our payment pathfinding though lol

I kinda doubt it - we've ended up with a few algorithmic tweaks in addition to basic dijkstra's, so I'd be surprised if we could munge it back.

ariard · 2022-08-19T00:42:49Z

+/// Find a path for sending an onion message.
+pub fn find_path<L: Deref, GL: Deref>(
+	our_node_pubkey: &PublicKey, receiver_pubkey: &PublicKey, network_graph: &NetworkGraph<GL>, first_hops: Option<&[&PublicKey]>, logger: L
+) -> Result<Vec<PublicKey>, LightningError> where L::Target: Logger, GL::Target: Logger


So we wouldn't reuse all the information we're learning and storing in our ProbabilisticScorer. I can see how we're limited with the current penalty being based on the link-level and here we might be interested by node-level reliability in the path construction. That said, we can also assume that a reliable channel == a reliable onion message communication channel and go with it. I don't know if the spec says anything here or what are the thinking of other implementations ?

There is an option in the spec to return a separate error for "peer offline" from "no capacity", but I'm not sure how common that is. Once we land the historical scoring PR, we could use the "time in the zero-available-capacity bucket" as a score here.

valentinewallace · 2022-09-02T17:11:32Z

Thanks for all the feedback! This needs a lot of work and I'm switching it up to work on custom OMs first -- closing for now.

valentinewallace · 2022-08-15T18:34:24Z

We could reuse router::get_route, but it's better to start from scratch
because get_route includes a lot of payment-specific computations that impact
performance.

Partially addresses #1607

Used in upcoming commit(s) to test onion message pathfinding

We *could* reuse router::get_route, but it's better to start from scratch because get_route includes a lot of payment-specific computations that impact performance.

TheBlueMatt

The Dijkstras implementation could be cleaned up a lot...

TheBlueMatt · 2022-08-16T19:45:08Z

+}
+
+// Heavily adapted from https://github.com/samueltardieu/pathfinding/blob/master/src/directed/dijkstra.rs
+// TODO: how2credit the repo (is that necessary?)?


Hmmmmm, good question. In general, the MIT and Apache licenses both require attribution, including of downstream projects. However, the Apache license only requires it if there is a file called "NOTICE" or any "copyright, patent, trademark, and attribution notices", which then must be provided downstream, and the MIT license only requires that "the above copyright notice be included", but the original repo doesn't actually include the MIT license anywhere, nor does it include any relevant notices as far as I can see, so there is no relevant "above copyright notice" to include, aside from the first paragraph of the MIT license, which we of course include as LICENSE-MIT. Thus, I think we can reasonably argue that a simple comment above this code indicating that it is adapted from (link) which is code Copyright Samuel Tardieu should suffice, which complies with the Apache "You must cause any modified files to carry prominent noticesstating that You changed the files" requirement.

#1724 ended up not adapting this crate's implementation anymore (shoutout to Wikpedia)

valentinewallace · 2022-08-17T17:49:01Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions

TheBlueMatt · 2022-08-17T20:52:33Z

I believe the upstream dijkstras you pulled from is actually buggy, anyway, so the generic-ness isn't worth much :p

tnull · 2022-08-18T05:20:19Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions

Ah, didn't realize keeping it generic had that intention. Well, it's not unreadable and reusing test vectors may be worth it.
If we however also see a way of refactoring the payment pathfinding to reuse the same generic implementation, we'd should definitely consider it, IMO.

valentinewallace · 2022-08-18T16:34:17Z

I believe the upstream dijkstras you pulled from is actually buggy, anyway, so the generic-ness isn't worth much :p

Lol I might've introduced bugs in my quest to eliminate all vecs from the original, to be fair

valentinewallace · 2022-08-18T16:45:29Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions
Ah, didn't realize keeping it generic had that intention. Well, it's not unreadable and reusing test vectors may be worth it. If we however also see a way of refactoring the payment pathfinding to reuse the same generic implementation, we'd should definitely consider it, IMO.

I did wonder if there were other cases where we'd want a generic implementation. I have no idea whether that'd be realistic with our payment pathfinding though lol, we'd need a way to "decrement available liquidity" for hops in paths that were already found, at least, but maybe that's easy? 🤔

TheBlueMatt · 2022-08-18T18:50:47Z

Oops, sorry lol didn't mean to close.

I did wonder if there were other cases where we'd want a generic implementation. I have no idea whether that'd be realistic with our payment pathfinding though lol

I kinda doubt it - we've ended up with a few algorithmic tweaks in addition to basic dijkstra's, so I'd be surprised if we could munge it back.

ariard · 2022-08-19T00:42:49Z

+/// Find a path for sending an onion message.
+pub fn find_path<L: Deref, GL: Deref>(
+	our_node_pubkey: &PublicKey, receiver_pubkey: &PublicKey, network_graph: &NetworkGraph<GL>, first_hops: Option<&[&PublicKey]>, logger: L
+) -> Result<Vec<PublicKey>, LightningError> where L::Target: Logger, GL::Target: Logger


So we wouldn't reuse all the information we're learning and storing in our ProbabilisticScorer. I can see how we're limited with the current penalty being based on the link-level and here we might be interested by node-level reliability in the path construction. That said, we can also assume that a reliable channel == a reliable onion message communication channel and go with it. I don't know if the spec says anything here or what are the thinking of other implementations ?

There is an option in the spec to return a separate error for "peer offline" from "no capacity", but I'm not sure how common that is. Once we land the historical scoring PR, we could use the "time in the zero-available-capacity bucket" as a score here.

valentinewallace · 2022-09-02T17:11:32Z

Thanks for all the feedback! This needs a lot of work and I'm switching it up to work on custom OMs first -- closing for now.

valentinewallace · 2022-08-15T18:34:24Z

We could reuse router::get_route, but it's better to start from scratch
because get_route includes a lot of payment-specific computations that impact
performance.

Partially addresses #1607

Used in upcoming commit(s) to test onion message pathfinding

We *could* reuse router::get_route, but it's better to start from scratch because get_route includes a lot of payment-specific computations that impact performance.

TheBlueMatt

The Dijkstras implementation could be cleaned up a lot...

TheBlueMatt · 2022-08-16T19:45:08Z

+}
+
+// Heavily adapted from https://github.com/samueltardieu/pathfinding/blob/master/src/directed/dijkstra.rs
+// TODO: how2credit the repo (is that necessary?)?


Hmmmmm, good question. In general, the MIT and Apache licenses both require attribution, including of downstream projects. However, the Apache license only requires it if there is a file called "NOTICE" or any "copyright, patent, trademark, and attribution notices", which then must be provided downstream, and the MIT license only requires that "the above copyright notice be included", but the original repo doesn't actually include the MIT license anywhere, nor does it include any relevant notices as far as I can see, so there is no relevant "above copyright notice" to include, aside from the first paragraph of the MIT license, which we of course include as LICENSE-MIT. Thus, I think we can reasonably argue that a simple comment above this code indicating that it is adapted from (link) which is code Copyright Samuel Tardieu should suffice, which complies with the Apache "You must cause any modified files to carry prominent noticesstating that You changed the files" requirement.

#1724 ended up not adapting this crate's implementation anymore (shoutout to Wikpedia)

valentinewallace · 2022-08-17T17:49:01Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions

TheBlueMatt · 2022-08-17T20:52:33Z

I believe the upstream dijkstras you pulled from is actually buggy, anyway, so the generic-ness isn't worth much :p

tnull · 2022-08-18T05:20:19Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions

Ah, didn't realize keeping it generic had that intention. Well, it's not unreadable and reusing test vectors may be worth it.
If we however also see a way of refactoring the payment pathfinding to reuse the same generic implementation, we'd should definitely consider it, IMO.

valentinewallace · 2022-08-18T16:34:17Z

I believe the upstream dijkstras you pulled from is actually buggy, anyway, so the generic-ness isn't worth much :p

Lol I might've introduced bugs in my quest to eliminate all vecs from the original, to be fair

valentinewallace · 2022-08-18T16:45:29Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions
Ah, didn't realize keeping it generic had that intention. Well, it's not unreadable and reusing test vectors may be worth it. If we however also see a way of refactoring the payment pathfinding to reuse the same generic implementation, we'd should definitely consider it, IMO.

I did wonder if there were other cases where we'd want a generic implementation. I have no idea whether that'd be realistic with our payment pathfinding though lol, we'd need a way to "decrement available liquidity" for hops in paths that were already found, at least, but maybe that's easy? 🤔

TheBlueMatt · 2022-08-18T18:50:47Z

Oops, sorry lol didn't mean to close.

I did wonder if there were other cases where we'd want a generic implementation. I have no idea whether that'd be realistic with our payment pathfinding though lol

I kinda doubt it - we've ended up with a few algorithmic tweaks in addition to basic dijkstra's, so I'd be surprised if we could munge it back.

ariard · 2022-08-19T00:42:49Z

+/// Find a path for sending an onion message.
+pub fn find_path<L: Deref, GL: Deref>(
+	our_node_pubkey: &PublicKey, receiver_pubkey: &PublicKey, network_graph: &NetworkGraph<GL>, first_hops: Option<&[&PublicKey]>, logger: L
+) -> Result<Vec<PublicKey>, LightningError> where L::Target: Logger, GL::Target: Logger


So we wouldn't reuse all the information we're learning and storing in our ProbabilisticScorer. I can see how we're limited with the current penalty being based on the link-level and here we might be interested by node-level reliability in the path construction. That said, we can also assume that a reliable channel == a reliable onion message communication channel and go with it. I don't know if the spec says anything here or what are the thinking of other implementations ?

There is an option in the spec to return a separate error for "peer offline" from "no capacity", but I'm not sure how common that is. Once we land the historical scoring PR, we could use the "time in the zero-available-capacity bucket" as a score here.

valentinewallace · 2022-09-02T17:11:32Z

Thanks for all the feedback! This needs a lot of work and I'm switching it up to work on custom OMs first -- closing for now.

valentinewallace · 2022-08-15T18:34:24Z

We could reuse router::get_route, but it's better to start from scratch
because get_route includes a lot of payment-specific computations that impact
performance.

Partially addresses #1607

Used in upcoming commit(s) to test onion message pathfinding

We *could* reuse router::get_route, but it's better to start from scratch because get_route includes a lot of payment-specific computations that impact performance.

TheBlueMatt

The Dijkstras implementation could be cleaned up a lot...

TheBlueMatt · 2022-08-16T19:45:08Z

+}
+
+// Heavily adapted from https://github.com/samueltardieu/pathfinding/blob/master/src/directed/dijkstra.rs
+// TODO: how2credit the repo (is that necessary?)?


Hmmmmm, good question. In general, the MIT and Apache licenses both require attribution, including of downstream projects. However, the Apache license only requires it if there is a file called "NOTICE" or any "copyright, patent, trademark, and attribution notices", which then must be provided downstream, and the MIT license only requires that "the above copyright notice be included", but the original repo doesn't actually include the MIT license anywhere, nor does it include any relevant notices as far as I can see, so there is no relevant "above copyright notice" to include, aside from the first paragraph of the MIT license, which we of course include as LICENSE-MIT. Thus, I think we can reasonably argue that a simple comment above this code indicating that it is adapted from (link) which is code Copyright Samuel Tardieu should suffice, which complies with the Apache "You must cause any modified files to carry prominent noticesstating that You changed the files" requirement.

#1724 ended up not adapting this crate's implementation anymore (shoutout to Wikpedia)

valentinewallace · 2022-08-17T17:49:01Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions

TheBlueMatt · 2022-08-17T20:52:33Z

I believe the upstream dijkstras you pulled from is actually buggy, anyway, so the generic-ness isn't worth much :p

tnull · 2022-08-18T05:20:19Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions

Ah, didn't realize keeping it generic had that intention. Well, it's not unreadable and reusing test vectors may be worth it.
If we however also see a way of refactoring the payment pathfinding to reuse the same generic implementation, we'd should definitely consider it, IMO.

valentinewallace · 2022-08-18T16:34:17Z

I believe the upstream dijkstras you pulled from is actually buggy, anyway, so the generic-ness isn't worth much :p

Lol I might've introduced bugs in my quest to eliminate all vecs from the original, to be fair

valentinewallace · 2022-08-18T16:45:29Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions
Ah, didn't realize keeping it generic had that intention. Well, it's not unreadable and reusing test vectors may be worth it. If we however also see a way of refactoring the payment pathfinding to reuse the same generic implementation, we'd should definitely consider it, IMO.

I did wonder if there were other cases where we'd want a generic implementation. I have no idea whether that'd be realistic with our payment pathfinding though lol, we'd need a way to "decrement available liquidity" for hops in paths that were already found, at least, but maybe that's easy? 🤔

TheBlueMatt · 2022-08-18T18:50:47Z

Oops, sorry lol didn't mean to close.

I did wonder if there were other cases where we'd want a generic implementation. I have no idea whether that'd be realistic with our payment pathfinding though lol

I kinda doubt it - we've ended up with a few algorithmic tweaks in addition to basic dijkstra's, so I'd be surprised if we could munge it back.

ariard · 2022-08-19T00:42:49Z

+/// Find a path for sending an onion message.
+pub fn find_path<L: Deref, GL: Deref>(
+	our_node_pubkey: &PublicKey, receiver_pubkey: &PublicKey, network_graph: &NetworkGraph<GL>, first_hops: Option<&[&PublicKey]>, logger: L
+) -> Result<Vec<PublicKey>, LightningError> where L::Target: Logger, GL::Target: Logger


So we wouldn't reuse all the information we're learning and storing in our ProbabilisticScorer. I can see how we're limited with the current penalty being based on the link-level and here we might be interested by node-level reliability in the path construction. That said, we can also assume that a reliable channel == a reliable onion message communication channel and go with it. I don't know if the spec says anything here or what are the thinking of other implementations ?

There is an option in the spec to return a separate error for "peer offline" from "no capacity", but I'm not sure how common that is. Once we land the historical scoring PR, we could use the "time in the zero-available-capacity bucket" as a score here.

valentinewallace · 2022-09-02T17:11:32Z

Thanks for all the feedback! This needs a lot of work and I'm switching it up to work on custom OMs first -- closing for now.

valentinewallace · 2022-08-15T18:34:24Z

We could reuse router::get_route, but it's better to start from scratch
because get_route includes a lot of payment-specific computations that impact
performance.

Partially addresses #1607

Used in upcoming commit(s) to test onion message pathfinding

We *could* reuse router::get_route, but it's better to start from scratch because get_route includes a lot of payment-specific computations that impact performance.

TheBlueMatt

The Dijkstras implementation could be cleaned up a lot...

TheBlueMatt · 2022-08-16T19:45:08Z

+}
+
+// Heavily adapted from https://github.com/samueltardieu/pathfinding/blob/master/src/directed/dijkstra.rs
+// TODO: how2credit the repo (is that necessary?)?


Hmmmmm, good question. In general, the MIT and Apache licenses both require attribution, including of downstream projects. However, the Apache license only requires it if there is a file called "NOTICE" or any "copyright, patent, trademark, and attribution notices", which then must be provided downstream, and the MIT license only requires that "the above copyright notice be included", but the original repo doesn't actually include the MIT license anywhere, nor does it include any relevant notices as far as I can see, so there is no relevant "above copyright notice" to include, aside from the first paragraph of the MIT license, which we of course include as LICENSE-MIT. Thus, I think we can reasonably argue that a simple comment above this code indicating that it is adapted from (link) which is code Copyright Samuel Tardieu should suffice, which complies with the Apache "You must cause any modified files to carry prominent noticesstating that You changed the files" requirement.

#1724 ended up not adapting this crate's implementation anymore (shoutout to Wikpedia)

valentinewallace · 2022-08-17T17:49:01Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions

TheBlueMatt · 2022-08-17T20:52:33Z

I believe the upstream dijkstras you pulled from is actually buggy, anyway, so the generic-ness isn't worth much :p

tnull · 2022-08-18T05:20:19Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions

Ah, didn't realize keeping it generic had that intention. Well, it's not unreadable and reusing test vectors may be worth it.
If we however also see a way of refactoring the payment pathfinding to reuse the same generic implementation, we'd should definitely consider it, IMO.

valentinewallace · 2022-08-18T16:34:17Z

I believe the upstream dijkstras you pulled from is actually buggy, anyway, so the generic-ness isn't worth much :p

Lol I might've introduced bugs in my quest to eliminate all vecs from the original, to be fair

valentinewallace · 2022-08-18T16:45:29Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions
Ah, didn't realize keeping it generic had that intention. Well, it's not unreadable and reusing test vectors may be worth it. If we however also see a way of refactoring the payment pathfinding to reuse the same generic implementation, we'd should definitely consider it, IMO.

I did wonder if there were other cases where we'd want a generic implementation. I have no idea whether that'd be realistic with our payment pathfinding though lol, we'd need a way to "decrement available liquidity" for hops in paths that were already found, at least, but maybe that's easy? 🤔

TheBlueMatt · 2022-08-18T18:50:47Z

Oops, sorry lol didn't mean to close.

I did wonder if there were other cases where we'd want a generic implementation. I have no idea whether that'd be realistic with our payment pathfinding though lol

I kinda doubt it - we've ended up with a few algorithmic tweaks in addition to basic dijkstra's, so I'd be surprised if we could munge it back.

ariard · 2022-08-19T00:42:49Z

+/// Find a path for sending an onion message.
+pub fn find_path<L: Deref, GL: Deref>(
+	our_node_pubkey: &PublicKey, receiver_pubkey: &PublicKey, network_graph: &NetworkGraph<GL>, first_hops: Option<&[&PublicKey]>, logger: L
+) -> Result<Vec<PublicKey>, LightningError> where L::Target: Logger, GL::Target: Logger


So we wouldn't reuse all the information we're learning and storing in our ProbabilisticScorer. I can see how we're limited with the current penalty being based on the link-level and here we might be interested by node-level reliability in the path construction. That said, we can also assume that a reliable channel == a reliable onion message communication channel and go with it. I don't know if the spec says anything here or what are the thinking of other implementations ?

There is an option in the spec to return a separate error for "peer offline" from "no capacity", but I'm not sure how common that is. Once we land the historical scoring PR, we could use the "time in the zero-available-capacity bucket" as a score here.

valentinewallace · 2022-09-02T17:11:32Z

Thanks for all the feedback! This needs a lot of work and I'm switching it up to work on custom OMs first -- closing for now.

valentinewallace · 2022-08-15T18:34:24Z

We could reuse router::get_route, but it's better to start from scratch
because get_route includes a lot of payment-specific computations that impact
performance.

Partially addresses #1607

Used in upcoming commit(s) to test onion message pathfinding

We *could* reuse router::get_route, but it's better to start from scratch because get_route includes a lot of payment-specific computations that impact performance.

TheBlueMatt

The Dijkstras implementation could be cleaned up a lot...

TheBlueMatt · 2022-08-16T19:45:08Z

+}
+
+// Heavily adapted from https://github.com/samueltardieu/pathfinding/blob/master/src/directed/dijkstra.rs
+// TODO: how2credit the repo (is that necessary?)?


Hmmmmm, good question. In general, the MIT and Apache licenses both require attribution, including of downstream projects. However, the Apache license only requires it if there is a file called "NOTICE" or any "copyright, patent, trademark, and attribution notices", which then must be provided downstream, and the MIT license only requires that "the above copyright notice be included", but the original repo doesn't actually include the MIT license anywhere, nor does it include any relevant notices as far as I can see, so there is no relevant "above copyright notice" to include, aside from the first paragraph of the MIT license, which we of course include as LICENSE-MIT. Thus, I think we can reasonably argue that a simple comment above this code indicating that it is adapted from (link) which is code Copyright Samuel Tardieu should suffice, which complies with the Apache "You must cause any modified files to carry prominent noticesstating that You changed the files" requirement.

#1724 ended up not adapting this crate's implementation anymore (shoutout to Wikpedia)

valentinewallace · 2022-08-17T17:49:01Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions

TheBlueMatt · 2022-08-17T20:52:33Z

I believe the upstream dijkstras you pulled from is actually buggy, anyway, so the generic-ness isn't worth much :p

tnull · 2022-08-18T05:20:19Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions

Ah, didn't realize keeping it generic had that intention. Well, it's not unreadable and reusing test vectors may be worth it.
If we however also see a way of refactoring the payment pathfinding to reuse the same generic implementation, we'd should definitely consider it, IMO.

valentinewallace · 2022-08-18T16:34:17Z

I believe the upstream dijkstras you pulled from is actually buggy, anyway, so the generic-ness isn't worth much :p

Lol I might've introduced bugs in my quest to eliminate all vecs from the original, to be fair

valentinewallace · 2022-08-18T16:45:29Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions
Ah, didn't realize keeping it generic had that intention. Well, it's not unreadable and reusing test vectors may be worth it. If we however also see a way of refactoring the payment pathfinding to reuse the same generic implementation, we'd should definitely consider it, IMO.

I did wonder if there were other cases where we'd want a generic implementation. I have no idea whether that'd be realistic with our payment pathfinding though lol, we'd need a way to "decrement available liquidity" for hops in paths that were already found, at least, but maybe that's easy? 🤔

TheBlueMatt · 2022-08-18T18:50:47Z

Oops, sorry lol didn't mean to close.

I did wonder if there were other cases where we'd want a generic implementation. I have no idea whether that'd be realistic with our payment pathfinding though lol

I kinda doubt it - we've ended up with a few algorithmic tweaks in addition to basic dijkstra's, so I'd be surprised if we could munge it back.

ariard · 2022-08-19T00:42:49Z

+/// Find a path for sending an onion message.
+pub fn find_path<L: Deref, GL: Deref>(
+	our_node_pubkey: &PublicKey, receiver_pubkey: &PublicKey, network_graph: &NetworkGraph<GL>, first_hops: Option<&[&PublicKey]>, logger: L
+) -> Result<Vec<PublicKey>, LightningError> where L::Target: Logger, GL::Target: Logger


So we wouldn't reuse all the information we're learning and storing in our ProbabilisticScorer. I can see how we're limited with the current penalty being based on the link-level and here we might be interested by node-level reliability in the path construction. That said, we can also assume that a reliable channel == a reliable onion message communication channel and go with it. I don't know if the spec says anything here or what are the thinking of other implementations ?

There is an option in the spec to return a separate error for "peer offline" from "no capacity", but I'm not sure how common that is. Once we land the historical scoring PR, we could use the "time in the zero-available-capacity bucket" as a score here.

valentinewallace · 2022-09-02T17:11:32Z

Thanks for all the feedback! This needs a lot of work and I'm switching it up to work on custom OMs first -- closing for now.

valentinewallace · 2022-08-15T18:34:24Z

We could reuse router::get_route, but it's better to start from scratch
because get_route includes a lot of payment-specific computations that impact
performance.

Partially addresses #1607

Used in upcoming commit(s) to test onion message pathfinding

We *could* reuse router::get_route, but it's better to start from scratch because get_route includes a lot of payment-specific computations that impact performance.

TheBlueMatt

The Dijkstras implementation could be cleaned up a lot...

TheBlueMatt · 2022-08-16T19:45:08Z

+}
+
+// Heavily adapted from https://github.com/samueltardieu/pathfinding/blob/master/src/directed/dijkstra.rs
+// TODO: how2credit the repo (is that necessary?)?


Hmmmmm, good question. In general, the MIT and Apache licenses both require attribution, including of downstream projects. However, the Apache license only requires it if there is a file called "NOTICE" or any "copyright, patent, trademark, and attribution notices", which then must be provided downstream, and the MIT license only requires that "the above copyright notice be included", but the original repo doesn't actually include the MIT license anywhere, nor does it include any relevant notices as far as I can see, so there is no relevant "above copyright notice" to include, aside from the first paragraph of the MIT license, which we of course include as LICENSE-MIT. Thus, I think we can reasonably argue that a simple comment above this code indicating that it is adapted from (link) which is code Copyright Samuel Tardieu should suffice, which complies with the Apache "You must cause any modified files to carry prominent noticesstating that You changed the files" requirement.

#1724 ended up not adapting this crate's implementation anymore (shoutout to Wikpedia)

valentinewallace · 2022-08-17T17:49:01Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions

TheBlueMatt · 2022-08-17T20:52:33Z

I believe the upstream dijkstras you pulled from is actually buggy, anyway, so the generic-ness isn't worth much :p

tnull · 2022-08-18T05:20:19Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions

Ah, didn't realize keeping it generic had that intention. Well, it's not unreadable and reusing test vectors may be worth it.
If we however also see a way of refactoring the payment pathfinding to reuse the same generic implementation, we'd should definitely consider it, IMO.

valentinewallace · 2022-08-18T16:34:17Z

I believe the upstream dijkstras you pulled from is actually buggy, anyway, so the generic-ness isn't worth much :p

Lol I might've introduced bugs in my quest to eliminate all vecs from the original, to be fair

valentinewallace · 2022-08-18T16:45:29Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions
Ah, didn't realize keeping it generic had that intention. Well, it's not unreadable and reusing test vectors may be worth it. If we however also see a way of refactoring the payment pathfinding to reuse the same generic implementation, we'd should definitely consider it, IMO.

I did wonder if there were other cases where we'd want a generic implementation. I have no idea whether that'd be realistic with our payment pathfinding though lol, we'd need a way to "decrement available liquidity" for hops in paths that were already found, at least, but maybe that's easy? 🤔

TheBlueMatt · 2022-08-18T18:50:47Z

Oops, sorry lol didn't mean to close.

I did wonder if there were other cases where we'd want a generic implementation. I have no idea whether that'd be realistic with our payment pathfinding though lol

I kinda doubt it - we've ended up with a few algorithmic tweaks in addition to basic dijkstra's, so I'd be surprised if we could munge it back.

ariard · 2022-08-19T00:42:49Z

+/// Find a path for sending an onion message.
+pub fn find_path<L: Deref, GL: Deref>(
+	our_node_pubkey: &PublicKey, receiver_pubkey: &PublicKey, network_graph: &NetworkGraph<GL>, first_hops: Option<&[&PublicKey]>, logger: L
+) -> Result<Vec<PublicKey>, LightningError> where L::Target: Logger, GL::Target: Logger


So we wouldn't reuse all the information we're learning and storing in our ProbabilisticScorer. I can see how we're limited with the current penalty being based on the link-level and here we might be interested by node-level reliability in the path construction. That said, we can also assume that a reliable channel == a reliable onion message communication channel and go with it. I don't know if the spec says anything here or what are the thinking of other implementations ?

There is an option in the spec to return a separate error for "peer offline" from "no capacity", but I'm not sure how common that is. Once we land the historical scoring PR, we could use the "time in the zero-available-capacity bucket" as a score here.

valentinewallace · 2022-09-02T17:11:32Z

Thanks for all the feedback! This needs a lot of work and I'm switching it up to work on custom OMs first -- closing for now.

valentinewallace · 2022-08-15T18:34:24Z

We could reuse router::get_route, but it's better to start from scratch
because get_route includes a lot of payment-specific computations that impact
performance.

Partially addresses #1607

Used in upcoming commit(s) to test onion message pathfinding

We *could* reuse router::get_route, but it's better to start from scratch because get_route includes a lot of payment-specific computations that impact performance.

TheBlueMatt

The Dijkstras implementation could be cleaned up a lot...

TheBlueMatt · 2022-08-16T19:45:08Z

+}
+
+// Heavily adapted from https://github.com/samueltardieu/pathfinding/blob/master/src/directed/dijkstra.rs
+// TODO: how2credit the repo (is that necessary?)?


Hmmmmm, good question. In general, the MIT and Apache licenses both require attribution, including of downstream projects. However, the Apache license only requires it if there is a file called "NOTICE" or any "copyright, patent, trademark, and attribution notices", which then must be provided downstream, and the MIT license only requires that "the above copyright notice be included", but the original repo doesn't actually include the MIT license anywhere, nor does it include any relevant notices as far as I can see, so there is no relevant "above copyright notice" to include, aside from the first paragraph of the MIT license, which we of course include as LICENSE-MIT. Thus, I think we can reasonably argue that a simple comment above this code indicating that it is adapted from (link) which is code Copyright Samuel Tardieu should suffice, which complies with the Apache "You must cause any modified files to carry prominent noticesstating that You changed the files" requirement.

#1724 ended up not adapting this crate's implementation anymore (shoutout to Wikpedia)

valentinewallace · 2022-08-17T17:49:01Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions

TheBlueMatt · 2022-08-17T20:52:33Z

I believe the upstream dijkstras you pulled from is actually buggy, anyway, so the generic-ness isn't worth much :p

tnull · 2022-08-18T05:20:19Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions

Ah, didn't realize keeping it generic had that intention. Well, it's not unreadable and reusing test vectors may be worth it.
If we however also see a way of refactoring the payment pathfinding to reuse the same generic implementation, we'd should definitely consider it, IMO.

valentinewallace · 2022-08-18T16:34:17Z

I believe the upstream dijkstras you pulled from is actually buggy, anyway, so the generic-ness isn't worth much :p

Lol I might've introduced bugs in my quest to eliminate all vecs from the original, to be fair

valentinewallace · 2022-08-18T16:45:29Z

Ok it's clearly less readable than I thought with a generic Dijkstra's. Staying generic would allow us to pretty much reuse the test vectors in the original repo, but it doesn't seem worth. Gonna convert to draft for now and rewrite the generic portions
Ah, didn't realize keeping it generic had that intention. Well, it's not unreadable and reusing test vectors may be worth it. If we however also see a way of refactoring the payment pathfinding to reuse the same generic implementation, we'd should definitely consider it, IMO.

I did wonder if there were other cases where we'd want a generic implementation. I have no idea whether that'd be realistic with our payment pathfinding though lol, we'd need a way to "decrement available liquidity" for hops in paths that were already found, at least, but maybe that's easy? 🤔

TheBlueMatt · 2022-08-18T18:50:47Z

Oops, sorry lol didn't mean to close.

I did wonder if there were other cases where we'd want a generic implementation. I have no idea whether that'd be realistic with our payment pathfinding though lol

I kinda doubt it - we've ended up with a few algorithmic tweaks in addition to basic dijkstra's, so I'd be surprised if we could munge it back.

ariard · 2022-08-19T00:42:49Z

+/// Find a path for sending an onion message.
+pub fn find_path<L: Deref, GL: Deref>(
+	our_node_pubkey: &PublicKey, receiver_pubkey: &PublicKey, network_graph: &NetworkGraph<GL>, first_hops: Option<&[&PublicKey]>, logger: L
+) -> Result<Vec<PublicKey>, LightningError> where L::Target: Logger, GL::Target: Logger


So we wouldn't reuse all the information we're learning and storing in our ProbabilisticScorer. I can see how we're limited with the current penalty being based on the link-level and here we might be interested by node-level reliability in the path construction. That said, we can also assume that a reliable channel == a reliable onion message communication channel and go with it. I don't know if the spec says anything here or what are the thinking of other implementations ?

There is an option in the spec to return a separate error for "peer offline" from "no capacity", but I'm not sure how common that is. Once we land the historical scoring PR, we could use the "time in the zero-available-capacity bucket" as a score here.

valentinewallace · 2022-09-02T17:11:32Z

Thanks for all the feedback! This needs a lot of work and I'm switching it up to work on custom OMs first -- closing for now.

valentinewallace added the Seeking Conceptual Review label Aug 15, 2022

tnull self-requested a review August 15, 2022 19:01

valentinewallace added 2 commits August 15, 2022 17:26

Expose router test utils as pub(crate)
8c68980
Used in upcoming commit(s) to test onion message pathfinding

Implement pathfinding for onion messages
6d446a8
We *could* reuse router::get_route, but it's better to start from scratch because get_route includes a lot of payment-specific computations that impact performance.

valentinewallace force-pushed the 2022-08-om-pathfinding branch from 5510763 to 6d446a8Compare August 15, 2022 21:26

TheBlueMatt reviewed Aug 16, 2022
View reviewed changes

tnull reviewed Aug 17, 2022
View reviewed changes

Comment threadlightning/src/onion_message/router.rs
Comment threadlightning/src/onion_message/router.rs
Comment threadlightning/src/onion_message/router.rs

valentinewallace marked this pull request as draft August 17, 2022 17:49

TheBlueMatt closed this Aug 18, 2022

TheBlueMatt reopened this Aug 18, 2022

ariard reviewed Aug 19, 2022
View reviewed changes

valentinewallace removed the Seeking Conceptual Review label Aug 26, 2022

valentinewallace closed this Sep 2, 2022

valentinewallace mentioned this pull request Sep 15, 2022
Support onion message pathfinding #1724
Closed
2 tasks

valentinewallace added the Seeking Conceptual Review label Aug 15, 2022

tnull self-requested a review August 15, 2022 19:01

valentinewallace added 2 commits August 15, 2022 17:26

Expose router test utils as pub(crate)
8c68980
Used in upcoming commit(s) to test onion message pathfinding

Implement pathfinding for onion messages
6d446a8
We *could* reuse router::get_route, but it's better to start from scratch because get_route includes a lot of payment-specific computations that impact performance.

valentinewallace force-pushed the 2022-08-om-pathfinding branch from 5510763 to 6d446a8Compare August 15, 2022 21:26

TheBlueMatt reviewed Aug 16, 2022
View reviewed changes

tnull reviewed Aug 17, 2022
View reviewed changes

Comment threadlightning/src/onion_message/router.rs
Comment threadlightning/src/onion_message/router.rs
Comment threadlightning/src/onion_message/router.rs

valentinewallace marked this pull request as draft August 17, 2022 17:49

TheBlueMatt closed this Aug 18, 2022

TheBlueMatt reopened this Aug 18, 2022

ariard reviewed Aug 19, 2022
View reviewed changes

valentinewallace removed the Seeking Conceptual Review label Aug 26, 2022

valentinewallace closed this Sep 2, 2022

valentinewallace mentioned this pull request Sep 15, 2022
Support onion message pathfinding #1724
Closed
2 tasks

valentinewallace added the Seeking Conceptual Review label Aug 15, 2022

tnull self-requested a review August 15, 2022 19:01

valentinewallace added 2 commits August 15, 2022 17:26

Expose router test utils as pub(crate)
8c68980
Used in upcoming commit(s) to test onion message pathfinding

Implement pathfinding for onion messages
6d446a8
We *could* reuse router::get_route, but it's better to start from scratch because get_route includes a lot of payment-specific computations that impact performance.

valentinewallace force-pushed the 2022-08-om-pathfinding branch from 5510763 to 6d446a8Compare August 15, 2022 21:26

TheBlueMatt reviewed Aug 16, 2022
View reviewed changes

tnull reviewed Aug 17, 2022
View reviewed changes

Comment threadlightning/src/onion_message/router.rs
Comment threadlightning/src/onion_message/router.rs
Comment threadlightning/src/onion_message/router.rs

valentinewallace marked this pull request as draft August 17, 2022 17:49

TheBlueMatt closed this Aug 18, 2022

TheBlueMatt reopened this Aug 18, 2022

ariard reviewed Aug 19, 2022
View reviewed changes

valentinewallace removed the Seeking Conceptual Review label Aug 26, 2022

valentinewallace closed this Sep 2, 2022

valentinewallace mentioned this pull request Sep 15, 2022
Support onion message pathfinding #1724
Closed
2 tasks

valentinewallace added the Seeking Conceptual Review label Aug 15, 2022

tnull self-requested a review August 15, 2022 19:01

valentinewallace added 2 commits August 15, 2022 17:26

Expose router test utils as pub(crate)
8c68980
Used in upcoming commit(s) to test onion message pathfinding

Implement pathfinding for onion messages
6d446a8
We *could* reuse router::get_route, but it's better to start from scratch because get_route includes a lot of payment-specific computations that impact performance.

valentinewallace force-pushed the 2022-08-om-pathfinding branch from 5510763 to 6d446a8Compare August 15, 2022 21:26

TheBlueMatt reviewed Aug 16, 2022
View reviewed changes

tnull reviewed Aug 17, 2022
View reviewed changes

Comment threadlightning/src/onion_message/router.rs
Comment threadlightning/src/onion_message/router.rs
Comment threadlightning/src/onion_message/router.rs

valentinewallace marked this pull request as draft August 17, 2022 17:49

TheBlueMatt closed this Aug 18, 2022

TheBlueMatt reopened this Aug 18, 2022

ariard reviewed Aug 19, 2022
View reviewed changes

valentinewallace removed the Seeking Conceptual Review label Aug 26, 2022

valentinewallace closed this Sep 2, 2022

valentinewallace mentioned this pull request Sep 15, 2022
Support onion message pathfinding #1724
Closed
2 tasks

valentinewallace added the Seeking Conceptual Review label Aug 15, 2022

tnull self-requested a review August 15, 2022 19:01

valentinewallace added 2 commits August 15, 2022 17:26

Expose router test utils as pub(crate)
8c68980
Used in upcoming commit(s) to test onion message pathfinding

Implement pathfinding for onion messages
6d446a8
We *could* reuse router::get_route, but it's better to start from scratch because get_route includes a lot of payment-specific computations that impact performance.

valentinewallace force-pushed the 2022-08-om-pathfinding branch from 5510763 to 6d446a8Compare August 15, 2022 21:26

TheBlueMatt reviewed Aug 16, 2022
View reviewed changes

tnull reviewed Aug 17, 2022
View reviewed changes

Comment threadlightning/src/onion_message/router.rs
Comment threadlightning/src/onion_message/router.rs
Comment threadlightning/src/onion_message/router.rs

valentinewallace marked this pull request as draft August 17, 2022 17:49

TheBlueMatt closed this Aug 18, 2022

TheBlueMatt reopened this Aug 18, 2022

ariard reviewed Aug 19, 2022
View reviewed changes

valentinewallace removed the Seeking Conceptual Review label Aug 26, 2022

valentinewallace closed this Sep 2, 2022

valentinewallace mentioned this pull request Sep 15, 2022
Support onion message pathfinding #1724
Closed
2 tasks

valentinewallace added the Seeking Conceptual Review label Aug 15, 2022

tnull self-requested a review August 15, 2022 19:01

valentinewallace added 2 commits August 15, 2022 17:26

Expose router test utils as pub(crate)
8c68980
Used in upcoming commit(s) to test onion message pathfinding

Implement pathfinding for onion messages
6d446a8
We *could* reuse router::get_route, but it's better to start from scratch because get_route includes a lot of payment-specific computations that impact performance.

valentinewallace force-pushed the 2022-08-om-pathfinding branch from 5510763 to 6d446a8Compare August 15, 2022 21:26

TheBlueMatt reviewed Aug 16, 2022
View reviewed changes

tnull reviewed Aug 17, 2022
View reviewed changes

Comment threadlightning/src/onion_message/router.rs
Comment threadlightning/src/onion_message/router.rs
Comment threadlightning/src/onion_message/router.rs

valentinewallace marked this pull request as draft August 17, 2022 17:49

TheBlueMatt closed this Aug 18, 2022

TheBlueMatt reopened this Aug 18, 2022

ariard reviewed Aug 19, 2022
View reviewed changes

valentinewallace removed the Seeking Conceptual Review label Aug 26, 2022

valentinewallace closed this Sep 2, 2022

valentinewallace mentioned this pull request Sep 15, 2022
Support onion message pathfinding #1724
Closed
2 tasks

valentinewallace added the Seeking Conceptual Review label Aug 15, 2022

tnull self-requested a review August 15, 2022 19:01

valentinewallace added 2 commits August 15, 2022 17:26

Expose router test utils as pub(crate)
8c68980
Used in upcoming commit(s) to test onion message pathfinding

Implement pathfinding for onion messages
6d446a8
We *could* reuse router::get_route, but it's better to start from scratch because get_route includes a lot of payment-specific computations that impact performance.

valentinewallace force-pushed the 2022-08-om-pathfinding branch from 5510763 to 6d446a8Compare August 15, 2022 21:26

TheBlueMatt reviewed Aug 16, 2022
View reviewed changes

tnull reviewed Aug 17, 2022
View reviewed changes

Comment threadlightning/src/onion_message/router.rs
Comment threadlightning/src/onion_message/router.rs
Comment threadlightning/src/onion_message/router.rs

valentinewallace marked this pull request as draft August 17, 2022 17:49

TheBlueMatt closed this Aug 18, 2022

TheBlueMatt reopened this Aug 18, 2022

ariard reviewed Aug 19, 2022
View reviewed changes

valentinewallace removed the Seeking Conceptual Review label Aug 26, 2022

valentinewallace closed this Sep 2, 2022

valentinewallace mentioned this pull request Sep 15, 2022
Support onion message pathfinding #1724
Closed
2 tasks

valentinewallace added the Seeking Conceptual Review label Aug 15, 2022

tnull self-requested a review August 15, 2022 19:01

valentinewallace added 2 commits August 15, 2022 17:26

Expose router test utils as pub(crate)
8c68980
Used in upcoming commit(s) to test onion message pathfinding

Implement pathfinding for onion messages
6d446a8
We *could* reuse router::get_route, but it's better to start from scratch because get_route includes a lot of payment-specific computations that impact performance.

valentinewallace force-pushed the 2022-08-om-pathfinding branch from 5510763 to 6d446a8Compare August 15, 2022 21:26

TheBlueMatt reviewed Aug 16, 2022
View reviewed changes

tnull reviewed Aug 17, 2022
View reviewed changes

Comment threadlightning/src/onion_message/router.rs
Comment threadlightning/src/onion_message/router.rs
Comment threadlightning/src/onion_message/router.rs

valentinewallace marked this pull request as draft August 17, 2022 17:49

TheBlueMatt closed this Aug 18, 2022

TheBlueMatt reopened this Aug 18, 2022

ariard reviewed Aug 19, 2022
View reviewed changes

valentinewallace removed the Seeking Conceptual Review label Aug 26, 2022

valentinewallace closed this Sep 2, 2022

valentinewallace mentioned this pull request Sep 15, 2022
Support onion message pathfinding #1724
Closed
2 tasks

Uh oh!

Conversation

valentinewallace commented Aug 15, 2022

Uh oh!

TheBlueMatt 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

valentinewallace commented Aug 17, 2022

Uh oh!

TheBlueMatt commented Aug 17, 2022

Uh oh!

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

Uh oh!

Uh oh!

valentinewallace commented Aug 18, 2022

Uh oh!

valentinewallace commented Aug 18, 2022

Uh oh!

TheBlueMatt commented Aug 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace commented Sep 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

valentinewallace commented Aug 15, 2022

Uh oh!

TheBlueMatt 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

valentinewallace commented Aug 17, 2022

Uh oh!

TheBlueMatt commented Aug 17, 2022

Uh oh!

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

Uh oh!

Uh oh!

valentinewallace commented Aug 18, 2022

Uh oh!

valentinewallace commented Aug 18, 2022

Uh oh!

TheBlueMatt commented Aug 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented Aug 18, 2022•
edited
Loading

tnull commented Aug 18, 2022•
edited
Loading

tnull commented Aug 18, 2022•
edited
Loading