Compact BlindedPath representation for onion messaging

[jkczyz]

BOLT 12 defines a compact representation for BlindedPaths, which uses short_channel_id in place of next_node_id for use in an OnionMessage packet's hop data: lightning/bolts@ba2b95a

Additionally, the blinded path's introduction node can use a short channel id with a direction byte: lightning/bolts@3db064e

TODO:

• Create a BlindedPath that uses a directed scid introduction node Compact blinded path creation #3011
• Parse a BlindedPath that uses a directed scid introduction node
• Pay a BlindedPath that uses a directed scid introduction node
• Resolve a directed scid from a BlindedPath introduction node
• Create BlindedHop::encrypted_payload that use an scid as the next hop Compact blinded path creation #3011
• Parse ForwardTlvs that use an scid as the next hop
• Resolve an scid from a PeeledOnion::Forward

Related issues:

• Create an OnionMessage using a BlindedPath that has the sender as introduction node correctly Fix sender is the introduction node onion messages #2951
• Pay using a BlindedPath that has the payer as the introduction node

[jkczyz]

To forward such messages, OnionMessenger needs a mapping from short_channel_id to next_node_id. Its MessageRouter can be extended with a next_node_id method to support this. And the DefaultMessageRouter can use its NetworkGraph for the lookup, as implemented here: https://github.com/jkczyz/rust-lightning/tree/2024-03-compact-blinded-paths

However, this will not work for unannounced channels since they aren't in the NetworkGraph. Instead, data from ChannelManager is needed. There are a few different approaches that may be taken:

1. Extend OnionMessageHandler with channel_opened and channel_closed methods akin to peer_connected and peer_disconnected.
2. Include unannounced channels in NetworkGraph.
3. Define a new NodeIdLookUp trait (implemented for ChannelManager) and parameterize (a)OnionMessenger or (b) DefaultMessageRouter with it.
4. Extend ChannelMessageHandler with a method returning a mapping such that PeerManager can call it and pass the mapping to OnionMessageHandler::handle_onion_message and OnionMessageHandler::next_onion_message_for_peer (needed for advance_path_by_one).

I've been leaning towards either (3) or (4) as the other solutions don't seem ideal. But (3b) may not be very feasible as currently DefaultRouter contains a DefaultMessageRouter, leading to a circular reference with ChannelManager which contains a Router.

Another concern for either (3) or (4) is whether this mapping should be maintained inside ChannelManager explicitly, generated on demand, or simply use a O(n) search through all peers/channels. It can also be a fallback option if a NetworkGraph lookup (through a MessageRouter) fails for an unannounced channel to avoid needing it for announced channels. This assumes MessageRouter::next_node_id from the above-mentioned branch.

@valentinewallace @TheBlueMatt Any strong preference on these approaches or any alternative approach?

[valentinewallace]

I think I like (3a). That way we don't have to pass in a full mapping, there can just be an scid_to_node_id method on NodeIdLookup, and we can use ChannelManager::short_to_chan_info to be O(1). (Correct me if I'm missing something.)

Arguably could add the method to OffersMessageHandler since it does payment stuff? Maybe too hacky though.

[TheBlueMatt]

1. Bleh, something still has to call these, which kinda feels like we'd end up with 3 anyway,
2. No way, we'd end up screwing up and leaking the existence of these channels publicly
3. Bleh, but, I guess? (a) feels more natural because (b) feels like a separate concept - why is the router holding a reference to the ChannelManager? (a) is also not tooo bad because its already there in the form of the OffersMessageHandler, so we can either extend that trait or just add a new one but just have a second ref to the ChannelManager.
4. Certainly is the simplest solution, but my worry here is that we usually don't need these results but we have to go interrogate the ChannelManager and walk the channel map every time we receive an onion message, including allocating a Vec/HashMap of ID -> PublicKey mappings.

[jkczyz]

I think I like (3a). That way we don't have to pass in a full mapping, there can just be an scid_to_node_id method on NodeIdLookup, and we can use ChannelManager::short_to_chan_info to be O(1). (Correct me if I'm missing something.)

Nice, I didn't notice ChannelManager::short_to_chan_info. I thought we had something like that.

Arguably could add the method to OffersMessageHandler since it does payment stuff? Maybe too hacky though.

The issue with extending OffersMessageHandler is that we also need the lookup for stand-alone functions (e.g, peel_onion_message). So better to use a dedicated trait, IMO.

[jkczyz]

3. Bleh, but, I guess? (a) feels more natural because (b) feels like a separate concept - why is the router holding a reference to the ChannelManager? (a) is also not tooo bad because its already there in the form of the OffersMessageHandler, so we can either extend that trait or just add a new one but just have a second ref to the ChannelManager.
4. Certainly is the simplest solution, but my worry here is that we usually don't need these results but we have to go interrogate the ChannelManager and walk the channel map every time we receive an onion message, including allocating a Vec/HashMap of ID -> PublicKey mappings.

Note that for (4) we could combine the NodeIdLookup trait from (3) as an associated type on ChannelMessageHandler, which the new method would return. Then have the ChannelManager implementation use &Self for the associated type. That way we don't need to allocate unless needed. And in the case of ChannelManager, not at all given we have ChannelManager::short_to_chan_info as @valentinewallace noted.

It feels a little weird having such a method on ChannelMessageHandler. Maybe it's ok given there are other methods that don't handle messages?

[valentinewallace]

The issue with extending OffersMessageHandler is that we also need the lookup for stand-alone functions (e.g, peel_onion_message). So better to use a dedicated trait, IMO.

IMO peel_onion_message can just return an scid or node_id as needed, probably preferable to passing in another trait to it anyway.

[TheBlueMatt]

Note that for (4) we could combine the NodeIdLookup trait from (3) as an associated type on ChannelMessageHandler, which the new method would return. Then have the ChannelManager implementation use &Self for the associated type. That way we don't need to allocate unless needed. And in the case of ChannelManager, not at all given we have ChannelManager::short_to_chan_info as @valentinewallace noted.

That's....a lot of type indirection just to pass a function pointer to the OnionMessageHandler. Seems we should just do 3 over that still.

[jkczyz]

IMO peel_onion_message can just return an scid or node_id as needed, probably preferable to passing in another trait to it anyway.

Ah, are you suggesting having PeeledOnion::Forward contain a NextHop instead of a PublicKey?

What we do for create_onion_message which may call advance_path_by_one? I suppose we would want to make the scid on the second hop into an introduction node using an sciddir. But we need to look up the node id to know whether the direction byte should be zero or one, IIUC.

[valentinewallace]

What we do for create_onion_message which may call advance_path_by_one? I suppose we would want to make the scid on the second hop into an introduction node using an sciddir. But we need to look up the node id to know whether the direction byte should be zero or one, IIUC.

Yeah, it does look pretty annoying to adapt create_onion_message without passing in a trait object that supports the mapping :( So maybe a separate trait would be best. Not a huge fan of adding more trait bounds but 🤷‍♀️