BOLT 12 Offers message handling support #2294

jkczyz · 2023-05-15T03:46:49Z

Add support for handling BOLT 12 Offers messages and, more generally, replying to onion messages.

Add InvoiceError message
Expand OnionMessageContents with Offers messages
Parameterize OnionMessenger with an OffersMessageHandler trait
Parameterize OnionMessenger with an MessageRouter trait for routing replies
Support replies in CustomOnionMessageHandler and OffersMessageHandler

A follow-up (#2371) implements OffersMessageHandler for ChannelManager.

codecov-commenter · 2023-05-15T21:49:04Z

Codecov Report

Patch coverage: 75.88% and project coverage change: +1.07 🎉

Comparison is base (d78dd48) 90.48% compared to head (3a316cc) 91.56%.

❗ Current head 3a316cc differs from pull request most recent head 3fd6b44. Consider uploading reports for the commit 3fd6b44 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2294      +/-   ##
==========================================
+ Coverage   90.48%   91.56%   +1.07%     
==========================================
  Files         104      106       +2     
  Lines       53920    66738   +12818     
  Branches    53920    66738   +12818     
==========================================
+ Hits        48792    61106   +12314     
- Misses       5128     5632     +504

Impacted Files	Coverage Δ
lightning-background-processor/src/lib.rs	`86.54% <ø> (+3.96%)`	⬆️
lightning/src/onion_message/offers.rs	`8.33% <8.33%> (ø)`
lightning/src/ln/peer_handler.rs	`66.42% <33.33%> (+4.60%)`	⬆️
lightning/src/onion_message/packet.rs	`76.92% <66.66%> (+1.39%)`	⬆️
lightning/src/onion_message/messenger.rs	`87.43% <76.31%> (-3.32%)`	⬇️
lightning/src/offers/invoice_error.rs	`82.64% <82.64%> (ø)`
lightning/src/util/ser.rs	`85.06% <83.33%> (-0.02%)`	⬇️
lightning/src/onion_message/functional_tests.rs	`92.88% <90.24%> (-1.78%)`	⬇️
lightning/src/util/ser_macros.rs	`69.06% <100.00%> (-1.74%)`	⬇️

... and 27 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

dunxen

Checked the TLV types against spec and have done a brief review while just brushing up some of the spec again.

Just a nit so far. Will do more review tomorrow.

lightning/src/onion_message/packet.rs

carlaKC

New here - hopefully some of what I've said makes sense.

lightning/src/onion_message/offers.rs

lightning/src/onion_message/messenger.rs

carlaKC · 2023-05-18T17:36:17Z

lightning/src/onion_message/messenger.rs

+///
+/// [`OnionMessage`]: msgs::OnionMessage
+pub trait MessageRouter {
+	/// Returns a route for sending an [`OnionMessage`] to the given [`Destination`].


Make it clearer here that find_route should return only intermediate_hops to the destination?
I would expect a find_route call to also include the pubkey of the final node (or intro node), but when it's used in the next commit with send_custom_message it's expected to only return the intermediate hops.

Also, empty vector = no path?

Make it clearer here that find_route should return only intermediate_hops to the destination?

Add clarification to the docs.

I would expect a find_route call to also include the pubkey of the final node (or intro node), but when it's used in the next commit with send_custom_message it's expected to only return the intermediate hops.

Perhaps there's a better name than find_route or at very least a better name for what it is returning. Open to suggestions.

Also, empty vector = no path?

This would be for if there is a direct connection between sender and recipient.

find_hops?
Sounds like a task for a brewery though.

Could be get_intermediate_hops since that's the wording used in send_custom_message? But no strong feelings if the docs are updated.

Another option could be to pass in a new onion_message::Path (or similar name) into send_onion_message, consolidating the intermediate_nodes and destination parameters. Then find_path could return a Path. This'd be in-line with our payment apis.

@TheBlueMatt Would you be ok with that or will this make bindings difficult?

Discussed offline and at the review club. Renamed to find_path taking a Destination by value and returning a new OnionMessagePath struct. Also, updated OnionMessenger::send_custom_message to take this struct instead of the two parameters as suggested earlier.

Regarding naming, not sure if I should rename MessageRouter to OnionMessageRouter for consistency with the new struct and OnionMessageContents.

@TheBlueMatt Would you be ok with that or will this make bindings difficult?

Not sure why this would make the bindings difficult?

The concern was around having two items in different modules named Path.

Ah, yea, we can't do that for bindings, and really shouldnt do it for rust users either.

lightning/src/onion_message/functional_tests.rs

jkczyz

Thanks for the review!

lightning/src/onion_message/offers.rs

lightning/src/onion_message/packet.rs

jkczyz · 2023-05-19T15:54:32Z

lightning/src/onion_message/messenger.rs

+///
+/// [`OnionMessage`]: msgs::OnionMessage
+pub trait MessageRouter {
+	/// Returns a route for sending an [`OnionMessage`] to the given [`Destination`].


Make it clearer here that find_route should return only intermediate_hops to the destination?

Add clarification to the docs.

I would expect a find_route call to also include the pubkey of the final node (or intro node), but when it's used in the next commit with send_custom_message it's expected to only return the intermediate hops.

Perhaps there's a better name than find_route or at very least a better name for what it is returning. Open to suggestions.

Also, empty vector = no path?

This would be for if there is a direct connection between sender and recipient.

lightning/src/onion_message/messenger.rs

dunxen · 2023-05-22T10:09:01Z

lightning/src/onion_message/messenger.rs

+///
+/// [`OnionMessage`]: msgs::OnionMessage
+pub trait MessageRouter {
+	/// Returns a route for sending an [`OnionMessage`] to the given [`Destination`].


find_hops?
Sounds like a task for a brewery though.

lightning/src/onion_message/messenger.rs

valentinewallace

Seems like mostly trivial changes? Hope we can land this soon!

lightning/src/offers/invoice_error.rs

valentinewallace · 2023-05-22T15:15:36Z

lightning/src/offers/invoice_error.rs

+
+		let erroneous_field = match (erroneous_field, suggested_value) {
+			(None, None) => None,
+			(None, Some(_)) => None,


This seems like an error?

Yeah, though the spec is underspecified. I left a comment: https://github.com/lightning/bolts/pull/798/files#r1200840518

I wonder if we should just ignore erroneous_field and suggested_value since they are odd and not very useful at the moment.

FYI, I added an additional error here. Discussion from the spec meeting indicated the error fields are meant to be generic enough for future uses. So might as well support reading them at least.

lightning/src/onion_message/offers.rs

valentinewallace · 2023-05-22T15:31:46Z

lightning/src/onion_message/offers.rs

+		match Self::parse(tlv_type, bytes) {
+			Ok(message) => Ok(message),
+			Err(ParseError::Decode(e)) => Err(e),
+			Err(_) => Err(DecodeError::InvalidValue), // TODO: log ParseError?


I'd be in favor of logging 👍

Hmm... seems I'd have to update all the Readable / ReadableArgs impls back to wire::Message to take a logger to use here. Perhaps it would be better to make a new DecodeError variant wrapping the ParseError to avoid that? That was we could log in PeerManager. Let me know if you have a preference or another idea.

Went ahead and made a couple new DecodeError variants. PTAL

lightning/src/onion_message/packet.rs

valentinewallace · 2023-05-22T16:02:06Z

lightning/src/onion_message/functional_tests.rs

@@ -115,6 +122,12 @@ impl CustomOnionMessageHandler for TestCustomMessageHandler {
 	}
 }

+impl ResponseErrorHandler for TestCustomMessageHandler {


Why do we need this additional trait, instead of returning a Result from handle_custom_message? We could also add an event for failed OM handling.

The error happens after calling handle_custom_message when OnionMessenger attempts to send the reply. This avoid reentrancy when if instead the user was given a reference to OnionMessenger to directly send the reply.

Would it be reasonable to implement EventsProvider for Messenger and generate OM failure events there? Seems in line with what we do for outbound payment path failures

Possibly... @TheBlueMatt is there any concern here? I'd imagine we may want to avoid onion messages from an untrusted source filling our event queue at no cost. At very least the cost for an outbound payment is to tie up liquidity.

Discussed offline. For now we'll leave the trait but may want to have the ChannelManager implementation produce an event in the future.

lightning/src/onion_message/messenger.rs

alecchendev

Most of the stuff I was thinking to comment on while reviewing was already addressed by others so LGTM 👍

alecchendev · 2023-05-23T22:16:52Z

lightning/src/onion_message/messenger.rs

+						OnionMessageContents::Offers(_) => self.respond_with_onion_message(
+							response, path_id, reply_path, &*self.offers_handler
+						),
+						OnionMessageContents::Custom(_) => self.respond_with_onion_message(
+							response, path_id, reply_path, &*self.custom_handler
+						),


Can someone explain the &* here? I've been running into this more lately and haven't quite understood it. Also I notice if I remove the Sized restriction on OMH::Target: OffersMessageHandler + Sized--which is self.offers_handler's type--this gets a warning and I also don't really understand why. I get basic de/referencing and that implementing the Sized trait means having a known constant size at compile time but don't understand it's use/need in this context.

Can someone explain the &* here? I've been running into this more lately and haven't quite understood it.

self.custom_handler is a type implementing Deref where the Target implements CustomOnionMessageHandler. respond_with_onion_message wants a reference to a type implementing ResponseErrorHandler, which is a supertrait. It seems just calling deref() would have sufficed.

Also I notice if I remove the Sized restriction on OMH::Target: OffersMessageHandler + Sized--which is self.offers_handler's type--this gets a warning and I also don't really understand why. I get basic de/referencing and that implementing the Sized trait means having a known constant size at compile time but don't understand it's use/need in this context.

IIUC, since Deref::Target is ?Sized (i.e., optionally sized) and respond_with_onion_message takes a reference which is Sized, we need to restrict its uses to Sized types.

self.custom_handler is a type implementing Deref where the Target implements CustomOnionMessageHandler. respond_with_onion_message wants a reference to a type implementing ResponseErrorHandler, which is a supertrait. It seems just calling deref() would have sufficed.

Oh I see, okay. Reading up a bit more on Deref made me realize there was more happening than I realized. Although, I would've thought Rust would be able to do the type coercion for the supertrait here, hmph.

IIUC, since Deref::Target is ?Sized (i.e., optionally sized) and respond_with_onion_message takes a reference which is Sized, we need to restrict its uses to Sized types.

I tried to add ?Sized on the EH type in respond_with_onion_message and that worked too lol, so I guess for some reason Rust needs us to tell it if it's going to be optionally sized or sized, but I still don't really get why... It doesn't make a difference to the actual code here, so whatever, but weird.

lightning/src/onion_message/offers.rs

jkczyz

Whoops, forgot to send these comments.

jkczyz · 2023-05-24T19:22:28Z

fuzz/src/onion_message.rs

@@ -209,6 +207,9 @@ mod tests {
 			let log_entries = logger.lines.lock().unwrap();
 			assert_eq!(log_entries.get(&("lightning::onion_message::messenger".to_string(),
 						"Received an onion message with path_id None and no reply_path".to_string())), Some(&1));
+			// TODO: Add reply path to one_hop_om instead?


@valentinewallace Does this sound right? How was that input generated?

Sorry for the delay. I just wrote a test with hardcoded keys + printed out the encoded onion message, I think. Here's the branch I used if it helps: https://github.com/valentinewallace/rust-lightning/tree/2022-10-GEN-OM-FUZZ

Would be nice to get something that manages to reply, indeed.

Seeing a test failure when cherrypicking this code. I pinged @valentinewallace offline but may need to wait for timezones to catch up.

Looked into this. For the test where we're sending directly to a blinded path, we spuriously advance the blinded path on send because in fuzzing all the node pks are the same (so we think we're the intro node to the blinded path).

Thanks for helping track this down! I've updated the fuzzer such that each node uses a different public key and was able to generate updated onion messages. Added a separate commit for this.

Also, I updated the fuzzer to test the code path for responding given a reply path. The least intrusive way of doing this was to have the unblinded one-hop case include a one-hop blinded path back. This required modifying BlindedPath::new_for_message to allow such paths. This branch contains the needed modifications for updating the generated onion messages: https://github.com/jkczyz/rust-lightning/tree/2023-02-offer-flow-fuzz

jkczyz · 2023-05-24T19:23:29Z

lightning/src/onion_message/offers.rs

+		match Self::parse(tlv_type, bytes) {
+			Ok(message) => Ok(message),
+			Err(ParseError::Decode(e)) => Err(e),
+			Err(_) => Err(DecodeError::InvalidValue), // TODO: log ParseError?


Went ahead and made a couple new DecodeError variants. PTAL

jkczyz · 2023-05-24T20:09:12Z

lightning/src/offers/invoice_error.rs

+
+		let erroneous_field = match (erroneous_field, suggested_value) {
+			(None, None) => None,
+			(None, Some(_)) => None,


FYI, I added an additional error here. Discussion from the spec meeting indicated the error fields are meant to be generic enough for future uses. So might as well support reading them at least.

lightning/src/offers/invoice_error.rs

TheBlueMatt · 2023-05-26T17:57:51Z

lightning/src/ln/msgs.rs

@@ -79,6 +80,10 @@ pub enum DecodeError {
 	Io(io::ErrorKind),
 	/// The message included zlib-compressed values, which we don't support.
 	UnsupportedCompression,
+	/// A BOLT 12 Offers message had invalid semantics.
+	InvalidSemantics(offers::parse::SemanticError),


Isn't this what InvalidValue is for? Is there a reason we need to have an offers specific error variant in the super general purpose decode failure enum?

This is for logging purposes. See discussion at #2294 (comment).

I could instead make a more general variant wrapping a string or something like Box<dyn Error> to avoid constructing the string if the log level is not hit.

I could instead make a more general variant wrapping a string or something like Box to avoid constructing the string if the log level is not hit.

Sure, we could do something like that. I don't think our serialization framework should have offers-specific code/variants in it, but a String (or &'static str) in an error path seems okay. We also might consider including the (optional) specific TLV that failed to parse as Rusty wants to get that back in errors too now.

We also might consider including the (optional) specific TLV that failed to parse as Rusty wants to get that back in errors too now.

Hmm... is that something outside of InvoiceError? There the TLV in question is not necessarily about a parse failure (i.e., DecodeError) but rather some semantic error, FWIW.

Yea, its not specific to the BOLT12 stuff, we may want it generally, but the invoice_error message has a specific TLV that generated the error - presumably that should be filled in whether the TLV was invalid (eg too many bytes/invalid value) or if it was a semantic error. For that to work it has to be generic in DecodeError and parsing BOLT12 messages have to map InvoiceError to DecodeError.

lightning/src/onion_message/packet.rs

TheBlueMatt · 2023-05-26T18:08:22Z

lightning/src/onion_message/messenger.rs

+	///
+	/// [`OnionMessage`]: msgs::OnionMessage
+	fn find_path(
+		&self, sender: &PublicKey, destination: Destination


Does this need some kind of currently-connected-peers set?

Hmm... perhaps? Currently, this is called from OnionMessenger::respond_with_onion_message via OnionMessenger::handle_onion_message which is passed the id of the peer who delivered the message. So maybe that id is sufficient? Otherwise, I suppose we'd have to pass all the ids to handle_onion_message from PeerManager.

I assume we should - better to find a path with the live peer set rather than just guessing based on the public channels we have (which won't work for private nodes anyway?). I don't think we need to pass a "live" set from PeerManager, just maintaining a copy via peer_{dis,}connected in the messenger should suffice.

Ah, I missed that we are already maintaining this as the keys in OnionMessenger::pending_messages.

Updated find_path to take the peers.

TheBlueMatt · 2023-05-26T18:11:39Z

lightning/src/onion_message/offers.rs

+	L::Target: Logger,
+{
+	fn handle_message(&self, _message: OffersMessage) -> Option<OffersMessage> {
+		todo!()


I don't think we can land this with a panic in it? As it stands you can hook this up and just hit panics left and right. We should instead leave IgnoringMessageHandler as the only implementer.

Just removed the todo! as it was much simpler than changing SimpleArcOnionMessenger.

TheBlueMatt · 2023-05-26T18:12:02Z

lightning/src/onion_message/offers.rs

+	}
+}
+
+impl<M: Deref, T: Deref, ES: Deref, NS: Deref, SP: Deref, F: Deref, R: Deref, L: Deref>


I think we can use AChannelManager here rather than writing it out?

Wasn't aware of that, but it looks like it is meant to be test-only.

I think we can just make it public. We did the same for APeerManager.

Moved everything into channelmanager.rs given all the utilities are there in the next PR. Also, prevents needing to make a bunch of fields pub(crate) then.

Note that AChannelManager was made public yesterday in a separate PR, so its now already usable.

lightning/src/onion_message/messenger.rs

TheBlueMatt · 2023-05-26T18:19:46Z

lightning/src/onion_message/messenger.rs

+///
+/// Each message handler may choose to reply to an onion message with a response. This handler is
+/// used when an error occurs when responding.
+pub trait ResponseErrorHandler {


Hmm, is this API not maybe simpler by simply having the OfferMessageHandler take the router itself and it can figure it out from there? Returning a message to send, then getting an immediate callback that it failed seems like a confusing multi-step process that may require storing additional state, vs just getting that feedback from a router and knowing you failed doesn't.

Further, in the future we may want to use onion message routing as a "free boolean probe" - when sending an invoice_request we may use the normal router seeking a route for the amount we want to send and if the request fails we know we shouldn't pay over that route. That kinda implies the routing has to happen in the message handler itself rather than in the messenger/caller.

Hmm, is this API not maybe simpler by simply having the OfferMessageHandler take the router itself and it can figure it out from there? Returning a message to send, then getting an immediate callback that it failed seems like a confusing multi-step process that may require storing additional state, vs just getting that feedback from a router and knowing you failed doesn't.

Hmmm... that pushes the onus on the user to not only do the routing but also to send the message. So we'd have to pass the OnionMessenger, too. Or I suppose we could just pass the OnionMessenger so that the handler can call respond_with_onion_message, which would do the routing using the messenger's MessageRouter. We'd have to also pipe the peer(s) through, too.

Either way, it will expose all OnionMessenger's type parameters to the handler traits. 🙁

FYI, another reason I did it this way is so logging was within OnionMessenger.

Further, in the future we may want to use onion message routing as a "free boolean probe" - when sending an invoice_request we may use the normal router seeking a route for the amount we want to send and if the request fails we know we shouldn't pay over that route. That kinda implies the routing has to happen in the message handler itself rather than in the messenger/caller.

Not quite sure I follow how that would even work. Wouldn't the user parameterize with some custom MessageRouter if they want to do such a probe?

So handle_custom_message returns Option<Self::CustomMessage, OnionMessagePath> and then the messenger is just responsible for send_onion_message? Agree that would simplify things - eg if you can't find a route to the intro node, your handler can just give up.

IIUC, I think some form of ResponseErrorHandler would still be needed to notify if send_onion_message fails so that your handler knows what to do next?

Hmmm... that pushes the onus on the user to not only do the routing but also to send the message. So we'd have to pass the OnionMessenger, too. Or I suppose we could just pass the OnionMessenger so that the handler can call respond_with_onion_message, which would do the routing using the messenger's MessageRouter. We'd have to also pipe the peer(s) through, too.

We could still return the message to be sent, just a routed one rather than the contents only. You'd have to pass it a router but not the messenger itself.

FYI, another reason I did it this way is so logging was within OnionMessenger.

I'm not convinced this should be a goal - it costs nothing to add a logger to the downstream implementation (eg the ChannelManager will already have one), and if it leads to avoiding a lot of strange API it seems worth it?

Not quite sure I follow how that would even work. Wouldn't the user parameterize with some custom MessageRouter if they want to do such a probe?

Oh, I take that, back, this is unrelated. I was thinking about the case where a ChannelManager does the initial invoice_request sending - it would like to be able to pick the path based on its own existing Router, but it can via a manual-path message send.

More broadly, the current PR doesn't actually implement handle_response_error anywhere, and its not super clear to me what we can do with the response error but log anyway? Like, if we receive an invoice_request and fail to send the invoice back, it may be worth logging, but we can't actually do anything - the payment may still complete with a second invoice_request received.

FYI, another reason I did it this way is so logging was within OnionMessenger.

I'm not convinced this should be a goal - it costs nothing to add a logger to the downstream implementation (eg the ChannelManager will already have one), and if it leads to avoiding a lot of strange API it seems worth it?

The problems isn't with having a logger, but rather with implementations needing to write the code that logs in five places in respond_with_onion_message across however many handler traits there happen to be going forward.

More broadly, the current PR doesn't actually implement handle_response_error anywhere, and its not super clear to me what we can do with the response error but log anyway? Like, if we receive an invoice_request and fail to send the invoice back, it may be worth logging, but we can't actually do anything - the payment may still complete with a second invoice_request received.

Yeah, it was just going to log in the ChannelManager implementation, anyhow. I'm inclined to drop ResponseErrorHandler for now and keep the interface as is. We can always rethink it later if there is some concrete action the user could take.

I'm inclined to drop ResponseErrorHandler for now and keep the interface as is.

Yea, that makes sense toe me, then.

Removed ResponseErrorHandler.

TheBlueMatt · 2023-05-26T18:20:08Z

lightning/src/onion_message/messenger.rs

+///
+/// [`OnionMessage`]: msgs::OnionMessage
+pub trait MessageRouter {
+	/// Returns a route for sending an [`OnionMessage`] to the given [`Destination`].


@TheBlueMatt Would you be ok with that or will this make bindings difficult?

Not sure why this would make the bindings difficult?

jkczyz

FYI, I'm, going to squash the most recent fixups, otherwise subsequent ones will cause many merge conflicts.

lightning/src/onion_message/packet.rs

jkczyz · 2023-05-30T18:42:41Z

lightning/src/onion_message/messenger.rs

+	///
+	/// [`OnionMessage`]: msgs::OnionMessage
+	fn find_path(
+		&self, sender: &PublicKey, destination: Destination


Ah, I missed that we are already maintaining this as the keys in OnionMessenger::pending_messages.

jkczyz · 2023-05-30T18:49:48Z

lightning/src/ln/msgs.rs

@@ -79,6 +80,10 @@ pub enum DecodeError {
 	Io(io::ErrorKind),
 	/// The message included zlib-compressed values, which we don't support.
 	UnsupportedCompression,
+	/// A BOLT 12 Offers message had invalid semantics.
+	InvalidSemantics(offers::parse::SemanticError),


We also might consider including the (optional) specific TLV that failed to parse as Rusty wants to get that back in errors too now.

Hmm... is that something outside of InvoiceError? There the TLV in question is not necessarily about a parse failure (i.e., DecodeError) but rather some semantic error, FWIW.

lightning/src/onion_message/messenger.rs

TheBlueMatt

Conceptually LGTM with one further question about errors.

TheBlueMatt · 2023-06-01T20:17:18Z

lightning/src/ln/msgs.rs

@@ -79,6 +79,8 @@ pub enum DecodeError {
 	Io(io::ErrorKind),
 	/// The message included zlib-compressed values, which we don't support.
 	UnsupportedCompression,
+	/// The message had invalid semantics.
+	InvalidSemantics(String),


This doesn't suffice to generate an InvoiceError, we need the TLV field that failed, is that an issue? Presumably if we fail to read an InvoiceRequest/Invoice we need to be able to map the generic DecodeError (or SemanticError) back into an InvoiceError to send to the peer. I'm okay leaving it as a followup but I'm not entirely clear on the final vision - I assume we'd like to ultimately phase InvalidValue out entirely in favor of a InvalidX { human_readable: String, tlv: Option<u64> }? Can we open an issue and/or leave a comment here describing that? Also, I'm not clear on if this is a good first step towards that, given we don't actually use this yet, does it make more sense to just use InvalidValue for now and then move to something like the above in a followup?

Hmm... currently this variant is only for logging purposes when failing to parse a message because of invalid semantics. I was thinking InvoiceError would only be used after we have a semantically valid message but couldn't handle it for some other reason (e.g., erroring when verifying a message, building an invoice response, or paying an invoice). The spec isn't really clear on this and, to be honest, the TLV portion doesn't seem that well thought out, IMO. My plan was to only use the error message portion.

Also, note that InvoiceError is sent back via an onion message to the originator, not to our peer. We'd need to re-think our packet parsing / message handling code to handle this as you're suggesting (i.e., for DecodeError::InvalidValue) or at very least only parse the message as TLVs and move semantic checks to the handler (?) in order to reply with an InvoiceError. But then we need to expose those TLV types in the handlers where previously they were private.

Hmm... currently this variant is only for logging purposes when failing to parse a message because of invalid semantics.

Why does it need to be a DecodeError then? Can't the OnionMessageHandler log the bolt12::ParseError directly rather than munging it into a DecodeError and then logging in peer_handler?

I was thinking InvoiceError would only be used after we have a semantically valid message but couldn't handle it for some other reason (e.g., erroring when verifying a message, building an invoice response, or paying an invoice). The spec isn't really clear on this and, to be honest, the TLV portion doesn't seem that well thought out, IMO. My plan was to only use the error message portion.

Yea, I have no idea what Rusty was really thinking here - the whole "point to the bad TLV" thing isn't really an actionable field, everyone's just gonna implement by passing the human readable string to the user and calling it a day anyway 🤷. Honestly I'd be okay just never filling it in unless someone actually starts using it in a way that makes a difference.

Why does it need to be a DecodeError then? Can't the OnionMessageHandler log the bolt12::ParseError directly rather than munging it into a DecodeError and then logging in peer_handler?

Ah, so back in #2294 (comment), I think I may have mistaken Payload for Packet and thus thought the Logger would need to be piped through a handful of ReadableArg implementations. But OnionMessenger handler will directly decode the Payload using onion_utils::decode_next_untagged_hop. I should be able to pass the logger through there. Let me give that a try.

Right, that sounds good, thanks. Also feel free to rebase when you do so.

Fixed and rebased. Should be good for another look now.

lightning/src/onion_message/packet.rs

TheBlueMatt · 2023-06-01T20:30:54Z

lightning/src/onion_message/offers.rs

+	}
+}
+
+impl<M: Deref, T: Deref, ES: Deref, NS: Deref, SP: Deref, F: Deref, R: Deref, L: Deref>


Note that AChannelManager was made public yesterday in a separate PR, so its now already usable.

lightning/src/offers/invoice_error.rs

lightning/src/onion_message/functional_tests.rs

lightning/src/ln/channelmanager.rs

lightning/src/onion_message/messenger.rs

TheBlueMatt · 2023-06-05T18:07:29Z

fuzz/src/onion_message.rs

@@ -209,6 +207,9 @@ mod tests {
 			let log_entries = logger.lines.lock().unwrap();
 			assert_eq!(log_entries.get(&("lightning::onion_message::messenger".to_string(),
 						"Received an onion message with path_id None and no reply_path".to_string())), Some(&1));
+			// TODO: Add reply path to one_hop_om instead?


Would be nice to get something that manages to reply, indeed.

TheBlueMatt

The commits up to the last two basically LGTM, obv need some kind of resolution on the fuzzing stuff though.

lightning/src/onion_message/offers.rs

lightning/src/onion_message/messenger.rs

jkczyz · 2023-06-07T20:47:40Z

Added a fixup for addressing the test compilation errors under no-std.

jkczyz · 2023-06-08T18:22:21Z

lightning/src/onion_message/messenger.rs

+where
+	ES::Target: EntropySource,
+	NS::Target: NodeSigner,
+	L::Target: Logger + Sized,


Sized here is causing problems with a lightning-background-processor doc test with --features=futures that uses Arc<dyn lightning::util::logger::Logger + Send + Sync>.

error[E0277]: the size for values of type `(dyn Logger + Send + std::marker::Sync + 'static)` cannot be known at compilation time --> src/lib.rs:529:295 | 34 | ...MyChannelManager>, my_gossip_sync: Arc<MyGossipSync>, my_logger: Arc<MyLogger>, my_scorer: Arc<MyScorer>, my_peer_manager: Arc<MyPeerManager>) { | ^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time | = help: the trait `Sized` is not implemented for `(dyn Logger + Send + std::marker::Sync + 'static)` = note: required because of the requirements on the impl of `OnionMessageHandler` for `OnionMessenger<Arc<KeysManager>, Arc<KeysManager>, Arc<(dyn Logger + Send + std::marker::Sync + 'static)>, IgnoringMessageHandler, IgnoringMessageHandler>` note: required by a bound in `PeerManager` --> /Users/jkczyz/src/rust-lightning/lightning/src/ln/peer_handler.rs:683:15 | 683 | OM::Target: OnionMessageHandler, | ^^^^^^^^^^^^^^^^^^^ required by this bound in `PeerManager`

The problem seems to be that handle_onion_message calls methods that take &L and where L: Logger.

@TheBlueMatt I can fix this by having those methods take &L and where L: Deref, L::Target: Logger. Just seems weird to have a reference to a Deref. Alternatively, keeping L and using where L: Deref + Clone, L::Target: Logger works since both & and Arc implement Clone. Not sure what the canonical way of doing this is, though.

We can drop the sizing bounds, this patch compiles for me:

diff --git a/lightning/src/onion_message/messenger.rs b/lightning/src/onion_message/messenger.rs index c0d28500f..32e8bb09d 100644 --- a/lightning/src/onion_message/messenger.rs +++ b/lightning/src/onion_message/messenger.rs @@ -413,7 +413,7 @@ for OnionMessenger<ES, NS, L, MR, OMH, CMH> where ES::Target: EntropySource, NS::Target: NodeSigner, - L::Target: Logger + Sized, + L::Target: Logger, MR::Target: MessageRouter, OMH::Target: OffersMessageHandler + Sized, CMH::Target: CustomOnionMessageHandler + Sized, diff --git a/lightning/src/onion_message/offers.rs b/lightning/src/onion_message/offers.rs index f208fdda7..f82afdd61 100644 --- a/lightning/src/onion_message/offers.rs +++ b/lightning/src/onion_message/offers.rs @@ -91,7 +91,7 @@ impl Writeable for OffersMessage { } } -impl<L: Logger> ReadableArgs<(u64, &L)> for OffersMessage { +impl<L: Logger + ?Sized> ReadableArgs<(u64, &L)> for OffersMessage { fn read<R: Read>(r: &mut R, read_args: (u64, &L)) -> Result<Self, DecodeError> { let (tlv_type, logger) = read_args; if tlv_type == INVOICE_ERROR_TLV_TYPE { diff --git a/lightning/src/onion_message/packet.rs b/lightning/src/onion_message/packet.rs index 6f21e9fcd..f532ab585 100644 --- a/lightning/src/onion_message/packet.rs +++ b/lightning/src/onion_message/packet.rs @@ -203,7 +203,7 @@ impl<T: CustomOnionMessageContents> Writeable for (Payload<T>, [u8; 32]) { } // Uses the provided secret to simultaneously decode and decrypt the control TLVs and data TLV. -impl<H: CustomOnionMessageHandler, L: Logger> +impl<H: CustomOnionMessageHandler, L: Logger + ?Sized> ReadableArgs<(SharedSecret, &H, &L)> for Payload<<H as CustomOnionMessageHandler>::CustomMessage> { fn read<R: Read>(r: &mut R, args: (SharedSecret, &H, &L)) -> Result<Self, DecodeError> { let (encrypted_tlvs_ss, handler, logger) = args;

Thanks, that did the trick. Removed the other Sized bounds on the the block while I was at it.

valentinewallace

Will take one more glance tomorrow when the jetlag wears off more, but this looks solid

lightning/src/onion_message/messenger.rs

lightning/src/onion_message/packet.rs

valentinewallace · 2023-06-12T18:43:39Z

fuzz/src/onion_message.rs

+			assert_eq!(log_entries.get(&("lightning::onion_message::messenger".to_string(),
+						"Responding to onion message with path_id None".to_string())), Some(&1));
+			assert_eq!(log_entries.get(&("lightning::onion_message::messenger".to_string(),
+						"Failed responding to onion message with path_id None: TooFewBlindedHops".to_string())), Some(&1));


Why not use a valid reply path?

We have an assumption because of the secret key in do_test that the handling node is the second node (i.e., node[1]), so this is the only realistic reply path. In all the other test cases, the handling node isn't the recipient node, so it wouldn't trigger the responding code path.

Hoping we can keep this as is and just change the log assertion once we support one-hop blinded paths.

lightning/src/offers/invoice_error.rs

lightning/src/onion_message/messenger.rs

valentinewallace · 2023-06-13T17:20:04Z

lightning/src/onion_message/messenger.rs

+		match onion_utils::decode_next_untagged_hop(
+			onion_decode_ss, &msg.onion_routing_packet.hop_data[..], msg.onion_routing_packet.hmac,
+			(control_tlvs_ss, &*self.custom_handler, &*self.logger)
+		) {
 			Ok((Payload::Receive::<<<CMH as Deref>::Target as CustomOnionMessageHandler>::CustomMessage> {
 				message, control_tlvs: ReceiveControlTlvs::Unblinded(ReceiveTlvs { path_id }), reply_path,
 			}, None)) => {
 				log_info!(self.logger,


Pre-existing, but along the lines of some other comments ISTM we could be spammed by this log

Added a final commit for this.

TheBlueMatt · 2023-06-13T17:26:35Z

Feel free to squash!

jkczyz · 2023-06-13T17:39:52Z

Feel free to squash!

Done!

jkczyz · 2023-06-13T18:06:29Z

Ah, gonna rebase since previous CI failures look to be resolved by #2353.

If an InvoiceRequest or an Invoice delivered via an onion message cannot be handled, the recipient should reply with an InvoiceError if a reply path was given. Define the message and conversion from SemanticError.

In an upcoming commit, messages for BOLT 12 offers are read from the onion payload. Passing a logger allows for logging semantic errors when parsing the messages.

BOLT 12 Offers makes use of onion messages to request and respond with invoices. Add these types and an error type to OnionMessageContents along with the necessary parsing and encoding.

Add a trait for handling BOLT 12 Offers messages to OnionMessenger and a skeleton implementation of it for ChannelManager. This allows users to either provide their own custom handling Offers messages or rely on a version provided by LDK using stateless verification.

To avoid confusion in the upcoming MessageRouter trait, introduce an OnionMessagePath struct that wraps the intermediate nodes and the destination. Use this in OnionMessenger::send_onion_message.

Add a trait for finding routes for onion messages and parameterize OnionMessenger with it. This allows OnionMessenger to reply to messages that it handles via one of its handlers (e.g., OffersMessageHandler).

Modify onion message handlers to return an optional response message for OnionMessenger to reply with.

This will allow for testing onion message replies.

When generating onion message fuzz data, the same public key was used for each node. However, the code now advances the blinded path if the sender is the introduction node. Use different node secrets for each node to avoid this. Note that the exercised handling code is for the sender's immediate peer.

jkczyz force-pushed the 2023-05-onion-message-replies branch from da21b8d to 64007e7 Compare May 15, 2023 20:07

dunxen self-requested a review May 16, 2023 10:23

dunxen reviewed May 16, 2023

View reviewed changes

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

carlaKC reviewed May 18, 2023

View reviewed changes

jkczyz mentioned this pull request May 18, 2023

BOLT12 Offers Tracking Issue #1970

Open

60 tasks

jkczyz force-pushed the 2023-05-onion-message-replies branch from 64007e7 to 238b72e Compare May 19, 2023 15:57

jkczyz commented May 19, 2023

View reviewed changes

dunxen reviewed May 22, 2023

View reviewed changes

dunxen added the Review club label May 22, 2023

valentinewallace self-requested a review May 22, 2023 15:41

valentinewallace reviewed May 22, 2023

View reviewed changes

alecchendev reviewed May 23, 2023

View reviewed changes

dunxen removed the Review club label May 24, 2023

jkczyz force-pushed the 2023-05-onion-message-replies branch from 238b72e to 5db5cc2 Compare May 24, 2023 19:21

jkczyz commented May 24, 2023

View reviewed changes

jkczyz force-pushed the 2023-05-onion-message-replies branch from 5db5cc2 to f22cea5 Compare May 25, 2023 22:08

TheBlueMatt reviewed May 26, 2023

View reviewed changes

jkczyz commented May 30, 2023

View reviewed changes

jkczyz force-pushed the 2023-05-onion-message-replies branch from f22cea5 to 5a1a162 Compare June 1, 2023 02:09

TheBlueMatt reviewed Jun 1, 2023

View reviewed changes

jkczyz force-pushed the 2023-05-onion-message-replies branch from 5a1a162 to 68be6f7 Compare June 2, 2023 17:44

TheBlueMatt reviewed Jun 5, 2023

View reviewed changes

jkczyz force-pushed the 2023-05-onion-message-replies branch from 68be6f7 to c0b3380 Compare June 6, 2023 19:14

TheBlueMatt reviewed Jun 6, 2023

View reviewed changes

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

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

jkczyz force-pushed the 2023-05-onion-message-replies branch 2 times, most recently from 74e8559 to c1c8316 Compare June 7, 2023 20:46

jkczyz commented Jun 8, 2023

View reviewed changes

jkczyz force-pushed the 2023-05-onion-message-replies branch from c1c8316 to d85cccc Compare June 8, 2023 19:36

TheBlueMatt previously approved these changes Jun 8, 2023

View reviewed changes

valentinewallace reviewed Jun 12, 2023

View reviewed changes

jkczyz dismissed TheBlueMatt’s stale review via 14b1557 June 12, 2023 21:06

jkczyz force-pushed the 2023-05-onion-message-replies branch from 3a316cc to 14b1557 Compare June 12, 2023 21:06

valentinewallace previously approved these changes Jun 13, 2023

View reviewed changes

jkczyz dismissed valentinewallace’s stale review via 13be0a9 June 13, 2023 17:39

jkczyz force-pushed the 2023-05-onion-message-replies branch from 14b1557 to 13be0a9 Compare June 13, 2023 17:39

jkczyz added 15 commits June 13, 2023 13:07

Support WithoutLength for UntrustedString

1feddae

Add InvoiceError message

9b3a35a

If an InvoiceRequest or an Invoice delivered via an onion message cannot be handled, the recipient should reply with an InvoiceError if a reply path was given. Define the message and conversion from SemanticError.

Pass logger to onion payload decoder

7533a3c

In an upcoming commit, messages for BOLT 12 offers are read from the onion payload. Passing a logger allows for logging semantic errors when parsing the messages.

Onion message payload for BOLT 12 Offers

a799fc9

BOLT 12 Offers makes use of onion messages to request and respond with invoices. Add these types and an error type to OnionMessageContents along with the necessary parsing and encoding.

Avoid an unnecessary unwrap

cff88aa

Add OnionMessagePath wrapper struct

f15da3d

To avoid confusion in the upcoming MessageRouter trait, introduce an OnionMessagePath struct that wraps the intermediate nodes and the destination. Use this in OnionMessenger::send_onion_message.

MessageRouter trait for OnionMessenger

a71000f

Add a trait for finding routes for onion messages and parameterize OnionMessenger with it. This allows OnionMessenger to reply to messages that it handles via one of its handlers (e.g., OffersMessageHandler).

Support onion message replies in OnionMessenger

c17c2ae

Modify onion message handlers to return an optional response message for OnionMessenger to reply with.

Remove unnecessary Sized bound

c83d5bf

Split TestCustomMessage into Request and Response

ceb9038

This will allow for testing onion message replies.

Test onion message replies

fac5f4b

Fuzz test onion message replies

c16ecba

Reduce log level of potential noisy log statement

3fd6b44

jkczyz force-pushed the 2023-05-onion-message-replies branch from 13be0a9 to 3fd6b44 Compare June 13, 2023 18:08

TheBlueMatt approved these changes Jun 13, 2023

View reviewed changes

valentinewallace approved these changes Jun 13, 2023

View reviewed changes

TheBlueMatt merged commit 74a9ed9 into lightningdevkit:main Jun 13, 2023

notmandatory mentioned this pull request Jul 8, 2023

Socratic Seminar #9 Topics NashBitDevs/nashbitdevs.org#10

Closed

BOLT 12 Offers message handling support #2294

BOLT 12 Offers message handling support #2294

Conversation

jkczyz commented May 15, 2023 • edited Loading

codecov-commenter commented May 15, 2023 • edited Loading

Codecov Report

dunxen left a comment

Choose a reason for hiding this comment

carlaKC 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 May 23, 2023 • 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

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace 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

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

alecchendev 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

jkczyz 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

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 May 30, 2023 • 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

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

jkczyz commented May 15, 2023 •

edited

Loading

codecov-commenter commented May 15, 2023 •

edited

Loading

valentinewallace May 23, 2023 •

edited

Loading

TheBlueMatt May 30, 2023 •

edited

Loading