Support custom onion messages #1748

valentinewallace · 2022-09-30T14:34:41Z

In ser_macros, we now allow decode_tlv_stream to decode 1 custom TLV within a
specified range of TLV types. We also need to fail to mid-deserialization if there is
more than 1 TLV in that specified range, otherwise peers could waste our
processing time.

This change is needed to support offers and async payment onion messages, for
the above peers-can-waste-cpu reason.

TODO: more docs, ~~fix fuzz~~

jkczyz · 2022-09-30T15:52:11Z

Continuing from our offline discussion yesterday, given for offer messages we need the bytes when parsing anyhow, can't we leave ser_macros.rs as is and just read each possible message as Vec<u8>? Should be cheap to read a sequence of bytes if we aren't parsing them.

jkczyz · 2022-09-30T15:55:17Z

Ah, I guess for custom message support we may need something like this. Though, given the above, wasting CPU shouldn't be a concern.

valentinewallace · 2022-09-30T16:01:00Z

Continuing from our offline discussion yesterday, given for offer messages we need the bytes when parsing anyhow, can't we leave ser_macros.rs as is and just read each possible message as Vec<u8>? Should be cheap to read a sequence of bytes if we aren't parsing them.

In the context of:

We also need to fail to deserialize if there is
more than 1 TLV in that specified range, otherwise peers could waste our
processing time.

My impression is that peers could still waste a significant amount of our processing time/heap if we don't satisfy this requirement 🤷‍♀️. This sorta makes sense to me because it could end up being a lot of vec allocs even if we just read bytes (and if we're reading a sufficient quantity of bytes, that could still add up?). But yeah, 100% also custom OMs.

jkczyz · 2022-10-03T15:56:49Z

Thinking about this more, this actually prevents the more general case (outside of onion messages) where there can be more than one custom type in a TLV stream. Given we aren't later going to parse these messages unless the code actually understands the type, I think we only want to pass supported custom types to the macro (rather than a range). This would limit what actually gets parsed.

Otherwise, in the more general case, a malicious sender could still provide a message with many custom types. For onion message payloads, we may still want to configure them as Vec<u8> at least for offer messages since we need the raw bytes. So the number of allocations is limited to the number of supported custom types. The question then is about configuring payload parsing with user-supported custom types.

valentinewallace · 2022-10-03T16:33:03Z

Otherwise, in the more general case, a malicious sender could still provide a message with many custom types.

We'll fail mid-deserialization if more than one type in the given range is set, so thankfully this shouldn't be an issue.

I'm a bit confused how we'd support user-provided custom types if we only parse types we understand? I think it's OK to punt the general case of multiple custom TLVs (I'm not clear on whether we ever plan to support this) but open to ideas here.

jkczyz · 2022-10-03T18:42:12Z

Otherwise, in the more general case, a malicious sender could still provide a message with many custom types.

We'll fail mid-deserialization if more than one type in the given range is set, so thankfully this shouldn't be an issue.

I was referring to the more general case (not onion messages) where having multiple custom types is possible. We don't want decode_tlv_stream to be specific to onion message's semantics.

I'm a bit confused how we'd support user-provided custom types if we only parse types we understand?

To be more precise, by "we" I meant some LDK component parameterized with a user-supplied handler. Currently, we have the decoder pass all the types LDK supports to decode_tlv_stream. I'm suggesting to additionally have the decoder directly use LDK's parameterized custom handler to parse any types it supports. Compare this to your draft proposal which instead passes to the custom handler any TLV record (even if just one in the onion message case) and having it decide if it knows how to decode the type. That said, it may be a difficult to accomplish the alternative with macros.

For send_onion_message, you would give it some T: Writeable instead of a Tlv.

I think it's OK to punt the general case of multiple custom TLVs (I'm not clear on whether we ever plan to support this) but open to ideas here.

My hesitancy here is that it is adding onion message semantics to decode_tlv_stream.

lightning/src/onion_message/functional_tests.rs

ariard · 2022-10-03T18:45:12Z

lightning/src/util/ser_macros.rs

@@ -175,7 +175,8 @@ macro_rules! decode_tlv {
 }

 macro_rules! decode_tlv_stream {
-	($stream: expr, {$(($type: expr, $field: ident, $fieldty: tt)),* $(,)*}) => { {
+		($stream: expr, {$(($type: expr, $field: ident, $fieldty: tt)),* $(,)*}
+		 $(, ($custom_type: tt, $custom_value: ident, $custom_range_min: expr, $custom_range_max: expr))?) => { {


Catching up with the conv, IIUC each custom TLV inside the onion would have min/max bounds defined at the protocol-level ? Otherwise if DoS protections are per-implementation, you might fail deser of a message gauged by valid as the sending implementation. I still have in mind the use-case of sending Lightning transactions inside onions, and a commitment transaction size can dynamic on the number of HTLC outputs.

IIUC each custom TLV inside the onion would have min/max bounds defined at the protocol-level ?

I don't think the bounds are spec'd for custom TLVs, but yup.

I still have in mind the use-case of sending Lightning transactions inside onions, and a commitment transaction size can dynamic on the number of HTLC outputs.

Cool, that should be supported. Currently we support reading 1 TLV within the type range 64..u64::max_value() (can be an invoice_request, etc or custom). Lmk if there's a change desired here.

IIUC each custom TLV inside the onion would have min/max bounds defined at the protocol-level ?

IIUC the closest spec is that the generalized TLV format defines that types between 2^16 and u64::max_value() are defined as custom types.
https://github.com/lightning/bolts/blob/16973e2b857e853308cafd59e42fa830d75b1642/01-messaging.md?plain=1#L131

valentinewallace · 2022-10-03T23:41:07Z

I'm suggesting to additionally have the decoder directly use LDK's parameterized custom handler to parse any types it supports. Compare this to your draft proposal which instead passes to the custom handler any TLV record (even if just one in the onion message case) and having it decide if it knows how to decode the type. That said, it may be a difficult to accomplish the alternative with macros.

For send_onion_message, you would give it some T: Writeable instead of a Tlv.

Ah, I think I see what you're saying. To restate: in this scheme, we would be initializing each known type (including the custom type) as None and reading one of them, failing to deserialize if we read a known type when any of the other known types have already been read?

The one concern I have is that T: Writeable doesn't give us the TLV's type number. And they may support reading multiple custom types, so the types would have to be passed in as a list somehow and individually initialized/checked for. May be surmountable though.

I think it's OK to punt the general case of multiple custom TLVs (I'm not clear on whether we ever plan to support this) but open to ideas here.

My hesitancy here is that it is adding onion message semantics to decode_tlv_stream.

We could have a separate decode_onion_message_tlv_stream and DRY with a decode_tlv_stream_inner, not sure if that addresses what you're thinking. I guess I don't see how the changes prevent multiple custom TLV support in the future

jkczyz · 2022-10-04T16:00:36Z

Ah, I think I see what you're saying. To restate: in this scheme, we would be initializing each known type (including the custom type) as None and reading one of them, failing to deserialize if we read a known type when any of the other known types have already been read?

Sorta. I'm just saying to treat custom types like standard types since they should be known at compile-time, even if configured by the user. Then at the onion-message layer, enforce that there can only be one.

Given that, I'm not really convinced there should be a concern with excessive heap allocations for onion message payloads since the number of allocations would be be limited to known types assuming each are parsed into Vec<u8>.

The one concern I have is that T: Writeable doesn't give us the TLV's type number.

Yeah, would need to either pass a type or do something like wire::Message and have T further bound by a trait defining the type as a constant.

And they may support reading multiple custom types, so the types would have to be passed in as a list somehow and individually initialized/checked for. May be surmountable though.

Yeah, I think calling decode_tlv_stream with known custom types would be the trickiest part.

We could have a separate decode_onion_message_tlv_stream and DRY with a decode_tlv_stream_inner, not sure if that addresses what you're thinking. I guess I don't see how the changes prevent multiple custom TLV support in the future

Ah, sorry. I guess "prevents" was a bit too strong or phrased poorly. I meant custom message support as written can't be reused for the more general case of multiple custom types. So we would have to add another param to decode_tlv_stream (and more complexity) later. Whereas, if decode_tlv_stream supported the general case instead and handled the onion message semantics (i.e., only one type) higher up, then it could be re-used without needing to add more complexity.

TheBlueMatt · 2022-10-06T17:52:46Z

We discussed this offline and concluded that we should first attempt replacing the in-range-read macro update with a lambda and see how that works.

ViktorTigerstrom

Started reviewing this yesterday before the comment about first proceeding with the in-range-read macro. I'm leaving some comments in case there's value in them once this is picked up again :).

ViktorTigerstrom · 2022-10-06T15:39:21Z

lightning/src/onion_message/packet.rs

@@ -188,8 +193,15 @@ impl ReadableArgs<SharedSecret> for Payload {
 				Ok(Payload::Forward(ForwardControlTlvs::Unblinded(tlvs)))


Hmmm I think the spec is a bit unclear here, but as the spec specifically states:
"Field numbers 64 and above are reserved for payloads for the final hop."
Given that phrasing, would we want this to fail and return an Err if message_type is set to an even number and it's a Payload::Forward?

Given the discussion about custom TLV types in this PR, and that this PR adds support custom types for the Payload "data" TLVs, maybe it's also worth adding here that the onion message test vectors also includes custom tags/types inside the encrypted_tlv_stream (the type 4 here aka our "control" TLVs). They're present for both Bob's (non-final hop) and Dave's (final hop) payloads. Would we like this PR to also update our ControlTlvs to support such types, in order to be able to use the ControlTlvs enums to represent that for the test vectors, or is that out of scope for this PR as those aren't the actual onion message?

would we want this to fail and return an Err if message_type is set to an even number and it's a Payload::Forward?

Good catch, adding this!

Re (2) I didn't quite get that, are you asking if we plan to support users sending custom control TLVs? I believe that'd be out-of-scope for now

ViktorTigerstrom · 2022-10-06T22:24:31Z

fuzz/src/onion_message.rs

 		}

-		let two_unblinded_hops_om = "020000000000000000000000000000000000000000000000000000000000000e01055600020000000000000000000000000000000000000000000000000000000000000e0135043304210200000000000000000000000000000000000000000000000000000000000000039500000000000000000000000000000058000000000000000000000000000000000000000000000000000000000000001204105e00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000b300000000000000000000000000000000000000000000000000000000000000";
+		let two_unblinded_hops_om = "020000000000000000000000000000000000000000000000000000000000000e01055600020000000000000000000000000000000000000000000000000000000000000e0135043304210200000000000000000000000000000000000000000000000000000000000000029500000000000000000000000000000036000000000000000000000000000000000000000000000000000000000000003604104b000000000000000000000000000000fd1092202a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000b200000000000000000000000000000000000000000000000000000000000000";


OT: Would we also like the payloads for the fuzzing to contain padding to better align with the spec? I could look into that for the add TLV padding PR that I haven't pushed yet (It's about time that I push it, so I'll do it asap).

Ah, yeah when padding is added the fuzz vectors will need to be updated again (since presumably most/all OMs will contain padding for at least one hop, which then changes the hmac of the payload). I can send you the test harness I set up to generate these vectors when the PR is open :) (no rush on opening it tho)

Prevents unresolved import error

codecov-commenter · 2022-10-07T19:37:38Z

Codecov Report

Base: 90.72% // Head: 92.08% // Increases project coverage by +1.35% 🎉

Coverage data is based on head (4905268) compared to base (7bc52aa).
Patch coverage: 87.12% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1748      +/-   ##
==========================================
+ Coverage   90.72%   92.08%   +1.35%     
==========================================
  Files          87       87              
  Lines       46688    57396   +10708     
  Branches    46688    57396   +10708     
==========================================
+ Hits        42360    52853   +10493     
- Misses       4328     4543     +215

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`55.82% <0.00%> (-0.14%)`	⬇️
lightning/src/util/ser.rs	`94.22% <ø> (+2.80%)`	⬆️
lightning/src/util/ser_macros.rs	`88.80% <0.00%> (-0.34%)`	⬇️
lightning/src/onion_message/packet.rs	`75.83% <87.50%> (+3.30%)`	⬆️
lightning/src/onion_message/functional_tests.rs	`95.48% <94.73%> (-0.85%)`	⬇️
lightning/src/onion_message/messenger.rs	`89.95% <100.00%> (+0.45%)`	⬆️
lightning/src/chain/mod.rs	`57.69% <0.00%> (-10.49%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`88.20% <0.00%> (-0.88%)`	⬇️
lightning/src/util/errors.rs	`71.42% <0.00%> (-0.80%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.73% <0.00%> (-0.31%)`	⬇️
... and 27 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

valentinewallace · 2022-10-07T19:46:00Z

lightning/src/onion_message/packet.rs

+				}
+				message_type = Some(msg_type);
+				decode_tlv!(msg_reader, message_bytes, vec_type);
+				return Ok(true)


@jkczyz Should this bool be an enum? Should we get rid of the result and always return a trinary enum? Let me know what you prefer

I had been thinking the custom decoder could signal an unknown type by returning a variant of DecodeError (possibly a new variant). This would presumed to be returned only if none of the bytes were consumed (i.e., only when checking the type). Otherwise, another custom decoder could not process it if the previous one consumed some bytes. If we make a trait for the custom decoder, we could have a separate method for checking if it understands the type and another method for decoding, perhaps?

Otherwise, another custom decoder could not process it if the previous one consumed some bytes.

Hmm, do we have a use-case for this? A given TLV should really only have one possible decoder, so once we pass the type through to the decode logic the decode logic should be able to select the specific decoder required, I'd think.

Ah, I think that I misread the ? repetition as * and thought this would handle known payloads (e.g., invoice_request) as a separate custom decoder. I guess logic for reading such known types would be added to the closure?

Related: Will handling be separate from decoding? If so, will we need the type still to figure out if handler for a known type is needed or a custom type, IIUC.

Right, I was anticipating multiple reads being handled all within the same closure. For handling, I was anticipating we'd do something like the custom P2P message handler - have an enum with all the messages we handle, and allow the user to specify the type for a Custom variant, which they'd presumably implement as their own enum.

lightning/src/util/ser_macros.rs

lightning/src/onion_message/packet.rs

TheBlueMatt · 2022-10-09T21:40:31Z

lightning/src/onion_message/packet.rs

+					return Err(DecodeError::InvalidValue)
+				}
+				message_type = Some(msg_type);
+				decode_tlv!(msg_reader, message_bytes, vec_type);


I don't think we need to call decode_tlv just to read_to_end a reader, but, more generally, I don't think this is sufficient. Instead of reading the TLV into a Vec, we should actually call a decode method based on the type. While we don't define any specific readers, we should have a trait, like CustomMessageReader, which we pass the reader here through to, and then have it read some generic enum that the user provides.

valentinewallace · 2022-10-11T20:37:44Z

I think we should be able to clean up the tests to not use log assertions anymore after this, but would appreciate some conceptual feedback on the latest approach first

fuzz/src/onion_message.rs

lightning/src/onion_message/messenger.rs

lightning/src/onion_message/packet.rs

lightning/src/onion_message/messenger.rs

lightning/src/onion_message/packet.rs

jkczyz

Feel free to squash

lightning/src/util/ser.rs

TheBlueMatt · 2022-10-17T19:01:37Z

Last commit needs to be reworded to describe the new behavior, also would kinda prefer it be split into the ser-changes/OM parts.

TheBlueMatt · 2022-10-17T19:03:38Z

lightning/src/onion_message/packet.rs

+#[derive(Debug)]
+/// The contents of an onion message. In the context of offers, this would be the invoice, invoice
+/// request, or invoice error.
+pub enum Message<T> where T: OnionMessageContents {


Type name conflicts with wire::Message which will break bindings (and be annoying for rust imports, in some cases).

Maybe name it OnionMessageContents and rename that OnionMessageType?

Went with Message --> OnionMessageContents and OnionMessageContents --> CustomOnionMessageContents

lightning/src/onion_message/packet.rs

lightning/src/onion_message/messenger.rs

lightning/src/util/ser_macros.rs

Useful in decoding a custom message, so (a) the message type can be provided to the handler and (b) None can be returned if the message type is unknown. Used in upcoming commit(s) to support custom onion messages.

Useful for decoding custom or user-provided TLVs. See macro docs for more info. Used in upcoming commit(s) to support custom onion message TLVs

OnionMessenger::new will now take a custom onion message handler trait implementation. This handler will be used in upcoming commit(s) to handle inbound custom onion messages. The new trait also specifies what custom messages are supported via its associated type, CustomMessage. This associated type must implement a new CustomOnionMessagesContents trait, which requires custom messages to support being written, being read, and supplying their TLV type.

OnionMessageContents specifies the data TLV that the sender wants in the onion message. This enum only has one variant for now, Custom. When offers are added, additional variants for invoice, invoice_request, and invoice_error will be added. This commit does not actually implement sending the custom OM contents, just the API change.

TheBlueMatt

One question I'd like an answer on, but if the answer is "yes", then I'm happy to land this.

TheBlueMatt · 2022-10-19T15:23:19Z

lightning/src/util/ser_macros.rs

+						if $decode_custom_tlv(t, &mut s)? {
+							// If a custom TLV was successfully read (i.e. decode_custom_tlv returns true),
+							// continue to the next TLV read.
+							s.eat_remaining()?;


nit: technically this potentially violates the TLV spec, which mandates that the TLV be exactly the expected length. I don't think it matters much, and, really, for users writing custom messages I think we should maybe prefer to be more flexible here cause users may want the extensibility. Just highlighting.

lightning/src/onion_message/packet.rs

This uses the work done in the preceding commits to implement encoding a user's custom TLV in outbound onion messages, and decoding custom TLVs in inbound onion messages, to be provided to the new CustomOnionMessageHandler.

Onion message data TLV types must be >= 64, enforce this on send

valentinewallace · 2022-10-19T20:08:17Z

Disallowed any data TLVs in forwarding hops for now. Waiting on spec clarification here: https://github.com/lightning/bolts/pull/759/files#r999879154

Only diff is:

diff --git a/lightning/src/onion_message/packet.rs b/lightning/src/onion_message/packet.rs
index 960b349f..74d253bf 100644
--- a/lightning/src/onion_message/packet.rs
+++ b/lightning/src/onion_message/packet.rs
@@ -230,7 +230,7 @@ impl<T: CustomOnionMessageContents> ReadableArgs<SharedSecret> for Payload<T> {
                match read_adapter {
                        None => return Err(DecodeError::InvalidValue),
                        Some(ChaChaPolyReadAdapter { readable: ControlTlvs::Forward(tlvs)}) => {
-                               if message_type.is_some() && message_type.unwrap() % 2 == 0 {
+                               if message_type.is_some() {
                                        return Err(DecodeError::InvalidValue)
                                }
                                Ok(Payload::Forward(ForwardControlTlvs::Unblinded(tlvs)))

valentinewallace force-pushed the 2022-10-custom-oms branch 2 times, most recently from 597f4c3 to a9883b1 Compare September 30, 2022 14:51

ariard reviewed Oct 3, 2022

View reviewed changes

valentinewallace force-pushed the 2022-10-custom-oms branch from a9883b1 to f2ab71c Compare October 4, 2022 14:43

ViktorTigerstrom reviewed Oct 7, 2022

View reviewed changes

Specify full import path in decode_tlv macro

a031ff9

Prevents unresolved import error

valentinewallace force-pushed the 2022-10-custom-oms branch from f2ab71c to a0997ed Compare October 7, 2022 19:36

valentinewallace commented Oct 7, 2022

View reviewed changes

valentinewallace force-pushed the 2022-10-custom-oms branch from a0997ed to ded359c Compare October 7, 2022 20:02

valentinewallace marked this pull request as ready for review October 7, 2022 20:03

TheBlueMatt reviewed Oct 9, 2022

View reviewed changes

valentinewallace force-pushed the 2022-10-custom-oms branch 5 times, most recently from 95bf6cd to cadc6ad Compare October 12, 2022 14:48

TheBlueMatt reviewed Oct 12, 2022

View reviewed changes

fuzz/src/onion_message.rs Outdated Show resolved Hide resolved

valentinewallace force-pushed the 2022-10-custom-oms branch 2 times, most recently from 4739d22 to 733e562 Compare October 12, 2022 19:12

jkczyz reviewed Oct 12, 2022

View reviewed changes

valentinewallace force-pushed the 2022-10-custom-oms branch 3 times, most recently from 48c41d7 to 92dc561 Compare October 13, 2022 01:21

jkczyz reviewed Oct 17, 2022

View reviewed changes

lightning/src/util/ser.rs Outdated Show resolved Hide resolved

valentinewallace force-pushed the 2022-10-custom-oms branch from 92dc561 to 17b22e0 Compare October 17, 2022 17:50

TheBlueMatt reviewed Oct 17, 2022

View reviewed changes

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

jkczyz reviewed Oct 17, 2022

View reviewed changes

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

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

lightning/src/util/ser_macros.rs Outdated Show resolved Hide resolved

lightning/src/util/ser_macros.rs Outdated Show resolved Hide resolved

valentinewallace added 6 commits October 17, 2022 16:05

ser: Add MaybeReadableArgs trait

a40e32b

Useful in decoding a custom message, so (a) the message type can be provided to the handler and (b) None can be returned if the message type is unknown. Used in upcoming commit(s) to support custom onion messages.

Optionally parameterize decode_tlv_stream with custom decode closure

2a8179e

Useful for decoding custom or user-provided TLVs. See macro docs for more info. Used in upcoming commit(s) to support custom onion message TLVs

Tweak OnionMessenger log on inbound onion message receipt

333be69

Fix accidental newline in OnionMessenger docs

87b637c

valentinewallace force-pushed the 2022-10-custom-oms branch from 17b22e0 to bda54b1 Compare October 18, 2022 17:34

jkczyz previously approved these changes Oct 18, 2022

View reviewed changes

jkczyz mentioned this pull request Oct 19, 2022

BOLT 12 offer encoding and building #1719

Merged

TheBlueMatt previously approved these changes Oct 19, 2022

View reviewed changes

valentinewallace added 2 commits October 19, 2022 16:06

Implement sending and receiving custom onion messages

7664957

This uses the work done in the preceding commits to implement encoding a user's custom TLV in outbound onion messages, and decoding custom TLVs in inbound onion messages, to be provided to the new CustomOnionMessageHandler.

Disallow sending invalid custom OM TLVs

4905268

Onion message data TLV types must be >= 64, enforce this on send

valentinewallace dismissed stale reviews from TheBlueMatt and jkczyz via 4905268 October 19, 2022 20:07

valentinewallace force-pushed the 2022-10-custom-oms branch from bda54b1 to 4905268 Compare October 19, 2022 20:07

TheBlueMatt approved these changes Oct 19, 2022

View reviewed changes

jkczyz approved these changes Oct 19, 2022

View reviewed changes

jkczyz merged commit d6321e6 into lightningdevkit:main Oct 19, 2022

jkczyz mentioned this pull request May 10, 2023

BOLT12 Offers Tracking Issue #1970

Open

60 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support custom onion messages #1748

Support custom onion messages #1748

valentinewallace commented Sep 30, 2022 •

edited

Loading

jkczyz commented Sep 30, 2022

jkczyz commented Sep 30, 2022 •

edited

Loading

valentinewallace commented Sep 30, 2022 •

edited

Loading

jkczyz commented Oct 3, 2022

valentinewallace commented Oct 3, 2022

jkczyz commented Oct 3, 2022

ariard Oct 3, 2022

valentinewallace Oct 3, 2022

ViktorTigerstrom Oct 7, 2022 •

edited

Loading

valentinewallace commented Oct 3, 2022 •

edited

Loading

jkczyz commented Oct 4, 2022

TheBlueMatt commented Oct 6, 2022

ViktorTigerstrom left a comment

ViktorTigerstrom Oct 6, 2022

valentinewallace Oct 7, 2022

ViktorTigerstrom Oct 6, 2022

valentinewallace Oct 7, 2022

codecov-commenter commented Oct 7, 2022 •

edited

Loading

valentinewallace Oct 7, 2022

jkczyz Oct 10, 2022

TheBlueMatt Oct 11, 2022

jkczyz Oct 11, 2022

TheBlueMatt Oct 11, 2022

TheBlueMatt Oct 9, 2022

valentinewallace commented Oct 11, 2022

jkczyz left a comment

TheBlueMatt commented Oct 17, 2022

TheBlueMatt Oct 17, 2022

jkczyz Oct 17, 2022

valentinewallace Oct 18, 2022

TheBlueMatt left a comment

TheBlueMatt Oct 19, 2022

valentinewallace commented Oct 19, 2022

		@@ -188,8 +193,15 @@ impl ReadableArgs<SharedSecret> for Payload {
		Ok(Payload::Forward(ForwardControlTlvs::Unblinded(tlvs)))

Support custom onion messages #1748

Support custom onion messages #1748

Conversation

valentinewallace commented Sep 30, 2022 • edited Loading

jkczyz commented Sep 30, 2022

jkczyz commented Sep 30, 2022 • edited Loading

valentinewallace commented Sep 30, 2022 • edited Loading

jkczyz commented Oct 3, 2022

valentinewallace commented Oct 3, 2022

jkczyz commented Oct 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

valentinewallace commented Oct 3, 2022 • edited Loading

jkczyz commented Oct 4, 2022

TheBlueMatt commented Oct 6, 2022

ViktorTigerstrom 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

codecov-commenter commented Oct 7, 2022 • edited Loading

Codecov Report

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 Oct 11, 2022

jkczyz left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Oct 17, 2022

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

valentinewallace commented Oct 19, 2022

valentinewallace commented Sep 30, 2022 •

edited

Loading

jkczyz commented Sep 30, 2022 •

edited

Loading

valentinewallace commented Sep 30, 2022 •

edited

Loading

ViktorTigerstrom Oct 7, 2022 •

edited

Loading

valentinewallace commented Oct 3, 2022 •

edited

Loading

codecov-commenter commented Oct 7, 2022 •

edited

Loading