Onion messages API #1432

valentinewallace · 2022-04-19T21:04:25Z

This PR's purpose is to gather initial feedback on the proposed onion messages API, both external and internal. After garnering ACKs to crystallize the docs, structs, and method signatures, it will be closed in favor of #1503 with filled in methods and tests.

Note that the initial PR will exclude some features: sending custom TLVs (receiving custom TLVs is supported), reply paths, and sending to blinded routes.

In a few cases, I included some small code-level changes if I thought they made it more obvious why an API change is being suggested.

TheBlueMatt

You know I'm gonna complain if I find any cases where we can drop a Vec and replace it without allocatiing. May be hard in a few cases but I see a lot of Vecs in message handling here :(

TheBlueMatt · 2022-04-20T02:37:46Z

lightning/src/ln/peer_handler.rs

+	/// object or an [`IgnoringMessageHandler`].
+	///
+	/// [`OnionMessager`]: crate::ln::onion_messages::OnionMessager
+	pub onion_message_handler: OM,


While you're here please move the custom message handler into this struct.

TheBlueMatt · 2022-04-20T13:05:20Z

lightning/src/ln/onion_message.rs

+
+/// A sender, receiver and forwarder of onion messages. In upcoming releases, this object will be
+/// used to retrieve invoices and fulfill invoice requests from offers.
+pub struct OnionMessager<Signer: Sign, K: Deref>


Should this be OnionMessager or InvoiceOnionMessager?

Will we still support sending arbitrary custom onion messages? I was thinking yes, so went with a general term

Ah, I wasn't sure quite what this struct was doing - I was thinking it was intended as an implementation of the onion message handling trait, handling invoice onion messages. I guess its more of an intermediary struct that handles p2p onion messages, decrypts them, then hands them off to various other handler traits that actually handle the messages?

I guess its more of an intermediary struct that handles p2p onion messages, decrypts them, then hands them off to various other handler traits that actually handle the messages?

That'd make sense to me. For inbound OMs, we'd have an InvoiceRequestHandler (this would be another object that marshalls an invoice and gives it back to the OnionMessager) and an InvoiceHandler (this would probably be ChannelManager under the hood, to pay the invoice?) And a CustomHandler for custom onion messages.

For outbound InvoiceRequests, OnionMessager could have a request_invoice method? Definitely need to think a bit here..

Feel like that should go out via whatever handles an invoice response, no? Like, you tell it to pay and it goes and requests the invoice and then receives the invoice and pays. That said, I don't think it should be the ChannelManager unless we need it to be. More like the InvoicePayer struct in lightning-invoice. It may be that we need to do the dependency-order swap now - make lightning depend on lightning-invoice and pull some of the trivial structs into lightning-invoice or into a lightning-datastructures crate.

Okay, so IIUC OnionMessenger will have a pointer to an InvoiceHandler that has a send_payment method that sends out an invoice_request and pays the invoice. I'm wondering, how does this InvoiceHandler communicate to the OnionMessenger when it has an invoice request it wants to send out?

lightning/src/ln/msgs.rs

lightning/src/ln/onion_utils.rs

lightning/src/util/ser_macros.rs

lightning/src/ln/onion_utils.rs

lightning/src/ln/onion_message.rs

TheBlueMatt · 2022-04-20T13:24:16Z

lightning/src/ln/msgs.rs

+pub struct OnionMessage {
+	/// The blinding point used in the shared secret that is used to decrypt the onion message's
+	/// `encrypted_data` field.
+	pub blinding_point: PublicKey,


Does this need to be a Result<PublicKey...? ie if we receive an onion message on the wire we shouldn't disconnect our peer just because the point is invalid (I mean our peers shouldnt do it, but still).

lightning/src/ln/onion_message.rs

jkczyz · 2022-05-17T17:20:16Z

lightning/src/ln/onion_message.rs

+	/// message.
+	///
+	/// [`encrypted_payload`]: BlindedNode::encrypted_payload
+	pub blinding_point: PublicKey,


Is this the same as the blinded_node_id of the first blinded hop?

No, the blinding_point is used as part of generating the shared secret that decrypts the EncryptedTlvs. The blinded_node_id is the hop's node_id multiplied by a blinding factor

lightning/src/ln/onion_message.rs

jkczyz · 2022-05-17T17:28:36Z

lightning/src/ln/onion_message.rs

+//   // Handles an incoming invoice error.
+//   fn handle_invoice_error(invoice_error: InvoiceError);
+//   // Sends a payment.
+//   fn send_payment(..);


High-level questions: What do you expect the parameters and intended use to be for this? Would handle_invoice call this?

This would take intermediate_ndoes, a Destination, and an amount. It would then send an invoice_request onion message (OM) to the destination, who would respond with an invoice OM, and then pay the invoice probably using an InvoicePayer under the hood.

Edit: not sure how this object is gonna communicate to its parent OnionMessenger yet, hmm.

jkczyz · 2022-05-17T17:31:48Z

lightning/src/ln/onion_message.rs

+
+	/// Send an empty onion message to `recipient`, routing it through `intermediate_nodes`.
+	// NOTE: sending custom TLVs and reply paths aren't included atm
+	pub fn send_onion_message(&self, recipient: Destination, intermediate_nodes: Vec<PublicKey>) -> Result<(), APIError> {}


I suppose routing would play a role in determining intermediate_nodes. Would it be a single-path route and do you need more than the PublicKey of each hop?

Yeah, single-path route. Only the PublicKey is needed!

jkczyz · 2022-05-17T17:41:59Z

lightning/src/ln/onion_message.rs

 	fn handle_onion_message(&self, peer_node_id: &PublicKey, msg: &msgs::OnionMessage) {
+		// calls:
+		// * onion_utils::decode_next_message_hop
+		// * onion_utils::next_hop_packet_pubkey
 	}


I take it users would typically just implement InvoiceHandler but we're giving the option to implement OnionMessageHandler to handle custom onion messages? Should we have a separate hook for that?

Also, is there a case where users would need custom logic outside of handling custom messages? Just wondering if it makes sense for users to ever do the decoding / forwarding. Maybe this akin to ChannelMessageHandler and ChannelManager, where the latter is almost always used.

Maybe this akin to ChannelMessageHandler and ChannelManager, where the latter is almost always used.

I think this is the case. I don't think users would want to do the decoding themselves, seems likely they'd only want to implement handling of custom onion messages if anything

TheBlueMatt

High level Concept ACK, few notes, but generally - aren't onion messages allowed to be more than 1300 bytes? IIRC they're allowed to be substantially large, too, implying we'll need to keep a Vec of the bytes and not try to shove many-KB onto the stack?

lightning/src/ln/msgs.rs

TheBlueMatt · 2022-05-17T20:06:39Z

lightning/src/ln/onion_message.rs

+// enum Message {
+// 	InvoiceRequest(InvoiceRequest),
+// 	Invoice(Invoice),
+//	InvoiceError(InvoiceError),


+CustomMessage<T>, hopefully :)

Does the T mean only one type of custom message will be allowed? Confused what that is

We do the same thing for P2P - basically we assume T will be another enum that implements all the various types that the user wants ie a "sub-enum".

TheBlueMatt · 2022-05-17T20:08:08Z

lightning/src/ln/onion_message.rs

+	// [`EncryptedTlvs`] for the hop when constructing the onion packet for sending.
+	//
+	// [`EncryptedTlvs`]: EncryptedTlvs
+	pub encrypted_payload: Vec<u8>,


I assume this will be a Message eventually?

These are supplied by the receiver, as the encrypted blobs of the blinded route, so they'll always be plain encrypted bytes I think?

Oh! Right, yea, okay. I guess we can't make them fixed-size? :(

I think their size can vary a lot especially with custom tlvs shoved in there :(

valentinewallace · 2022-06-04T22:07:57Z

Updated to the latest API/struct changes and reworked the commit history

Git makes it difficult to add parts of a new file unless the file is already tracked. This was the quickest workaround to split up adding onion_message.rs in parts

Blinded routes can be provided as destinations for onion messages, when the recipient prefers to remain anonymous. We also add supporting utilities for constructing blinded path keys, and a ControlTlvs struct representing blinded payloads prior to being encoded/encrypted. These utilities and struct will be re-used in upcoming commits for sending and receiving/forwarding onion messages.

We need to add a new Packet struct because onion message packet hop_data fields can be of variable length, whereas regular payment packets are always 1366 bytes.

This fills in the boilerplate needed to hook up the OnionMessenger to send and receive messages through the PeerManager. It also sets up the OnionMessenger and its struct fields.

This adds several utilities in service of then adding OnionMessenger::send_onion_message, which can send to either an unblinded pubkey or a blinded route. Sending custom TLVs and sending an onion message containing a reply path are not yet supported.

This required adapting `onion_utils::decode_next_hop` to work for both payments and onion messages. Currently we just print out the path_id of any onion messages we receive. In the future, these received onion messages will be redirected to their respective handlers: i.e. an invoice_request will go to an InvoiceHandler, custom onion messages will go to a custom handler, etc.

ariard · 2022-06-07T22:48:53Z

lightning/src/ln/onion_message.rs

+// 	InvoiceRequest(InvoiceRequest),
+// 	Invoice(Invoice),
+//	InvoiceError(InvoiceError),
+//	CustomMessage<T>,


Maybe don't forget to prefix like CustomOnionMessage we're already using CustomMessage name to design trait type requirement in wire.rs.

Similar to the usages of onion_utils:: and msgs:: across the codebase, the intent is for usages external to the onion_message module to utilize the onion_message:: prefix, e.g. onion_message::Packet. Does this address this comment and the below?

Yes, SGTM as the module serves as prefix.

ariard · 2022-06-07T22:52:06Z

lightning/src/ln/onion_message.rs

+pub(crate) const BIG_PACKET_HOP_DATA_LEN: usize = 32768;
+
+#[derive(Clone, Debug, PartialEq)]
+pub(crate) struct Packet {


Maybe it's only me but I would prefix everything with Onion{Packet, Payload, Message} even if it's already the onion message file and it sounds redundant. Really likely at some point we introduce ChannelPacket as an abstraction of both HTLC and PTLC so if we use Packet everywhere gonna be confusing..

I'm somewhat indifferent on this. Have heard arguments both ways. Internally, there is no conflict. Externally, you could always use as to rename the import or qualify with the module name, FWIW. Probably fine as is.

ariard · 2022-06-07T23:18:06Z

lightning/src/ln/onion_message.rs

+	/// sometimes user-provided custom "data" TLVs. See [`EncryptedTlvs`] for more information.
+	encrypted_tlvs: EncryptedTlvs,
+	// Coming soon:
+	// * custom TLVs


Hmmmmm I'm confused here with the presence of both custom TLVs and CustomMessage<T>. AFAIU your distinction between "data" TLVs that are expected to be final recipient stuff like invoice/invoice-request and control for more routing stuff can also overlap with "custom".

So I would say you can drop "custom TLV" either as if they're "data" they would go in Message::CustomMessage and if routing they would go in EncryptedTLVs (unless someone would like to provide non-encrypted custom control TLVs though not good for privacy)

Sure, removing this!

ariard · 2022-06-07T23:29:31Z

lightning/src/ln/onion_message.rs

+	secp_ctx: Secp256k1<secp256k1::All>,
+	// Coming soon:
+	// invoice_handler: InvoiceHandler,
+	// custom_handler: CustomHandler, // handles custom onion messages


Beyond invoice traffic, I would be interested to route on-chain transactions through the onion messages. Thinking more I would say this interface should work to implement such custom use-case.

Let's say you have TxOverOnionManager a struct implementing both BroadcasterInterface and CustomHandler.
broadcast_transaction is called, the transaction is given to TxOverOnion::Writer which yield back a TLV-serialized transaction as a Payload::CustomMessage<T>. It's blinded inside Packet and then make available to OnionMessenger through CustomHandler::get_custom_onion_msg().
And routed to the destination by send_onion_message().

At the receiver, the onion message is unblinded, the TLV record is detected as non-canonical and sent to CustomHandler::handle_custom_onion_message. If this trait implements Bitcoind, the transaction is published through sendrawtransaction.

Does it make sense ?

Without thinking too hard about the details, the high level concept of using OMs to relay an on-chain transaction to a tx broadcaster seems reasonable to me. However, unless the OMs are providing chain data too, it seems you'll still need a chain source, and that chain source is likely to provide broadcasting anyway?

Currently, broadcasting transactions in a privacy-preserving way is far from being a solved problem (e.g why Dandelion). OM offers an interesting alternative if your chain source is a lightclient or bitcoind with a poor tx-relay topology. Further, it could also serve as a tx-relay redundancy mechanism if your bitcoind or lightclient tx-relay peers topology are eclipsed, or we discover a transaction censorship vulnerability in Core tx-relay stack.

And yes one step further would be to relay block headers with your connected peers (e.g see LND's lightningnetwork/lnd#5621). I don't think we need to use OM for them as blocks are public data contrary to transactions.

ariard · 2022-06-09T23:29:29Z

Concept ACK on the high-level API. There is still this comment unsolved about the Custom API, though I guess we won't get it right before we have some actually trying to implement it for a custom use-case.

jkczyz

Concept ACK. Will move on to reviewing #1518 and #1503.

For documentation, I'd say having the bulk it as module-level docs for onion_message describing the public structs and use by other modules, cross-referencing as needed, should be fine.

jkczyz · 2022-06-16T20:42:10Z

lightning/src/ln/onion_message.rs

+/// Onion messages have "control" TLVs and "data" TLVs. Control TLVs are used to control the
+/// direction and routing of an onion message from hop to hop, whereas data TLVs contain the onion
+/// message content itself.
+pub(crate) enum ControlTlvs {


nit: Consider ordering these structs outside-in instead of inside-out. The former lends to a more natural top-down reading order (i.e., the step-down rule) while the latter is a relic of C/C++ where an identifier needs to be declared before referenced.

Hmm, I'm a bit confused which ones aren't already outside-in? Packet>Payload>EncryptedTlvs>ControlTlvs is outside-in

Ah, I was reviewing by commit, so this was when looking at a79d741. I'd expect the order there to go BlindedRoute > BlindedNode and assumed that ControlTlvs was a lower-level abstraction since it was pub(crate), so would follow.

Anyhow, that order you have is fine modulo flipping BlindedRoute and BlindedNode. Though I'd consider moving the pub types upfront.

I tried to do something like this in https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/routing/scoring.rs, but not sure how others feel about defining types together in groups and have impl blocks for each group follow. I'm sure there's room for author opinion here.

lightning/src/ln/onion_message.rs

jkczyz · 2022-06-16T20:50:11Z

lightning/src/ln/onion_message.rs

+/// Construct keys for constructing a blinded route along the given `unblinded_path`.
+///
+/// Returns: `(encrypted_tlvs_keys, blinded_node_ids)`
+/// where the encrypted tlvs keys are used to encrypt the blinded route's blinded payloads and the
+/// blinded node ids are used to set the [`blinded_node_id`]s of the [`BlindedRoute`].
+fn construct_blinded_route_keys<T: secp256k1::Signing + secp256k1::Verification>(


Maybe name as create_blinded_route_keys and document as Creates ... to avoid double use of "construct".

Hmm, both methods do basically the same thing IMO. I think the symmetry is good/intentional?

By "double", I meant "Construct" and "constructing" in the documentation (not across types). So I'd say used create_ in both names. But feel free to keep as-is if construct_ is common already. Figured create_ is fairly standard but we probably haven't been consistent.

Ah, these are consistent with the analogous construct_*_keys method names in onion_utils. Searching the codebase, it seems that construct_* tends to be used for creating internal intermediate structs, and create_* tends to be used for creating user-facing things like channels and invoices.

Rephrasing to "Construct keys for creating a blinded route" for now, but no strong opinion

jkczyz · 2022-06-16T21:01:24Z

lightning/src/ln/onion_message.rs

+
+/// Used to construct the blinded hops portion of a blinded route. These hops cannot be identified
+/// by outside observers and thus can be used to hide the identity of the recipient.
+pub struct BlindedNode {


Could we call this BlindedHop to be consistent with our Route / RouteHop terminology?

Good idea! Doing so

jkczyz · 2022-06-16T21:17:49Z

lightning/src/ln/onion_message.rs

+pub(crate) const BIG_PACKET_HOP_DATA_LEN: usize = 32768;
+
+#[derive(Clone, Debug, PartialEq)]
+pub(crate) struct Packet {


I'm somewhat indifferent on this. Have heard arguments both ways. Internally, there is no conflict. Externally, you could always use as to rename the import or qualify with the module name, FWIW. Probably fine as is.

jkczyz · 2022-06-16T21:20:25Z

lightning/src/ln/onion_message.rs

+	/// We don't want to disconnect a peer just because they provide a bogus public key, so we hold a
+	/// Result instead of a PublicKey as we'd like.
+	pub(crate) public_key: Result<PublicKey, secp256k1::Error>,


Haven't looked at the full context of where this is constructed / used, but could that return a Err if the public key is bad?

Here's the context: #1432 (comment) lmk if more clarification is desired

valentinewallace · 2022-06-22T17:43:55Z

I believe I've incorporated everything except #1432 (review) (and possibly rearranging the order of onion_messages.rs) on #1503, so closing this and marking 1503 as ready for review. Thanks for all the conceptual review here.

valentinewallace requested a review from jkczyz April 19, 2022 21:04

valentinewallace force-pushed the 2022-04-onion-msgs-api branch from 82e433b to 8245c31 Compare April 19, 2022 21:13

TheBlueMatt added the Seeking Conceptual Review label Apr 19, 2022

ariard self-assigned this Apr 20, 2022

TheBlueMatt reviewed Apr 20, 2022

View reviewed changes

ariard reviewed Apr 21, 2022

View reviewed changes

lightning/src/ln/onion_message.rs Outdated Show resolved Hide resolved

lightning/src/ln/onion_message.rs Outdated Show resolved Hide resolved

valentinewallace force-pushed the 2022-04-onion-msgs-api branch 2 times, most recently from 100d4c3 to 179adca Compare May 17, 2022 00:53

jkczyz reviewed May 17, 2022

View reviewed changes

TheBlueMatt reviewed May 17, 2022

View reviewed changes

valentinewallace force-pushed the 2022-04-onion-msgs-api branch from 179adca to 2278768 Compare May 18, 2022 02:31

valentinewallace mentioned this pull request Jun 4, 2022

Onion messages v1 #1503

Merged

6 tasks

valentinewallace force-pushed the 2022-04-onion-msgs-api branch from 2278768 to d2653d6 Compare June 4, 2022 21:56

valentinewallace added 6 commits June 4, 2022 15:29

Add onion_messages module

1b9bd8f

Git makes it difficult to add parts of a new file unless the file is already tracked. This was the quickest workaround to split up adding onion_message.rs in parts

Add onion_messages::Packet and adapt relevant onion util

0e184cb

We need to add a new Packet struct because onion message packet hop_data fields can be of variable length, whereas regular payment packets are always 1366 bytes.

Add empty OnionMessenger, OnionMessageHandler trait, and boilerplate

6daa533

This fills in the boilerplate needed to hook up the OnionMessenger to send and receive messages through the PeerManager. It also sets up the OnionMessenger and its struct fields.

Support sending onion messages

e3ececc

This adds several utilities in service of then adding OnionMessenger::send_onion_message, which can send to either an unblinded pubkey or a blinded route. Sending custom TLVs and sending an onion message containing a reply path are not yet supported.

valentinewallace force-pushed the 2022-04-onion-msgs-api branch from d2653d6 to 6916934 Compare June 4, 2022 22:30

ariard reviewed Jun 7, 2022

View reviewed changes

jkczyz reviewed Jun 16, 2022

View reviewed changes

valentinewallace closed this Jun 22, 2022

Onion messages API #1432

Onion messages API #1432

Conversation

valentinewallace commented Apr 19, 2022 • edited Loading

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace May 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Jun 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard commented Jun 9, 2022 • edited Loading

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Jun 22, 2022 • edited Loading

valentinewallace commented Apr 19, 2022 •

edited

Loading

valentinewallace Apr 29, 2022 •

edited

Loading

valentinewallace May 18, 2022 •

edited

Loading

ariard Jun 9, 2022 •

edited

Loading

ariard commented Jun 9, 2022 •

edited

Loading

valentinewallace Jun 21, 2022 •

edited

Loading

valentinewallace commented Jun 22, 2022 •

edited

Loading