Provide Common Base For Custom Message Combinators

[TheBlueMatt]

We've had some users ask about publishing standalone crates that use the LDK Custom Message API to expose some functionality that users can adopt. Sadly, our API isn't super conducive to this - we don't have "combine two custom message handlers into one" API, so the end-user wishing to use the functionality and their own custom messages would have to build a combinator themselves, which is doable but annoying.

It should be trivial to expose a simple custom message combinator (as long as it doesnt worry about duplicate message IDs), so we should probably Just Do It (tm).

[ZmnSCPxj-jr]

In particular, there is no way, given an object implementing CustomMessageHandler, to determine what message types that object will handle or not handle.

It would be undesirable to have the client code provide this information: if the protocol updates to a new version that has more message types, then the client code can go out of sync with the protocol code. It should be provided by the CustomMessageHandler itsaelf, as that is the protocol-implementing code.

It is also undesirable to just claim a single message ID range. Consider this situation: protocol A takes the range M..N. It stabilizes for a while. Then protocol B takes the range L..M-1, and later protocol C takes the range N+1..O. Then protocol A wants to upgrade, and needs an additional message type ID; it wants to be back-compatible with older protocol A versions, so messages in the range M..N should still be understood, but it cannot extend the range in either direction. Protocol A then has to define new message types that are not contiguous with the original range. Thus it would be impossible to define a single message ID range.

Finally, it is undesirable to force protocol-writers to separately define which messages IDs they use, from the code that parses the message ID. i.e. we do not want separate functions fn supports_message_type(&self, message_type: u16) -> bool and fn read<R:Read>(&self, message_type: u16, buffer: R) -> whatever. We want a single function that handles all the message types, because DRY.

[ZmnSCPxj-jr]

So my concrete proposal is this:

trait CustomMessageReader {
    type CustomMessage: Type;

    fn read<R: Read>(&self, message_type: u16) -> Option<Box<dyn FnOnce(R) -> Result<CustomMessage, DecodeError>>>;
}

impl CustomMessageReader for (L, R)
where L: CustomMessageReader, R: CustomMessageReader {
    type CustomMessage = Either<L::CustomMessage, R::CustomMessage>;

    fn read<R: Read>(&self, message_type: u16) {
       let (l, r) = self;
       if let Some(lfn) = l.read(message_type) {
           Some(Box::new(move |buffer| {
              Ok(Left(lfn(buffer)?))
           }))
       } let Some(rfn) = r.read(message_type) {
           Some(Box::new(move |buffer| {
              Ok(Right(rfn(buffer)?))
           }))
       } else {
           None
       }
    }
}

// etc... for other tuple types

Then protocol writers would write:

impl CustomMessageReader for MyProtocol {
    type CustomMessage = MyProtocolMessage;
    fn read<R: Read>(&self, message_type: u16) {
        match message_type {
            MESSAGE_TYPE_FOO => Some(Box::new(|buffer| {
                let payload = parse_foo(buffer)?;
                Ok(MyProtocolMessage::Foo(payload))
            }))
           // ... etc.
            _ => None
        }
    }
}

and end-clients would write:

    let custom_message_handler = (
         MyProtocol::new(/*whatever*/),
         YourProtocol::new(/*whatever*/)
   );

[ZmnSCPxj-jr]

Maybe this can be leveraged instead? https://gist.github.com/kvark/f067ba974446f7c5ce5bd544fe370186#avoid-boxtrait

[jkczyz]

Drafted a simple macro approach in #1832 for consideration. User needs to define the message ranges for each inlcuded handler, though. Could add min and max constants to one of the traits, but then the composite would need to define these, too. Not sure how to do that and what that values should be, especially considering arbitrary nesting. Open to feedback on this approach.

[ZmnSCPxj-jr]

Yeah user needs to define the messages, and that should really be provided by the custom message handler, especially if the custom message handler is upgraded to take a new message ID.

Can it handle noncontiguous message ID ranges? e.g. 1 | 3 | 99? This could be trouble later once more protocols start consuming space on the available message ranges.

If the point is to avoid Box and dynamic allocation, maybe this approach?

pub struct PossibleCustomMessage<H: ComposableCustomMessageHandler, R: Read> {
    pub message_id: u16,
    // ...rest elided ...
}
impl PossibleCustomMessage<H: ComposableCustomMessageHandler, R: Read> {
    /* take self by ownership transfer, so accept is only callable once */
    pub fn accept<F: FnOnce(&mut R) -> Result<H::CustomMessage, DecodeError>>(self, function: F) -> () {
        // dunno how to implement this
    }
}

trait ComposableCustomMessageHandler {
    type CustomMessage: Type;

    fn read<R:Read>(&self, message: PossibleCustomMessage<Self, R>) -> ();
}

Then on the protocl writer end:

struct MyProtocol { /* ... */ }
impl ComposableCustomMessageHandler for MyProtocol {
    type CustomMessage = MyProtocolMessage;

    fn read<R:Read>(&self, message: PossibleCustomMessage<Self, R>) -> () {
        match message.message_id {
            MESSAGE_TYPE_FOO => message.accept(|buffer| {
                let payload = parse_foo(buffer);
                Ok(MyProtocolMessage::Foo(payload))
            })
            _ => ()
        }
    }
}

For the PossibleCustomMessage, I am imagining that it is holding a &mut Option<Result<H::CustomMessage, DecodeError>>, and if accept is called, loads the result into that. Then the caller of read, which owns that, can look at it and determine if it was handled by one protocol or if it will query the next protocol.

Drawback is that we cannot statically check message ids that are recognized. Thing is, we really need to express a set of messages, not a range. Then we need to do the static check of overlapping sets. I doubt current Rust has this at the compiler level. The nearest is match with its 1 | 3 | 99 syntax but I do not know if you can associate such syntaxes to traits.

[TheBlueMatt]

If the point is to avoid Box and dynamic allocation, maybe this approach?

I'm not quite sure what problem the code snippet is trying to solve? The existing message handler API seems to accomplish the same thing as the above snippets? If our goal is to just add a wrapper without statically checking the messages that can be done with very minimal overhead on top of the current trait, and without a macro.

Thing is, we really need to express a set of messages, not a range. Then we need to do the static check of overlapping sets. I doubt current Rust has this at the compiler level. The nearest is match with its 1 | 3 | 99 syntax but I do not know if you can associate such syntaxes to traits.

Kinda, if its a macro call you can pass a pat, which will work exactly like you say, but as far as I know you can't have that in a trait associated, it has to be explicitly given as an argument to the macro directly. One way we could accomplish that is if crates which wish to provide a message handler wrap the macro and then re-export it. eg, in a downstream crate:

#[macro_export
macro_rules use_our_messenger { ($fallback_messenger, $its_range: pat...) => {
   lightning::define_custom_message_handler($fallback_mesesenger, $its_range; ProvidedCustomMessageHandler, 0|4|42);
} }
struct ProvidedCustomMessageHandler{}
impl lightning::CustomMessageHandler for ProvidedCustomMessageHandler {}

[ZmnSCPxj-jr]

The code snippet does not work, as it turns out, because it cannot be used to actually compose. As you noted it is just the same as the current trait anyway, which is not composable.

use_our_messenger is not easily composable either. How do I combine mycrate::use_our_messenger! with yourcrate::use_our_messenger!? You have to use:

mycrate::use_our_messenger! IntermediateMessenger {

}
yourcrate::use_our_messenger! FinalMessenger {
   (IntermediateMessenger, /* what goes here??? */ )
}

[ZmnSCPxj-jr]

Perhaps the simplest solution is just to add a MessageIdNotRecognized variant to DecodeError, and require that CustomMessageHandlers absolutely promise to not consume any bytes on the given Read buffer if they return that error. Then lend the buffer in read by mutable reference instead of transferring ownership. That way, a composition (L: CustomMessageHandler, R: CustomMessageHandler) will just take one, and if it errors with MessageIdNotRecognized will take the other. Then support composition by providing impl<T1: CustomMessageHandler, T2: CustomMessageHandler ....> CustomMessageHandler for (T1, T2 ...) for any sufficiently large number of tuple arguments.

Drawback is absolutely no compile-time checks. Maybe fine since the expected structure of any CustomMessageHandler is just a big match message_id statement terminated by _ => Err(DecodeError::MessageIdNotRecognized) and anything else is sus.

[ZmnSCPxj-jr]

Is something like the below possible with Rust macros?

So for example in mycrate/lib.rs:

lightning::define_composable_tag! pub tag {
    (MyProtocolHandler, MY_PROTOCOL_FOO | MY_PROTOCOL_BAR),
}

then in yourcrate/lib.rs:

lightning::define_composable_tag! pub tag {
    (YourProtocolHandler, YOUR_PROTOCOL_FOO | YOUR_PROTOCOL_BAR )
}

then in the user wallet:

lightning::compose_handler! pub(crate) CombinedProtocolHandler {
    mycrate::tag, yourcrate::tag
}

Secretly, the above desugars to:

mycrate::tag! CombinedProtocolHandler {
   { // macros that still need to be expanded.
      yourcrate::tag
   }
   { // types and patterns already expanded.
   }
}

which desugars to:

yourcrate::tag! CombinedProtocolHandler {
   { // macros that still need to be expanded.
   }
   { // types and patterns already expanded.
     (MyProtocolHandler, MY_PROTOCOL_FOO | MY_PROTOCOL_BAR),
   }
}

which finally desugars to:

lightning::composite_custom_message_handler! CombinedProtocolHandler {
    (MyProtocolHandler, MY_PROTOCOL_FOO | MY_PROTOCOL_BAR),
    (YourProtocolHandler, YOUR_PROTOCOL_FOO | YOUR_PROTOCOL_BAR)
}

That way the patterns are provided by the individual crates, and the crate-exported macros are composable.

[jkczyz]

Is something like the below possible with Rust macros?

I'm not 100% sure on the rules, but most attempts with nested macro calls haven't worked with me. However, I did manage to get this to work with a single pattern in #1832.

--- a/lightning/src/ln/peer_handler.rs
+++ b/lightning/src/ln/peer_handler.rs
@@ -155,6 +155,10 @@ macro_rules! composite_custom_message_handler {
        }
 }
 
+macro_rules! some_pattern {
+       () => { 1..=10 }
+}
+
 composite_custom_message_handler!(
        pub struct CompositeMessageHandler {
                ignoring: IgnoringMessageHandler,
@@ -163,7 +167,7 @@ composite_custom_message_handler!(
 
        pub enum CompositeMessage {
                Infallible(0 | 2 | 4),
-               Infallible2(1..=10 | 99),
+               Infallible2(some_pattern!()),
        }
 );

But it fails with multiple patterns.

--- a/lightning/src/ln/peer_handler.rs
+++ b/lightning/src/ln/peer_handler.rs
@@ -155,6 +155,10 @@ macro_rules! composite_custom_message_handler {
        }
 }
 
+macro_rules! some_pattern {
+       () => { 1..=10 | 99 }
+}
+
 composite_custom_message_handler!(
        pub struct CompositeMessageHandler {
                ignoring: IgnoringMessageHandler,
@@ -163,7 +167,7 @@ composite_custom_message_handler!(
 
        pub enum CompositeMessage {
                Infallible(0 | 2 | 4),
-               Infallible2(1..=10 | 99),
+               Infallible2(some_pattern!()),
        }
 );

error: macro expansion ignores token `|` and any following
   --> lightning/src/ln/peer_handler.rs:159:17
    |
159 |     () => { 1..=10 | 99 }
    |                    ^
...
170 |         Infallible2(some_pattern!()),
    |                     --------------- caused by the macro expansion here
    |
    = note: the usage of `some_pattern!` is likely invalid in pattern context

Looks to be resolved in edition 2021:

https://doc.rust-lang.org/edition-guide/rust-2021/or-patterns-macro-rules.html
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c326fdc0b67d49adc176fc4e293fa149

So we could possibly publish a separate crate with the composite macro, in that case.

[ZmnSCPxj-jr]

What I proposed would be a "typical, everyday" pattern for Scheme macros, and Rust macros look awfully suspiciously like Scheme macros re-applied to a "true" AST rather than s-expressions, but really it does depend on how Rust macros work. I guess having a macro like mycrate::message_pattern! would be acceptable and achieve the goals (1) the crate is what declares the messages it uses and not the client (2) support non-contiguous message IDs. It fails to achieve DRY since the protocol writer has to update both the message_pattern! macro and the actual MyProtocolMessageHandler but I guess that is acceptable.

[TheBlueMatt]

use_our_messenger is not easily composable either. How do I combine mycrate::use_our_messenger! with yourcrate::use_our_messenger!? You have to use:

Yes, I'm not sure I understand why that is an issue? Its pretty clean, if we can do it.

One thing we could do to get there is having the composite_custom_message_handler macro actually define a new macro that can be used to build a new composite. That way we ultimately end up calling through to one final macro that has the full pattern list in it.

[jkczyz]

Tried a workaround for edition 2018:

diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs
index 08ea5535..1a926d9c 100644
--- a/lightning/src/ln/peer_handler.rs
+++ b/lightning/src/ln/peer_handler.rs
@@ -156,9 +156,28 @@ macro_rules! composite_custom_message_handler {
 }
 
 macro_rules! some_pattern {
+       () => { zero_pattern!() | two_pattern!() | four_pattern!() }
+}
+
+macro_rules! zero_pattern {
+       () => { 0 }
+}
+
+macro_rules! two_pattern {
+       () => { 2 }
+}
+
+macro_rules! four_pattern {
+       () => { 4 }
+}
+
+macro_rules! some_pattern1 {
        () => { 1..=10 }
 }
 
+macro_rules! some_pattern2 {
+       () => { 11..=20 }
+}
 composite_custom_message_handler!(
        pub struct CompositeMessageHandler {
                ignoring: IgnoringMessageHandler,
@@ -166,8 +185,8 @@ composite_custom_message_handler!(
        }
 
        pub enum CompositeMessage {
-               Infallible(0 | 2 | 4),
-               Infallible2(some_pattern!()),
+               Infallible(some_pattern!()),
+               Infallible2(some_pattern1!() | some_pattern2!()),
        }
 );

This only works for some_pattern1!() | some_pattern2!(). It doesn't like zero_pattern!() | two_pattern!() | four_pattern!():

error: macro expansion ignores token `|` and any following
   --> lightning/src/ln/peer_handler.rs:159:26
    |
159 |     () => { zero_pattern!() | two_pattern!() | four_pattern!() }
    |                             ^
...
188 |         Infallible(some_pattern!()),
    |                    --------------- caused by the macro expansion here
    |
    = note: the usage of `some_pattern!` is likely invalid in pattern context

@TheBlueMatt Was that what you had in mind?