Dlc version generic #1031

Tibo-lg · 2021-08-02T06:32:15Z

Add the ability to send and receive BOLT-1 compliant custom messages, not part of the base LN protocol.

TheBlueMatt · 2021-08-02T15:46:19Z

lightning/src/util/ser_macros.rs

@@ -120,15 +126,17 @@ macro_rules! check_missing_tlv {
 	}};
 }

+/// Decode a TLV.


Seems strange to export this - it doesn't really do anything by itself, it just writes a single field. I think the more useful things to export are encode_tlv_stream and decode_tlv_stream. That would require exporting this, but I think we should #[doc(hidden)] this and prefix it with a _ to indicate that it isn't, itself, a stable API or something users should use directly.

Yes sorry this needs to be re-worked.

I removed the serialization commit for now, need more time to work on that.

lightning/src/ln/wire.rs

TheBlueMatt · 2021-08-02T15:48:13Z

lightning/src/ln/wire.rs

@@ -96,6 +105,7 @@ impl Message {
 			&Message::ReplyChannelRange(ref msg) => msg.type_id(),
 			&Message::GossipTimestampFilter(ref msg) => msg.type_id(),
 			&Message::Unknown(type_id) => type_id,
+			&Message::Custom(_) => MessageType(0),


A message shouldn't have a default type, I think Custom needs to store the type somehow.

Agreed. I think we want T: Encode. Then implementors would typically use an enum for T as the the custom message type and each variant would wrap a struct that also implements Encode, just like wire::Message.

@jkczyz how do you implement Encode for an enum with the const requirement?

Hmm... good point. I guess it could instead be Custom(MessageType, T) or implement Encode for the enum as 0 and override type_id.

With Custom(MessageType, T) if I'm not mistaken it would only allow you to have a single value for MessageType so I don't think it's great. The override would probably work but doesn't feel super clean, and it would make the API a bit complicated on the user side I feel.

Here T is the enum, right? If so, it could take on an variant.

I still don't really see what you mean tbh but since it's not relevant with the new approach maybe no need to spend more time on that.

So it's back to being relevant and I think I got it (I hope at least).

TheBlueMatt · 2021-08-02T15:49:30Z

lightning/src/ln/wire.rs

 use util::ser::{Readable, Writeable, Writer};

 /// A Lightning message returned by [`read()`] when decoding bytes received over the wire. Each
 /// variant contains a message from [`msgs`] or otherwise the message type if unknown.
 #[allow(missing_docs)]
 #[derive(Debug)]
-pub enum Message {
+pub enum Message<T> where T: std::fmt::Debug {


Why store a T in Custom? Can't we get away with just making Custom store a Vec<u8> instead, avoiding a ton of generic typing for a similar outcome?

See background discussion here: https://lightningdevkit.slack.com/archives/C01AWTU1QAF/p1627528316011300?thread_ts=1627456353.011200&cid=C01AWTU1QAF

This is a bit more elegant to a previous approach and reduces the amount of code that needs to change in peer handler.

reduces the amount of code that needs to change in peer handler

I'm pretty dubious of this - there's a lot of new generic types flying everywhere and we already need to call a new method in the decode pipeline anyway. Maybe more importantly, the new API isn't trivially composable. Imagine I want to have a DLCMessageHandler and a MattsToSMessageHandler attached at once - I can't just match the message type and pass the message onwards to the next handler, I have to create a whole new enum that maps to multiple different potential message enums, which blows up the complexity substantially.

I'm pretty dubious of this - there's a lot of new generic types flying everywhere and we already need to call a new method in the decode pipeline anyway.

The proposed changes to peer_handler seemed quite extensive comparatively: p2pderivatives#3.

The approach in this PR is an alternative that I thought was much simpler. Additionally, separating parsing from handling seemed a cleaner separation of abstractions similar to wire and ChannelMessageHandler.

Happy to consider other alternatives.

Maybe more importantly, the new API isn't trivially composable. Imagine I want to have a DLCMessageHandler and a MattsToSMessageHandler attached at once - I can't just match the message type and pass the message onwards to the next handler, I have to create a whole new enum that maps to multiple different potential message enums, which blows up the complexity substantially.

We should be able to implement the trait on a 2-tuple like we do for chain::Listen. It would require defining an enum with two variants to use for the return value. The implementation of read would cascade as needed. And by nesting tuples it could be arbitrarily composable.

I suppose we could also use a trait object for the return type if you want to reduce the number of generic types. Figured we wanted to avoid dyn however.

W.r.t to composability I did have in mind to push the conversation behind the UMH interface. One implementation could be a proxy chaining processing calls on a vec of dynamic objects ?

TheBlueMatt · 2021-08-02T15:51:21Z

lightning/src/ln/peer_handler.rs

@@ -311,15 +344,15 @@ fn _check_usize_is_32_or_64() {
 /// lifetimes). Other times you can afford a reference, which is more efficient, in which case
 /// SimpleRefPeerManager is the more appropriate type. Defining these type aliases prevents
 /// issues such as overly long function definitions.
-pub type SimpleArcPeerManager<SD, M, T, F, C, L> = PeerManager<SD, Arc<SimpleArcChannelManager<M, T, F, L>>, Arc<NetGraphMsgHandler<Arc<C>, Arc<L>>>, Arc<L>>;
+pub type SimpleArcPeerManager<SD, M, T, F, C, L, UMH, UM> = PeerManager<SD, Arc<SimpleArcChannelManager<M, T, F, L>>, Arc<NetGraphMsgHandler<Arc<C>, Arc<L>>>, Arc<L>, UMH, UM>;


I think most users won't set an unknown message handler, so this should instead default to IgnoringUnknownMessageHandler and not get the new templates.

TheBlueMatt · 2021-08-02T15:52:08Z

lightning/src/ln/peer_handler.rs

-		L::Target: Logger {
+		L::Target: Logger,
+		UMH::Target: UnknownMessageHandler<T> + CustomMessageReader<T>,
+		T: ::std::fmt::Debug {


I don't get why we need the T here. Even if we keep the generic type for unknown messages, I think it should be an associated type on the UnknownMessageReader.

jkczyz · 2021-08-02T16:59:22Z

lightning/src/ln/wire.rs

 use util::ser::{Readable, Writeable, Writer};

 /// A Lightning message returned by [`read()`] when decoding bytes received over the wire. Each
 /// variant contains a message from [`msgs`] or otherwise the message type if unknown.
 #[allow(missing_docs)]
 #[derive(Debug)]
-pub enum Message {
+pub enum Message<T> where T: std::fmt::Debug {


See background discussion here: https://lightningdevkit.slack.com/archives/C01AWTU1QAF/p1627528316011300?thread_ts=1627456353.011200&cid=C01AWTU1QAF

This is a bit more elegant to a previous approach and reduces the amount of code that needs to change in peer handler.

jkczyz · 2021-08-02T17:03:37Z

lightning/src/ln/wire.rs

@@ -96,6 +105,7 @@ impl Message {
 			&Message::ReplyChannelRange(ref msg) => msg.type_id(),
 			&Message::GossipTimestampFilter(ref msg) => msg.type_id(),
 			&Message::Unknown(type_id) => type_id,
+			&Message::Custom(_) => MessageType(0),


Agreed. I think we want T: Encode. Then implementors would typically use an enum for T as the the custom message type and each variant would wrap a struct that also implements Encode, just like wire::Message.

jkczyz · 2021-08-02T17:08:37Z

lightning/src/ln/peer_handler.rs

+}
+
+/// Handler for messages external to the LN protocol.
+pub trait UnknownMessageHandler<T> : CustomMessageReader<T> {


For consistency, could you name this CustomMessageHandler?

jkczyz · 2021-08-02T17:15:49Z

lightning/src/ln/wire.rs

@@ -19,13 +19,14 @@
 //! [BOLT #1]: https://github.com/lightningnetwork/lightning-rfc/blob/master/01-messaging.md

 use ln::msgs;
+use ln::peer_handler::CustomMessageReader;


My preference would be to remove the circular dependency between peer_handler and wire by defining this here as Read perhaps, so it can be used elsewhere as wire::Read.

Done (note that then it requires making the wire module public but I think it's fine?)

lightning/src/ln/wire.rs

jkczyz · 2021-08-02T17:19:15Z

lightning/src/ln/wire.rs

+	}
+}
+
+impl<T> Message<T>  where T: std::fmt::Debug {


Is Debug needed?

ariard · 2021-08-02T23:20:01Z

lightning/src/ln/peer_handler.rs

@@ -173,6 +194,18 @@ pub struct MessageHandler<CM: Deref, RM: Deref> where
 	pub route_handler: RM,
 }

+/// Reader for messages external to the LN protocol.


nit: I would say "unrelated to the channel/gossip layers" as those custom/unknown messages are still defined in the terms of BOLT 1 and 8.

Used your phrasing in new version

ariard · 2021-08-02T23:35:08Z

lightning/src/util/ser.rs

@@ -486,6 +486,7 @@ impl_array!(16); // for IPv6
 impl_array!(32); // for channel id & hmac
 impl_array!(PUBLIC_KEY_SIZE); // for PublicKey
 impl_array!(COMPACT_SIGNATURE_SIZE); // for Signature
+impl_array!(162); // for ECDSA adaptor signatures


I think leftover from downstream.

ariard · 2021-08-02T23:44:05Z

lightning/src/ln/peer_handler.rs

+		match peers.node_id_to_descriptor.get(node_id) {
+			Some(descriptor) => match peers.peers.get_mut(&descriptor) {
+				Some(mut peer) => {
+					if peer.their_features.is_some() {


Maybe you could pass a predicate to this new method to evaluate if the peer not only identify to the given public key but also announce the feature signaling support for the relayed messages ?

IIUC this is just to check that the init message was received before we try to send something. I kind of see what you mean I think but I feel better to keep it simple for now and maybe add this kind of possibilities after.

ariard · 2021-08-02T23:58:52Z

lightning/src/ln/wire.rs

 use util::ser::{Readable, Writeable, Writer};

 /// A Lightning message returned by [`read()`] when decoding bytes received over the wire. Each
 /// variant contains a message from [`msgs`] or otherwise the message type if unknown.
 #[allow(missing_docs)]
 #[derive(Debug)]
-pub enum Message {
+pub enum Message<T> where T: std::fmt::Debug {


W.r.t to composability I did have in mind to push the conversation behind the UMH interface. One implementation could be a proxy chaining processing calls on a vec of dynamic objects ?

codecov · 2021-08-05T12:08:28Z

Codecov Report

Merging #1031 (eb46f47) into main (803d804) will increase coverage by 0.03%.
The diff coverage is 72.54%.

@@            Coverage Diff             @@
##             main    #1031      +/-   ##
==========================================
+ Coverage   90.82%   90.86%   +0.03%     
==========================================
  Files          65       65              
  Lines       32801    33055     +254     
==========================================
+ Hits        29793    30034     +241     
- Misses       3008     3021      +13

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	`90.00% <ø> (ø)`
lightning/src/ln/peer_handler.rs	`45.78% <42.85%> (+0.02%)`	⬆️
lightning/src/ln/wire.rs	`59.68% <86.66%> (+6.15%)`	⬆️
lightning-background-processor/src/lib.rs	`93.99% <100.00%> (+0.05%)`	⬆️
lightning-net-tokio/src/lib.rs	`76.98% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`97.42% <0.00%> (-0.09%)`	⬇️
lightning-block-sync/src/convert.rs	`96.23% <0.00%> (+3.42%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 803d804...eb46f47. Read the comment docs.

Tibo-lg · 2021-08-05T12:45:32Z

Updated as discussed on slack. I think this approach is pretty neat personally (thanks to your suggestions). For the sending side I for now kept a simple enqueue_external_message because AFAICT everything is locked with the peers mutex so it seems safe, but if there is still some concerns my proposal would be to have this method encode the messages and keep them in a list that is the pushed in the pending_output_buffer on a call to process_events.
There is probably some things to fix (especially in naming) and tests to be added but I wanted to make sure this approach works for everybody before spending more time on it.

TheBlueMatt · 2021-08-05T20:55:02Z

lightning/src/ln/wire.rs

@@ -22,6 +22,24 @@ use io;
 use ln::msgs;
 use util::ser::{Readable, Writeable, Writer};

+/// Trait to be implemented by custom message (unrelated to the channel/gossip LN layers)
+/// decoders.
+pub trait Read {


Can we name this something more specific (eg CustomMessageReader)? Read conflicts with std::io::Read which is used pretty extensively.

TheBlueMatt · 2021-08-05T21:40:35Z

lightning/src/ln/wire.rs

+}
+
+// Really needs a better name
+pub(crate) enum MessageOrCustom<T> {


So I think Jeff was originally thinking this would be in PeerManager with the original read code from https://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=commitdiff;h=7a586455c10d835a425fa1de8ec447acbf9c67f3 there. That said, your approach here definitely cleans up the PeerManager message handling pipeline a lot. Still, the line count here is huge, I wonder if its easier to to just make wire::Message pub(crate) instead of pub, then just inline the Custom variant there instead of having a separate enum.

That way we'd avoid most of the diff, but still keep the read stuff nice and tight.

Thanks that works as well indeed, I had a misunderstood I guess.

TheBlueMatt · 2021-08-05T21:41:14Z

lightning/src/ln/peer_handler.rs

@@ -174,6 +195,13 @@ pub struct MessageHandler<CM: Deref, RM: Deref> where
 	pub route_handler: RM,
 }

+/// Handler for messages external to the LN protocol.
+pub trait CustomMessageHandler : wire::Read {


I'm not a huge fan of this being defined in a different file from wire::Read, tbh. Maybe it makes the most sense to put both here?

Moved, but note that the reason for having them separated was to avoid circular dependency between wire and peer_handler as noted before (#1031 (comment)). Now it's back because CustomMessageHandler refers to MessageHandlingError which is in peer_handler.

Ah, I mean I don't care either way, then. Feels strange, but whatever.

I don't care either, @jkczyz do you prefer that I put it back in peer_handler to avoid the circular dependency?

I'd prefer to keep them separated, but note that they can be independent traits. No need to make one a super trait of the other since you already constrain the generic type by both traits.

I moved back the CustomMessageHandler trait into peer_handler.rs to remove the circular dependency. However I didn't manage to make them independent traits. The issue AFAIU is that the compiler needs to know that both traits' associated type are the same in order to be able to use the output of read as an input to handle_custom_message. Maybe it's possible to put this constraint somewhere else? (Although I must say even if it's possible I personally find it cleaner to have the constraint directly on the traits since I can't see a case where you'd want the types to differ).

Tibo-lg · 2021-08-06T03:00:14Z

lightning/src/ln/peer_handler.rs

@@ -868,9 +916,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D

 	/// Process an incoming message and return a decision (ok, lightning error, peer handling error) regarding the next action with the peer
 	/// Returns the message back if it needs to be broadcasted to all other peers.
-	fn handle_message(&self, peer: &mut Peer, message: wire::Message) -> Result<Option<wire::Message>, MessageHandlingError> {
-		log_trace!(self.logger, "Received message {:?} from {}", message, log_pubkey!(peer.their_node_id.unwrap()));


I don't know if this log message is important or not (looks useful but not critical to me). I couldn't find a nice way to keep it without putting a Debug constraint on the T in Message (which is maybe also fine).

Ah, I didn't realize that is why it was needed. I think we want the constraint on T then.

Added and restored the log message

jkczyz · 2021-08-06T13:55:58Z

lightning/src/ln/wire.rs

+/// decoders.
+pub trait CustomMessageReader {
+	/// The type of the message decoded by the implementation.
+	type CustomMessageType;


How about CustomMessage? Type is redundant and could be confused with the MessageType struct.

Agreed, updated

jkczyz · 2021-08-06T14:27:36Z

lightning/src/ln/wire.rs

+pub(crate) fn read<R: io::Read, T, H: core::ops::Deref>(buffer: &mut R, custom_reader: &H) -> Result<Message<T>, msgs::DecodeError>
+	where
+		H::Target: CustomMessageReader<CustomMessageType = T> {


We haven't been consistent, but when breaking long lines could you use these conventions?

https://github.com/rust-dev-tools/fmt-rfcs/blob/master/guide/items.md#where-clauses

Done (also improved indents in other places following these guidelines)

jkczyz · 2021-08-06T14:30:56Z

lightning/src/ln/wire.rs

+}
+
+/// Handler for messages external to the LN protocol.
+pub trait CustomMessageHandler : CustomMessageReader {


This doesn't need to be a subtrait since peer_handler uses both as a trait bound.

See #1031 (comment)

jkczyz · 2021-08-06T14:40:16Z

lightning/src/ln/peer_handler.rs

-	fn handle_message(&self, peer: &mut Peer, message: wire::Message) -> Result<Option<wire::Message>, MessageHandlingError> {
-		log_trace!(self.logger, "Received message {:?} from {}", message, log_pubkey!(peer.their_node_id.unwrap()));
-
+	fn handle_message(&self, peer: &mut Peer, message: wire::Message<<<CMH as core::ops::Deref>::Target as wire::CustomMessageReader>::CustomMessageType>) -> Result<Option<wire::Message<<<CMH as core::ops::Deref>::Target as wire::CustomMessageReader>::CustomMessageType>>, MessageHandlingError> {


Could you break this line up and possibly use a where clause for readability?

Broke up the line, but couldn't use a where clause because it's not a trait AFAIU. I also tried to use a type alias but it seems like type aliases don't support associated types for now.

jkczyz · 2021-08-06T14:44:51Z

lightning/src/ln/peer_handler.rs

@@ -868,9 +916,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D

 	/// Process an incoming message and return a decision (ok, lightning error, peer handling error) regarding the next action with the peer
 	/// Returns the message back if it needs to be broadcasted to all other peers.
-	fn handle_message(&self, peer: &mut Peer, message: wire::Message) -> Result<Option<wire::Message>, MessageHandlingError> {
-		log_trace!(self.logger, "Received message {:?} from {}", message, log_pubkey!(peer.their_node_id.unwrap()));


Ah, I didn't realize that is why it was needed. I think we want the constraint on T then.

TheBlueMatt

Sorry for the delay on this.

fuzz/src/full_stack.rs

lightning/src/ln/peer_handler.rs

TheBlueMatt · 2021-08-12T19:45:46Z

lightning/src/ln/peer_handler.rs

@@ -360,8 +390,11 @@ pub struct PeerManager<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: De
 	logger: L,
 }

-enum MessageHandlingError {
+/// An error indicating a failure to handle a received message.
+pub enum MessageHandlingError {


Actually, on second thought, can we not expose this and just have CustomMessageHandlers return LightningErrors? LightningError should cover all possible cases that users will want to trigger, PeerHandleError is somewhat more of a thing that we return to users from PeerManager, making both be used for receiving errors from users and returning errors to use blurs them a bit much IMO.

Works for me, done.

TheBlueMatt · 2021-08-12T19:50:19Z

lightning/src/ln/peer_handler.rs

@@ -673,6 +708,25 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
 		}
 	}

+	/// Find the peer corresponding to the given node id, and if found appends
+	/// a custom message to the outbound/write buffer.
+	pub fn enqueue_custom_message<M: Encode + Writeable + Debug>(&self, node_id: &PublicKey, message: &M) {


This seems really hard to use correctly - right now if you receive a message via handle_custom_message and are in the midst of processing it, you cannot call enqueue_custom_message or you'll deadlock. This seems like precisely the way users would implement this API, though. I think the only other way you can reasonably go about this would be to add the ability to get the message id from the CustomMessage associated type (which is already going to be an enum.

Oh that's a really good point.

Though I must say I don't understand what you would like to do instead. I tried several things previously including reusing the associated type for sending, but IIRC the main issue is the const requirement on the Encode trait, which makes it impossible to implement for an enum (or anything that has more than one type id).
I had also tried some complicated things like passing callbacks around but it always ended up requirement some boxing which didn't feel really nice.

For now I added a (mutexed) vector of node id + encoded messages on the peer handler, that can be used by the custom handler through the enqueue_custom_message method. It's a tiny bit a shame to have this while it might be mostly unneeded, but it felt like the simplest solution. Let me know what you think.

tried several things previously including reusing the associated type for sending, but IIRC the main issue is the const requirement on the Encode trait, which makes it impossible to implement for an enum (or anything that has more than one type id).

Yep, I think the only practical solution here is to create a new trait for public consumption that doesn't use the const. I suppose we could also drop the const in Encode and swap it for just a method, but that's a lot more diff and I dunno if its worth it.

I added the TypedMessage trait and made a blanket implementation for T: Encode so that the write function can be used by both. Because of that Encode needs to be public but apart from that I think that's a good way to reduce the diff.

I also updated the CustomMessageHandler to have a get_and_clear_pending_msg method so that it's more consistent with other handlers.

TheBlueMatt

This looks basically good IMO.

TheBlueMatt · 2021-08-16T18:11:14Z

lightning/src/ln/wire.rs

+/// A message that has an associated type id.
+pub trait TypedMessage {
+	/// The type id for the implementing message.
+	fn msg_type(&self) -> u16;


Does it make sense to use the MessageType newtype here (and basically in all the new code with message types) instead of u16?

You mean the MessageType(u16)? I tried at some point but it would require making the u16 public, and then from a user perspective you'd have to wrap your u16 into MessageType and unwrap them when matching, so I didn't feel like it provided big benefit.
If I'm being honest even within LDK I didn't feel like it provided a lot apart from the is_even method, but maybe I'm missing something. It would be nice if it could somehow abstract the underlying type but since it needs to be serialized as a u16 at the end I'm not sure it's possible.

Also even with the nice dummy wrapper module it doesn't seem to be possible to make MessgeType non public.

I don't feel strongly, tbh, it just felt awkward to have a newtype and not use it, maybe @jkczyz feels strongly.

Sorry I fell off the review. I think we could get rid of MessageType and clean up Encode by moving type_id to the new trait. Do you mind taking the commits in https://github.com/jkczyz/rust-lightning/tree/2011-08-wire-type? I did this there along with some other clean-ups with naming. It moves is_even to Message, which can be used on the Custom variant.

I cherry picked your commits. It makes things really clean IMHO, thanks!

TheBlueMatt · 2021-08-16T18:29:35Z

lightning/src/ln/wire.rs

+	fn msg_type(&self) -> u16;
+}
+
+impl<T> TypedMessage for T where T: Encode {


Note that you can avoid making Encode public with a dummy wrapper module - the following compiles (but obviously use a better module name and fix the indentation).

@@ -251,2 +251,4 @@ pub fn write<M: TypedMessage + Writeable, W: Writer>(message: &M, buffer: &mut W +pub(crate) mod lulz { +use super::*; /// Defines a type-identified encoding for sending messages over the wire. @@ -264,2 +266,4 @@ pub trait Encode { } +} +pub(crate) use self::lulz::Encode;

That's a nice trick thanks!

ariard

ACK on the CustomMessageHandler/CustomMessageReader new interfaces. One last round and I think it's pretty mature to land.

This will also serve to match the use-case of flowing headers-over-ln : https://github.com/ariard/rust-lightning/tree/2021-07-app-header.

ariard · 2021-08-20T00:23:03Z

lightning/src/ln/peer_handler.rs

@@ -44,6 +43,17 @@ use bitcoin::hashes::sha256::Hash as Sha256;
 use bitcoin::hashes::sha256::HashEngine as Sha256Engine;
 use bitcoin::hashes::{HashEngine, Hash};

+/// Handler for messages external to the LN protocol.


nit: I would say "Handler for BOLT1-compliant messages" as afaict we still expect the messages to respect the LN messages typing and be in the range 32768 - 65535.

ariard · 2021-08-20T00:53:59Z

lightning/src/ln/wire.rs

-		MessageType(Self::TYPE)
+pub trait Type {
+	/// Returns the type identifying the message payload.
+	fn type_id(&self) -> u16;


Should we had a trait implem requirement that T::TYPE must be in the custom LN range ?

Not sure how you would do that? Also this trait is used also for non custom message now so we'd need a different trait I guess? And tbh I'm not convinced it would be a great restriction to have, it feels like it could be more something annoying than really useful, but that's just my opinion.

Not sure how you would do that? Also this trait is used also for non custom message now so we'd need a different trait I guess?

Yeah afaict future spec usage of the range Setup & Control / Channel / Commitment / Routing. I think such usages should be covered by the compliant part of LDK, i.e in the named part of our enum Message

ariard · 2021-08-20T00:55:23Z

lightning/src/ln/wire.rs

+			if message_type == CUSTOM_MESSAGE_TYPE {
+				return Ok(Some(TestCustomMessage{}));
+			}
+


I have a whitespace here ?

Thanks fixed

ariard · 2021-08-20T01:05:22Z

lightning/src/ln/peer_handler.rs

+
+	/// Gets the list of pending messages which were generated by the custom message
+	/// handler, clearing the list in the process.
+	fn get_and_clear_pending_msg(&self) -> Vec<(PublicKey, Self::CustomMessage)>;


Precise the PublicKey must be node id ?

If you want to send the same message to N peers, should the return type be instead Vec<CustomMessage, Vec<PublicKey>> ? More memory conservative.

Added some doc.

Updated to Vec<PublicKey>. Though I would note that in terms of memory it probably means an increase for the single recipient case.

If you want to send the same message to N peers, should the return type be instead Vec<CustomMessage, Vec> ? More memory conservative.

Strongly disagree, I don't think this is a particularly useful API - if you want to broadcast a message, getting the list of peers connected seems like a lot of work, so I'm not sure this is the right API for it anyway. If you want to unicast a message (which I believe is the common case), now you're wasting a ton of extra memory to do a full second heap allocated Vec just to store a single publickey.

so I'm not sure this is the right API for it anyway. If you want to unicast a message (which I believe is the common case),

Yeah I see where we diverge, I've more in mind multicast type of broadcast such as headers or dlc offers. Especially if you start to broadcast dlc offers per-subset such as future, options, twitter bets...I think we can revisit the API in the future, if memory starts to be a real bottleneck.

Though note as it's public API, won't be free to migrate library users.

TheBlueMatt

Looks good. I think if these comments are addressed I'm ACK here.

TheBlueMatt · 2021-08-20T19:31:45Z

lightning/src/ln/peer_handler.rs

+	/// handler, clearing the list in the process. The first tuple element must
+	/// correspond to the intended recipients node ids. If no connection to one of the
+	/// specified node does not exist, the message is simply not sent to it.
+	fn get_and_clear_pending_msg(&self) -> Vec<(Vec<PublicKey>, Self::CustomMessage)>;


Is there a reason to have a Vec of PublicKeys here? Isn't the "common" use-case to send a single message to a single peer, not a single message to many peers?

Reverted to single PublicKey.

TheBlueMatt · 2021-08-20T19:34:16Z

lightning/src/ln/peer_handler.rs

+
+impl Writeable for DummyCustomType {
+	fn write<W: Writer>(&self, _: &mut W) -> Result<(), io::Error> {
+		Ok(())


Arguably this is unreachable as well.

True changed to unreachable!()

TheBlueMatt · 2021-08-20T19:36:46Z

lightning/src/ln/peer_handler.rs

@@ -314,15 +364,15 @@ fn _check_usize_is_32_or_64() {
 /// lifetimes). Other times you can afford a reference, which is more efficient, in which case
 /// SimpleRefPeerManager is the more appropriate type. Defining these type aliases prevents
 /// issues such as overly long function definitions.
-pub type SimpleArcPeerManager<SD, M, T, F, C, L> = PeerManager<SD, Arc<SimpleArcChannelManager<M, T, F, L>>, Arc<NetGraphMsgHandler<Arc<C>, Arc<L>>>, Arc<L>>;
+pub type SimpleArcPeerManager<SD, M, T, F, C, L> = PeerManager<SD, Arc<SimpleArcChannelManager<M, T, F, L>>, Arc<NetGraphMsgHandler<Arc<C>, Arc<L>>>, Arc<L>, Arc<IgnoringCustomMessageHandler>>;


You can drop the Arc here, no reason to Arc to the ignoring handler.

If I drop the Arc I get some errors in lightning-net-tokio. I think I could solve them by putting IgnoringMessageHandler everywhere but IIUC it would then prevent me from using lightning-net-tokio. Or maybe I'm missing something so let me know.

To be more precise I think the issue is that in the case where one wants to provide a custom message handler, one would do so using an arc (for the tokio case) and so that's what the lightning-net-tokio expect (or at least how I updated things). So if I remove the Arc here then SimpleArcPeerManager is not compatible with the functions in lightning-net-tokio anymore (at least that's my understanding).

The error that pops up when running cargo test in lightning-net-tokio while removing the Arc:

test src/lib.rs - (line 24) ... FAILED failures: ---- src/lib.rs - (line 24) stdout ---- error[E0308]: mismatched types --> src/lib.rs:44:40 | 23 | lightning_net_tokio::connect_outbound(peer_manager, their_node_id, addr).await; | ^^^^^^^^^^^^ expected struct `Arc`, found struct `IgnoringMessageHandler` | = note: expected struct `Arc<lightning::ln::peer_handler::PeerManager<_, Arc<_>, Arc<_>, Arc<_>, Arc<_>>>` found struct `Arc<lightning::ln::peer_handler::PeerManager<_, Arc<lightning::ln::channelmanager::ChannelManager<InMemorySigner, Arc<lightning::chain::chainmonitor::ChainMonitor<InMemorySigner, Arc<dyn lightning::chain::Filter + Send + Sync>, Arc<dyn BroadcasterInterface + Send + Sync>, Arc<dyn lightning::chain::chaininterface::FeeEstimator + Send + Sync>, Arc<dyn lightning::util::logger::Logger + Send + Sync>, Arc<dyn Persist<InMemorySigner> + Send + Sync>>>, Arc<dyn BroadcasterInterface + Send + Sync>, Arc<KeysManager>, Arc<dyn lightning::chain::chaininterface::FeeEstimator + Send + Sync>, Arc<dyn lightning::util::logger::Logger + Send + Sync>>>, Arc<NetGraphMsgHandler<Arc<dyn Access + Send + Sync>, Arc<dyn lightning::util::logger::Logger + Send + Sync>>>, Arc<dyn lightning::util::logger::Logger + Send + Sync>, IgnoringMessageHandler>>` error[E0308]: mismatched types --> src/lib.rs:58:37 | 37 | lightning_net_tokio::setup_inbound(peer_manager, socket); | ^^^^^^^^^^^^ expected struct `Arc`, found struct `IgnoringMessageHandler` | = note: expected struct `Arc<lightning::ln::peer_handler::PeerManager<_, Arc<_>, Arc<_>, Arc<_>, Arc<_>>>` found struct `Arc<lightning::ln::peer_handler::PeerManager<_, Arc<lightning::ln::channelmanager::ChannelManager<InMemorySigner, Arc<lightning::chain::chainmonitor::ChainMonitor<InMemorySigner, Arc<dyn lightning::chain::Filter + Send + Sync>, Arc<dyn BroadcasterInterface + Send + Sync>, Arc<dyn lightning::chain::chaininterface::FeeEstimator + Send + Sync>, Arc<dyn lightning::util::logger::Logger + Send + Sync>, Arc<dyn Persist<InMemorySigner> + Send + Sync>>>, Arc<dyn BroadcasterInterface + Send + Sync>, Arc<KeysManager>, Arc<dyn lightning::chain::chaininterface::FeeEstimator + Send + Sync>, Arc<dyn lightning::util::logger::Logger + Send + Sync>>>, Arc<NetGraphMsgHandler<Arc<dyn Access + Send + Sync>, Arc<dyn lightning::util::logger::Logger + Send + Sync>>>, Arc<dyn lightning::util::logger::Logger + Send + Sync>, IgnoringMessageHandler>>` error: aborting due to 2 previous errors For more information about this error, try `rustc --explain E0308`. Couldn't compile the test. failures: src/lib.rs - (line 24) test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.31s error: test failed, to rerun pass '--doc'

Ah, yea, OK, I see that, makes sense.

TheBlueMatt · 2021-08-20T19:41:45Z

lightning/src/ln/wire.rs

@@ -13,7 +13,7 @@
 //! The [`Message`] enum returned by [`read()`] wraps the decoded message or the message type (if
 //! unknown) to use with pattern matching.
 //!
-//! Messages implementing the [`Encode`] trait define a message type and can be sent over the wire
+//! Messages implementing the [`Type`] trait define a message type and can be sent over the wire


These docs should be updated to only describe the public interface, currently cargo doc says:

warning: public documentation for `wire` links to private item `read` --> lightning/src/ln/wire.rs:12:68 | 12 | //! Messages known by this module can be read from the wire using [`read()`]. | ^^^^^^^^ this item is private | = note: `#[warn(private_intra_doc_links)]` on by default = note: this link will resolve properly if you pass `--document-private-items` warning: public documentation for `wire` links to private item `Message` --> lightning/src/ln/wire.rs:13:10 | 13 | //! The [`Message`] enum returned by [`read()`] wraps the decoded message or the message type (if | ^^^^^^^^^ this item is private | = note: this link will resolve properly if you pass `--document-private-items` warning: public documentation for `wire` links to private item `read` --> lightning/src/ln/wire.rs:13:39 | 13 | //! The [`Message`] enum returned by [`read()`] wraps the decoded message or the message type (if | ^^^^^^^^ this item is private | = note: this link will resolve properly if you pass `--document-private-items` warning: 3 warnings emitted

Updated the docs to remove private item mentions.

TheBlueMatt · 2021-08-20T19:44:10Z

lightning/src/ln/wire.rs

@@ -219,22 +231,32 @@ pub fn read<R: io::Read>(buffer: &mut R) -> Result<Message, msgs::DecodeError> {
 /// # Errors
 ///
 /// Returns an I/O error if the write could not be completed.
-pub fn write<M: Encode + Writeable, W: Writer>(message: &M, buffer: &mut W) -> Result<(), io::Error> {
-	M::TYPE.write(buffer)?;
+pub fn write<M: Type + Writeable, W: Writer>(message: &M, buffer: &mut W) -> Result<(), io::Error> {


Does this need to remain pub?

No put as pub(crate).

This doesn't appear to have changed in the latest PR commits?

Oups sorry it got lost during rebase. Fixed

jkczyz

Yep, mostly looks good.

jkczyz · 2021-08-20T22:23:22Z

lightning/src/ln/peer_handler.rs

@@ -44,6 +43,19 @@ use bitcoin::hashes::sha256::Hash as Sha256;
 use bitcoin::hashes::sha256::HashEngine as Sha256Engine;
 use bitcoin::hashes::{HashEngine, Hash};

+/// Handler for BOLT1-compliant messages.
+pub trait CustomMessageHandler : wire::CustomMessageReader {


nit: remove space before :

jkczyz · 2021-08-20T22:25:31Z

lightning/src/ln/wire.rs

+/// decoders.
+pub trait CustomMessageReader {
+	/// The type of the message decoded by the implementation.
+	type CustomMessage : core::fmt::Debug + Type + Writeable;


nit: remove space before :

jkczyz · 2021-08-20T22:27:30Z

lightning/src/ln/peer_handler.rs

@@ -69,6 +81,44 @@ impl Deref for IgnoringMessageHandler {
 	fn deref(&self) -> &Self { self }
 }

+/// A dummy implementation of `CustomMessageHandler` that does nothing.
+pub struct IgnoringCustomMessageHandler{}


Instead of defining a new struct, could you reuse IgnoringMessageHandler?

Yes that works as well updated

jkczyz · 2021-08-20T22:30:54Z

lightning/src/ln/peer_handler.rs

+/// A dummy implementation of `CustomMessageHandler` that does nothing.
+pub struct IgnoringCustomMessageHandler{}
+
+type DummyCustomType = ();


Instead of defining a new type, just use () where needed. If not, how about NullType per the null object pattern?

Used () I think it's easier.

jkczyz · 2021-08-20T22:40:29Z

lightning/src/ln/wire.rs

+	/// Decodes a custom message to `CustomMessageType`. If the given message type is known to the implementation and
+	/// the message could be decoded, must return `Ok(Some(message))`. If the message type
+	/// is unknown to the implementation, must return `Ok(None)`. If a decoding error
+	/// occur, must return `Err(DecodeError::X)` where `X` details the encountered error.


Wrap at 100 characters

Done (I assume you meant only the doc part)

jkczyz · 2021-08-20T22:41:07Z

lightning/src/ln/wire.rs

@@ -56,17 +68,13 @@ pub enum Message {
 	ReplyChannelRange(msgs::ReplyChannelRange),
 	GossipTimestampFilter(msgs::GossipTimestampFilter),
 	/// A message that could not be decoded because its type is unknown.
-	Unknown(MessageType),
+	Unknown(u16),
+	Custom(T),


Could you document this variant?

jkczyz · 2021-08-20T22:42:00Z

lightning/src/ln/wire.rs

-pub fn read<R: io::Read>(buffer: &mut R) -> Result<Message, msgs::DecodeError> {
+pub(crate) fn read<R: io::Read, T, H: core::ops::Deref>(
+	buffer: &mut R,
+	custom_reader: &H


Add a , after the last parameter when they are on their own lines.

jkczyz · 2021-08-20T22:42:10Z

lightning/src/ln/wire.rs

+) -> Result<Message<T>, msgs::DecodeError>
+where
+	T: core::fmt::Debug + Type + Writeable,
+	H::Target: CustomMessageReader<CustomMessage = T>


jkczyz · 2021-08-20T22:43:32Z

lightning/src/ln/wire.rs

+				return Ok(Some(TestCustomMessage{}));
+			}
+
+			Ok(None)


Could you add a unit test for this case?

Good point added. I wanted to do a more direct test using TestCustomMessageHandler directly but it complains about recursive references, so I'd need to drop the Deref or something, let me know if you think it's enough like that.

Testing through wire::read is fine, but curious as to why you need to implement Deref? Taking a double reference like &&TestCustomMessageReader {} should work.

That said, does wire::read need to take a reference to something that implements Deref? Could it take a simple reference instead? I think PeerManager would need to dereference its custom handler and take a reference of the result for passing to wire::read.

Ah good point about the double reference. Updated and added a direct test (kept the one through read as it was there).

The reason for using Deref is because if I just use a simple reference it complains that it cannot be made into an object due to the function using a generic parameter. Here is the error:

error[E0038]: the trait `CustomMessageReader` cannot be made into an object --> lightning/src/ln/wire.rs:122:17 | 122 | custom_reader: &CustomMessageReader<CustomMessage = T>, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `CustomMessageReader` cannot be made into an object | = help: consider moving `read` to another trait note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety> --> lightning/src/ln/wire.rs:28:5 | 21 | pub trait CustomMessageReader { | ------------------- this trait cannot be made into an object... ... 28 | fn read<R: io::Read>(&self, message_type: u16, buffer: &mut R) -> Result<Option<Self::CustomMessage>, msgs::DecodeError>; | ^^^^ ...because method `read` has generic type parameters

Happy to hear if there is another way to do it though or if I'm misunderstanding something.

Ah, that's right, it's a trait... sorry, I was confused. You can change the parameter type from &H to H and at the call site pass &*self.custom_message_handler.

You can change the parameter type from &H to H and at the call site pass &*self.custom_message_handler

Done

TheBlueMatt · 2021-08-23T18:41:59Z

LGTM aside from #1031 (comment)

jkczyz

Looks good other than one comment on Deref.

jkczyz · 2021-08-24T13:21:21Z

lightning/src/ln/wire.rs

+				return Ok(Some(TestCustomMessage{}));
+			}
+
+			Ok(None)


Testing through wire::read is fine, but curious as to why you need to implement Deref? Taking a double reference like &&TestCustomMessageReader {} should work.

That said, does wire::read need to take a reference to something that implements Deref? Could it take a simple reference instead? I think PeerManager would need to dereference its custom handler and take a reference of the result for passing to wire::read.

TheBlueMatt

Could you squash the one fixup commit and then we can land this 🎉

With custom messages, wire::Type was introduced. wire::MessageType is a bit redundant, so use u16 instead and move is_even to wire::Message.

Tibo-lg · 2021-08-25T04:22:39Z

Could you squash the one fixup commit and then we can land this 🎉

Done!

TheBlueMatt reviewed Aug 2, 2021

View reviewed changes

jkczyz self-requested a review August 2, 2021 16:57

jkczyz reviewed Aug 2, 2021

View reviewed changes

ariard reviewed Aug 2, 2021

View reviewed changes

Tibo-lg force-pushed the dlc-version-generic branch 3 times, most recently from 58bc3fe to f1915e8 Compare August 5, 2021 12:00

Tibo-lg force-pushed the dlc-version-generic branch from f1915e8 to 70ea041 Compare August 5, 2021 12:13

TheBlueMatt reviewed Aug 5, 2021

View reviewed changes

Tibo-lg force-pushed the dlc-version-generic branch 4 times, most recently from c2470b3 to 1a336e5 Compare August 6, 2021 02:56

Tibo-lg commented Aug 6, 2021

View reviewed changes

jkczyz reviewed Aug 6, 2021

View reviewed changes

Tibo-lg force-pushed the dlc-version-generic branch 2 times, most recently from b121669 to 510c7bc Compare August 10, 2021 03:21

TheBlueMatt reviewed Aug 12, 2021

View reviewed changes

Tibo-lg force-pushed the dlc-version-generic branch 6 times, most recently from 68db917 to be92584 Compare August 16, 2021 06:25

TheBlueMatt reviewed Aug 16, 2021

View reviewed changes

Tibo-lg force-pushed the dlc-version-generic branch 2 times, most recently from cfecfe9 to 6fbbc74 Compare August 19, 2021 00:41

ariard reviewed Aug 20, 2021

View reviewed changes

Tibo-lg force-pushed the dlc-version-generic branch from 6fbbc74 to f819d2c Compare August 20, 2021 01:48

Tibo-lg marked this pull request as ready for review August 20, 2021 05:14

TheBlueMatt reviewed Aug 20, 2021

View reviewed changes

jkczyz reviewed Aug 20, 2021

View reviewed changes

Tibo-lg force-pushed the dlc-version-generic branch 2 times, most recently from 4dff41b to f2f3f5a Compare August 23, 2021 06:26

Tibo-lg force-pushed the dlc-version-generic branch from f2f3f5a to 78992f5 Compare August 24, 2021 00:44

jkczyz approved these changes Aug 24, 2021

View reviewed changes

Tibo-lg force-pushed the dlc-version-generic branch 2 times, most recently from d44f514 to 213d2ba Compare August 25, 2021 02:01

TheBlueMatt approved these changes Aug 25, 2021

View reviewed changes

Tibo-lg and others added 2 commits August 25, 2021 13:22

Add custom message handler to peer manager

2e5e40e

Remove wire::MessageType in favor of u16

eb46f47

With custom messages, wire::Type was introduced. wire::MessageType is a bit redundant, so use u16 instead and move is_even to wire::Message.

Tibo-lg force-pushed the dlc-version-generic branch from 213d2ba to eb46f47 Compare August 25, 2021 04:22

jkczyz approved these changes Aug 25, 2021

View reviewed changes

TheBlueMatt merged commit 45853b3 into lightningdevkit:main Aug 25, 2021

Dlc version generic #1031

Dlc version generic #1031

Conversation

Tibo-lg commented Aug 2, 2021 • 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

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

codecov bot commented Aug 5, 2021 • edited Loading

Codecov Report

Tibo-lg commented Aug 5, 2021

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

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

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

Tibo-lg commented Aug 2, 2021 •

edited

Loading

codecov bot commented Aug 5, 2021 •

edited

Loading