Skip to content
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 representation for node id #1138

Closed
wants to merge 2 commits into from

Conversation

thomash-acinq
Copy link

To save space in offers (#798) that need to fit on a QR code, we can identify public nodes by one of their public channel, using only 9 bytes instead of 33.
This was suggested by @jkczyz in 3db064e, I'm adding using it in the next_node_id field inside blinded routes.
Using it in the next_node_id field is needed for nodes that rely on trampoline and do not know the network graph. When such a node is given a compact blinded route, it can't find the corresponding public key for the introduction node id and needs to rely on its trampoline provider to do it.

01-messaging.md Outdated Show resolved Hide resolved
@@ -468,7 +468,7 @@ may contain the following TLV fields:
* [`short_channel_id`:`short_channel_id`]
1. type: 4 (`next_node_id`)
2. data:
* [`point`:`node_id`]
* [`sciddir_or_pubkey`:`node_id`]
Copy link
Collaborator

@t-bast t-bast Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewers: this is the important part of that PR. This is a breaking change. But since nobody actually uses onion messages right now, we can afford to make that breaking change now (and avoid introducing a feature bit just for this).

This is necessary for nodes who want to send onion messages to blinded paths without an access to the graph data to resolve an sciddir into a node_id. The drawback is that intermediate nodes in an onion message path may need to do that resolution (but it's just a map lookup so it shouldn't be a DoS vector?).

Also, it removes this small chunk from the Bolt 12 PR and thus makes it slightly smaller!

We're curious to know what you all think @rustyrussell @TheBlueMatt @jkczyz @valentinewallace

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I mean I still really hate having to look at the network graph to find an onion message first hop, but it saves enough space in the offer that its quite nice. I'm not sure if we want to do this as a general matter in payment blinded paths, though, maybe we split that out and say that any payment blinded paths can't use sciddir here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the rationale in the PR description with regards to trampoline. Didn't we include ba2b95a in #798 for this purpose? The solution in this PR uses an extra byte per hop compared to that commit, IIUC.

Copy link
Collaborator

@t-bast t-bast Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description of the PR isn't very clear, let me try to detail that a bit more. Alice is a mobile wallet user who wants to pay an offer from Bob containing a blinded path that uses an sciddir for the introduction node. Alice doesn't have access to the graph, so she cannot resolve the sciddir to figure out the introduction node of the blinded path. Alice needs to send an onion message to that blinded path and relies on her LSP for that, so the only thing she can do is create an onion message that uses the following route:

Alice ---> LSP -- (sciddir) --> IntroductionNode -- (blinded) --> Bob

When creating her onion message path, the only value she can put in the outgoing_node_id of the payload for her LSP is the introduction node's sciddir, and let the LSP resolve that to a node_id. Does that make sense?

Didn't we include ba2b95a in #798 for this purpose?

This is different, because here the LSP and the introduction node aren't necessarily connected (and Alice cannot check that because she doesn't have the graph data). An scid would thus not be enough, because the LSP wouldn't know which node of that channel is the target.

Thinking about this more, wouldn't it make more sense to drop ba2b95a entirely if we change the outgoing_node_id TLV to contain an sciddir_or_pubkey? In the case where the nodes are connected by that channel, we could save 1 byte by ommitting the direction, but the simplicity of not having to handle another case is probably worth it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I mean I still really hate having to look at the network graph to find an onion message first hop, but it saves enough space in the offer that its quite nice.

To be honest, I'm really not a fan of the sciddir idea in the first place, I find it really annoying to have to resolve it to a node_id...I'm not even convinced that the 24 bytes we're saving are worth the extra complexity, but it looks like LDK and CLN think it's worth it, so we're just following along.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work without a network graph for the case when Alice wants to be paid? Do the blinded paths in her offer simply use the LSP as the introduction node?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the introduction node, Alice can choose any node to which she knows a path. Even without knowing the full network graph that includes herself and all the nodes she has a channel with.
To use the compact representation, she only needs to know one public channel of the introduction node. In case Alice only has private channels and only knows her own channels, then she can't use this compact representation, she would need her LSP to give her a compact node id (that would require a new message type that would only be useful between a wallet and the LSP).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, sorry, I brain-farted. We did discuss this on the call two days ago, however, I'm still not a fan of this - I don't like introducing an explicit requirement that forwarding onion messages may require a network graph lookup. Some nodes (AFAIU? maybe not anymore?) don't keep the graph in memory and having to do a disk hit to forward an onion message seems unacceptable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You already need a network graph lookup today when using next_node_id. If you don't already have a channel with the next node, you need to query the network graph to get the IP address of the next node.
The way I see it, when a node advertises that they will relay onion messages you should assume that they will relay it only if they have a channel with the next node. Some nodes will open a connection to the next node if they are not already connected (LSP will do that for their customers) but most nodes will just drop the message. To do that you don't need the network graph.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nodes absolutely should not be opening a new connection to forward an onion message. If you don't have a connection with the next-hop you should drop the onion message, as you describe. In that case, there's also no need to do a network graph lookup.

Co-authored-by: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>
@ProofOfKeags
Copy link
Contributor

Is this purely a space saving procedure? Is there any other benefit? If not, how consequential is it that we're saving 24 bytes? 🤔

@jkczyz
Copy link
Contributor

jkczyz commented Feb 26, 2024

Is this purely a space saving procedure? Is there any other benefit? If not, how consequential is it that we're saving 24 bytes? 🤔

Yes, for space. But it's per hop / introduction node and times 8/5 to account for bech32 encoding.

@t-bast
Copy link
Collaborator

t-bast commented Feb 27, 2024

Is this purely a space saving procedure? Is there any other benefit?

No, this isn't just for space saving, it's mostly for nodes who don't have access to the graph data (see the comment above and the PR description).

Unless you're talking more generally about the sciddir_or_pubkey feature that is introduced in 3db064e? This was indeed added for space saving in minimal invoices, and while it is saving space, I must admit that it is quite annoying to support on the implementation side...but the space savings may be worth it 🤷‍♂️

@t-bast
Copy link
Collaborator

t-bast commented Mar 26, 2024

Closing as discussed during the last two spec meetings. We don't want to impose a graph lookup on every node relaying onion messages: this is something that service providers can offer to their wallet if necessary, by reading a node_id as an scid_dir_or_pubkey instead (as long as wallets only use this when going through their service provider).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants