diff --git a/src/builder.rs b/src/builder.rs index b0ff1d03b..0b44dc153 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -10,7 +10,7 @@ use std::convert::TryInto; use std::default::Default; use std::net::ToSocketAddrs; use std::path::PathBuf; -use std::sync::{Arc, Mutex, Once, RwLock, Weak}; +use std::sync::{Arc, Mutex, Once, RwLock}; use std::time::SystemTime; use std::{fmt, fs}; @@ -1735,15 +1735,8 @@ fn build_with_store_internal( })?; } - // This hook resolves a circular dependency: - // 1. PeerManager requires OnionMessenger (via MessageHandler). - // 2. OnionMessenger (via HRN resolver) needs to call PeerManager::process_events. - // - // We provide the resolver with a Weak pointer via this Mutex-protected "hook." - // This allows us to initialize the resolver before the PeerManager exists, - // and prevents a reference cycle (memory leak). - let peer_manager_hook: Arc>>> = Arc::new(Mutex::new(None)); let hrn_resolver; + let mut blip32_resolver = None; let runtime_handle = runtime.handle(); @@ -1755,16 +1748,8 @@ fn build_with_store_internal( let hrn_res = Arc::new(LDKOnionMessageDNSSECHrnResolver::new(Arc::clone(&network_graph))); hrn_resolver = HRNResolver::Onion(Arc::clone(&hrn_res)); + blip32_resolver = Some(Arc::clone(&hrn_res)); - // We clone the hook because it's moved into a Send + Sync closure that outlives this scope. - let pm_hook_clone = Arc::clone(&peer_manager_hook); - hrn_res.register_post_queue_action(Box::new(move || { - if let Ok(guard) = pm_hook_clone.lock() { - if let Some(pm) = guard.as_ref().and_then(|weak| weak.upgrade()) { - pm.process_events(); - } - } - })); hrn_res as Arc }, HRNResolverConfig::Dns { dns_server_address, enable_hrn_resolution_service, .. } => { @@ -1772,7 +1757,7 @@ fn build_with_store_internal( .to_socket_addrs() .map_err(|_| BuildError::DNSResolverSetupFailed)? .next() - .ok_or({ + .ok_or_else(|| { log_error!(logger, "No valid address found for: {}", dns_server_address); BuildError::DNSResolverSetupFailed })?; @@ -1945,8 +1930,13 @@ fn build_with_store_internal( Arc::clone(&keys_manager), )); - if let Ok(mut guard) = peer_manager_hook.lock() { - *guard = Some(Arc::downgrade(&peer_manager)); + if let Some(res) = blip32_resolver { + let pm_weak = Arc::downgrade(&peer_manager); + res.register_post_queue_action(Box::new(move || { + if let Some(upgraded_pm) = pm_weak.upgrade() { + upgraded_pm.process_events(); + } + })); } liquidity_source.as_ref().map(|l| l.set_peer_manager(Arc::downgrade(&peer_manager))); @@ -2061,7 +2051,7 @@ fn build_with_store_internal( node_metrics, om_mailbox, async_payments_role, - hrn_resolver: Arc::new(hrn_resolver), + hrn_resolver, #[cfg(cycle_tests)] _leak_checker, }) diff --git a/src/config.rs b/src/config.rs index 6f786d764..014d6216a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -118,18 +118,18 @@ pub(crate) const LNURL_AUTH_TIMEOUT_SECS: u64 = 15; /// ### Defaults /// /// | Parameter | Value | -/// |----------------------------------------|--------------------| -/// | `storage_dir_path` | /tmp/ldk_node/ | -/// | `network` | Bitcoin | -/// | `listening_addresses` | None | -/// | `announcement_addresses` | None | -/// | `node_alias` | None | -/// | `trusted_peers_0conf` | [] | -/// | `probing_liquidity_limit_multiplier` | 3 | -/// | `anchor_channels_config` | Some(..) | -/// | `route_parameters` | None | -/// | `tor_config` | None | -/// | `hrn_config` | HumanReadableNamesConfig::default() | +/// |----------------------------------------|--------------------------------------| +/// | `storage_dir_path` | /tmp/ldk_node/ | +/// | `network` | Bitcoin | +/// | `listening_addresses` | None | +/// | `announcement_addresses` | None | +/// | `node_alias` | None | +/// | `trusted_peers_0conf` | [] | +/// | `probing_liquidity_limit_multiplier` | 3 | +/// | `anchor_channels_config` | Some(..) | +/// | `route_parameters` | None | +/// | `tor_config` | None | +/// | `hrn_config` | HumanReadableNamesConfig::default() | /// /// See [`AnchorChannelsConfig`] and [`RouteParametersConfig`] for more information regarding their /// respective default values. @@ -264,7 +264,7 @@ pub struct HumanReadableNamesConfig { /// /// By default, this uses the `Dns` variant with the following settings: /// * **DNS Server**: `8.8.8.8:53` (Google Public DNS) - /// * **Resolution Service**: Enabled (`false`) + /// * **Resolution Service**: Disabled (`false`) pub resolution_config: HRNResolverConfig, } diff --git a/src/lib.rs b/src/lib.rs index faeb6d339..db63d6258 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -238,7 +238,7 @@ pub struct Node { node_metrics: Arc>, om_mailbox: Option>, async_payments_role: Option, - hrn_resolver: Arc, + hrn_resolver: HRNResolver, #[cfg(cycle_tests)] _leak_checker: LeakChecker, } @@ -1006,7 +1006,7 @@ impl Node { self.bolt12_payment().into(), Arc::clone(&self.config), Arc::clone(&self.logger), - Arc::clone(&self.hrn_resolver), + self.hrn_resolver.clone(), ) } @@ -1027,7 +1027,7 @@ impl Node { self.bolt12_payment(), Arc::clone(&self.config), Arc::clone(&self.logger), - Arc::clone(&self.hrn_resolver), + self.hrn_resolver.clone(), )) } diff --git a/src/payment/unified.rs b/src/payment/unified.rs index 9352ee974..3708afe8e 100644 --- a/src/payment/unified.rs +++ b/src/payment/unified.rs @@ -74,7 +74,7 @@ pub struct UnifiedPayment { bolt12_payment: Arc, config: Arc, logger: Arc, - hrn_resolver: Arc, + hrn_resolver: HRNResolver, #[cfg(hrn_tests)] test_offer: std::sync::Mutex>, } @@ -83,7 +83,7 @@ impl UnifiedPayment { pub(crate) fn new( onchain_payment: Arc, bolt11_invoice: Arc, bolt12_payment: Arc, config: Arc, logger: Arc, - hrn_resolver: Arc, + hrn_resolver: HRNResolver, ) -> Self { Self { onchain_payment, @@ -197,7 +197,7 @@ impl UnifiedPayment { } let parse_fut = - PaymentInstructions::parse(uri_str, target_network, self.hrn_resolver.as_ref(), false); + PaymentInstructions::parse(uri_str, target_network, &self.hrn_resolver, false); let instructions = tokio::time::timeout(Duration::from_secs(HRN_RESOLUTION_TIMEOUT_SECS), parse_fut) @@ -223,7 +223,7 @@ impl UnifiedPayment { Error::InvalidAmount })?; - let fut = instr.set_amount(amt, self.hrn_resolver.as_ref()); + let fut = instr.set_amount(amt, &self.hrn_resolver); tokio::time::timeout(Duration::from_secs(HRN_RESOLUTION_TIMEOUT_SECS), fut) .await @@ -350,8 +350,8 @@ impl UnifiedPayment { /// offers allow us to bypass this resolution step and test the subsequent payment flow. /// /// [BIP 353]: https://github.com/bitcoin/bips/blob/master/bip-0353.mediawiki - pub fn set_test_offer(&self, _offer: Offer) { - let _ = self.test_offer.lock().map(|mut guard| *guard = Some(_offer)).map_err(|e| { + pub fn set_test_offer(&self, offer: Offer) { + let _ = self.test_offer.lock().map(|mut guard| *guard = Some(offer)).map_err(|e| { log_error!(self.logger, "Failed to set test offer due to poisoned lock: {:?}", e) }); } diff --git a/src/types.rs b/src/types.rs index 5d5515dcc..962be7f64 100644 --- a/src/types.rs +++ b/src/types.rs @@ -330,6 +330,7 @@ pub(crate) type OnionMessenger = lightning::onion_message::messenger::OnionMesse IgnoringMessageHandler, >; +#[derive(Clone)] pub enum HRNResolver { Onion(Arc, Arc>>), Local(Arc),