From eecaf1487990f132ce4ca7b750eabb43a7207525 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 22 Aug 2024 01:42:55 +0000 Subject: [PATCH] 2/3 Use `MessageSendInstructions` instead of `PendingOnionMessage` 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. --- lightning/src/ln/channelmanager.rs | 56 +++++++++--------- lightning/src/ln/offers_tests.rs | 72 ++++++++++-------------- lightning/src/onion_message/messenger.rs | 53 +++++++++-------- lightning/src/onion_message/offers.rs | 14 +---- 4 files changed, 88 insertions(+), 107 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index b6ae70836..b215880c9 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -71,7 +71,7 @@ use crate::offers::parse::Bolt12SemanticError; use crate::offers::refund::{Refund, RefundBuilder}; use crate::offers::signer; use crate::onion_message::async_payments::{AsyncPaymentsMessage, HeldHtlcAvailable, ReleaseHeldHtlc, AsyncPaymentsMessageHandler}; -use crate::onion_message::messenger::{new_pending_onion_message, Destination, MessageRouter, PendingOnionMessage, Responder, ResponseInstruction}; +use crate::onion_message::messenger::{Destination, MessageRouter, PendingOnionMessage, Responder, ResponseInstruction, MessageSendInstructions}; use crate::onion_message::offers::{OffersMessage, OffersMessageHandler}; use crate::sign::{EntropySource, NodeSigner, Recipient, SignerProvider}; use crate::sign::ecdsa::EcdsaChannelSigner; @@ -2277,9 +2277,9 @@ where needs_persist_flag: AtomicBool, #[cfg(not(any(test, feature = "_test_utils")))] - pending_offers_messages: Mutex>>, + pending_offers_messages: Mutex>, #[cfg(any(test, feature = "_test_utils"))] - pub(crate) pending_offers_messages: Mutex>>, + pub(crate) pending_offers_messages: Mutex>, /// Tracks the message events that are to be broadcasted when we are connected to some peer. pending_broadcast_messages: Mutex>, @@ -9068,21 +9068,21 @@ where .flat_map(|reply_path| offer.paths().iter().map(move |path| (path, reply_path))) .take(OFFERS_MESSAGE_REQUEST_LIMIT) .for_each(|(path, reply_path)| { - let message = new_pending_onion_message( - OffersMessage::InvoiceRequest(invoice_request.clone()), - Destination::BlindedPath(path.clone()), - Some(reply_path.clone()), - ); - pending_offers_messages.push(message); + let instructions = MessageSendInstructions::WithSpecifiedReplyPath { + destination: Destination::BlindedPath(path.clone()), + reply_path: reply_path.clone(), + }; + let message = OffersMessage::InvoiceRequest(invoice_request.clone()); + pending_offers_messages.push((message, instructions)); }); } else if let Some(signing_pubkey) = offer.signing_pubkey() { for reply_path in reply_paths { - let message = new_pending_onion_message( - OffersMessage::InvoiceRequest(invoice_request.clone()), - Destination::Node(signing_pubkey), - Some(reply_path), - ); - pending_offers_messages.push(message); + let instructions = MessageSendInstructions::WithSpecifiedReplyPath { + destination: Destination::Node(signing_pubkey), + reply_path, + }; + let message = OffersMessage::InvoiceRequest(invoice_request.clone()); + pending_offers_messages.push((message, instructions)); } } else { debug_assert!(false); @@ -9162,12 +9162,12 @@ where let mut pending_offers_messages = self.pending_offers_messages.lock().unwrap(); if refund.paths().is_empty() { for reply_path in reply_paths { - let message = new_pending_onion_message( - OffersMessage::Invoice(invoice.clone()), - Destination::Node(refund.payer_id()), - Some(reply_path), - ); - pending_offers_messages.push(message); + let instructions = MessageSendInstructions::WithSpecifiedReplyPath { + destination: Destination::Node(refund.payer_id()), + reply_path, + }; + let message = OffersMessage::Invoice(invoice.clone()); + pending_offers_messages.push((message, instructions)); } } else { reply_paths @@ -9175,12 +9175,12 @@ where .flat_map(|reply_path| refund.paths().iter().map(move |path| (path, reply_path))) .take(OFFERS_MESSAGE_REQUEST_LIMIT) .for_each(|(path, reply_path)| { - let message = new_pending_onion_message( - OffersMessage::Invoice(invoice.clone()), - Destination::BlindedPath(path.clone()), - Some(reply_path.clone()), - ); - pending_offers_messages.push(message); + let instructions = MessageSendInstructions::WithSpecifiedReplyPath { + destination: Destination::BlindedPath(path.clone()), + reply_path: reply_path.clone(), + }; + let message = OffersMessage::Invoice(invoice.clone()); + pending_offers_messages.push((message, instructions)); }); } @@ -10937,7 +10937,7 @@ where } } - fn release_pending_messages(&self) -> Vec> { + fn release_pending_messages(&self) -> Vec<(OffersMessage, MessageSendInstructions)> { core::mem::take(&mut self.pending_offers_messages.lock().unwrap()) } } diff --git a/lightning/src/ln/offers_tests.rs b/lightning/src/ln/offers_tests.rs index b1c09fd54..8bbf9a0ad 100644 --- a/lightning/src/ln/offers_tests.rs +++ b/lightning/src/ln/offers_tests.rs @@ -59,7 +59,7 @@ use crate::offers::invoice_error::InvoiceError; use crate::offers::invoice_request::{InvoiceRequest, InvoiceRequestFields}; use crate::offers::nonce::Nonce; use crate::offers::parse::Bolt12SemanticError; -use crate::onion_message::messenger::{Destination, PeeledOnion, new_pending_onion_message}; +use crate::onion_message::messenger::{Destination, PeeledOnion, MessageSendInstructions}; use crate::onion_message::offers::OffersMessage; use crate::onion_message::packet::ParsedOnionMessageContents; use crate::routing::gossip::{NodeAlias, NodeId}; @@ -1313,13 +1313,10 @@ fn fails_authentication_when_handling_invoice_request() { expect_recent_payment!(david, RecentPaymentDetails::AwaitingInvoice, payment_id); connect_peers(david, alice); - #[cfg(not(c_bindings))] { - david.node.pending_offers_messages.lock().unwrap().first_mut().unwrap().destination = - Destination::Node(alice_id); - } - #[cfg(c_bindings)] { - david.node.pending_offers_messages.lock().unwrap().first_mut().unwrap().1 = - Destination::Node(alice_id); + match &mut david.node.pending_offers_messages.lock().unwrap().first_mut().unwrap().1 { + MessageSendInstructions::WithSpecifiedReplyPath { destination, .. } => + *destination = Destination::Node(alice_id), + _ => panic!(), } let onion_message = david.onion_messenger.next_onion_message_for_peer(alice_id).unwrap(); @@ -1341,13 +1338,10 @@ fn fails_authentication_when_handling_invoice_request() { .unwrap(); expect_recent_payment!(david, RecentPaymentDetails::AwaitingInvoice, payment_id); - #[cfg(not(c_bindings))] { - david.node.pending_offers_messages.lock().unwrap().first_mut().unwrap().destination = - Destination::BlindedPath(invalid_path); - } - #[cfg(c_bindings)] { - david.node.pending_offers_messages.lock().unwrap().first_mut().unwrap().1 = - Destination::BlindedPath(invalid_path); + match &mut david.node.pending_offers_messages.lock().unwrap().first_mut().unwrap().1 { + MessageSendInstructions::WithSpecifiedReplyPath { destination, .. } => + *destination = Destination::BlindedPath(invalid_path), + _ => panic!(), } connect_peers(david, bob); @@ -1427,11 +1421,9 @@ fn fails_authentication_when_handling_invoice_for_offer() { let mut pending_offers_messages = david.node.pending_offers_messages.lock().unwrap(); let pending_invoice_request = pending_offers_messages.pop().unwrap(); pending_offers_messages.clear(); - #[cfg(not(c_bindings))] { - pending_invoice_request.reply_path - } - #[cfg(c_bindings)] { - pending_invoice_request.2 + match pending_invoice_request.1 { + MessageSendInstructions::WithSpecifiedReplyPath { reply_path, .. } => reply_path, + _ => panic!(), } }; @@ -1445,11 +1437,10 @@ fn fails_authentication_when_handling_invoice_for_offer() { { let mut pending_offers_messages = david.node.pending_offers_messages.lock().unwrap(); let mut pending_invoice_request = pending_offers_messages.first_mut().unwrap(); - #[cfg(not(c_bindings))] { - pending_invoice_request.reply_path = invalid_reply_path; - } - #[cfg(c_bindings)] { - pending_invoice_request.2 = invalid_reply_path; + match &mut pending_invoice_request.1 { + MessageSendInstructions::WithSpecifiedReplyPath { reply_path, .. } => + *reply_path = invalid_reply_path, + _ => panic!(), } } @@ -1531,13 +1522,10 @@ fn fails_authentication_when_handling_invoice_for_refund() { let expected_invoice = alice.node.request_refund_payment(&refund).unwrap(); connect_peers(david, alice); - #[cfg(not(c_bindings))] { - alice.node.pending_offers_messages.lock().unwrap().first_mut().unwrap().destination = - Destination::Node(david_id); - } - #[cfg(c_bindings)] { - alice.node.pending_offers_messages.lock().unwrap().first_mut().unwrap().1 = - Destination::Node(david_id); + match &mut alice.node.pending_offers_messages.lock().unwrap().first_mut().unwrap().1 { + MessageSendInstructions::WithSpecifiedReplyPath { destination, .. } => + *destination = Destination::Node(david_id), + _ => panic!(), } let onion_message = alice.onion_messenger.next_onion_message_for_peer(david_id).unwrap(); @@ -1565,13 +1553,10 @@ fn fails_authentication_when_handling_invoice_for_refund() { let expected_invoice = alice.node.request_refund_payment(&refund).unwrap(); - #[cfg(not(c_bindings))] { - alice.node.pending_offers_messages.lock().unwrap().first_mut().unwrap().destination = - Destination::BlindedPath(invalid_path); - } - #[cfg(c_bindings)] { - alice.node.pending_offers_messages.lock().unwrap().first_mut().unwrap().1 = - Destination::BlindedPath(invalid_path); + match &mut alice.node.pending_offers_messages.lock().unwrap().first_mut().unwrap().1 { + MessageSendInstructions::WithSpecifiedReplyPath { destination, .. } => + *destination = Destination::BlindedPath(invalid_path), + _ => panic!(), } connect_peers(alice, charlie); @@ -2155,10 +2140,11 @@ fn fails_paying_invoice_with_unknown_required_features() { .build_and_sign(&secp_ctx).unwrap(); // Enqueue an onion message containing the new invoice. - let pending_message = new_pending_onion_message( - OffersMessage::Invoice(invoice), Destination::BlindedPath(reply_path), None - ); - alice.node.pending_offers_messages.lock().unwrap().push(pending_message); + let instructions = MessageSendInstructions::WithoutReplyPath { + destination: Destination::BlindedPath(reply_path), + }; + let message = OffersMessage::Invoice(invoice); + alice.node.pending_offers_messages.lock().unwrap().push((message, instructions)); let onion_message = alice.onion_messenger.next_onion_message_for_peer(charlie_id).unwrap(); charlie.onion_messenger.handle_onion_message(&alice_id, &onion_message); diff --git a/lightning/src/onion_message/messenger.rs b/lightning/src/onion_message/messenger.rs index cf885aaee..ba2fe982d 100644 --- a/lightning/src/onion_message/messenger.rs +++ b/lightning/src/onion_message/messenger.rs @@ -403,6 +403,14 @@ impl ResponseInstruction { /// Instructions for how and where to send a message. #[derive(Clone)] pub enum MessageSendInstructions { + /// Indicates that a message should be sent including the provided reply path for the recipient + /// to respond. + WithSpecifiedReplyPath { + /// The destination where we need to send our message. + destination: Destination, + /// The reply path which should be included in the message. + reply_path: BlindedMessagePath, + }, /// Indicates that a message should be sent including a reply path for the recipient to /// respond. WithReplyPath { @@ -1183,24 +1191,25 @@ where fn send_onion_message_internal( &self, message: T, instructions: MessageSendInstructions, log_suffix: fmt::Arguments, ) -> Result, SendError> { - let (destination, context) = match instructions { - MessageSendInstructions::WithReplyPath { destination, context } => (destination, Some(context)), - MessageSendInstructions::WithoutReplyPath { destination } => (destination, None), - }; - - let reply_path = if let Some(context) = context { - match self.create_blinded_path(context) { - Ok(reply_path) => Some(reply_path), - Err(err) => { - log_trace!( - self.logger, - "Failed to create reply path {}: {:?}", - log_suffix, err - ); - return Err(err); + let (destination, reply_path) = match instructions { + MessageSendInstructions::WithSpecifiedReplyPath { destination, reply_path } => + (destination, Some(reply_path)), + MessageSendInstructions::WithReplyPath { destination, context } => { + match self.create_blinded_path(context) { + Ok(reply_path) => (destination, Some(reply_path)), + Err(err) => { + log_trace!( + self.logger, + "Failed to create reply path {}: {:?}", + log_suffix, err + ); + return Err(err); + } } - } - } else { None }; + }, + MessageSendInstructions::WithoutReplyPath { destination } => + (destination, None), + }; self.find_path_and_enqueue_onion_message( message, destination, reply_path, log_suffix, @@ -1737,13 +1746,9 @@ where // node, and then enqueue the message for sending to the first peer in the full path. fn next_onion_message_for_peer(&self, peer_node_id: PublicKey) -> Option { // Enqueue any initiating `OffersMessage`s to send. - for message in self.offers_handler.release_pending_messages() { - #[cfg(not(c_bindings))] - let PendingOnionMessage { contents, destination, reply_path } = message; - #[cfg(c_bindings)] - let (contents, destination, reply_path) = message; - let _ = self.find_path_and_enqueue_onion_message( - contents, destination, reply_path, format_args!("when sending OffersMessage") + for (message, instructions) in self.offers_handler.release_pending_messages() { + let _ = self.send_onion_message_internal( + message, instructions, format_args!("when sending OffersMessage") ); } diff --git a/lightning/src/onion_message/offers.rs b/lightning/src/onion_message/offers.rs index 1042080f8..1b3274ae1 100644 --- a/lightning/src/onion_message/offers.rs +++ b/lightning/src/onion_message/offers.rs @@ -22,9 +22,7 @@ use crate::offers::static_invoice::StaticInvoice; use crate::onion_message::packet::OnionMessageContents; use crate::util::logger::Logger; use crate::util::ser::{Readable, ReadableArgs, Writeable, Writer}; -use crate::onion_message::messenger::{ResponseInstruction, Responder}; -#[cfg(not(c_bindings))] -use crate::onion_message::messenger::PendingOnionMessage; +use crate::onion_message::messenger::{ResponseInstruction, Responder, MessageSendInstructions}; use crate::prelude::*; @@ -53,15 +51,7 @@ pub trait OffersMessageHandler { /// /// Typically, this is used for messages initiating a payment flow rather than in response to /// another message. The latter should use the return value of [`Self::handle_message`]. - #[cfg(not(c_bindings))] - fn release_pending_messages(&self) -> Vec> { vec![] } - - /// Releases any [`OffersMessage`]s that need to be sent. - /// - /// Typically, this is used for messages initiating a payment flow rather than in response to - /// another message. The latter should use the return value of [`Self::handle_message`]. - #[cfg(c_bindings)] - fn release_pending_messages(&self) -> Vec<(OffersMessage, crate::onion_message::messenger::Destination, Option)> { vec![] } + fn release_pending_messages(&self) -> Vec<(OffersMessage, MessageSendInstructions)> { vec![] } } /// Possible BOLT 12 Offers messages sent and received via an [`OnionMessage`]. -- 2.39.5