From d8e33c15baf82a4ce46754745f0a33582e1d54df Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 30 Dec 2025 14:09:27 +0000 Subject: [PATCH 1/4] Fix circular `Arc` reference in `LiquiditySource` `LiquiditySource` takes a reference to our `PeerManager` but the `PeerManager` holds an indirect reference to the `LiquiditySource`. As a result, after our `Node` instance is `stop`ped and the `Node` `drop`ped, much of the node's memory will stick around, including the `NetworkGraph`. Here we fix this issue by using `Weak` pointers, though note that there is another issue caused by LDK's gossip validation API. --- src/builder.rs | 2 +- src/liquidity.rs | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/builder.rs b/src/builder.rs index 187f780d2..ee8931127 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -1603,7 +1603,7 @@ fn build_with_store_internal( peer_manager_clone.process_events(); })); - liquidity_source.as_ref().map(|l| l.set_peer_manager(Arc::clone(&peer_manager))); + liquidity_source.as_ref().map(|l| l.set_peer_manager(Arc::downgrade(&peer_manager))); gossip_source.set_gossip_verifier( Arc::clone(&chain_source), diff --git a/src/liquidity.rs b/src/liquidity.rs index 74e6098dd..2151110b6 100644 --- a/src/liquidity.rs +++ b/src/liquidity.rs @@ -9,7 +9,7 @@ use std::collections::HashMap; use std::ops::Deref; -use std::sync::{Arc, Mutex, RwLock}; +use std::sync::{Arc, Mutex, RwLock, Weak}; use std::time::Duration; use bitcoin::hashes::{sha256, Hash}; @@ -291,7 +291,7 @@ where lsps2_service: Option, wallet: Arc, channel_manager: Arc, - peer_manager: RwLock>>, + peer_manager: RwLock>>, keys_manager: Arc, liquidity_manager: Arc, config: Arc, @@ -302,7 +302,7 @@ impl LiquiditySource where L::Target: LdkLogger, { - pub(crate) fn set_peer_manager(&self, peer_manager: Arc) { + pub(crate) fn set_peer_manager(&self, peer_manager: Weak) { *self.peer_manager.write().unwrap() = Some(peer_manager); } @@ -715,8 +715,8 @@ where return; }; - let init_features = if let Some(peer_manager) = - self.peer_manager.read().unwrap().as_ref() + let init_features = if let Some(Some(peer_manager)) = + self.peer_manager.read().unwrap().as_ref().map(|weak| weak.upgrade()) { // Fail if we're not connected to the prospective channel partner. if let Some(peer) = peer_manager.peer_by_node_id(&their_network_key) { From a7d2b6a1869d25c88ec08db3059f635ae537769f Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 8 Jan 2026 14:47:22 +0000 Subject: [PATCH 2/4] Fix circular `Arc` reference in HRN resolver action In added logic to use the HRN resolver from `bitcoin-payment-instructions`, we created a circular `Arc` reference - the `LDKOnionMessageDNSSECHrnResolver` is used as a handler for the `OnionMessenger` but we also set a post-queue-action which holds a reference to the `PeerManager`. As a result, after our `Node` instance is `stop`ped and the `Node` `drop`ped, much of the node's memory will stick around, including the `NetworkGraph`. Here we fix this issue by using `Weak` pointers. --- src/builder.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/builder.rs b/src/builder.rs index ee8931127..cea3d09f5 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -1597,10 +1597,11 @@ fn build_with_store_internal( Arc::clone(&keys_manager), )); - let peer_manager_clone = Arc::clone(&peer_manager); - + let peer_manager_clone = Arc::downgrade(&peer_manager); hrn_resolver.register_post_queue_action(Box::new(move || { - peer_manager_clone.process_events(); + if let Some(upgraded_pointer) = peer_manager_clone.upgrade() { + upgraded_pointer.process_events(); + } })); liquidity_source.as_ref().map(|l| l.set_peer_manager(Arc::downgrade(&peer_manager))); From 809a2270efbbfb017e333d03e610086be5152627 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 8 Jan 2026 14:21:10 +0000 Subject: [PATCH 3/4] Update LDK, fixing a circular `Arc` reference in gossip validation LDK's gossip validation API basically forced us to have a circular `Arc` reference, leading to memory leaks after `drop`ping an instance of `Node`. This is fixed upstream in LDK PR #4294 which we update to here. --- Cargo.toml | 26 +++++++++++++------------- src/builder.rs | 16 +++++++--------- src/gossip.rs | 38 ++++++++++---------------------------- src/types.rs | 2 +- 4 files changed, 31 insertions(+), 51 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index cc2a4b194..431d6b8d8 100755 --- a/Cargo.toml +++ b/Cargo.toml @@ -39,17 +39,17 @@ default = [] #lightning-liquidity = { version = "0.2.0", features = ["std"] } #lightning-macros = { version = "0.2.0" } -lightning = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "1c730c8a16e28cc8e0c4817717ee63c97abcf4b0", features = ["std"] } -lightning-types = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "1c730c8a16e28cc8e0c4817717ee63c97abcf4b0" } -lightning-invoice = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "1c730c8a16e28cc8e0c4817717ee63c97abcf4b0", features = ["std"] } -lightning-net-tokio = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "1c730c8a16e28cc8e0c4817717ee63c97abcf4b0" } -lightning-persister = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "1c730c8a16e28cc8e0c4817717ee63c97abcf4b0", features = ["tokio"] } -lightning-background-processor = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "1c730c8a16e28cc8e0c4817717ee63c97abcf4b0" } -lightning-rapid-gossip-sync = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "1c730c8a16e28cc8e0c4817717ee63c97abcf4b0" } -lightning-block-sync = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "1c730c8a16e28cc8e0c4817717ee63c97abcf4b0", features = ["rest-client", "rpc-client", "tokio"] } -lightning-transaction-sync = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "1c730c8a16e28cc8e0c4817717ee63c97abcf4b0", features = ["esplora-async-https", "time", "electrum-rustls-ring"] } -lightning-liquidity = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "1c730c8a16e28cc8e0c4817717ee63c97abcf4b0", features = ["std"] } -lightning-macros = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "1c730c8a16e28cc8e0c4817717ee63c97abcf4b0" } +lightning = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "5236dba053a3f4f01cf0c32ce42b609a93738891", features = ["std"] } +lightning-types = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "5236dba053a3f4f01cf0c32ce42b609a93738891" } +lightning-invoice = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "5236dba053a3f4f01cf0c32ce42b609a93738891", features = ["std"] } +lightning-net-tokio = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "5236dba053a3f4f01cf0c32ce42b609a93738891" } +lightning-persister = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "5236dba053a3f4f01cf0c32ce42b609a93738891", features = ["tokio"] } +lightning-background-processor = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "5236dba053a3f4f01cf0c32ce42b609a93738891" } +lightning-rapid-gossip-sync = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "5236dba053a3f4f01cf0c32ce42b609a93738891" } +lightning-block-sync = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "5236dba053a3f4f01cf0c32ce42b609a93738891", features = ["rest-client", "rpc-client", "tokio"] } +lightning-transaction-sync = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "5236dba053a3f4f01cf0c32ce42b609a93738891", features = ["esplora-async-https", "time", "electrum-rustls-ring"] } +lightning-liquidity = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "5236dba053a3f4f01cf0c32ce42b609a93738891", features = ["std"] } +lightning-macros = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "5236dba053a3f4f01cf0c32ce42b609a93738891" } bdk_chain = { version = "0.23.0", default-features = false, features = ["std"] } bdk_esplora = { version = "0.22.0", default-features = false, features = ["async-https-rustls", "tokio"]} @@ -78,13 +78,13 @@ log = { version = "0.4.22", default-features = false, features = ["std"]} vss-client = { package = "vss-client-ng", version = "0.4" } prost = { version = "0.11.6", default-features = false} #bitcoin-payment-instructions = { version = "0.6" } -bitcoin-payment-instructions = { git = "https://github.com/tnull/bitcoin-payment-instructions", rev = "a9ad849a0eb7b155a688d713de6d9010cb48f073" } +bitcoin-payment-instructions = { git = "https://github.com/tnull/bitcoin-payment-instructions", rev = "fdca6c62f2fe2c53427d3e51e322a49aa7323ee2" } [target.'cfg(windows)'.dependencies] winapi = { version = "0.3", features = ["winbase"] } [dev-dependencies] -lightning = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "1c730c8a16e28cc8e0c4817717ee63c97abcf4b0", features = ["std", "_test_utils"] } +lightning = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "5236dba053a3f4f01cf0c32ce42b609a93738891", features = ["std", "_test_utils"] } proptest = "1.0.0" regex = "1.5.6" criterion = { version = "0.7.0", features = ["async_tokio"] } diff --git a/src/builder.rs b/src/builder.rs index cea3d09f5..ca8e71d03 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -20,7 +20,7 @@ use bitcoin::key::Secp256k1; use bitcoin::secp256k1::PublicKey; use bitcoin::{BlockHash, Network}; use bitcoin_payment_instructions::onion_message_resolver::LDKOnionMessageDNSSECHrnResolver; -use lightning::chain::{chainmonitor, BestBlock, Watch}; +use lightning::chain::{chainmonitor, BestBlock}; use lightning::ln::channelmanager::{self, ChainParameters, ChannelManagerReadArgs}; use lightning::ln::msgs::{RoutingMessageHandler, SocketAddress}; use lightning::ln::peer_handler::{IgnoringMessageHandler, MessageHandler}; @@ -1481,8 +1481,12 @@ fn build_with_store_internal( let gossip_source = match gossip_source_config { GossipSourceConfig::P2PNetwork => { - let p2p_source = - Arc::new(GossipSource::new_p2p(Arc::clone(&network_graph), Arc::clone(&logger))); + let p2p_source = Arc::new(GossipSource::new_p2p( + Arc::clone(&network_graph), + Arc::clone(&chain_source), + Arc::clone(&runtime), + Arc::clone(&logger), + )); // Reset the RGS sync timestamp in case we somehow switch gossip sources { @@ -1606,12 +1610,6 @@ fn build_with_store_internal( liquidity_source.as_ref().map(|l| l.set_peer_manager(Arc::downgrade(&peer_manager))); - gossip_source.set_gossip_verifier( - Arc::clone(&chain_source), - Arc::clone(&peer_manager), - Arc::clone(&runtime), - ); - let connection_manager = Arc::new(ConnectionManager::new(Arc::clone(&peer_manager), Arc::clone(&logger))); diff --git a/src/gossip.rs b/src/gossip.rs index 563d9e1ea..2b524d9ae 100644 --- a/src/gossip.rs +++ b/src/gossip.rs @@ -17,7 +17,7 @@ use crate::chain::ChainSource; use crate::config::RGS_SYNC_TIMEOUT_SECS; use crate::logger::{log_trace, LdkLogger, Logger}; use crate::runtime::Runtime; -use crate::types::{GossipSync, Graph, P2PGossipSync, PeerManager, RapidGossipSync, UtxoLookup}; +use crate::types::{GossipSync, Graph, P2PGossipSync, RapidGossipSync}; use crate::Error; pub(crate) enum GossipSource { @@ -33,12 +33,15 @@ pub(crate) enum GossipSource { } impl GossipSource { - pub fn new_p2p(network_graph: Arc, logger: Arc) -> Self { - let gossip_sync = Arc::new(P2PGossipSync::new( - network_graph, - None::>, - Arc::clone(&logger), - )); + pub fn new_p2p( + network_graph: Arc, chain_source: Arc, runtime: Arc, + logger: Arc, + ) -> Self { + let verifier = chain_source.as_utxo_source().map(|utxo_source| { + Arc::new(GossipVerifier::new(Arc::new(utxo_source), RuntimeSpawner::new(runtime))) + }); + + let gossip_sync = Arc::new(P2PGossipSync::new(network_graph, verifier, logger)); Self::P2PNetwork { gossip_sync } } @@ -62,27 +65,6 @@ impl GossipSource { } } - pub(crate) fn set_gossip_verifier( - &self, chain_source: Arc, peer_manager: Arc, - runtime: Arc, - ) { - match self { - Self::P2PNetwork { gossip_sync } => { - if let Some(utxo_source) = chain_source.as_utxo_source() { - let spawner = RuntimeSpawner::new(Arc::clone(&runtime)); - let gossip_verifier = Arc::new(GossipVerifier::new( - Arc::new(utxo_source), - spawner, - Arc::clone(gossip_sync), - peer_manager, - )); - gossip_sync.add_utxo_lookup(Some(gossip_verifier)); - } - }, - _ => (), - } - } - pub async fn update_rgs_snapshot(&self) -> Result { match self { Self::P2PNetwork { gossip_sync: _, .. } => Ok(0), diff --git a/src/types.rs b/src/types.rs index 5e9cd74c9..2b7d3829a 100644 --- a/src/types.rs +++ b/src/types.rs @@ -254,7 +254,7 @@ pub(crate) type Scorer = CombinedScorer, Arc>; pub(crate) type Graph = gossip::NetworkGraph>; -pub(crate) type UtxoLookup = GossipVerifier, Arc>; +pub(crate) type UtxoLookup = GossipVerifier>; pub(crate) type P2PGossipSync = lightning::routing::gossip::P2PGossipSync, Arc, Arc>; From 6a32f363c68c2d0484fef9aa7d6188066ae9495e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 30 Dec 2025 14:09:57 +0000 Subject: [PATCH 4/4] Add test for circular references leading to `NetworkGraph` leaks Due to two circular `Arc` references, after `stop`ping and `drop`ping the `Node` instance the bulk of ldk-node's memory (in the form of the `NetworkGraph`) would hang around. Here we add a test for this in our integration tests, checking if the `NetworkGraph` (as a proxy for other objects held in reference by the `PeerManager`) hangs around after `Node`s are `drop`ped. --- .github/workflows/rust.yml | 4 ++-- .github/workflows/vss-integration.yml | 2 +- Cargo.toml | 1 + src/builder.rs | 14 ++++++++++++++ src/lib.rs | 21 +++++++++++++++++++++ 5 files changed, 39 insertions(+), 3 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 661703ded..1ccade444 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -80,11 +80,11 @@ jobs: - name: Test on Rust ${{ matrix.toolchain }} if: "matrix.platform != 'windows-latest'" run: | - RUSTFLAGS="--cfg no_download" cargo test + RUSTFLAGS="--cfg no_download --cfg cycle_tests" cargo test - name: Test with UniFFI support on Rust ${{ matrix.toolchain }} if: "matrix.platform != 'windows-latest' && matrix.build-uniffi" run: | - RUSTFLAGS="--cfg no_download" cargo test --features uniffi + RUSTFLAGS="--cfg no_download --cfg cycle_tests" cargo test --features uniffi doc: name: Documentation diff --git a/.github/workflows/vss-integration.yml b/.github/workflows/vss-integration.yml index 8473ed413..b5c4e9a0b 100644 --- a/.github/workflows/vss-integration.yml +++ b/.github/workflows/vss-integration.yml @@ -45,4 +45,4 @@ jobs: cd ldk-node export TEST_VSS_BASE_URL="http://localhost:8080/vss" RUSTFLAGS="--cfg vss_test" cargo test io::vss_store - RUSTFLAGS="--cfg vss_test" cargo test --test integration_tests_vss + RUSTFLAGS="--cfg vss_test --cfg cycle_tests" cargo test --test integration_tests_vss diff --git a/Cargo.toml b/Cargo.toml index 431d6b8d8..207ad92a1 100755 --- a/Cargo.toml +++ b/Cargo.toml @@ -124,6 +124,7 @@ check-cfg = [ "cfg(tokio_unstable)", "cfg(cln_test)", "cfg(lnd_test)", + "cfg(cycle_tests)", ] [[bench]] diff --git a/src/builder.rs b/src/builder.rs index ca8e71d03..510d86bdd 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -1684,6 +1684,18 @@ fn build_with_store_internal( let pathfinding_scores_sync_url = pathfinding_scores_sync_config.map(|c| c.url.clone()); + #[cfg(cycle_tests)] + let mut _leak_checker = crate::LeakChecker(Vec::new()); + #[cfg(cycle_tests)] + { + use std::any::Any; + use std::sync::Weak; + + _leak_checker.0.push(Arc::downgrade(&channel_manager) as Weak); + _leak_checker.0.push(Arc::downgrade(&network_graph) as Weak); + _leak_checker.0.push(Arc::downgrade(&wallet) as Weak); + } + Ok(Node { runtime, stop_sender, @@ -1716,6 +1728,8 @@ fn build_with_store_internal( om_mailbox, async_payments_role, hrn_resolver, + #[cfg(cycle_tests)] + _leak_checker, }) } diff --git a/src/lib.rs b/src/lib.rs index d9bca4551..a76df1977 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -110,6 +110,8 @@ use std::default::Default; use std::net::ToSocketAddrs; use std::sync::{Arc, Mutex, RwLock}; use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; +#[cfg(cycle_tests)] +use std::{any::Any, sync::Weak}; pub use balance::{BalanceDetails, LightningBalance, PendingSweepBalance}; use bitcoin::secp256k1::PublicKey; @@ -173,6 +175,23 @@ use crate::scoring::setup_background_pathfinding_scores_sync; #[cfg(feature = "uniffi")] uniffi::include_scaffolding!("ldk_node"); +#[cfg(cycle_tests)] +/// A list of [`Weak`]s which can be used to check that a [`Node`]'s inner fields are being +/// properly released after the [`Node`] is dropped. +pub struct LeakChecker(Vec>); + +#[cfg(cycle_tests)] +impl LeakChecker { + /// Asserts that all the stored [`Weak`]s point to contents which have been freed. + /// + /// This will (obviously) panic if the [`Node`] has not yet been dropped. + pub fn assert_no_leaks(&self) { + for weak in self.0.iter() { + assert_eq!(weak.strong_count(), 0); + } + } +} + /// The main interface object of LDK Node, wrapping the necessary LDK and BDK functionalities. /// /// Needs to be initialized and instantiated through [`Builder::build`]. @@ -208,6 +227,8 @@ pub struct Node { om_mailbox: Option>, async_payments_role: Option, hrn_resolver: Arc, + #[cfg(cycle_tests)] + _leak_checker: LeakChecker, } impl Node {