Provide Common Base For Custom Message Combinators #1813

TheBlueMatt · 2022-10-28T22:28:25Z

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 · 2022-10-28T23:29:18Z

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 · 2022-10-28T23:38:22Z

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 · 2022-11-03T10:50:23Z

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

jkczyz · 2022-11-04T12:59:14Z

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 · 2022-11-04T21:32:04Z

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 · 2022-11-04T22:26:18Z

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 · 2022-11-05T00:02:31Z

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 · 2022-11-05T00:12:12Z

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 · 2022-11-05T02:13:45Z

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 · 2022-11-08T03:20:54Z

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 · 2022-11-08T08:17:46Z

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 · 2022-11-08T19:16:01Z

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 · 2022-11-09T17:46:12Z

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?

jkczyz · 2022-11-09T17:48:25Z

Oddly, it doesn't work there but is fine if I replace some_pattern!() with its expansion.

jkczyz · 2022-11-09T17:57:19Z

Here's the nested version exhibiting the same problem:

diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs
index 08ea5535..70feacb0 100644
--- a/lightning/src/ln/peer_handler.rs
+++ b/lightning/src/ln/peer_handler.rs
@@ -155,10 +155,30 @@ macro_rules! composite_custom_message_handler {
        }
 }
 
-macro_rules! some_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 }
+}
+
+macro_rules! all_pattern {
+       () => { zero_pattern!() | two_pattern!() | four_pattern!() | some_pattern1!() | some_pattern2!() }
+}
+
 composite_custom_message_handler!(
        pub struct CompositeMessageHandler {
                ignoring: IgnoringMessageHandler,
@@ -166,8 +186,18 @@ composite_custom_message_handler!(
        }
 
        pub enum CompositeMessage {
-               Infallible(0 | 2 | 4),
-               Infallible2(some_pattern!()),
+               Infallible(zero_pattern!() | two_pattern!() | four_pattern!()),
+               Infallible2(some_pattern1!() | some_pattern2!()),
+       }
+);
+
+composite_custom_message_handler!(
+       pub struct OuterCompositeMessageHandler {
+               inner: CompositeMessageHandler,
+       }
+
+       pub enum OuterCompositeMessage {
+               Inner(all_pattern!()),
        }
 );

TheBlueMatt · 2022-12-14T19:44:32Z

Slipping to 114, sadly it doesn't look like there's any good way to actually do this with current Rust.

jkczyz · 2023-01-03T19:55:06Z

I took some time over the holidays to update and clean up #1832. The macro is defined in a separate crate using Rust edition 2021. This allows nesting without needing to explicitly repeat the type ids from the reused handler. This is important as otherwise updating to a new version of the reused handler may not work properly if the updated version covered more type ids.

@ZmnSCPxj-jr could you see if this covers your concerns. The docs in the PR has an example, but essentially someone may use the macro to publish their handler along with a macro giving the type pattern. If each message has its own handler, then it should be DRY. Then anyone can use these (i.e., the published handler and type id macro) to compose it with their own custom handler in an arbitrarily nested manner.

TheBlueMatt · 2023-01-03T23:19:30Z

The changes there basically LGTM. Would like to see if @ZmnSCPxj-jr agrees and then we can ship it 🎉

TheBlueMatt added this to the 0.0.113 milestone Oct 28, 2022

jkczyz mentioned this issue Nov 4, 2022

Macro for composing custom message handlers #1832

Merged

TheBlueMatt modified the milestones: 0.0.113, 0.0.114 Dec 14, 2022

TheBlueMatt closed this as completed in #1832 Feb 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide Common Base For Custom Message Combinators #1813

Provide Common Base For Custom Message Combinators #1813

TheBlueMatt commented Oct 28, 2022

ZmnSCPxj-jr commented Oct 28, 2022 •

edited

Loading

ZmnSCPxj-jr commented Oct 28, 2022 •

edited

Loading

ZmnSCPxj-jr commented Nov 3, 2022

jkczyz commented Nov 4, 2022

ZmnSCPxj-jr commented Nov 4, 2022 •

edited

Loading

TheBlueMatt commented Nov 4, 2022

ZmnSCPxj-jr commented Nov 5, 2022 •

edited

Loading

ZmnSCPxj-jr commented Nov 5, 2022 •

edited

Loading

ZmnSCPxj-jr commented Nov 5, 2022

jkczyz commented Nov 8, 2022

ZmnSCPxj-jr commented Nov 8, 2022

TheBlueMatt commented Nov 8, 2022

jkczyz commented Nov 9, 2022

jkczyz commented Nov 9, 2022

jkczyz commented Nov 9, 2022

TheBlueMatt commented Dec 14, 2022

jkczyz commented Jan 3, 2023

TheBlueMatt commented Jan 3, 2023

Provide Common Base For Custom Message Combinators #1813

Provide Common Base For Custom Message Combinators #1813

Comments

TheBlueMatt commented Oct 28, 2022

ZmnSCPxj-jr commented Oct 28, 2022 • edited Loading

ZmnSCPxj-jr commented Oct 28, 2022 • edited Loading

ZmnSCPxj-jr commented Nov 3, 2022

jkczyz commented Nov 4, 2022

ZmnSCPxj-jr commented Nov 4, 2022 • edited Loading

TheBlueMatt commented Nov 4, 2022

ZmnSCPxj-jr commented Nov 5, 2022 • edited Loading

ZmnSCPxj-jr commented Nov 5, 2022 • edited Loading

ZmnSCPxj-jr commented Nov 5, 2022

jkczyz commented Nov 8, 2022

ZmnSCPxj-jr commented Nov 8, 2022

TheBlueMatt commented Nov 8, 2022

jkczyz commented Nov 9, 2022

jkczyz commented Nov 9, 2022

jkczyz commented Nov 9, 2022

TheBlueMatt commented Dec 14, 2022

jkczyz commented Jan 3, 2023

TheBlueMatt commented Jan 3, 2023

ZmnSCPxj-jr commented Oct 28, 2022 •

edited

Loading

ZmnSCPxj-jr commented Oct 28, 2022 •

edited

Loading

ZmnSCPxj-jr commented Nov 4, 2022 •

edited

Loading

ZmnSCPxj-jr commented Nov 5, 2022 •

edited

Loading

ZmnSCPxj-jr commented Nov 5, 2022 •

edited

Loading