Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Macro for composing custom message handlers #1832

Merged
merged 4 commits into from
Feb 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ jobs:
build-no-std: true
build-futures: true
build-tx-sync: true
- toolchain: stable
test-custom-message: true
- toolchain: beta
platform: macos-latest
build-net-tokio: true
Expand All @@ -54,6 +56,8 @@ jobs:
build-no-std: true
build-futures: true
build-tx-sync: true
- toolchain: beta
test-custom-message: true
- toolchain: 1.41.1
build-no-std: false
test-log-variants: true
Expand Down Expand Up @@ -226,6 +230,11 @@ jobs:
RUSTFLAGS="-C link-dead-code" cargo test --verbose --color always --features rpc-client
RUSTFLAGS="-C link-dead-code" cargo test --verbose --color always --features rpc-client,rest-client
RUSTFLAGS="-C link-dead-code" cargo test --verbose --color always --features rpc-client,rest-client,tokio
- name: Test Custom Message Macros on Rust ${{ matrix.toolchain }}
if: "matrix.test-custom-message"
run: |
cd lightning-custom-message
cargo test --verbose --color always
- name: Install deps for kcov
if: matrix.coverage
run: |
Expand Down
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ Cargo.lock
.idea
lightning/target
lightning/ldk-net_graph-*.bin
lightning-custom-message/target
no-std-check/target

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ members = [
]

exclude = [
"lightning-custom-message",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason it's excluded?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to compile using cargo +1.41.1 check gives:

error: failed to parse manifest at `/Users/jkczyz/src/rust-lightning/lightning-custom-message/Cargo.toml`

Caused by:
  failed to parse the `edition` key

Caused by:
  supported edition values are `2015` or `2018`, but `2021` is unknown

But after rebasing, it looks like that's also a problem for one one of lightning-transaction-sync's dependencies, as well. I guess we just can't build the entire workspace with an older rust version now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I guess CI doesn't try to build the workspace at all then since it's passing without excluding lightning-transaction-sync. We probably shouldn't break the default way to build for users on our MSRV, so we should exclude lightning-transaction-sync as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, correction, the above command also doesn't work because lightning-net-tokio requires a higher version. So moving lightning-custom-message back into the members should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoke to soon. CI is unhappy because of the edition:

error: failed to parse manifest at `/home/runner/work/rust-lightning/rust-lightning/lightning-custom-message/Cargo.toml`

Caused by:
  failed to parse the `edition` key

Caused by:
  supported edition values are `2015` or `2018, but `2021` is unknown

"no-std-check",
]

Expand Down
18 changes: 18 additions & 0 deletions lightning-custom-message/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
[package]
name = "lightning-custom-message"
version = "0.0.113"
authors = ["Jeffrey Czyz"]
license = "MIT OR Apache-2.0"
repository = "http://github.com/lightningdevkit/rust-lightning"
description = """
Utilities for supporting custom peer-to-peer messages in LDK.
"""
edition = "2021"

[package.metadata.docs.rs]
all-features = true
rustdoc-args = ["--cfg", "docsrs"]

[dependencies]
bitcoin = "0.29.0"
lightning = { version = "0.0.113", path = "../lightning" }
310 changes: 310 additions & 0 deletions lightning-custom-message/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,310 @@
//! Utilities for supporting custom peer-to-peer messages in LDK.
//!
//! [BOLT 1] specifies a custom message type range for use with experimental or application-specific
//! messages. While a [`CustomMessageHandler`] can be defined to support more than one message type,
//! defining such a handler requires a significant amount of boilerplate and can be error prone.
//!
//! This crate provides the [`composite_custom_message_handler`] macro for easily composing
//! pre-defined custom message handlers into one handler. The resulting handler can be further
//! composed with other custom message handlers using the same macro.
//!
//! The following example demonstrates defining a `FooBarHandler` to compose separate handlers for
//! `Foo` and `Bar` messages, and further composing it with a handler for `Baz` messages.
//!
//!```
//! # extern crate bitcoin;
//! extern crate lightning;
//! #[macro_use]
//! extern crate lightning_custom_message;
//!
//! # use bitcoin::secp256k1::PublicKey;
//! # use lightning::io;
//! # use lightning::ln::msgs::{DecodeError, LightningError};
//! use lightning::ln::peer_handler::CustomMessageHandler;
//! use lightning::ln::wire::{CustomMessageReader, self};
//! use lightning::util::ser::Writeable;
//! # use lightning::util::ser::Writer;
//!
//! // Assume that `FooHandler` and `BarHandler` are defined in one crate and `BazHandler` is
//! // defined in another crate, handling messages `Foo`, `Bar`, and `Baz`, respectively.
//!
//! #[derive(Debug)]
//! pub struct Foo;
//!
//! macro_rules! foo_type_id {
//! () => { 32768 }
//! }
//!
//! impl wire::Type for Foo {
//! fn type_id(&self) -> u16 { foo_type_id!() }
//! }
//! impl Writeable for Foo {
//! // ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: tabs not spaces

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I believe all (most?) of our doctests use spaces. IIRC, there was some weirdness around the leading "//! " and the first tab not indenting enough. Especially, if the space after the comment was skipped in lieu of the first tab.

//! # fn write<W: Writer>(&self, _: &mut W) -> Result<(), io::Error> {
//! # unimplemented!()
//! # }
//! }
//!
//! pub struct FooHandler;
//!
//! impl CustomMessageReader for FooHandler {
//! // ...
//! # type CustomMessage = Foo;
//! # fn read<R: io::Read>(
//! # &self, _message_type: u16, _buffer: &mut R
//! # ) -> Result<Option<Self::CustomMessage>, DecodeError> {
//! # unimplemented!()
//! # }
//! }
//! impl CustomMessageHandler for FooHandler {
//! // ...
//! # fn handle_custom_message(
//! # &self, _msg: Self::CustomMessage, _sender_node_id: &PublicKey
//! # ) -> Result<(), LightningError> {
//! # unimplemented!()
//! # }
//! # fn get_and_clear_pending_msg(&self) -> Vec<(PublicKey, Self::CustomMessage)> {
//! # unimplemented!()
//! # }
//! }
//!
//! #[derive(Debug)]
//! pub struct Bar;
//!
//! macro_rules! bar_type_id {
//! () => { 32769 }
//! }
//!
//! impl wire::Type for Bar {
//! fn type_id(&self) -> u16 { bar_type_id!() }
//! }
//! impl Writeable for Bar {
//! // ...
//! # fn write<W: Writer>(&self, _: &mut W) -> Result<(), io::Error> {
//! # unimplemented!()
//! # }
//! }
//!
//! pub struct BarHandler;
//!
//! impl CustomMessageReader for BarHandler {
//! // ...
//! # type CustomMessage = Bar;
//! # fn read<R: io::Read>(
//! # &self, _message_type: u16, _buffer: &mut R
//! # ) -> Result<Option<Self::CustomMessage>, DecodeError> {
//! # unimplemented!()
//! # }
//! }
//! impl CustomMessageHandler for BarHandler {
//! // ...
//! # fn handle_custom_message(
//! # &self, _msg: Self::CustomMessage, _sender_node_id: &PublicKey
//! # ) -> Result<(), LightningError> {
//! # unimplemented!()
//! # }
//! # fn get_and_clear_pending_msg(&self) -> Vec<(PublicKey, Self::CustomMessage)> {
//! # unimplemented!()
//! # }
//! }
//!
//! #[derive(Debug)]
//! pub struct Baz;
//!
//! macro_rules! baz_type_id {
//! () => { 32770 }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be cool to demonstrate baz supporting two different message ids internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably better left for CustomMessageHandler docs?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well with the macro_rules-specific stuff for this crate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like what is done using the FooBarHandler? I thought you had meant showing a leaf handler handling more than one message.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I did mean a leaf handler handling more than one message feeding into one MessageHandler. I guess it doesn't matter, though, as long as the documentation mentions that you can do any pattern (which it does) and maybe mention in the example that they don't have to be in different crate(s).

//! }
//!
//! impl wire::Type for Baz {
//! fn type_id(&self) -> u16 { baz_type_id!() }
//! }
//! impl Writeable for Baz {
//! // ...
//! # fn write<W: Writer>(&self, _: &mut W) -> Result<(), io::Error> {
//! # unimplemented!()
//! # }
//! }
//!
//! pub struct BazHandler;
//!
//! impl CustomMessageReader for BazHandler {
//! // ...
//! # type CustomMessage = Baz;
//! # fn read<R: io::Read>(
//! # &self, _message_type: u16, _buffer: &mut R
//! # ) -> Result<Option<Self::CustomMessage>, DecodeError> {
//! # unimplemented!()
//! # }
//! }
//! impl CustomMessageHandler for BazHandler {
//! // ...
//! # fn handle_custom_message(
//! # &self, _msg: Self::CustomMessage, _sender_node_id: &PublicKey
//! # ) -> Result<(), LightningError> {
//! # unimplemented!()
//! # }
//! # fn get_and_clear_pending_msg(&self) -> Vec<(PublicKey, Self::CustomMessage)> {
//! # unimplemented!()
//! # }
//! }
//!
//! # fn main() {
//! // The first crate may define a handler composing `FooHandler` and `BarHandler` and export the
//! // corresponding message type ids as a macro to use in further composition.
//!
//! composite_custom_message_handler!(
//! pub struct FooBarHandler {
//! foo: FooHandler,
//! bar: BarHandler,
//! }
//!
//! pub enum FooBarMessage {
//! Foo(foo_type_id!()),
//! Bar(bar_type_id!()),
//! }
//! );
//!
//! #[macro_export]
//! macro_rules! foo_bar_type_ids {
//! () => { foo_type_id!() | bar_type_id!() }
//! }
//!
//! // Another crate can then define a handler further composing `FooBarHandler` with `BazHandler`
//! // and similarly export the composition of message type ids as a macro.
//!
//! composite_custom_message_handler!(
//! pub struct FooBarBazHandler {
//! foo_bar: FooBarHandler,
//! baz: BazHandler,
//! }
//!
//! pub enum FooBarBazMessage {
//! FooBar(foo_bar_type_ids!()),
//! Baz(baz_type_id!()),
//! }
//! );
Comment on lines +175 to +185
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason someone would opt for using the macro multiple times like this as opposed to just listing each message + handler in one? i.e.

composite_custom_message_handler!(
  pub struct FooBarBazHandler {
    foo: FooHandler,
    bar: BarHandler,
    baz: BazHandler,
  }
  ...
}

Also is there significance to the foo_bar_type_ids being the | of the two type ids or was that mostly arbitrary? Otherwise, I thought the FooBar example was very helpful/straightforward!

Copy link
Contributor Author

@jkczyz jkczyz Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason someone would opt for using the macro multiple times like this as opposed to just listing each message + handler in one? i.e.

composite_custom_message_handler!(
  pub struct FooBarBazHandler {
    foo: FooHandler,
    bar: BarHandler,
    baz: BazHandler,
  }
  ...
}

That is also possible, though the example is meant to demonstrate arbitrary composition. This may be useful when FooBarHandler is defined in one crate and you want to use it along with BazHandler in your own crate. I've added some comments to the example to clarify this use case.

Also is there significance to the foo_bar_type_ids being the | of the two type ids or was that mostly arbitrary? Otherwise, I thought the FooBar example was very helpful/straightforward!

Yes, the macro matcher $pattern uses a pat specifier, meaning it can be anything rust considers a pattern, including the composition of patterns using |.

https://doc.rust-lang.org/reference/patterns.html

Someone using the macro could use a range of type ids instead, e.g., 32768..=32769. Though the | approach is preferred for the reason given in the macro's docs.

Copy link
Contributor

@alecchendev alecchendev Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is also possible, though the example is meant to demonstrate arbitrary composition. This may be useful when FooBarHandler is defined in one crate and you want to use it along with BazHandler in your own crate. I've added some comments to the example to clarify this use case.

Ah makes sense!

Yes, the macro matcher $pattern uses a pat specifier, meaning it can be anything rust considers a pattern, including the composition of patterns using |.

Ohh I see I didn't realize it was being used as a pattern I thought it was doing a bitwise OR of the two type ids, that's cool.

//!
//! #[macro_export]
//! macro_rules! foo_bar_baz_type_ids {
//! () => { foo_bar_type_ids!() | baz_type_id!() }
//! }
//! # }
//!```
//!
//! [BOLT 1]: https://github.com/lightning/bolts/blob/master/01-messaging.md
//! [`CustomMessageHandler`]: crate::lightning::ln::peer_handler::CustomMessageHandler

#![doc(test(no_crate_inject, attr(deny(warnings))))]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is a no_crate_inject and why are we denying compilation warnings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no_crate_inject is needed otherwise we can't add #[macro_use] on the crate in the doc test.

For warnings, I was testing out unreachable patterns in the doc test, so needed that for it fail. I can remove it now, I suppose.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I just generally worry about deny(warnings) cause that means CI will start failing on a new rustc without us touching anything. We'll fix warnings when we find them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this only affects doc tests.


pub extern crate bitcoin;
pub extern crate lightning;

/// Defines a composite type implementing [`CustomMessageHandler`] (and therefore also implementing
/// [`CustomMessageReader`]), along with a corresponding enumerated custom message [`Type`], from
/// one or more previously defined custom message handlers.
///
/// Useful for parameterizing [`PeerManager`] with custom message handling for one or more sets of
/// custom messages. Message type ids may be given as a valid `match` pattern, including ranges,
/// though using OR-ed literal patterns is preferred in order to catch unreachable code for
/// conflicting handlers.
///
/// See [crate documentation] for example usage.
///
/// [`CustomMessageHandler`]: crate::lightning::ln::peer_handler::CustomMessageHandler
/// [`CustomMessageReader`]: crate::lightning::ln::wire::CustomMessageReader
/// [`Type`]: crate::lightning::ln::wire::Type
/// [`PeerManager`]: crate::lightning::ln::peer_handler::PeerManager
/// [crate documentation]: self
#[macro_export]
macro_rules! composite_custom_message_handler {
(
$handler_visibility:vis struct $handler:ident {
$($field_visibility:vis $field:ident: $type:ty),* $(,)*
}

$message_visibility:vis enum $message:ident {
$($variant:ident($pattern:pat)),* $(,)*
}
) => {
#[allow(missing_docs)]
$handler_visibility struct $handler {
$(
$field_visibility $field: $type,
)*
}

#[allow(missing_docs)]
#[derive(Debug)]
$message_visibility enum $message {
$(
$variant(<$type as $crate::lightning::ln::wire::CustomMessageReader>::CustomMessage),
)*
}

impl $crate::lightning::ln::peer_handler::CustomMessageHandler for $handler {
fn handle_custom_message(
&self, msg: Self::CustomMessage, sender_node_id: &$crate::bitcoin::secp256k1::PublicKey
) -> Result<(), $crate::lightning::ln::msgs::LightningError> {
match msg {
$(
$message::$variant(message) => {
$crate::lightning::ln::peer_handler::CustomMessageHandler::handle_custom_message(
&self.$field, message, sender_node_id
)
},
)*
}
}

fn get_and_clear_pending_msg(&self) -> Vec<($crate::bitcoin::secp256k1::PublicKey, Self::CustomMessage)> {
vec![].into_iter()
$(
.chain(
self.$field
.get_and_clear_pending_msg()
.into_iter()
.map(|(pubkey, message)| (pubkey, $message::$variant(message)))
)
)*
.collect()
}
}

impl $crate::lightning::ln::wire::CustomMessageReader for $handler {
type CustomMessage = $message;
fn read<R: $crate::lightning::io::Read>(
&self, message_type: u16, buffer: &mut R
) -> Result<Option<Self::CustomMessage>, $crate::lightning::ln::msgs::DecodeError> {
match message_type {
$(
$pattern => match <$type>::read(&self.$field, message_type, buffer)? {
None => unreachable!(),
Some(message) => Ok(Some($message::$variant(message))),
},
)*
_ => Ok(None),
}
}
}

impl $crate::lightning::ln::wire::Type for $message {
fn type_id(&self) -> u16 {
match self {
$(
Self::$variant(message) => message.type_id(),
)*
}
}
}

impl $crate::lightning::util::ser::Writeable for $message {
fn write<W: $crate::lightning::util::ser::Writer>(&self, writer: &mut W) -> Result<(), $crate::lightning::io::Error> {
match self {
$(
Self::$variant(message) => message.write(writer),
)*
}
}
}
}
}
Loading