From 9782aec7f2cf911fbbd3292f0b3df264da1f4ec6 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 21 Aug 2024 15:32:14 +0000 Subject: [PATCH] Remove message type bound on `ResponseInstruction` 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)`. --- lightning/src/ln/channelmanager.rs | 26 +++---- lightning/src/ln/peer_handler.rs | 10 +-- lightning/src/onion_message/async_payments.rs | 2 +- .../src/onion_message/functional_tests.rs | 21 +++--- lightning/src/onion_message/messenger.rs | 75 ++++++++++++------- lightning/src/onion_message/offers.rs | 2 +- 6 files changed, 80 insertions(+), 56 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 167ab0ac8fd..df2cd7bde3d 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -10749,7 +10749,7 @@ where { fn handle_message( &self, message: OffersMessage, context: Option, responder: Option, - ) -> ResponseInstruction { + ) -> Option<(OffersMessage, ResponseInstruction)> { let secp_ctx = &self.secp_ctx; let expanded_key = &self.inbound_payment_key; @@ -10757,13 +10757,13 @@ where OffersMessage::InvoiceRequest(invoice_request) => { let responder = match responder { Some(responder) => responder, - None => return ResponseInstruction::NoResponse, + None => return None, }; let nonce = match context { None if invoice_request.metadata().is_some() => None, Some(OffersContext::InvoiceRequest { nonce }) => Some(nonce), - _ => return ResponseInstruction::NoResponse, + _ => return None, }; let invoice_request = match nonce { @@ -10771,11 +10771,11 @@ where nonce, expanded_key, secp_ctx, ) { Ok(invoice_request) => invoice_request, - Err(()) => return ResponseInstruction::NoResponse, + Err(()) => return None, }, None => match invoice_request.verify_using_metadata(expanded_key, secp_ctx) { Ok(invoice_request) => invoice_request, - Err(()) => return ResponseInstruction::NoResponse, + Err(()) => return None, }, }; @@ -10859,7 +10859,7 @@ where OffersMessage::Invoice(invoice) => { let payment_id = match self.verify_bolt12_invoice(&invoice, context.as_ref()) { Ok(payment_id) => payment_id, - Err(()) => return ResponseInstruction::NoResponse, + Err(()) => return None, }; let logger = WithContext::from( @@ -10871,7 +10871,7 @@ where payment_id, invoice, context, responder, }; self.pending_events.lock().unwrap().push_back((event, None)); - return ResponseInstruction::NoResponse; + return None; } let error = match self.send_payment_for_verified_bolt12_invoice( @@ -10890,14 +10890,14 @@ where }, Err(Bolt12PaymentError::UnexpectedInvoice) | Err(Bolt12PaymentError::DuplicateInvoice) - | Ok(()) => return ResponseInstruction::NoResponse, + | Ok(()) => return None, }; match responder { Some(responder) => responder.respond(OffersMessage::InvoiceError(error)), None => { log_trace!(logger, "No reply path to send error: {:?}", error); - ResponseInstruction::NoResponse + None }, } }, @@ -10909,7 +10909,7 @@ where InvoiceError::from_string("Static invoices not yet supported".to_string()) )) }, - None => return ResponseInstruction::NoResponse, + None => return None, } }, OffersMessage::InvoiceError(invoice_error) => { @@ -10932,7 +10932,7 @@ where _ => {}, } - ResponseInstruction::NoResponse + None }, } } @@ -10956,8 +10956,8 @@ where { fn held_htlc_available( &self, _message: HeldHtlcAvailable, _responder: Option - ) -> ResponseInstruction { - ResponseInstruction::NoResponse + ) -> Option<(ReleaseHeldHtlc, ResponseInstruction)> { + None } fn release_held_htlc(&self, _message: ReleaseHeldHtlc) {} diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index 6b03b365457..bde5454b49c 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/lightning/src/ln/peer_handler.rs @@ -144,21 +144,21 @@ impl OnionMessageHandler for IgnoringMessageHandler { } impl OffersMessageHandler for IgnoringMessageHandler { - fn handle_message(&self, _message: OffersMessage, _context: Option, _responder: Option) -> ResponseInstruction { - ResponseInstruction::NoResponse + fn handle_message(&self, _message: OffersMessage, _context: Option, _responder: Option) -> Option<(OffersMessage, ResponseInstruction)> { + None } } impl AsyncPaymentsMessageHandler for IgnoringMessageHandler { fn held_htlc_available( &self, _message: HeldHtlcAvailable, _responder: Option, - ) -> ResponseInstruction { - ResponseInstruction::NoResponse + ) -> Option<(ReleaseHeldHtlc, ResponseInstruction)> { + None } fn release_held_htlc(&self, _message: ReleaseHeldHtlc) {} } impl CustomOnionMessageHandler for IgnoringMessageHandler { type CustomMessage = Infallible; - fn handle_custom_message(&self, _message: Self::CustomMessage, _context: Option>, _responder: Option) -> ResponseInstruction { + fn handle_custom_message(&self, _message: Self::CustomMessage, _context: Option>, _responder: Option) -> Option<(Infallible, ResponseInstruction)> { // Since we always return `None` in the read the handle method should never be called. unreachable!(); } diff --git a/lightning/src/onion_message/async_payments.rs b/lightning/src/onion_message/async_payments.rs index 4a080101915..dcc52830566 100644 --- a/lightning/src/onion_message/async_payments.rs +++ b/lightning/src/onion_message/async_payments.rs @@ -30,7 +30,7 @@ pub trait AsyncPaymentsMessageHandler { /// the held funds. fn held_htlc_available( &self, message: HeldHtlcAvailable, responder: Option, - ) -> ResponseInstruction; + ) -> Option<(ReleaseHeldHtlc, ResponseInstruction)>; /// Handle a [`ReleaseHeldHtlc`] message. If authentication of the message succeeds, an HTLC /// should be released to the corresponding payee. diff --git a/lightning/src/onion_message/functional_tests.rs b/lightning/src/onion_message/functional_tests.rs index 508e7982b40..875dc57b139 100644 --- a/lightning/src/onion_message/functional_tests.rs +++ b/lightning/src/onion_message/functional_tests.rs @@ -76,8 +76,8 @@ impl Drop for MessengerNode { struct TestOffersMessageHandler {} impl OffersMessageHandler for TestOffersMessageHandler { - fn handle_message(&self, _message: OffersMessage, _context: Option, _responder: Option) -> ResponseInstruction { - ResponseInstruction::NoResponse + fn handle_message(&self, _message: OffersMessage, _context: Option, _responder: Option) -> Option<(OffersMessage, ResponseInstruction)> { + None } } @@ -86,8 +86,8 @@ struct TestAsyncPaymentsMessageHandler {} impl AsyncPaymentsMessageHandler for TestAsyncPaymentsMessageHandler { fn held_htlc_available( &self, _message: HeldHtlcAvailable, _responder: Option, - ) -> ResponseInstruction { - ResponseInstruction::NoResponse + ) -> Option<(ReleaseHeldHtlc, ResponseInstruction)> { + None } fn release_held_htlc(&self, _message: ReleaseHeldHtlc) {} } @@ -174,7 +174,7 @@ impl Drop for TestCustomMessageHandler { impl CustomOnionMessageHandler for TestCustomMessageHandler { type CustomMessage = TestCustomMessage; - fn handle_custom_message(&self, msg: Self::CustomMessage, context: Option>, responder: Option) -> ResponseInstruction { + fn handle_custom_message(&self, msg: Self::CustomMessage, context: Option>, responder: Option) -> Option<(Self::CustomMessage, ResponseInstruction)> { let expectation = self.get_next_expectation(); assert_eq!(msg, expectation.expect); @@ -193,7 +193,7 @@ impl CustomOnionMessageHandler for TestCustomMessageHandler { responder.respond_with_reply_path(response, MessageContext::Custom(context.unwrap_or_else(Vec::new))) }, Some(responder) => responder.respond(response), - None => ResponseInstruction::NoResponse, + None => None } } fn read_custom_message(&self, message_type: u64, buffer: &mut R) -> Result, DecodeError> where Self: Sized { @@ -444,8 +444,9 @@ fn async_response_over_one_blinded_hop() { let response_instruction = nodes[0].custom_message_handler.handle_custom_message(message, None, responder); // 6. Simulate Alice asynchronously responding back to Bob with a response. + let (msg, instructions) = response_instruction.unwrap(); assert_eq!( - nodes[0].messenger.handle_onion_message_response(response_instruction), + nodes[0].messenger.handle_onion_message_response(msg, instructions), Ok(Some(SendSuccess::Buffered)), ); @@ -477,8 +478,9 @@ fn async_response_with_reply_path_succeeds() { alice.custom_message_handler.expect_message_and_response(message.clone()); let response_instruction = alice.custom_message_handler.handle_custom_message(message, None, Some(responder)); + let (msg, instructions) = response_instruction.unwrap(); assert_eq!( - alice.messenger.handle_onion_message_response(response_instruction), + alice.messenger.handle_onion_message_response(msg, instructions), Ok(Some(SendSuccess::Buffered)), ); @@ -516,8 +518,9 @@ fn async_response_with_reply_path_fails() { alice.custom_message_handler.expect_message_and_response(message.clone()); let response_instruction = alice.custom_message_handler.handle_custom_message(message, None, Some(responder)); + let (msg, instructions) = response_instruction.unwrap(); assert_eq!( - alice.messenger.handle_onion_message_response(response_instruction), + alice.messenger.handle_onion_message_response(msg, instructions), Err(SendError::PathNotFound), ); } diff --git a/lightning/src/onion_message/messenger.rs b/lightning/src/onion_message/messenger.rs index 89aa4dbce68..14a89046e9d 100644 --- a/lightning/src/onion_message/messenger.rs +++ b/lightning/src/onion_message/messenger.rs @@ -365,30 +365,24 @@ impl Responder { /// Creates a [`ResponseInstruction::WithoutReplyPath`] for a given response. /// /// Use when the recipient doesn't need to send back a reply to us. - pub fn respond(self, response: T) -> ResponseInstruction { - ResponseInstruction::WithoutReplyPath(OnionMessageResponse { - message: response, - reply_path: self.reply_path, - }) + pub fn respond(self, response: T) -> Option<(T, ResponseInstruction)> { + Some((response, ResponseInstruction::WithoutReplyPath { + send_path: self.reply_path, + })) } /// Creates a [`ResponseInstruction::WithReplyPath`] for a given response. /// /// Use when the recipient needs to send back a reply to us. - pub fn respond_with_reply_path(self, response: T, context: MessageContext) -> ResponseInstruction { - ResponseInstruction::WithReplyPath(OnionMessageResponse { - message: response, - reply_path: self.reply_path, - }, context) + pub fn respond_with_reply_path(self, response: T, context: MessageContext) -> Option<(T, ResponseInstruction)> { + Some((response, ResponseInstruction::WithReplyPath { + send_path: self.reply_path, + context: context, + })) } } -/// This struct contains the information needed to reply to a received message. -pub struct OnionMessageResponse { - message: T, - reply_path: BlindedMessagePath, -} - +/*#[cfg(not(c_bindings))] /// `ResponseInstruction` represents instructions for responding to received messages. pub enum ResponseInstruction { /// Indicates that a response should be sent including a reply path for @@ -401,6 +395,26 @@ pub enum ResponseInstruction { NoResponse, } +#[cfg(c_bindings)]*/ +/// `ResponseInstruction` represents instructions for responding to received messages. +pub enum ResponseInstruction { + /// Indicates that a response should be sent including a reply path for + /// the recipient to respond back. + WithReplyPath { + /// The path over which we'll send our reply. + send_path: BlindedMessagePath, + /// The context to include in the reply path we'll give the recipient so they can respond + /// to us. + context: MessageContext, + }, + /// Indicates that a response should be sent without including a reply path + /// for the recipient to respond back. + WithoutReplyPath { + /// The path over which we'll send our reply. + send_path: BlindedMessagePath, + } +} + /// An [`OnionMessage`] for [`OnionMessenger`] to send. /// /// These are obtained when released from [`OnionMessenger`]'s handlers after which they are @@ -799,7 +813,9 @@ pub trait CustomOnionMessageHandler { /// Called with the custom message that was received, returning a response to send, if any. /// /// The returned [`Self::CustomMessage`], if any, is enqueued to be sent by [`OnionMessenger`]. - fn handle_custom_message(&self, message: Self::CustomMessage, context: Option>, responder: Option) -> ResponseInstruction; + fn handle_custom_message( + &self, message: Self::CustomMessage, context: Option>, responder: Option + ) -> Option<(Self::CustomMessage, ResponseInstruction)>; /// Read a custom message of type `message_type` from `buffer`, returning `Ok(None)` if the /// message type is unknown. @@ -1320,15 +1336,14 @@ where /// the response is prepared and ready for sending, that task can invoke this method to enqueue /// the response for delivery. pub fn handle_onion_message_response( - &self, response: ResponseInstruction + &self, response_message: T, response: ResponseInstruction, ) -> Result, SendError> { - let (response, context) = match response { - ResponseInstruction::WithReplyPath(response, context) => (response, Some(context)), - ResponseInstruction::WithoutReplyPath(response) => (response, None), - ResponseInstruction::NoResponse => return Ok(None), + let (response_path, context) = match response { + ResponseInstruction::WithReplyPath { send_path, context } => (send_path, Some(context)), + ResponseInstruction::WithoutReplyPath { send_path } => (send_path, None), }; - let message_type = response.message.msg_type(); + let message_type = response_message.msg_type(); let reply_path = if let Some(context) = context { match self.create_blinded_path(context) { Ok(reply_path) => Some(reply_path), @@ -1344,7 +1359,7 @@ where } else { None }; self.find_path_and_enqueue_onion_message( - response.message, Destination::BlindedPath(response.reply_path), reply_path, + response_message, Destination::BlindedPath(response_path), reply_path, format_args!( "when responding with {} to an onion message", message_type, @@ -1564,14 +1579,18 @@ where } }; let response_instructions = self.offers_handler.handle_message(msg, context, responder); - let _ = self.handle_onion_message_response(response_instructions); + if let Some((msg, instructions)) = response_instructions { + let _ = self.handle_onion_message_response(msg, instructions); + } }, #[cfg(async_payments)] ParsedOnionMessageContents::AsyncPayments(AsyncPaymentsMessage::HeldHtlcAvailable(msg)) => { let response_instructions = self.async_payments_handler.held_htlc_available( msg, responder ); - let _ = self.handle_onion_message_response(response_instructions); + if let Some((msg, instructions)) = response_instructions { + let _ = self.handle_onion_message_response(msg, instructions); + } }, #[cfg(async_payments)] ParsedOnionMessageContents::AsyncPayments(AsyncPaymentsMessage::ReleaseHeldHtlc(msg)) => { @@ -1587,7 +1606,9 @@ where } }; let response_instructions = self.custom_handler.handle_custom_message(msg, context, responder); - let _ = self.handle_onion_message_response(response_instructions); + if let Some((msg, instructions)) = response_instructions { + let _ = self.handle_onion_message_response(msg, instructions); + } }, } }, diff --git a/lightning/src/onion_message/offers.rs b/lightning/src/onion_message/offers.rs index 28f6d7d6fe7..1042080f8be 100644 --- a/lightning/src/onion_message/offers.rs +++ b/lightning/src/onion_message/offers.rs @@ -47,7 +47,7 @@ pub trait OffersMessageHandler { /// [`OnionMessenger`]: crate::onion_message::messenger::OnionMessenger fn handle_message( &self, message: OffersMessage, context: Option, responder: Option, - ) -> ResponseInstruction; + ) -> Option<(OffersMessage, ResponseInstruction)>; /// Releases any [`OffersMessage`]s that need to be sent. ///