Support onion message pathfinding #1724
Conversation
|
I think I addressed all the feedback from #1669. Much cleaner! If there are rough conceptual ACKs, I can add more testing, docs, feature bit checks, and open the first commit in a separate PR. |
valentinewallace
force-pushed
the
2022-09-om-pathfinding
branch
2 times, most recently
from
bd24563 to
a019106
Compare
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #1724 +/- ##
==========================================
+ Coverage 87.21% 90.80% +3.59%
==========================================
Files 100 88 -12
Lines 44516 47645 +3129
Branches 44516 47645 +3129
==========================================
+ Hits 38825 43266 +4441
+ Misses 5691 4379 -1312
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
valentinewallace
force-pushed
the
2022-09-om-pathfinding
branch
from
a019106 to
9343199
Compare
Sounds good, will likely make the module private and do it in follow-up if that works |
|
Can be rebased now, I believe. |
valentinewallace
force-pushed
the
2022-09-om-pathfinding
branch
2 times, most recently
from
a571f8c to
980bbc1
Compare
| impl PartialOrd for PathBuildingHop { | ||
| fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> { | ||
| // We need a min-heap, whereas `BinaryHeap`s are a max-heap, so compare the costs in reverse. | ||
| other.cost.partial_cmp(&self.cost) |
There was a problem hiding this comment.
Assuming the cost is equal, can someone grind lower node-id to attract more message flows? After staring at rust documentation for 5 minutes, I'm not sure.
There was a problem hiding this comment.
Added a TODO for this
There was a problem hiding this comment.
So you confirm it's possible by looking at the rust doc?
There was a problem hiding this comment.
Ah, I misread your comment. After testing in the playground, I don't think this is the case. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3912286ccee3e036de27c9e811b40f83 It seems like if the cost is equal, then whatever node was pushed first will be the one popped first, regardless of node id.
There was a problem hiding this comment.
Perhaps fine to keep a todo anyway because:
- the way nodes sorted before insertion into heap might play a role then
- heap implementation may change
valentinewallace
force-pushed
the
2022-09-om-pathfinding
branch
2 times, most recently
from
c2ff655 to
f349312
Compare
valentinewallace
force-pushed
the
2022-09-om-pathfinding
branch
3 times, most recently
from
5280490 to
fdaadac
Compare
valentinewallace
force-pushed
the
2022-09-om-pathfinding
branch
from
fdaadac to
d773b04
Compare
| //! between channel peers are likely to be prioritized over those between non-channel peers. | ||
| //! | ||
| //! [Onion message]: crate::onion_message | ||
| //! [offers]: <https://github.com/lightning/bolts/pull/798> |
There was a problem hiding this comment.
Heh, maybe add a TODO to point to the offers module once it lands?
| //! requests from [offers]. It differs from payment pathfinding in that channel liquidity, fees, and | ||
| //! knobs such as `htlc_maximum_msat` do not factor into path selection -- onion messages require a | ||
| //! peer connection and nothing more. However, we still use the network graph because onion messages | ||
| //! between channel peers are likely to be prioritized over those between non-channel peers. |
There was a problem hiding this comment.
s/prioritized/nodes will refuse to forward unless they're already connected/ (ie there is no way to get a message through at all otherwise).
| /// Finds a route from us to the given `destination` node. | ||
| /// If we have private channels, it may be useful to fill in `first_hops` with the results from | ||
| /// [`ChannelManager::list_usable_channels`]. If `first_hops` is not filled in, connected peers | ||
| /// without public channels will not be forwarded over. |
There was a problem hiding this comment.
This is not true, we outright will fail to find a route if its not filled in. I think this is fine, but we need to update the docs. More generally, I think these docs really need some kind of general discussion of OM pathfinding principles - something about how we should try to find a path, and try to forward over it, but most nodes currently dont support OMs, so path finding is likely to fail. In that case, users should consider implementing something to directly connect to the destination node if possible, noting that this compromises sender privacy and this should be disabled in the future.
| } | ||
| if let Some(node_info) = network_nodes.get(&node_id) { | ||
| // Only consider the network graph if first_hops does not override it. | ||
| if valid_first_hops.contains(&node_id) || node_id == our_node_id { |
There was a problem hiding this comment.
If we got back to ourself, we definitely dont want to add more channels to the frontier set.
| { continue; } | ||
| } else { continue; } | ||
| for scid in &node_info.channels { | ||
| if let Some(chan_info) = network_channels.get(&scid) { |
There was a problem hiding this comment.
Interesting question - would this be faster to actually just push the SCID onto the heap? We'd have to handle the first-hops even more explicitly, but that's okay. It would avoid a full round of channel-gets at the frontier when we find the target, which may be worth quite a bit.
There was a problem hiding this comment.
I'm getting somewhat comparable times, with scid-heap narrowly taking the lead.
inserting node_ids into heap:
test routing::onion_message::benches::generate_simple_routes ... bench: 42,412,441 ns/iter (+/- 26,718,082)
test routing::onion_message::benches::generate_simple_routes ... bench: 40,244,529 ns/iter (+/- 27,297,264)
test routing::onion_message::benches::generate_simple_routes ... bench: 38,865,025 ns/iter (+/- 27,147,061)
test routing::onion_message::benches::generate_simple_routes ... bench: 43,582,987 ns/iter (+/- 23,421,789)
inserting scids into heap:
test routing::onion_message::benches::generate_simple_routes ... bench: 38,450,779 ns/iter (+/- 29,381,214)
test routing::onion_message::benches::generate_simple_routes ... bench: 37,930,562 ns/iter (+/- 27,023,358)
test routing::onion_message::benches::generate_simple_routes ... bench: 41,410,850 ns/iter (+/- 27,262,014)
test routing::onion_message::benches::generate_simple_routes ... bench: 39,008,820 ns/iter (+/- 26,009,606)
I pushed a WIP commit (d9acdf1) with the scid-heap version if you want to test it out and/or check that I implemented it the way you expected. (The commit needs a bit of cleanup..)
| //! peer connection and nothing more. However, we still use the network graph because onion messages | ||
| //! between channel peers are likely to be prioritized over those between non-channel peers. |
There was a problem hiding this comment.
So while I think relying on the existence of channels between peers to flow onion messages align well the incentives between bandwidth consumed and onion forwards accepted, I don't know if this is a direction we would like for the long-term in term of privacy. Indeed, overlapping both offers flows and HTLC flows, if we re-use the same heuristics than for classic pathfinding we might have overlap and show more "data trails" to any payment observers. Especially onion messages routing might invalidate some privacy-preserving enhancing we have implemented with HTLCs such as #1286. The lack of timing padding in onion routing might cancel the CLTV delta offset. Of course, this is a subject we can think more and improve with time. Just to be aware of about the privacy regression we might have.
Thanks, I don't know if I'll be prioritizing the follow-ups for a while (open to someone taking them over), but since this seems close(?) I'll try to get back to it in the new year |
There was a problem hiding this comment.
Beyond offer transport, I think onion messages pathfinding could be useful to two more use-cases: bitcoin transaction relay and jamming credentials. For the former, I believe the set of heuristics is going to be really-specific, as you might avoid centralised hops to improve fault-tolerance. For the latter, the use-case requirements are more likely to be similar to offers, however you might do synthetic traffic to mask the economic volume.
Just to say, I don't think we'll need more than one onion messages pathfinding algorithms for other use-cases, however I believe they might need each to plug a CustomHeuristicsOnionScore similar to scoring::Score, or combine those CustomHeuristicsOnionScore. Thinking aloud for jamming.
| let dest_node_id = NodeId::from_pubkey(destination); | ||
| if our_node_id == dest_node_id { return Err(Error::InvalidDestination) } | ||
|
|
||
| // Add our start and first-hops to `frontier`. |
There was a problem hiding this comment.
Can define what is meant by "frontier". Understood as "intermediate node". Or maybe it's just us the start.
| let start = NodeId::from_pubkey(&our_node_pubkey); | ||
| let mut valid_first_hops = HashSet::new(); | ||
| let mut frontier = BinaryHeap::new(); | ||
| frontier.push(PathBuildingHop { cost: 0, node_id: start, parent_node_id: start }); |
There was a problem hiding this comment.
Can define the reasoning behind PathBuildingHop::cost. I don't know if the onion bandwidth we're expecting from hops on the network is scaled on the channel capacity. The most rule of thumb metric I would guess.
|
@naumenkogs if you're bored, I'm pretty sure onion message pathfinding will be a must to play with a bunch of things, notably jamming mitigations :) |
Used incoming 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.
Or that require unknown features
As judged by whether the channel is disabled, similar to payment routing
Don't allow finding a path to ourselves and reject first hops that contain our node id as the counterparty node id.
valentinewallace
force-pushed
the
2022-09-om-pathfinding
branch
from
d773b04 to
b07e707
Compare
valentinewallace
force-pushed
the
2022-09-om-pathfinding
branch
2 times, most recently
from
28318ac to
d9acdf1
Compare
valentinewallace
force-pushed
the
2022-09-om-pathfinding
branch
from
d9acdf1 to
c9fafc3
Compare
|
I see @moneyball added this to the 119 milestone. But I don't see the rush on this, because most cases will use direct-connect? It seems like this will only be useful once OMs are more widely supported across the network. @TheBlueMatt @jkczyz Let me know if I'm good to remove this from the milestone for now. |
|
Yes I'll remove it. I had added it to 119 due to a misunderstanding of how it could work in the short-term. |
Take 2, Github wouldn't let me reopen #1669 🤔
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
Based on #1731Known followups: