Merge pull request #887 from chuksys/add-support-for-hrn-resolution-follow-up

HRN resolution refinement and architectural clean-up

Author: tnull

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<Mutex<Option<Weak<PeerManager>>>> = Arc::new(Mutex::new(None));
 	let hrn_resolver;
+	let mut blip32_resolver = None;
 
 	let runtime_handle = runtime.handle();
 
@@ -1755,24 +1748,16 @@ 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<dyn DNSResolverMessageHandler + Send + Sync>
 		},
 		HRNResolverConfig::Dns { dns_server_address, enable_hrn_resolution_service, .. } => {
 			let addr = dns_server_address
 				.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,
 	})

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,
 }
 

src/lib.rs

@@ -238,7 +238,7 @@ pub struct Node {
 	node_metrics: Arc<RwLock<NodeMetrics>>,
 	om_mailbox: Option<Arc<OnionMessageMailbox>>,
 	async_payments_role: Option<AsyncPaymentsRole>,
-	hrn_resolver: Arc<HRNResolver>,
+	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(),
 		))
 	}
 

src/payment/unified.rs

@@ -74,7 +74,7 @@ pub struct UnifiedPayment {
 	bolt12_payment: Arc<Bolt12Payment>,
 	config: Arc<Config>,
 	logger: Arc<Logger>,
-	hrn_resolver: Arc<HRNResolver>,
+	hrn_resolver: HRNResolver,
 	#[cfg(hrn_tests)]
 	test_offer: std::sync::Mutex<Option<Offer>>,
 }
@@ -83,7 +83,7 @@ impl UnifiedPayment {
 	pub(crate) fn new(
 		onchain_payment: Arc<OnchainPayment>, bolt11_invoice: Arc<Bolt11Payment>,
 		bolt12_payment: Arc<Bolt12Payment>, config: Arc<Config>, logger: Arc<Logger>,
-		hrn_resolver: Arc<HRNResolver>,
+		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)
 		});
 	}

src/types.rs

@@ -328,6 +328,7 @@ pub(crate) type OnionMessenger = lightning::onion_message::messenger::OnionMesse
 	IgnoringMessageHandler,
 >;
 
+#[derive(Clone)]
 pub enum HRNResolver {
 	Onion(Arc<LDKOnionMessageDNSSECHrnResolver<Arc<Graph>, Arc<Logger>>>),
 	Local(Arc<DNSHrnResolver>),