Support onion message pathfinding #1669
Conversation
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.
valentinewallace
force-pushed
the
2022-08-om-pathfinding
branch
from
5510763
to
6d446a8
Compare
| } | ||
|
|
||
| // Heavily adapted from https://github.com/samueltardieu/pathfinding/blob/master/src/directed/dijkstra.rs | ||
| // TODO: how2credit the repo (is that necessary?)? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
#1724 ended up not adapting this crate's implementation anymore (shoutout to Wikpedia)
|
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 |
|
I believe the upstream dijkstras you pulled from is actually buggy, anyway, so the generic-ness isn't worth much :p |
Ah, didn't realize keeping it generic had that intention. Well, it's not unreadable and reusing test vectors may be worth it. |
Lol I might've introduced bugs in my quest to eliminate all vecs from the original, to be fair |
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? 🤔 |
|
Oops, sorry lol didn't mean to close.
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. |
| /// 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 |
There was a problem hiding this comment.
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 was a problem hiding this comment.
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.
|
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. |
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