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.

Author: TheBlueMatt

.github/workflows/cln-integration.yml

@@ -27,4 +27,4 @@ jobs:
           socat -d -d UNIX-LISTEN:/tmp/lightning-rpc,reuseaddr,fork TCP:127.0.0.1:9937&
 
       - name: Run CLN integration tests
-        run: RUSTFLAGS="--cfg cln_test" cargo test --test integration_tests_cln
+        run: RUSTFLAGS="--cfg cln_test --cfg cycle_tests" cargo test --test integration_tests_cln

.github/workflows/lnd-integration.yml

@@ -51,6 +51,6 @@ jobs:
 
       - name: Run LND integration tests
         run: LND_CERT_PATH=$LND_DATA_DIR/tls.cert LND_MACAROON_PATH=$LND_DATA_DIR/data/chain/bitcoin/regtest/admin.macaroon
-          RUSTFLAGS="--cfg lnd_test" cargo test --test integration_tests_lnd -- --exact --show-output
+          RUSTFLAGS="--cfg lnd_test --cfg cycle_tests" cargo test --test integration_tests_lnd -- --exact --show-output
         env:
           LND_DATA_DIR: ${{ env.LND_DATA_DIR }}

.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

.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

Cargo.toml

@@ -124,6 +124,7 @@ check-cfg = [
 	"cfg(tokio_unstable)",
 	"cfg(cln_test)",
 	"cfg(lnd_test)",
+	"cfg(cycle_tests)",
 ]
 
 [[bench]]

src/lib.rs

@@ -1761,6 +1761,12 @@ impl Node {
 			Error::PersistenceFailed
 		})
 	}
+
+	#[cfg(cycle_tests)]
+	/// Fetch a reference to the inner NetworkGraph, for Arc cycle detection
+	pub fn fetch_ref(&self) -> std::sync::Weak<Graph> {
+		Arc::downgrade(&self.network_graph)
+	}
 }
 
 impl Drop for Node {

src/logger.rs

@@ -171,6 +171,14 @@ impl LogWriter for Writer {
 	}
 }
 
+#[cfg(cycle_tests)]
+/// Our log writer
+pub struct Logger {
+	/// Specifies the logger's writer.
+	writer: Writer,
+}
+
+#[cfg(not(cycle_tests))]
 pub(crate) struct Logger {
 	/// Specifies the logger's writer.
 	writer: Writer,
@@ -195,10 +203,12 @@ impl Logger {
 		Ok(Self { writer: Writer::FileWriter { file_path, max_log_level } })
 	}
 
+	/// Creates a new logger writing to the `log` crate
 	pub fn new_log_facade() -> Self {
 		Self { writer: Writer::LogFacadeWriter }
 	}
 
+	/// Creates a new logger writing to an underlying [`LogWriter`]
 	pub fn new_custom_writer(log_writer: Arc<dyn LogWriter>) -> Self {
 		Self { writer: Writer::CustomWriter(log_writer) }
 	}

tests/common/mod.rs

@@ -13,6 +13,7 @@ pub(crate) mod logging;
 use std::collections::{HashMap, HashSet};
 use std::env;
 use std::future::Future;
+use std::ops::Deref;
 use std::path::PathBuf;
 use std::sync::{Arc, RwLock};
 use std::time::Duration;
@@ -261,10 +262,51 @@ pub(crate) fn random_config(anchor_channels: bool) -> TestConfig {
 	TestConfig { node_config, ..Default::default() }
 }
 
+pub struct TestNode {
+	#[cfg(feature = "uniffi")]
+	inner: Option<Arc<Node>>,
+	#[cfg(not(feature = "uniffi"))]
+	inner: Option<Node>,
+}
+
 #[cfg(feature = "uniffi")]
-type TestNode = Arc<Node>;
+impl From<Arc<Node>> for TestNode {
+	fn from(inner: Arc<Node>) -> Self {
+		Self { inner: Some(inner) }
+	}
+}
+
 #[cfg(not(feature = "uniffi"))]
-type TestNode = Node;
+impl From<Node> for TestNode {
+	fn from(inner: Node) -> Self {
+		Self { inner: Some(inner) }
+	}
+}
+
+impl Deref for TestNode {
+	type Target = Node;
+	fn deref(&self) -> &Node {
+		#[cfg(feature = "uniffi")]
+		{
+			self.inner.as_ref().unwrap().deref()
+		}
+		#[cfg(not(feature = "uniffi"))]
+		{
+			&self.inner.as_ref().unwrap()
+		}
+	}
+}
+
+#[cfg(cycle_tests)]
+impl Drop for TestNode {
+	fn drop(&mut self) {
+		if !std::thread::panicking() {
+			let graph_ref = (**self).fetch_ref();
+			self.inner.take();
+			assert_eq!(graph_ref.strong_count(), 0);
+		}
+	}
+}
 
 #[derive(Clone)]
 pub(crate) enum TestChainSource<'a> {
@@ -430,7 +472,7 @@ pub(crate) fn setup_node_for_async_payments(
 	node.start().unwrap();
 	assert!(node.status().is_running);
 	assert!(node.status().latest_fee_rate_cache_update_timestamp.is_some());
-	node
+	node.into()
 }
 
 pub(crate) async fn generate_blocks_and_wait<E: ElectrumApi>(

tests/integration_tests_rust.rs

@@ -23,8 +23,8 @@ use common::{
 	expect_splice_pending_event, generate_blocks_and_wait, open_channel, open_channel_push_amt,
 	premine_and_distribute_funds, premine_blocks, prepare_rbf, random_config,
 	random_listening_addresses, setup_bitcoind_and_electrsd, setup_builder, setup_node,
-	setup_node_for_async_payments, setup_two_nodes, wait_for_tx, TestChainSource, TestStoreType,
-	TestSyncStore,
+	setup_node_for_async_payments, setup_two_nodes, wait_for_tx, TestChainSource, TestNode,
+	TestStoreType, TestSyncStore,
 };
 use ldk_node::config::{AsyncPaymentsRole, EsploraSyncConfig};
 use ldk_node::liquidity::LSPS2ServiceConfig;
@@ -154,15 +154,15 @@ async fn multi_hop_sending() {
 	let esplora_url = format!("http://{}", electrsd.esplora_url.as_ref().unwrap());
 
 	// Setup and fund 5 nodes
-	let mut nodes = Vec::new();
+	let mut nodes: Vec<TestNode> = Vec::new();
 	for _ in 0..5 {
 		let config = random_config(true);
 		let sync_config = EsploraSyncConfig { background_sync_config: None };
 		setup_builder!(builder, config.node_config);
 		builder.set_chain_source_esplora(esplora_url.clone(), Some(sync_config));
-		let node = builder.build(config.node_entropy.into()).unwrap();
+		let node: TestNode = builder.build(config.node_entropy.into()).unwrap().into();
 		node.start().unwrap();
-		nodes.push(node);
+		nodes.push(node.into());
 	}
 
 	let addresses = nodes.iter().map(|n| n.onchain_payment().new_address().unwrap()).collect();
@@ -1730,7 +1730,8 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) {
 	setup_builder!(service_builder, service_config.node_config);
 	service_builder.set_chain_source_esplora(esplora_url.clone(), Some(sync_config));
 	service_builder.set_liquidity_provider_lsps2(lsps2_service_config);
-	let service_node = service_builder.build(service_config.node_entropy.into()).unwrap();
+	let service_node: TestNode =
+		service_builder.build(service_config.node_entropy.into()).unwrap().into();
 	service_node.start().unwrap();
 
 	let service_node_id = service_node.node_id();
@@ -1740,13 +1741,15 @@ async fn do_lsps2_client_service_integration(client_trusts_lsp: bool) {
 	setup_builder!(client_builder, client_config.node_config);
 	client_builder.set_chain_source_esplora(esplora_url.clone(), Some(sync_config));
 	client_builder.set_liquidity_source_lsps2(service_node_id, service_addr, None);
-	let client_node = client_builder.build(client_config.node_entropy.into()).unwrap();
+	let client_node: TestNode =
+		client_builder.build(client_config.node_entropy.into()).unwrap().into();
 	client_node.start().unwrap();
 
 	let payer_config = random_config(true);
 	setup_builder!(payer_builder, payer_config.node_config);
 	payer_builder.set_chain_source_esplora(esplora_url.clone(), Some(sync_config));
-	let payer_node = payer_builder.build(payer_config.node_entropy.into()).unwrap();
+	let payer_node: TestNode =
+		payer_builder.build(payer_config.node_entropy.into()).unwrap().into();
 	payer_node.start().unwrap();
 
 	let service_addr = service_node.onchain_payment().new_address().unwrap();
@@ -2047,7 +2050,8 @@ async fn lsps2_client_trusts_lsp() {
 	setup_builder!(service_builder, service_config.node_config);
 	service_builder.set_chain_source_esplora(esplora_url.clone(), Some(sync_config));
 	service_builder.set_liquidity_provider_lsps2(lsps2_service_config);
-	let service_node = service_builder.build(service_config.node_entropy.into()).unwrap();
+	let service_node: TestNode =
+		service_builder.build(service_config.node_entropy.into()).unwrap().into();
 	service_node.start().unwrap();
 	let service_node_id = service_node.node_id();
 	let service_addr = service_node.listening_addresses().unwrap().first().unwrap().clone();
@@ -2056,14 +2060,16 @@ async fn lsps2_client_trusts_lsp() {
 	setup_builder!(client_builder, client_config.node_config);
 	client_builder.set_chain_source_esplora(esplora_url.clone(), Some(sync_config));
 	client_builder.set_liquidity_source_lsps2(service_node_id, service_addr.clone(), None);
-	let client_node = client_builder.build(client_config.node_entropy.into()).unwrap();
+	let client_node: TestNode =
+		client_builder.build(client_config.node_entropy.into()).unwrap().into();
 	client_node.start().unwrap();
 	let client_node_id = client_node.node_id();
 
 	let payer_config = random_config(true);
 	setup_builder!(payer_builder, payer_config.node_config);
 	payer_builder.set_chain_source_esplora(esplora_url.clone(), Some(sync_config));
-	let payer_node = payer_builder.build(payer_config.node_entropy.into()).unwrap();
+	let payer_node: TestNode =
+		payer_builder.build(payer_config.node_entropy.into()).unwrap().into();
 	payer_node.start().unwrap();
 
 	let service_addr_onchain = service_node.onchain_payment().new_address().unwrap();
@@ -2220,7 +2226,8 @@ async fn lsps2_lsp_trusts_client_but_client_does_not_claim() {
 	setup_builder!(service_builder, service_config.node_config);
 	service_builder.set_chain_source_esplora(esplora_url.clone(), Some(sync_config));
 	service_builder.set_liquidity_provider_lsps2(lsps2_service_config);
-	let service_node = service_builder.build(service_config.node_entropy.into()).unwrap();
+	let service_node: TestNode =
+		service_builder.build(service_config.node_entropy.into()).unwrap().into();
 	service_node.start().unwrap();
 
 	let service_node_id = service_node.node_id();
@@ -2230,15 +2237,17 @@ async fn lsps2_lsp_trusts_client_but_client_does_not_claim() {
 	setup_builder!(client_builder, client_config.node_config);
 	client_builder.set_chain_source_esplora(esplora_url.clone(), Some(sync_config));
 	client_builder.set_liquidity_source_lsps2(service_node_id, service_addr.clone(), None);
-	let client_node = client_builder.build(client_config.node_entropy.into()).unwrap();
+	let client_node: TestNode =
+		client_builder.build(client_config.node_entropy.into()).unwrap().into();
 	client_node.start().unwrap();
 
 	let client_node_id = client_node.node_id();
 
 	let payer_config = random_config(true);
 	setup_builder!(payer_builder, payer_config.node_config);
 	payer_builder.set_chain_source_esplora(esplora_url.clone(), Some(sync_config));
-	let payer_node = payer_builder.build(payer_config.node_entropy.into()).unwrap();
+	let payer_node: TestNode =
+		payer_builder.build(payer_config.node_entropy.into()).unwrap().into();
 	payer_node.start().unwrap();
 
 	let service_addr_onchain = service_node.onchain_payment().new_address().unwrap();