From cd93a41bd690373933d42e9b78e4b16c75be9f93 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)`. --- fuzz/src/onion_message.rs | 20 ++--- lightning/src/ln/channelmanager.rs | 44 +++++----- lightning/src/ln/peer_handler.rs | 10 +-- lightning/src/onion_message/async_payments.rs | 2 +- .../src/onion_message/functional_tests.rs | 25 +++--- lightning/src/onion_message/messenger.rs | 80 ++++++++++--------- lightning/src/onion_message/offers.rs | 2 +- 7 files changed, 97 insertions(+), 86 deletions(-) diff --git a/fuzz/src/onion_message.rs b/fuzz/src/onion_message.rs index 0de791c55..97e8ae243 100644 --- a/fuzz/src/onion_message.rs +++ b/fuzz/src/onion_message.rs @@ -109,8 +109,8 @@ impl OffersMessageHandler for TestOffersMessageHandler { fn handle_message( &self, _message: OffersMessage, _context: Option, _responder: Option, - ) -> ResponseInstruction { - ResponseInstruction::NoResponse + ) -> Option<(OffersMessage, ResponseInstruction)> { + None } } @@ -119,13 +119,15 @@ struct TestAsyncPaymentsMessageHandler {} impl AsyncPaymentsMessageHandler for TestAsyncPaymentsMessageHandler { fn held_htlc_available( &self, message: HeldHtlcAvailable, responder: Option, - ) -> ResponseInstruction { + ) -> 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(), + )) } fn release_held_htlc(&self, _message: ReleaseHeldHtlc) {} } @@ -158,10 +160,10 @@ impl CustomOnionMessageHandler for TestCustomMessageHandler { fn handle_custom_message( &self, message: Self::CustomMessage, _context: Option>, responder: Option, - ) -> ResponseInstruction { + ) -> Option<(Self::CustomMessage, ResponseInstruction)> { match responder { - Some(responder) => responder.respond(message), - None => ResponseInstruction::NoResponse, + Some(responder) => Some((message, responder.respond())), + None => None, } } fn read_custom_message( diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 167ab0ac8..b6ae70836 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, }, }; @@ -10783,7 +10783,7 @@ where &invoice_request.inner ) { Ok(amount_msats) => amount_msats, - Err(error) => return responder.respond(OffersMessage::InvoiceError(error.into())), + Err(error) => return Some((OffersMessage::InvoiceError(error.into()), responder.respond())), }; let relative_expiry = DEFAULT_RELATIVE_EXPIRY.as_secs() as u32; @@ -10793,7 +10793,7 @@ where Ok((payment_hash, payment_secret)) => (payment_hash, payment_secret), Err(()) => { let error = Bolt12SemanticError::InvalidAmount; - return responder.respond(OffersMessage::InvoiceError(error.into())); + return Some((OffersMessage::InvoiceError(error.into()), responder.respond())); }, }; @@ -10807,7 +10807,7 @@ where Ok(payment_paths) => payment_paths, Err(()) => { let error = Bolt12SemanticError::MissingPaths; - return responder.respond(OffersMessage::InvoiceError(error.into())); + return Some((OffersMessage::InvoiceError(error.into()), responder.respond())); }, }; @@ -10852,14 +10852,14 @@ where }; match response { - Ok(invoice) => responder.respond(OffersMessage::Invoice(invoice)), - Err(error) => responder.respond(OffersMessage::InvoiceError(error.into())), + Ok(invoice) => Some((OffersMessage::Invoice(invoice), responder.respond())), + Err(error) => Some((OffersMessage::InvoiceError(error.into()), responder.respond())), } }, 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)), + Some(responder) => Some((OffersMessage::InvoiceError(error), responder.respond())), None => { log_trace!(logger, "No reply path to send error: {:?}", error); - ResponseInstruction::NoResponse + None }, } }, @@ -10905,11 +10905,11 @@ where OffersMessage::StaticInvoice(_invoice) => { match responder { Some(responder) => { - responder.respond(OffersMessage::InvoiceError( - InvoiceError::from_string("Static invoices not yet supported".to_string()) - )) + return Some((OffersMessage::InvoiceError( + InvoiceError::from_string("Static invoices not yet supported".to_string()) + ), responder.respond())); }, - 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 6b03b3654..bde5454b4 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 4a0801019..dcc528305 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 508e7982b..01b90314b 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); @@ -190,10 +190,10 @@ impl CustomOnionMessageHandler for TestCustomMessageHandler { match responder { Some(responder) if expectation.include_reply_path => { - responder.respond_with_reply_path(response, MessageContext::Custom(context.unwrap_or_else(Vec::new))) + Some((response, responder.respond_with_reply_path(MessageContext::Custom(context.unwrap_or_else(Vec::new))))) }, - Some(responder) => responder.respond(response), - None => ResponseInstruction::NoResponse, + Some(responder) => Some((response, responder.respond())), + 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 89aa4dbce..3d83e58a8 100644 --- a/lightning/src/onion_message/messenger.rs +++ b/lightning/src/onion_message/messenger.rs @@ -365,40 +365,40 @@ 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) -> ResponseInstruction { + 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, context: MessageContext) -> ResponseInstruction { + 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, -} - /// `ResponseInstruction` represents instructions for responding to received messages. -pub enum ResponseInstruction { +pub enum ResponseInstruction { /// Indicates that a response should be sent including a reply path for /// the recipient to respond back. - WithReplyPath(OnionMessageResponse, MessageContext), + 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(OnionMessageResponse), - /// Indicates that there's no response to send back. - NoResponse, + WithoutReplyPath { + /// The path over which we'll send our reply. + send_path: BlindedMessagePath, + } } /// An [`OnionMessage`] for [`OnionMessenger`] to send. @@ -799,7 +799,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. @@ -1314,21 +1316,19 @@ where /// enqueueing any response for sending. /// /// This function is useful for asynchronous handling of [`OnionMessage`]s. - /// Handlers have the option to return [`ResponseInstruction::NoResponse`], indicating that - /// no immediate response should be sent. Then, they can transfer the associated [`Responder`] - /// to another task responsible for generating the response asynchronously. Subsequently, when - /// the response is prepared and ready for sending, that task can invoke this method to enqueue - /// the response for delivery. + /// Handlers have the option to return `None`, indicating that no immediate response should be + /// sent. Then, they can transfer the associated [`Responder`] to another task responsible for + /// generating the response asynchronously. Subsequently, when 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: T, instructions: 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 instructions { + 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.msg_type(); let reply_path = if let Some(context) = context { match self.create_blinded_path(context) { Ok(reply_path) => Some(reply_path), @@ -1344,7 +1344,7 @@ where } else { None }; self.find_path_and_enqueue_onion_message( - response.message, Destination::BlindedPath(response.reply_path), reply_path, + response, Destination::BlindedPath(response_path), reply_path, format_args!( "when responding with {} to an onion message", message_type, @@ -1564,14 +1564,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 +1591,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 28f6d7d6f..1042080f8 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. /// -- 2.39.5