Uh oh!
There was an error while loading. Please reload this page.
Compact blinded path creation - #3011
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@## main #3011 +/- ##
==========================================
+ Coverage 89.87% 90.29% +0.42%
==========================================
Files 117 117 Lines 96952 100069 +3117 Branches 96952 100069 +3117 ==========================================
+ Hits 87134 90357 +3223 + Misses 7273 7130 -143 - Partials 2545 2582 +37 ☔ View full report in Codecov by Sentry. |
Uh oh!
There was an error while loading. Please reload this page.
| /// Attempts to a use a compact representation for the [`IntroductionNode`] by using a directed | ||
| /// short channel id from a channel in `network_graph` leading to the introduction node. | ||
| pub fn compact_introduction_node(&mut self, network_graph: &ReadOnlyNetworkGraph) { |
There was a problem hiding this comment.
Pedantic but the convention is for setters to use the set_ prefix: https://github.com/rust-lang/rfcs/blob/master/text/0344-conventions-galore.md#gettersetter-apis and I think it would read a bit cleaner in this case.
There was a problem hiding this comment.
Hmmm... this is not a setter in the normal sense, though, as we don't pass a value to set a field with. Nor is the field private, which would necessitate a setter. It also may not update the field if a value cannot be found in the network graph. Happy to go with a better name. Just not sure if a set_ prefix is appropriate here.
There was a problem hiding this comment.
Still, it does seem a bit strange that we're mutating the blinded path with a fn name that sounds like it could just as well be a straight getter.
There was a problem hiding this comment.
Ok, added a use_ prefix. I can see how the docs make it less clear that compact was a verb in the name.
| T: secp256k1::Signing + secp256k1::Verification | ||
| >( | ||
| &self, recipient: PublicKey, peers: Vec<PublicKey>, secp_ctx: &Secp256k1<T>, | ||
| &self, recipient: PublicKey, peers: Vec<PublicKey>, scid_lookup: &SL, |
There was a problem hiding this comment.
Rather than passing a list of our peers and a trait so that create_blinded_paths can make a callback to the caller to ask for an SCID for a given peer, shouldn't we just pass in the peers as a ForwardNode or (PublicKey, u64)?
There was a problem hiding this comment.
Yeah, come to think of it that is probably cleaner. That would also allow the caller to use the compact hop representation only when desired. (e.g., in offer/refund but not in reply paths).
084d070 to
a74e84fCompare
TheBlueMatt
left a comment
There was a problem hiding this comment.
Looks like this needs a rebase.
| node_id: *node_id, | ||
| short_channel_id: peer.channel_by_id | ||
| .iter() | ||
| .find(|(_, channel)| channel.context().is_usable()) |
There was a problem hiding this comment.
Do we want to sort by oldest or biggest or something?
There was a problem hiding this comment.
Good idea! Oldest is probably best as this is only for onion messages, so we just want to make sure the channel announcement has been propagated in the gossip.
5d08b1f to
f3726a7Comparejkczyz
commented
May 13, 2024
Rebased |
TheBlueMatt
left a comment
There was a problem hiding this comment.
Basically LGTM, but IMO we really should make this optional somehow. It could happen in a followup if we want, cause we should also change how many hops we include based on if an offfer is long-lived.
| /// Attempts to a use a compact representation for the [`IntroductionNode`] by using a directed | ||
| /// short channel id from a channel in `network_graph` leading to the introduction node. | ||
| pub fn compact_introduction_node(&mut self, network_graph: &ReadOnlyNetworkGraph) { |
There was a problem hiding this comment.
Still, it does seem a bit strange that we're mutating the blinded path with a fn name that sounds like it could just as well be a straight getter.
| } | ||
| /// Attempts to a use a compact representation for the [`IntroductionNode`] by using a directed | ||
| /// short channel id from a channel in `network_graph` leading to the introduction node. |
There was a problem hiding this comment.
We should probably have some kind of discussion of how this makes paths shorter but if a channel closes will invalidate it.
| if let Some((scid, channel_info)) = node_info | ||
| .channels | ||
| .iter() | ||
| .find_map(|scid| network_graph.channel(*scid).map(|info| (*scid, info))) |
There was a problem hiding this comment.
Similar here, should we sort by size/age?
There was a problem hiding this comment.
Yeah, good catch. Using the block height from the scid as it seems the timestamp in the update is really a counter specific to the channel.
| .filter(|(_, peer)| peer.latest_features.supports_onion_messages()) | ||
| .map(|(node_id, peer)| ForwardNode { | ||
| node_id: *node_id, | ||
| short_channel_id: peer.channel_by_id |
There was a problem hiding this comment.
Should we make setting this optional somehow? I feel like if I'm building a super long-term offer I may have a different preference from something being scanned right now.
There was a problem hiding this comment.
Yeah... arguably we shouldn't bother with it for reply paths, either. Just not sure exactly how we want to convey it through the MessageRouter trait. Currently, the caller makes the decision for the penultimate hop using ForwardNode, but when adding more hops the MessageRouter makes the decision since it needs a NetworkGraph to find more hops. Similarly for the introduction node.
So right now it's partly an implementation concern given you need a NetworkGraph. I guess we can just add a bool parameter and it say it is best effort? Any other ideas?
There was a problem hiding this comment.
I see what you mean... if there's an obvious "default behavior" that stands out, we could have a separate method, e.g. create_long_term_blinded_paths, or have a Config struct. That way we could also have a config setting for compact offers vs offers that don't need to be QR-scanned, or privacy-oriented offers that want longer blinded paths.
There was a problem hiding this comment.
Opened #3080 with separate MessageRouter methods, using compact paths for Offer::paths and Refund::paths and non-compact paths for onion message reply paths. Basically, the trait allows either for the caller to decide.
As for how this is exposed in ChannelManager utilities, I'm not sure how we should handle short- vs long-lived offers. One thought was to chose the type of path based on the expiry. But for offers, the expiry is set by the user after the builder is returned and path already set. For refunds created via ChannelManager, we require an expiration (even though the spec does not), though, so we could infer there.
Alternatives would be:
- making a special purpose
create_offer_builderfor long-lived offers - adding a parameter to
create_offer_builderindicating if short- or long-lived - adding a
Optional absolute expiry parameter tocreate_offer_builderused to infer which type of path to create
Any preferences other alternatives?
There was a problem hiding this comment.
I'm good with whichever of those options you think is best!
| }, | ||
| }?; | ||
| for path in &mut paths { | ||
| path.compact_introduction_node(&network_graph); |
There was a problem hiding this comment.
Similar here, can we make this optional on a per-offer/message basis?
There was a problem hiding this comment.
#3080 refactors MessageRouter to have two different methods.
f3726a7 to
b295e98Compare
jkczyz
left a comment
There was a problem hiding this comment.
Oops, forgot to publish these comments.
| /// Attempts to a use a compact representation for the [`IntroductionNode`] by using a directed | ||
| /// short channel id from a channel in `network_graph` leading to the introduction node. | ||
| pub fn compact_introduction_node(&mut self, network_graph: &ReadOnlyNetworkGraph) { |
There was a problem hiding this comment.
Ok, added a use_ prefix. I can see how the docs make it less clear that compact was a verb in the name.
| if let Some((scid, channel_info)) = node_info | ||
| .channels | ||
| .iter() | ||
| .find_map(|scid| network_graph.channel(*scid).map(|info| (*scid, info))) |
There was a problem hiding this comment.
Yeah, good catch. Using the block height from the scid as it seems the timestamp in the update is really a counter specific to the channel.
| .filter(|(_, peer)| peer.latest_features.supports_onion_messages()) | ||
| .map(|(node_id, peer)| ForwardNode { | ||
| node_id: *node_id, | ||
| short_channel_id: peer.channel_by_id |
There was a problem hiding this comment.
Yeah... arguably we shouldn't bother with it for reply paths, either. Just not sure exactly how we want to convey it through the MessageRouter trait. Currently, the caller makes the decision for the penultimate hop using ForwardNode, but when adding more hops the MessageRouter makes the decision since it needs a NetworkGraph to find more hops. Similarly for the introduction node.
So right now it's partly an implementation concern given you need a NetworkGraph. I guess we can just add a bool parameter and it say it is best effort? Any other ideas?
valentinewallace
left a comment
There was a problem hiding this comment.
LGTM pending maybe making the compact encoding optional. Looks like it needs another rebase as well.
Uh oh!
There was an error while loading. Please reload this page.
2dd24ed to
7d85abdComparejkczyz
commented
May 14, 2024
Rebased now. Yeah, maybe best to do that in a follow-up as it may be a little more involved. Let me know if you have any thoughts on #3011 (comment). |
TheBlueMatt
commented
May 17, 2024
Feel free to squash and we can land this IMO. We should open an issue and block the next release on the followup. |
e7f5035 to
bb5dc03Comparejkczyz
commented
May 22, 2024
Rebased and opened #3080. Looking for feedback on the |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| T: secp256k1::Signing + secp256k1::Verification | ||
| > ( | ||
| &self, recipient: PublicKey, peers: Vec<PublicKey>, secp_ctx: &Secp256k1<T>, | ||
| &self, recipient: PublicKey, peers: Vec<message::ForwardNode>, secp_ctx: &Secp256k1<T>, |
There was a problem hiding this comment.
If we're going to split this into two methods, do we still need message::ForwardNode? Can't one method take pubkeys and the other take scids?
There was a problem hiding this comment.
We alwasys need the pubkeys when creating the blinded path, so we'd need to either make a new struct or use a tuple. It's kinda nice having it use an Option for short_channel_id, though. It makes it easy in ChannelManager::create_blinded_path to fallback to None if a usable channel can't be found.
bb5dc03 to
f0d81eaCompareUh oh!
There was an error while loading. Please reload this page.
When sending an onion message to a blinded path, the short channel id between hops isn't need in each hop's encrypted_payload since it is not a payment. However, using the short channel id instead of the node id gives a more compact representation. Update BlindedPath::new_for_message to allow for this.
Instead of passing Vec<PublicKey> to MessageRouter::crate_blinded_path, pass Vec<ForwardNode>. This way callers can include a short_channel_id for a more compact BlindedPath encoding.
Add a method to BlindedPath that given a network graph will compact the IntroductionNode as the DirectedShortChannelId variant. Call this method from DefaultMessageRouter so that Offer paths use the compact representation (along with reply paths). This leaves payment paths in Bolt12Invoice using the NodeId variant, as the compact representation isn't as useful there.
f0d81ea to
e4661feCompare
commented
May 28, 2024
Added derives to |
Add support for creating a compact
BlindedPath, which consists of:BlindedHop::encrypted_payloadIntroductionNode::DirectedShortChannelIdThe first is accomplished by specifying SCIDs when calling
BlindedPath::new_for_messageusing a newmessage::ForwardNodestruct. The second is through callingBlindedPath::use_compact_introduction_node. Both are called byDefaultMessageRouter.