Remove message type bound on ResponseInstruction
#3263
Conversation
|
I'm good with upstreaming this (actually prefer |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3263 +/- ##
==========================================
+ Coverage 89.82% 90.01% +0.19%
==========================================
Files 125 125
Lines 102867 106409 +3542
Branches 102867 106409 +3542
==========================================
+ Hits 92398 95782 +3384
- Misses 7753 7976 +223
+ Partials 2716 2651 -65 ☔ View full report in Codecov by Sentry. |
|
Yeah, I'm fine with this change |
TheBlueMatt
force-pushed
the
2024-08-bindings-om
branch
from
9782aec
to
224c39a
Compare
|
Okay, fixed the excess commit and build....and then got carried away and fixed #3178 and cleaned up the onion message sending API a bit. |
| pub fn respond<T: OnionMessageContents>(self, response: T) -> Option<(T, ResponseInstruction)> { | ||
| Some((response, ResponseInstruction::WithoutReplyPath { | ||
| send_path: self.reply_path, | ||
| })) |
There was a problem hiding this comment.
Should we drop Option from here and have the caller wrap if this will always be Some? The interface is also a little strange in that the message is passed only to be returned. Probably don't need to pass it at all.
There was a problem hiding this comment.
Yea, I was thinking it makes the calling code a bit more readable to return the exact type they need to return, but you're right its a bit awkward to take a parameter which is just immediately returned...I'll swap it.
Oops, was reviewing before you pushed. Will take another look. |
| &self, message: OffersMessage, context: Option<OffersContext>, responder: Option<Responder>, | ||
| ) -> Option<(OffersMessage, ResponseInstruction)>; | ||
| ) -> Option<(OffersMessage, MessageSendInstructions)>; |
There was a problem hiding this comment.
Hmmm... the purpose of Responder was to ensure that the response is only sent over the reply path. But this change makes it possible for a handler to respond over any destination (blinded or not).
It would be preferable to still have something like OnionMessageResponse which is used in the handler interfaces and can only be created by Responder since it is not publicly constructible. And then still have a MessageSendInstructions for the more general interface but with a variant that wraps OnionMessageResponse.
There was a problem hiding this comment.
Hmmm... the purpose of Responder was to ensure that the response is only sent over the reply path. But this change makes it possible for a handler to respond over any destination (blinded or not).
I don't think that's a terrible thing? I mean basically it would no longer be a response but rather just a message they're sending, but, like, okay, if they want to do that 🤷♂️.
There was a problem hiding this comment.
Responder is kinda useless then if you don't need it to construct instructions.
There was a problem hiding this comment.
Yea, kinda? I mean it does give a type description and make the code a bit more readable, but you're right that it could be fully removed and wouldn't change too much. I don't think that's a reason why we should introduce more wrapping to make it useful, though. If you'd rather see it removed happy to do that too, though.
There was a problem hiding this comment.
How about keeping ResponseInstructions for the handlers and converting it into MessageSendInstructions inside `handle_onion_message_response? Then there is no wrapping.
There was a problem hiding this comment.
Right, but still near-identical. Not quite sure I understand the motivation for adding a new type just to restrict the cases that the user doesn't even deal with?
There was a problem hiding this comment.
The motivation is to enforce the correct behavior of only using the reply path for the response. And it prevents users from doing something wrong like manually creating a MessageSendInstructions to return when the responder is None.
There was a problem hiding this comment.
The motivation is to enforce the correct behavior of only using the reply path for the response.
You mean only using the reply path we were given in the message to respond to that message? Is the concern there that someone would accidentally use that reply path to send another message or to receive some message (treating it as our own reply path)? I'm not sure how likely that is, though maybe leaving the "return an identical type from respond as the method returns" thing would help there (without having to define yet another type)?
And it prevents users from doing something wrong like manually creating a MessageSendInstructions to return when the responder is None.
I mean in order to do so they'd have to fill in the Destination with something, which seems like it'd be pretty hard to get wrong?
There was a problem hiding this comment.
You mean only using the reply path we were given in the message to respond to that message? Is the concern there that someone would accidentally use that reply path to send another message or to receive some message (treating it as our own reply path)? I'm not sure how likely that is, though maybe leaving the "return an identical type from
respondas the method returns" thing would help there (without having to define yet another type)?
It's more so having them be confused about how to create the return value and think they need to create a Destination themselves (and thus either give it a node id or create a blinded path) when they should use the Responder to create it. If the return type is ResponseInstructions and the only way to create one is through Responder, then they can't screw it up.
Though I guess ResponseInstructions can't have a OnionMessageResponse (because it has a type parameter). So instead of a nearly identical enum, it would need to be a struct with private constructors:
pub struct ResponseInstructions {
destination: BlindedMessagePath,
include_reply_path: bool,
}I mean in order to do so they'd have to fill in the
Destinationwith something, which seems like it'd be pretty hard to get wrong?
The point is they shouldn't be doing that at all, but our interface allows for it.
There was a problem hiding this comment.
Hmm, so I think I agree with you, but in order to fix #3178 we end up with a ForResponse variant in MessageSendInstructions which I find a bit weird, but is tolerable.
TheBlueMatt
force-pushed
the
2024-08-bindings-om
branch
4 times, most recently
from
f59c9c0
to
d577b24
Compare
| /// Indicates that a message is being sent as a reply to a received message. | ||
| ForReply { | ||
| instructions: ResponseInstruction, | ||
| }, |
There was a problem hiding this comment.
Isn't into_instructions sufficient?
There was a problem hiding this comment.
Hmm, I guess we could make that public. I was thinking of leaving it private to differentiate the public API which seemed like part of the goal. This would (though I didn't bother yet) allow us to log differently if its a reply, but I'm not sure if that's really worth changing the API for. Don't really feel strongly here at all, happy to do whatever you think makes more sense.
There was a problem hiding this comment.
Alternatively, would having "async" versions of the Responder methods returning the appropriate MessageSendInstructions be too confusing? Otherwise, the extra variant here is also fine, IMO.
There was a problem hiding this comment.
Hmm, I guess we could? I kinda prefer the other two options, though, "async" versions just seems like more stuff users have to think about before they use Responder.
| message: response, | ||
| reply_path: self.reply_path, | ||
| }, context) | ||
| pub fn respond_with_reply_path(self, context: MessageContext) -> ResponseInstruction { |
There was a problem hiding this comment.
It looks like this is only used in tests atm. Are we keeping it public for non-LDK use, or for future use, or?
There was a problem hiding this comment.
Yea, for non-LDK use.
There was a problem hiding this comment.
And probably future use...
There was a problem hiding this comment.
Yeah, this is used in #3163
TheBlueMatt
force-pushed
the
2024-08-bindings-om
branch
from
d577b24
to
ce6a125
Compare
TheBlueMatt
force-pushed
the
2024-08-bindings-om
branch
from
438ae15
to
2ddaffd
Compare
|
LGTM when jeff's happy |
TheBlueMatt
force-pushed
the
2024-08-bindings-om
branch
from
2ddaffd
to
f43e85e
Compare
|
Squashed and addressed last nits: $ git diff-tree -U5 2ddaffd3d f43e85ec1
diff --git a/lightning/src/onion_message/messenger.rs b/lightning/src/onion_message/messenger.rs
index a33ee4f27..7d215e365 100644
--- a/lightning/src/onion_message/messenger.rs
+++ b/lightning/src/onion_message/messenger.rs
@@ -384,10 +384,13 @@ impl Responder {
}
/// Instructions for how and where to send the response to an onion message.
#[derive(Clone)]
pub struct ResponseInstruction {
+ /// The destination in a response is always a [`Destination::BlindedPath`] but using a
+ /// [`Destination`] rather than an explicit [`BlindedMessagePath`] simplifies the logic in
+ /// [`OnionMessenger::send_onion_message_internal`] somewhat.
destination: Destination,
context: Option<MessageContext>,
}
impl ResponseInstruction {
@@ -1157,12 +1160,13 @@ where
&self, contents: T, instructions: MessageSendInstructions, log_suffix: fmt::Arguments,
) -> Result<SendSuccess, SendError> {
let (destination, reply_path) = match instructions {
MessageSendInstructions::WithSpecifiedReplyPath { destination, reply_path } =>
(destination, Some(reply_path)),
- MessageSendInstructions::WithReplyPath { destination, context }|
- MessageSendInstructions::ForReply { instructions: ResponseInstruction { destination, context: Some(context) } } => {
+ MessageSendInstructions::WithReplyPath { destination, context }
+ |MessageSendInstructions::ForReply { instructions: ResponseInstruction { destination, context: Some(context) } } =>
+ {
match self.create_blinded_path(context) {
Ok(reply_path) => (destination, Some(reply_path)),
Err(err) => {
log_trace!(
self.logger,
@@ -1171,12 +1175,12 @@ where
);
return Err(err);
}
}
},
- MessageSendInstructions::WithoutReplyPath { destination }|
- MessageSendInstructions::ForReply { instructions: ResponseInstruction { destination, context: None } } =>
+ MessageSendInstructions::WithoutReplyPath { destination }
+ |MessageSendInstructions::ForReply { instructions: ResponseInstruction { destination, context: None } } =>
(destination, None),
};
let mut logger = WithContext::from(&self.logger, None, None, None);
let result = self.find_path(destination).and_then(|path| { |
TheBlueMatt
force-pushed
the
2024-08-bindings-om
branch
from
f43e85e
to
d09e732
Compare
|
Oops, missed one more: $ git diff-tree -U2 f43e85ec1 d09e7320f
diff --git a/lightning/src/onion_message/async_payments.rs b/lightning/src/onion_message/async_payments.rs
index a0e56cdc5..279d9fddb 100644
--- a/lightning/src/onion_message/async_payments.rs
+++ b/lightning/src/onion_message/async_payments.rs
@@ -12,5 +12,4 @@
use crate::io;
use crate::ln::msgs::DecodeError;
-#[cfg(not(c_bindings))]
use crate::onion_message::messenger::{Responder, ResponseInstruction, MessageSendInstructions};
use crate::onion_message::packet::OnionMessageContents; |
Our onion message handlers generally have one or more methods which return a `ResponseInstruction` parameterized with the expected message type (enum) of the message handler. Sadly, that restriction is impossible to represent in our bindings - our bindings concretize all LDK structs, enums, and traits into a single concrete instance with generics set to our concrete trait instances (which hold a jump table). This prevents us from having multiple instances of `ResponseInstruction` structs for different message types. Our bindings do, however, support different parameterizations of standard enums, including `Option`s and tuples. In order to support bindings for the onion message handlers, we are thus forced into std types bound by expected message types, which we do here by making `ResponseInstruction` contain only the instructions and generally using it as a two-tuple of `(message, ResponseInstruction)`.
In the coming commits we'll use the `ResponseInstruction` enum's contents for all messages, allowing message senders to rely on the in-`OnionMessegner` reply path selection logic. In order to do so and avoid users confusing the new `MessageSendInstructions` for `ResponseInstruction`, we leave `ResponseInstruction` as a now-unconstructible struct which wraps a `MessageSendInstructions`.
In the next commit we'll use `MessageSendInstructions` for all messages, so it needs the ability to take any `Destination`, not only a `Destination::Blinded`.
Now that the `MessageRouter` can `create_blinded_paths` forcing callers of the `OnionMessenger` to provide it with a reply path up front is unnecessary complexity, doubly so in message handlers. Here we take the first step towards untangling that, moving from `PendingOnionMessage` to `MessageSendInstructions` for the outbound message queue in `CustomMessageHandler`. Better, we can also drop the `c_bindings`-specific message queue variant, unifying the APIs.
Now that the `MessageRouter` can `create_blinded_paths` forcing callers of the `OnionMessenger` to provide it with a reply path up front is unnecessary complexity, doubly so in message handlers. Here we take the next step towards untangling that, moving from `PendingOnionMessage` to `MessageSendInstructions` for the outbound message queue in `OffersMessageHandler`. Better, we can also drop the `c_bindings`-specific message queue variant, unifying the APIs. Because `ChannelManager` needs to actually control the reply path set in individual messages, however, we have to halfway this patch, adding a new `MessageSendInstructions` variant that allows specifying the `reply_path` explicitly. Still, because other message handlers are moving this way, its nice to be consistent.
Now that the `MessageRouter` can `create_blinded_paths` forcing callers of the `OnionMessenger` to provide it with a reply path up front is unnecessary complexity, doubly so in message handlers. Here we take the next step towards untangling that, moving from `PendingOnionMessage` to `MessageSendInstructions` for the outbound message queue in `AsyncPaymentsMessageHandler`. Better, we can also drop the `c_bindings`-specific message queue variant, unifying the APIs. Here we also drop `PendingOnionMessage` entirely.
This lets callers include a `reply_path` without doing the path-finding at the callsite, utilizing the built-in path-finding logic in `OnionMessenger`
In order to allow onion message handlers to reply asynchronously without introducing a circular dependency graph, the message handlers need to be able to send replies described by `MessageSendInstructions`. This allows them to send replies via the normal message queuing (i.e. without making a function call to `OnionMessenger`). Here we enable that by adding a `MessageSendInstructions::ForReply` variant which holds `ReplyInstruction`s. Fixes lightningdevkit#3178
TheBlueMatt
dismissed stale reviews from valentinewallace and jkczyz
via
47b527a
TheBlueMatt
force-pushed
the
2024-08-bindings-om
branch
from
d09e732
to
47b527a
Compare
|
Grrrrr, screwed up the fuzzers: $ git diff-tree -U1 d09e7320f 47b527a65
diff --git a/fuzz/src/onion_message.rs b/fuzz/src/onion_message.rs
index 0de791c55..33458436c 100644
--- a/fuzz/src/onion_message.rs
+++ b/fuzz/src/onion_message.rs
@@ -18,4 +18,4 @@ use lightning::onion_message::async_payments::{
use lightning::onion_message::messenger::{
- CustomOnionMessageHandler, Destination, MessageRouter, OnionMessagePath, OnionMessenger,
- PendingOnionMessage, Responder, ResponseInstruction,
+ CustomOnionMessageHandler, Destination, MessageRouter, MessageSendInstructions,
+ OnionMessagePath, OnionMessenger, Responder, ResponseInstruction,
};
@@ -111,4 +111,4 @@ impl OffersMessageHandler for TestOffersMessageHandler {
_responder: Option<Responder>,
- ) -> ResponseInstruction<OffersMessage> {
- ResponseInstruction::NoResponse
+ ) -> Option<(OffersMessage, ResponseInstruction)> {
+ None
}
@@ -121,9 +121,11 @@ impl AsyncPaymentsMessageHandler for TestAsyncPaymentsMessageHandler {
&self, message: HeldHtlcAvailable, responder: Option<Responder>,
- ) -> ResponseInstruction<ReleaseHeldHtlc> {
+ ) -> Option<(ReleaseHeldHtlc, ResponseInstruction)> {
let responder = match responder {
Some(resp) => resp,
- None => return ResponseInstruction::NoResponse,
+ None => return None,
};
- responder
- .respond(ReleaseHeldHtlc { payment_release_secret: message.payment_release_secret })
+ Some((
+ ReleaseHeldHtlc { payment_release_secret: message.payment_release_secret },
+ responder.respond(),
+ ))
}
@@ -160,6 +162,6 @@ impl CustomOnionMessageHandler for TestCustomMessageHandler {
responder: Option<Responder>,
- ) -> ResponseInstruction<Self::CustomMessage> {
+ ) -> Option<(Self::CustomMessage, ResponseInstruction)> {
match responder {
- Some(responder) => responder.respond(message),
- None => ResponseInstruction::NoResponse,
+ Some(responder) => Some((message, responder.respond())),
+ None => None,
}
@@ -173,3 +175,3 @@ impl CustomOnionMessageHandler for TestCustomMessageHandler {
}
- fn release_pending_custom_messages(&self) -> Vec<PendingOnionMessage<Self::CustomMessage>> {
+ fn release_pending_custom_messages(&self) -> Vec<(TestCustomMessage, MessageSendInstructions)> {
vec![]
diff --git a/lightning/src/onion_message/async_payments.rs b/lightning/src/onion_message/async_payments.rs
index 279d9fddb..37e4a5341 100644
--- a/lightning/src/onion_message/async_payments.rs
+++ b/lightning/src/onion_message/async_payments.rs
@@ -13,3 +13,3 @@ use crate::io;
use crate::ln::msgs::DecodeError;
-use crate::onion_message::messenger::{Responder, ResponseInstruction, MessageSendInstructions};
+use crate::onion_message::messenger::{MessageSendInstructions, Responder, ResponseInstruction};
use crate::onion_message::packet::OnionMessageContents; |
Our onion message handlers generally have one or more methods which return a
ResponseInstructionparameterized with the expected message type (enum) of the message handler.Sadly, that restriction is impossible to represent in our bindings - our bindings concretize all LDK structs, enums, and traits into a single concrete instance with generics set to our concrete trait instances (which hold a jump table). This prevents us from having multiple instances of
ResponseInstructionstructs for different message types.Our bindings do, however, support different parameterizations of standard enums, including
Options and tuples.In order to support bindings for the onion message handlers, we are thus forced into std types bound by expected message types, which we do here by making
ResponseInstructioncontain only the instructions and generally using it as a two-tuple of(message, ResponseInstruction).I'm torn on whether this belongs upstream - its not quite as nice an API, but its really not all that much different, so it seems kinda fine. We could also make this a bindings-specific change, but that's yet more diff for the bindings, in an area that is set to grow over time as we use OMs more.