New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Compact blinded path handling #2961
Compact blinded path handling #2961
Conversation
Mostly looking for high-level feedback before moving on to the creation methods. |
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 #2961 +/- ##
==========================================
- Coverage 89.38% 89.37% -0.01%
==========================================
Files 117 117
Lines 95622 96720 +1098
Branches 95622 96720 +1098
==========================================
+ Hits 85468 86443 +975
- Misses 7924 8059 +135
+ Partials 2230 2218 -12 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few really quick comments, can do a more careful review later but at a high level it seems fine.
Remind me why we decided to do a separate trait from MessageRouter
? I'm fine with that, just don't recall why that decision was made.
/// [`OnionMessage`]: crate::ln::msgs::OnionMessage | ||
pub trait NodeIdLookUp { | ||
/// Returns the node if of the channel counterparty with `short_channel_id`. | ||
fn next_node_id(&self, short_channel_id: u64) -> Option<PublicKey>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this need a Direction
? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could provide one, but this is only for forwarding so we know the origin (us!). Maybe it would be useful for some remote lookup though? Or if we implement this for ReadOnlyNetworkGraph
(see other comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I totally missed the context here. Maybe lets include an "our channel's counterparty" in the docs just so people as dense as me can't miss it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added "forwarding node's " as "our" can be ambiguous, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still kinda find the trait and method docs not 100% clear that this is specific to us. Maybe add a second sentence to one of them that says like "ie usually our peers"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Matt here
It feels like it isn't immediately clear that the forwarding node in this context means "our" node.
Maybe a small cue in the docs might help that indicates such some way.
@@ -318,15 +321,21 @@ where | |||
ES::Target: EntropySource, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ISTM we should implement the lookup on DefaultMessageRouter
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentioned in an earlier comment, but it would only work for announced channels. Thus, we need to a way to use the mapping in ChannelManager
.
We could give DefaultMessageRouter
a reference to ChannelManager
, but we start getting into circular references. This is because ChannelManager
has a DefaultRouter
which has a DefaultMessageRouter
in order to construct blinded paths for offers without adding another type parameter to ChannelManager
. Any thoughts on how to re-work this are welcome, though.
The trait is only needed for forwarding onion messages, but it may need to be used for finding the next node id for an unannounced channel. For example, a user constructs a two-hop blinded path in an offer where the last hop in the blinded path uses the unannounced channel with their LSP. When forwarding an invoice request for the offer, the LSP's |
Should we restrict blinded paths in response_paths to require that they include already-resolved introduction points and not allow resolution in response_paths? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
/// hops. | ||
/// | ||
/// This is not exported to bindings users as lifetimes are not expressable in most languages. | ||
pub source_node_id: &'a NodeId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like if we don't use a reference for source_node_id
, we can avoid cloning first_channels
a few times down below. I feel like I might be missing some tradeoff here though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I think we want to avoid putting 33-bytes in this struct, thus why we try to use references. (cc: @TheBlueMatt)
Regarding the clones, fortunately the Vec
only stores references, so we aren't cloning ChannelDetails
just allocating a new Vec
holding their references. If the find_route
took RouteParameters
by value, we could do a first pass to resolve IntroductionNode::DirectedShortChannelId
to IntroductionNode::NodeId
. So to do that with the current interface, we'd need to clone RouteParameters
. Not sure if that's any better. 🙁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, we're super size-sensitive on CandiateRouteHop
, sadly. Because BlindedPathCandidate
is one of the smaller entries it may be that we could get away with 33 bytes here, but lets leave it as a reference for now initially.
@@ -614,7 +619,7 @@ where | |||
let our_node_id = node_signer.get_node_id(Recipient::Node) | |||
.map_err(|()| SendError::GetNodeIdFailed)?; | |||
if introduction_node_id == our_node_id { | |||
advance_path_by_one(blinded_path, node_signer, &secp_ctx) | |||
advance_path_by_one(blinded_path, node_signer, node_id_lookup, &secp_ctx) | |||
.map_err(|()| SendError::BlindedPathAdvanceFailed)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SendError::BlindedPathAdvanceFailed
docs could use an update after this. Looks like we could possibly get rid of that variant and return a SendError
from advance_path
but I didn't check too closely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean by get rid of the variant. What variant would we return here and possibly in advance_path_by_one
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
advance_path_by_one
has several different errors that it returns, and at a glance it looked like they may be covered by existing SendError
variants, e.g. next_hop_pubkey
failure is covered by SendError::Secp256k1
. But as I said I didn't look too closely so a docs update for BlindedPathAdvanceFailed
is also fine by me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, we'd need NodeSigner::ecdh
to return SendError::Secp256k1
. But the KeysManager
implementation returns ()
when given a Recipient::PhantomNode
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think any errors returned by the signer need to be a bit more general than Secp256k1
to cover the case where we have a remote signer that times out, or indeed the phantom case. One option would be to replace SendError::GetNodeIdFailed
with a more generic SignerFailed
variant, but that's not related to this PR at all so probably not worth it here.
Hmmm... I'm not sure if we can assume that. Should it really matter though? We'll hit |
If our goal is to only allow graph lookups for user-initiated actions, rather than actions initiated by others, we shouldn't be doing lookups in response to inbound messages. |
Don't we need to do a graph lookup anyway to determine if a direct connection is needed? And when onion messages are more ubiquitous, a more complex |
Errr, I'm lost. Sorry lol. Indeed, it doesn't matter. |
match &self.introduction_node { | ||
IntroductionNode::NodeId(pubkey) => { | ||
let node_id = NodeId::from_pubkey(pubkey); | ||
network_graph.nodes().get_key_value(&node_id).map(|(key, _)| key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its super confusing that introduction_node_id
could fail spuriously just because the introduction node_id isn't in the graph. Can we rename it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you care if I make it a method on ReadOnlyNetworkGraph
instead? I'd like to keep the name short unless you have a better alternative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure that that solves it. Can we just call it publicly_reachable_introduction_node_id
? Or even just public_introduction_node_id
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name sounds good, but honestly, I got a bit confused when I first read its name as it wasn't immediately clear to me what it does.
I think we should update the function docs, to clearly explain that public in this context means "publicly reachable" node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
lightning/src/routing/router.rs
Outdated
return Err(LightningError{err: "Cannot generate a route to blinded paths if we are the introduction node to all of them".to_owned(), action: ErrorAction::IgnoreError}); | ||
} | ||
for (_, blinded_path) in route_hints.iter() { | ||
if blinded_path.blinded_hops.len() == 0 { | ||
return Err(LightningError{err: "0-hop blinded path provided".to_owned(), action: ErrorAction::IgnoreError}); | ||
} else if &blinded_path.introduction_node_id == our_node_pubkey { | ||
} else if blinded_path.introduction_node_id(network_graph) == Some(&our_node_id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be caching the output here so we don't repeat graph lookups?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing!
let introduction_node_id = match blinded_path.introduction_node { | ||
IntroductionNode::NodeId(pubkey) => pubkey, | ||
IntroductionNode::DirectedShortChannelId(..) => { | ||
return Err(SendError::UnresolvedIntroductionNode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we're just using this to check if we're the introduction node, so we should be able to use the node_id_lookup
, no? We'd have to check the direction flag here to decide if we're the intro but that seems doable. It seems a bit strange to fail here in a public function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, good call.
97d71d6
to
139b3f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still working down through the commit, but everything looks great so far!
Just wanted to quickly write down a possible simplification I found in the code.
lightning/src/blinded_path/mod.rs
Outdated
let (node_one, node_two) = if node_a < node_b { | ||
(node_a, node_b) | ||
} else { | ||
(node_b, node_a) | ||
}; | ||
match self { | ||
Direction::NodeOne => node_one, | ||
Direction::NodeTwo => node_two, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can simplify this function by using min, and max. Also, I think it will good idea to use them here considering the wording we have used in the Direction
Enum.
pub enum Direction {
/// The lesser node id when compared lexicographically in ascending order.
NodeOne,
/// The greater node id when compared lexicographically in ascending order.
NodeTwo,
}
let (node_one, node_two) = if node_a < node_b { | |
(node_a, node_b) | |
} else { | |
(node_b, node_a) | |
}; | |
match self { | |
Direction::NodeOne => node_one, | |
Direction::NodeTwo => node_two, | |
} | |
} | |
match self { | |
Direction::NodeOne => core::cmp::min(node_a, node_b), | |
Direction::NodeTwo => core::cmp::max(node_a, node_b), | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
139b3f9
to
b96d442
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks great! A few more small improvements, otherwise things look good!
if let Destination::BlindedPath(path) = self { | ||
let introduction_node = &mut path.introduction_node; | ||
if let IntroductionNode::DirectedShortChannelId(direction, scid) = introduction_node { | ||
let pubkey = network_graph | ||
.channel(*scid) | ||
.map(|c| match direction { | ||
Direction::NodeOne => c.node_one, | ||
Direction::NodeTwo => c.node_two, | ||
}) | ||
.and_then(|node_id| node_id.as_pubkey().ok()); | ||
if let Some(pubkey) = pubkey { | ||
*introduction_node = IntroductionNode::NodeId(pubkey); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have already defined public_introduction_node_id
in BlindedPath. We can utilize it here to keep this function clean.
if let Destination::BlindedPath(path) = self { | |
let introduction_node = &mut path.introduction_node; | |
if let IntroductionNode::DirectedShortChannelId(direction, scid) = introduction_node { | |
let pubkey = network_graph | |
.channel(*scid) | |
.map(|c| match direction { | |
Direction::NodeOne => c.node_one, | |
Direction::NodeTwo => c.node_two, | |
}) | |
.and_then(|node_id| node_id.as_pubkey().ok()); | |
if let Some(pubkey) = pubkey { | |
*introduction_node = IntroductionNode::NodeId(pubkey); | |
} | |
} | |
} | |
if let Destination::BlindedPath(path) = self { | |
if let Some(pubkey) = path | |
.public_introduction_node_id(network_graph) | |
.and_then(|node_id| node_id.as_pubkey().ok()) | |
{ | |
path.introduction_node = IntroductionNode::NodeId(pubkey); | |
} | |
} | |
On another note, router.rs
defines NodeId
, and IntroductionNode
also has a variant NodeId
. But these seem to be incompatible with each other, which doesn’t seem to me like ideal behavior.
Maybe we can think about IntroductionNode::NodeId
taking NodeId
as the field value, or maybe we can define an explicit conversion between them. Let me know what you think about this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have already defined
public_introduction_node_id
in BlindedPath. We can utilize it here to keep this function clean.
Good point, though we should only do this for the DirectedShortChannelId
. If we do it for the NodeId
variant then it could fail for an unannounced node even if the node is a peer.
On another note,
router.rs
definesNodeId
, andIntroductionNode
also has a variantNodeId
. But these seem to be incompatible with each other, which doesn’t seem to me like ideal behavior. Maybe we can think aboutIntroductionNode::NodeId
takingNodeId
as the field value, or maybe we can define an explicit conversion between them. Let me know what you think about this!
So generally, a node id is a pubkey. NodeId
is simply a convenience type used to avoid PublicKey
's costly verification logic while deserializing a NetworkGraph
(see #1107). That said, there should already be conversion functions on NodeId
(e.g., the as_pubkey
method used here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do it for the NodeId variant then it could fail for an unannounced node even if the node is a peer.
Right! That makes sense!
That said, there should already be conversion functions on NodeId (e.g., the as_pubkey method used here).
Yep, that's true!
Initially, I was wondering if instead of (gossip.rs) NodeId <-> pubkey <-> IntroductionNode::NodeId
, we can have a direct conversion between (gossip.rs) NodeId <-> IntroductionNode::NodeId
.
But now I think about it, it would just be introducing extra complexity for not a lot of pay-off
Thanks a bunch for your thorough response!
NextHop::ShortChannelId(scid) => match self.node_id_lookup.next_node_id(scid) { | ||
Some(pubkey) => pubkey, | ||
None => { | ||
log_trace!(self.logger, "Dropping forwarded onion message to unresolved peer using SCID {}", scid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Maybe we can rephrase it a little.
log_trace!(self.logger, "Dropping forwarded onion message to unresolved peer using SCID {}", scid); | |
log_trace!(self.logger, "Dropping forwarded onion message: Unable to resolve next peer using SCID {}", scid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
match &self.introduction_node { | ||
IntroductionNode::NodeId(pubkey) => { | ||
let node_id = NodeId::from_pubkey(pubkey); | ||
network_graph.nodes().get_key_value(&node_id).map(|(key, _)| key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name sounds good, but honestly, I got a bit confused when I first read its name as it wasn't immediately clear to me what it does.
I think we should update the function docs, to clearly explain that public in this context means "publicly reachable" node.
b96d442
to
bd1b7ee
Compare
Grr, this needs a rebase. |
352ad8a
to
565e456
Compare
Rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, LGTM modulo a few small nits left. Feel free to squash.
/// [`OnionMessage`]: crate::ln::msgs::OnionMessage | ||
pub trait NodeIdLookUp { | ||
/// Returns the node if of the channel counterparty with `short_channel_id`. | ||
fn next_node_id(&self, short_channel_id: u64) -> Option<PublicKey>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still kinda find the trait and method docs not 100% clear that this is specific to us. Maybe add a second sentence to one of them that says like "ie usually our peers"?
lightning/src/blinded_path/mod.rs
Outdated
Direction::NodeOne => core::cmp::min(node_a, node_b), | ||
Direction::NodeTwo => core::cmp::max(node_a, node_b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this comparison is equivalent (and PublicKey
doesn't seem to even impl Cmp
is newer libsecp)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow here. Why isn't equivalent? Note also that PublicKey
does implement Ord
, which is what allows us to use comparisons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PublicKey
's Ord
implementation compares libsecp's internal representation (I believe the full X/Y without serializing) and is thus different from the NodeId
comparison. Newer versions of the secp256k1
crate have removed the Ord
impl because its confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. Was assuming the comment was more about min
/max
. I'll covert to NodeId
.
Allow either using a node id or a short channel id when forwarding an onion message. This allows for a more compact representation of blinded paths, which is advantageous for reducing offer QR code size. Follow-up commits will implement handling the short channel id case as it requires looking up the destination node id.
Useful for applying Option::map when needing both the key and value for when needing a reference to the key with a longer lifetime.
Blinded paths use a pubkey to identify the introduction node, but it will soon allow using a directed short channel id instead. Add an introduction_node_id method to BlindedPath to facilitate lookup in the latter case.
af613c1
to
ba13326
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo one small nit! 🚀
/// [`OnionMessage`]: crate::ln::msgs::OnionMessage | ||
pub trait NodeIdLookUp { | ||
/// Returns the node if of the channel counterparty with `short_channel_id`. | ||
fn next_node_id(&self, short_channel_id: u64) -> Option<PublicKey>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Matt here
It feels like it isn't immediately clear that the forwarding node in this context means "our" node.
Maybe a small cue in the docs might help that indicates such some way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your turn @valentinewallace
/// | ||
/// Returns [`SendError::UnresolvedIntroductionNode`] if: | ||
/// - `destination` contains a blinded path with an [`IntroductionNode::DirectedShortChannelId`], | ||
/// - unless it can be resolved by [`NodeIdLookUp::next_node_id`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is true as long as the intro node is us? If its not us we'll still fail I think.
@@ -1863,17 +1888,19 @@ where L::Target: Logger { | |||
} | |||
}, | |||
Payee::Blinded { route_hints, .. } => { | |||
if route_hints.iter().all(|(_, path)| &path.introduction_node_id == our_node_pubkey) { | |||
if introduction_node_id_cache.iter().all(|introduction_node_id| *introduction_node_id == Some(&our_node_id)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be worth spending some time fuzzing the router changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do, was already planning on it pre-release.
Blinded paths currently contain a node id for the introduction node. However, a compact representation will allow a directed short channel id instead. Update BlindedPathCandidate and OneHopBlindedPathCandidate to store a NodeId reference from either the NetworkGraph or from the user- provided first hops. This approach avoids looking up the introduction node id on demand, which may not be resolvable. Thus, that would require returning an Option from CandidateRouteHop::source and handle None accordingly.
Allow using either a node id or a directed short channel id in blinded paths. This allows for a more compact representation of blinded paths, which is advantageous for reducing offer QR code size. Follow-up commits will implement handling the directed short channel id case in OnionMessenger as it requires resolving the introduction node in MessageRouter.
When OnionMessenger creates an OnionMessage to a Destination, the latter may contain an IntroductionNode::DirectedShortChannelId inside a BlindedPath. Resolve these in DefaultMessageRouter and handle unresolved ones in OnionMessenger.
When forwarding onion messages, the next node may be represented by a short channel id instead of a node id. Parameterize OnionMessenger with a NodeIdLookUp trait to find which node is the next hop. Implement the trait for ChannelManager for forwarding to channel counterparties. Also use this trait when advancing a blinded path one hop when the sender is the introduction node.
Add a version of the create_onion_message utility function that attempts to resolved the introduction node of the destination's BlindedPath using a ReadOnlyNetworkGraph`. Otherwise, if given a path using the compact representation, using create_onion_message would fail. Keep the current version for use internally and for external uses where the blinded path is known to be resolved.
fe114ef
ba13326
to
fe114ef
Compare
Blinded paths may be specified using a more compact representation, replacing pubkeys with short channel ids as described in #2921. This PR implements this in the following manner:
BlindedPath
andmessage::ForwardTlvs
to support the compact representationNodeIdLookUp
trait used inOnionMessenger
to handle forwardingNodeIdLookUp
forChannelManager
IntroductionNode
:NetworkGraph
inDefaultMessageRouter
NetworkGraph
or first hops inDefaultRouter
Does not yet include any tests as the creation methods need to be updated still.