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.

Sign up for GitHub

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

Jump to bottom

Compact representation for node id #1138

Closed
wants to merge 2 commits into from
Closed

Compact representation for node id #1138

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7add56a
Compact representation for node id
thomash-acinq Feb 14, 2024
8287b02
Update 01-messaging.md
thomash-acinq Feb 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions 01-messaging.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,12 @@ The following convenience types are also defined:
* `signature`: a 64-byte bitcoin Elliptic Curve signature
* `point`: a 33-byte Elliptic Curve point (compressed encoding as per [SEC 1 standard](http://www.secg.org/sec1-v2.pdf#subsubsection.2.3.3))
* `short_channel_id`: an 8 byte value identifying a channel (see [BOLT #7](07-routing-gossip.md#definition-of-short-channel-id))
* `sciddir_or_pubkey`: either 9 or 33 bytes referencing or identifying a node, respectively
* if the first byte is 0 or 1, then an 8-byte `short_channel_id` follows for a total of 9 bytes
* 0 for the first byte indicates this refers to `node_id_1` in the `channel_announcement` for `short_channel_id`
* 1 for the first byte indicates this refers to `node_id_2` in the `channel_announcement` for `short_channel_id`
(see [BOLT #7](07-routing-gossip.md#the-channel_announcement-message)
thomash-acinq marked this conversation as resolved.
Show resolved Hide resolved
* if the first byte is 2 or 3, then the value is a 33-byte `point`
* `bigsize`: a variable-length, unsigned integer similar to Bitcoin's CompactSize encoding, but big-endian. Described in [BigSize](#appendix-a-bigsize-test-vectors).

## Setup Messages
Expand Down
4 changes: 2 additions & 2 deletions 04-onion-routing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

1. type: 6 (`path_id`)
2. data:
* [`...*byte`:`data`]
Expand Down Expand Up @@ -1476,7 +1476,7 @@ even, of course!).

1. subtype: `blinded_path`
2. data:
* [`point`:`first_node_id`]
* [`sciddir_or_pubkey`:`first_node_id`]
* [`point`:`blinding`]
* [`byte`:`num_hops`]
* [`num_hops*onionmsg_hop`:`path`]
Expand Down