diff --git a/CHANGELOG.md b/CHANGELOG.md index 7028b8ac8..aa56a549b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,8 @@ -# 0.7.0-rc.58 (Synonym Fork) +# 0.7.0-rc.60 (Synonym Fork) ## Bug Fixes +- Prevented Electrum runtime self-drop crashes and unbounded shutdown waits. - Scaled Electrum full-scan timeouts for additional wallets with the configured stop gap and batch size. - Return `OnchainWalletAccountNotRegistered` consistently for unloaded derived-account operations. diff --git a/Cargo.toml b/Cargo.toml index c79af3b88..53103a3d9 100755 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,7 +4,7 @@ exclude = ["bindings/uniffi-bindgen"] [package] name = "ldk-node" -version = "0.7.0-rc.58" +version = "0.7.0-rc.60" authors = ["Elias Rohrer "] homepage = "https://lightningdevkit.org/" license = "MIT OR Apache-2.0" diff --git a/Package.swift b/Package.swift index 59c8a6c3d..8bc8e4241 100644 --- a/Package.swift +++ b/Package.swift @@ -3,8 +3,8 @@ import PackageDescription -let tag = "v0.7.0-rc.58" -let checksum = "caca6b4ef9df81cd84279438894aa37116c69fd4b86e3070722c824e7af5f634" +let tag = "v0.7.0-rc.60" +let checksum = "15d23d3252422fd0f6f77c388c65c877fd95c5ef8c805ac4f7255368a4efda2b" let url = "https://github.com/synonymdev/ldk-node/releases/download/\(tag)/LDKNodeFFI.xcframework.zip" let package = Package( diff --git a/bindings/kotlin/ldk-node-android/gradle.properties b/bindings/kotlin/ldk-node-android/gradle.properties index 590b5081e..5fc9217cd 100644 --- a/bindings/kotlin/ldk-node-android/gradle.properties +++ b/bindings/kotlin/ldk-node-android/gradle.properties @@ -3,4 +3,4 @@ android.useAndroidX=true android.enableJetifier=true kotlin.code.style=official group=com.synonym -version=0.7.0-rc.58 +version=0.7.0-rc.60 diff --git a/bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/arm64-v8a/libldk_node.so b/bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/arm64-v8a/libldk_node.so index ebe0d3727..a7c7d6621 100755 Binary files a/bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/arm64-v8a/libldk_node.so and b/bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/arm64-v8a/libldk_node.so differ diff --git a/bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/armeabi-v7a/libldk_node.so b/bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/armeabi-v7a/libldk_node.so index 76aba6141..2d5cb2b3e 100755 Binary files a/bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/armeabi-v7a/libldk_node.so and b/bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/armeabi-v7a/libldk_node.so differ diff --git a/bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/x86_64/libldk_node.so b/bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/x86_64/libldk_node.so index 17611733c..10e61335f 100755 Binary files a/bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/x86_64/libldk_node.so and b/bindings/kotlin/ldk-node-android/lib/src/main/jniLibs/x86_64/libldk_node.so differ diff --git a/bindings/kotlin/ldk-node-jvm/gradle.properties b/bindings/kotlin/ldk-node-jvm/gradle.properties index 962764ca0..75e4f1c9b 100644 --- a/bindings/kotlin/ldk-node-jvm/gradle.properties +++ b/bindings/kotlin/ldk-node-jvm/gradle.properties @@ -1,4 +1,4 @@ org.gradle.jvmargs=-Xmx1536m kotlin.code.style=official group=com.synonym -version=0.7.0-rc.58 +version=0.7.0-rc.60 diff --git a/bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-aarch64/libldk_node.dylib b/bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-aarch64/libldk_node.dylib index 48d7b19ae..f34dd2907 100644 Binary files a/bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-aarch64/libldk_node.dylib and b/bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-aarch64/libldk_node.dylib differ diff --git a/bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-x86-64/libldk_node.dylib b/bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-x86-64/libldk_node.dylib index fdd2c6c3b..0f27feffd 100644 Binary files a/bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-x86-64/libldk_node.dylib and b/bindings/kotlin/ldk-node-jvm/lib/src/main/resources/darwin-x86-64/libldk_node.dylib differ diff --git a/bindings/python/pyproject.toml b/bindings/python/pyproject.toml index 39f8cbf70..23225f566 100644 --- a/bindings/python/pyproject.toml +++ b/bindings/python/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "ldk_node" -version = "0.7.0-rc.58" +version = "0.7.0-rc.60" authors = [ { name="Elias Rohrer", email="dev@tnull.de" }, ] diff --git a/src/chain/electrum.rs b/src/chain/electrum.rs index 04706311c..4e994f617 100644 --- a/src/chain/electrum.rs +++ b/src/chain/electrum.rs @@ -24,12 +24,14 @@ use lightning::chain::{Confirm, Filter, WatchedOutput}; use lightning::log_warn; use lightning::util::ser::Writeable; use lightning_transaction_sync::ElectrumSyncClient; +use tokio::runtime::Handle; use super::{periodically_archive_fully_resolved_monitors, WalletSyncStatus}; use crate::config::{ AddressTypeRuntimeConfig, Config, ElectrumSyncConfig, BDK_CLIENT_STOP_GAP, BDK_ELECTRUM_CLIENT_BATCH_SIZE, BDK_WALLET_SYNC_TIMEOUT_SECS, - FEE_RATE_CACHE_UPDATE_TIMEOUT_SECS, LDK_WALLET_SYNC_TIMEOUT_SECS, TX_BROADCAST_TIMEOUT_SECS, + DEFAULT_ELECTRUM_CONNECTION_TIMEOUT_SECS, FEE_RATE_CACHE_UPDATE_TIMEOUT_SECS, + LDK_WALLET_SYNC_TIMEOUT_SECS, TX_BROADCAST_TIMEOUT_SECS, }; use crate::error::Error; use crate::fee_estimator::{ @@ -38,12 +40,35 @@ use crate::fee_estimator::{ }; use crate::io::utils::write_node_metrics; use crate::logger::{log_bytes, log_error, log_info, log_trace, LdkLogger, Logger}; -use crate::runtime::Runtime; use crate::types::{ChainMonitor, ChannelManager, DynStore, Sweeper, Wallet}; use crate::NodeMetrics; const ELECTRUM_CLIENT_NUM_RETRIES: u8 = 3; +fn effective_connection_timeout_secs(configured_timeout_secs: u64, logger: &Logger) -> u8 { + let requested_timeout = if configured_timeout_secs == 0 { + log_warn!( + logger, + "Electrum connection_timeout_secs is 0; using the safe default of {} seconds.", + DEFAULT_ELECTRUM_CONNECTION_TIMEOUT_SECS, + ); + DEFAULT_ELECTRUM_CONNECTION_TIMEOUT_SECS + } else { + configured_timeout_secs + }; + let capped_timeout = requested_timeout.min(u8::MAX as u64) as u8; + if capped_timeout as u64 != requested_timeout { + log_warn!( + logger, + "Electrum connection_timeout_secs ({}) exceeds maximum of {}; capping to {}.", + requested_timeout, + u8::MAX, + capped_timeout, + ); + } + capped_timeout +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] struct FullScanSettings { stop_gap: usize, @@ -130,10 +155,10 @@ impl ElectrumChainSource { } } - pub(super) fn start(&self, runtime: Arc) -> Result<(), Error> { + pub(super) fn start(&self, runtime_handle: Handle) -> Result<(), Error> { self.electrum_runtime_status.write().unwrap().start( self.server_url.clone(), - Arc::clone(&runtime), + runtime_handle, Arc::clone(&self.config), Arc::clone(&self.logger), self.sync_config.connection_timeout_secs, @@ -537,14 +562,14 @@ impl ElectrumRuntimeStatus { } pub(super) fn start( - &mut self, server_url: String, runtime: Arc, config: Arc, + &mut self, server_url: String, runtime_handle: Handle, config: Arc, logger: Arc, connection_timeout_secs: u64, ) -> Result<(), Error> { match self { Self::Stopped { pending_registered_txs, pending_registered_outputs } => { let client = Arc::new(ElectrumRuntimeClient::new( server_url.clone(), - runtime, + runtime_handle, config, logger, connection_timeout_secs, @@ -602,38 +627,23 @@ struct ElectrumRuntimeClient { electrum_client: Arc, bdk_electrum_client: Arc>>, tx_sync: Arc>>, - runtime: Arc, + runtime_handle: Handle, config: Arc, logger: Arc, } impl ElectrumRuntimeClient { fn new( - server_url: String, runtime: Arc, config: Arc, logger: Arc, + server_url: String, runtime_handle: Handle, config: Arc, logger: Arc, connection_timeout_secs: u64, ) -> Result { - // 0 disables the socket timeout entirely. Values above u8::MAX are capped to 255 - // because the electrum_client crate's timeout field is a u8. - let timeout_opt = match connection_timeout_secs { - 0 => None, - n => { - let capped = n.min(u8::MAX as u64) as u8; - if capped as u64 != n { - log_warn!( - logger, - "Electrum connection_timeout_secs ({}) exceeds maximum of {}; capping to {}.", - n, - u8::MAX, - capped, - ); - } - Some(capped) - }, - }; + // Every socket operation needs a finite bound so a cancelled blocking job eventually + // completes and cannot hold owned-runtime shutdown open indefinitely. + let timeout = effective_connection_timeout_secs(connection_timeout_secs, logger.as_ref()); let electrum_config = ElectrumConfigBuilder::new() .retry(ELECTRUM_CLIENT_NUM_RETRIES) - .timeout(timeout_opt) + .timeout(Some(timeout)) .build(); let electrum_client = Arc::new( @@ -660,7 +670,7 @@ impl ElectrumRuntimeClient { }, )?, ); - Ok(Self { electrum_client, bdk_electrum_client, tx_sync, runtime, config, logger }) + Ok(Self { electrum_client, bdk_electrum_client, tx_sync, runtime_handle, config, logger }) } pub(crate) async fn get_address_balance(&self, address: &bitcoin::Address) -> Option { @@ -670,7 +680,7 @@ impl ElectrumRuntimeClient { let electrum_client = Arc::clone(&self.electrum_client); let script_clone = script.clone(); let balance_result = self - .runtime + .runtime_handle .spawn_blocking(move || { electrum_client .script_get_balance(&script_clone) @@ -694,7 +704,7 @@ impl ElectrumRuntimeClient { let now = Instant::now(); let tx_sync = Arc::clone(&self.tx_sync); - let spawn_fut = self.runtime.spawn_blocking(move || tx_sync.sync(confirmables)); + let spawn_fut = self.runtime_handle.spawn_blocking(move || tx_sync.sync(confirmables)); let timeout_fut = tokio::time::timeout(Duration::from_secs(LDK_WALLET_SYNC_TIMEOUT_SECS), spawn_fut); @@ -730,7 +740,7 @@ impl ElectrumRuntimeClient { let bdk_electrum_client = Arc::clone(&self.bdk_electrum_client); bdk_electrum_client.populate_tx_cache(cached_txs); - let spawn_fut = self.runtime.spawn_blocking(move || { + let spawn_fut = self.runtime_handle.spawn_blocking(move || { bdk_electrum_client.full_scan(request, settings.stop_gap, settings.batch_size, true) }); let wallet_sync_timeout_fut = tokio::time::timeout(timeout, spawn_fut); @@ -760,7 +770,7 @@ impl ElectrumRuntimeClient { bdk_electrum_client.populate_tx_cache(cached_txs); let spawn_fut = self - .runtime + .runtime_handle .spawn_blocking(move || bdk_electrum_client.sync(request, batch_size, true)); let wallet_sync_timeout_fut = tokio::time::timeout(timeout, spawn_fut); @@ -787,7 +797,7 @@ impl ElectrumRuntimeClient { let tx_bytes = tx.encode(); let spawn_fut = - self.runtime.spawn_blocking(move || electrum_client.transaction_broadcast(&tx)); + self.runtime_handle.spawn_blocking(move || electrum_client.transaction_broadcast(&tx)); let timeout_fut = tokio::time::timeout(Duration::from_secs(TX_BROADCAST_TIMEOUT_SECS), spawn_fut); @@ -833,7 +843,8 @@ impl ElectrumRuntimeClient { batch.estimate_fee(num_blocks); } - let spawn_fut = self.runtime.spawn_blocking(move || electrum_client.batch_call(&batch)); + let spawn_fut = + self.runtime_handle.spawn_blocking(move || electrum_client.batch_call(&batch)); let timeout_fut = tokio::time::timeout( Duration::from_secs(FEE_RATE_CACHE_UPDATE_TIMEOUT_SECS), @@ -916,7 +927,87 @@ impl Filter for ElectrumRuntimeClient { #[cfg(test)] mod tests { + use std::net::TcpListener; + use std::process::Command; + use std::sync::mpsc::sync_channel; + use std::thread; + use super::*; + use crate::runtime::Runtime; + + const RUNTIME_SELF_DROP_CHILD_ENV: &str = "LDK_NODE_ELECTRUM_RUNTIME_SELF_DROP_CHILD"; + + #[test] + fn inflight_electrum_worker_does_not_own_runtime_lifecycle() { + if std::env::var_os(RUNTIME_SELF_DROP_CHILD_ENV).is_some() { + run_inflight_electrum_worker_drop(); + return; + } + + let status = Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "chain::electrum::tests::inflight_electrum_worker_does_not_own_runtime_lifecycle", + "--nocapture", + ]) + .env(RUNTIME_SELF_DROP_CHILD_ENV, "1") + .status() + .unwrap(); + + assert!(status.success(), "in-flight Electrum worker aborted during runtime teardown"); + } + + fn run_inflight_electrum_worker_drop() { + std::panic::set_hook(Box::new(|panic_info| { + eprintln!("{panic_info}"); + std::process::exit(101); + })); + + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let server_url = format!("tcp://{}", listener.local_addr().unwrap()); + let (release_server_tx, release_server_rx) = sync_channel(0); + let server_thread = thread::spawn(move || { + let connections = [listener.accept().unwrap().0, listener.accept().unwrap().0]; + let _ = release_server_rx.recv(); + drop(connections); + }); + + let logger = Arc::new(Logger::new_log_facade()); + let runtime = Arc::new(Runtime::new(Arc::clone(&logger)).unwrap()); + let client = Arc::new( + ElectrumRuntimeClient::new( + server_url, + runtime.handle().clone(), + Arc::new(Config::default()), + logger, + 1, + ) + .unwrap(), + ); + let (worker_started_tx, worker_started_rx) = sync_channel(0); + let (release_worker_tx, release_worker_rx) = tokio::sync::oneshot::channel(); + + runtime.spawn_background_task(async move { + worker_started_tx.send(()).unwrap(); + let _ = release_worker_rx.await; + drop(client); + }); + worker_started_rx.recv_timeout(Duration::from_secs(1)).unwrap(); + + let release_thread = thread::spawn(move || { + thread::sleep(Duration::from_millis(100)); + let _ = release_worker_tx.send(()); + }); + + // A runtime handle lets this drop cancel the in-flight worker from the caller thread. An + // Arc stored by the Electrum client would instead defer the last runtime drop to + // the worker itself and abort the subprocess. + drop(runtime); + release_thread.join().unwrap(); + let _ = release_server_tx.send(()); + server_thread.join().unwrap(); + thread::sleep(Duration::from_millis(500)); + } #[test] fn additional_full_scan_settings_do_not_change_primary_defaults() { @@ -941,6 +1032,18 @@ mod tests { ); } + #[test] + fn electrum_connection_timeout_is_always_finite_and_bounded() { + let logger = Logger::new_log_facade(); + + assert_eq!( + effective_connection_timeout_secs(0, &logger), + DEFAULT_ELECTRUM_CONNECTION_TIMEOUT_SECS as u8 + ); + assert_eq!(effective_connection_timeout_secs(1, &logger), 1); + assert_eq!(effective_connection_timeout_secs(300, &logger), u8::MAX); + } + #[test] fn additional_full_scan_settings_are_never_zero() { let config = ElectrumSyncConfig { diff --git a/src/chain/mod.rs b/src/chain/mod.rs index 522e04de1..07aaf6252 100644 --- a/src/chain/mod.rs +++ b/src/chain/mod.rs @@ -34,7 +34,6 @@ use crate::event::{Event, EventQueue, SyncType, TransactionDetails}; use crate::fee_estimator::OnchainFeeEstimator; use crate::io::utils::write_node_metrics; use crate::logger::{log_debug, log_error, log_info, log_trace, LdkLogger, Logger}; -use crate::runtime::Runtime; use crate::types::{Broadcaster, ChainMonitor, ChannelManager, DynStore, Sweeper, Wallet}; use crate::{check_and_emit_balance_update, Error, NodeMetrics}; @@ -710,10 +709,10 @@ impl ChainSource { ) } - pub(crate) fn start(&self, runtime: Arc) -> Result<(), Error> { + pub(crate) fn start(&self, runtime_handle: tokio::runtime::Handle) -> Result<(), Error> { match &self.kind { ChainSourceKind::Electrum(electrum_chain_source) => { - electrum_chain_source.start(runtime)? + electrum_chain_source.start(runtime_handle)? }, _ => { // Nothing to do for other chain sources. diff --git a/src/config.rs b/src/config.rs index 3e1bca7cc..c271b7120 100644 --- a/src/config.rs +++ b/src/config.rs @@ -841,10 +841,10 @@ pub struct ElectrumSyncConfig { /// indefinitely under total packet loss, eventually exhausting Tokio's blocking thread pool /// and causing subsequent `sync_wallets` calls to block indefinitely. /// - /// Set to `0` to disable the socket timeout entirely. The effective maximum is 255 seconds - /// (the underlying `electrum_client` crate uses a `u8`); larger values are capped with a - /// warning. Note that two TCP connections are opened at node start (on-chain and Lightning - /// sync), so budget accordingly when tuning this value. Defaults to 10 seconds. + /// Set to `0` to use the safe 10-second default. The effective maximum is 255 seconds (the + /// underlying `electrum_client` crate uses a `u8`); larger values are capped with a warning. + /// Note that two TCP connections are opened at node start (on-chain and Lightning sync), so + /// budget accordingly when tuning this value. Defaults to 10 seconds. pub connection_timeout_secs: u64, /// Batch size for Electrum full scans of non-primary wallets. /// diff --git a/src/lib.rs b/src/lib.rs index 46b9fc3f2..50f0591cf 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -302,7 +302,9 @@ impl Node { ); // Start up any runtime-dependant chain sources (e.g. Electrum) - self.chain_source.start(Arc::clone(&self.runtime)).map_err(|e| { + // Electrum needs execution access without owning the runtime lifecycle. + let runtime_handle = self.runtime.handle().clone(); + self.chain_source.start(runtime_handle).map_err(|e| { log_error!(self.logger, "Failed to start chain syncing: {}", e); e })?; diff --git a/src/runtime.rs b/src/runtime.rs index 1e9883ae4..d00734a31 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -97,15 +97,6 @@ impl Runtime { *background_processor_task = Some(handle); } - pub fn spawn_blocking(&self, func: F) -> JoinHandle - where - F: FnOnce() -> R + Send + 'static, - R: Send + 'static, - { - let handle = self.handle(); - handle.spawn_blocking(func) - } - pub fn block_on(&self, future: F) -> F::Output { // While we generally decided not to overthink via which call graph users would enter our // runtime context, we'd still try to reuse whatever current context would be present @@ -207,7 +198,7 @@ impl Runtime { ); } - fn handle(&self) -> &tokio::runtime::Handle { + pub(crate) fn handle(&self) -> &tokio::runtime::Handle { match &self.mode { RuntimeMode::Owned(rt) => rt.handle(), RuntimeMode::Handle(handle) => handle, @@ -219,3 +210,22 @@ enum RuntimeMode { Owned(tokio::runtime::Runtime), Handle(tokio::runtime::Handle), } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn runtime_handle_does_not_retain_runtime_owner() { + let runtime = Arc::new(Runtime::new(Arc::new(Logger::new_log_facade())).unwrap()); + let weak_runtime = Arc::downgrade(&runtime); + + let runtime_handle = runtime.handle().clone(); + + assert_eq!(Arc::strong_count(&runtime), 1); + drop(runtime); + assert!(weak_runtime.upgrade().is_none()); + + drop(runtime_handle); + } +} diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs index f368cab78..7cb56ed19 100644 --- a/tests/integration_tests_rust.rs +++ b/tests/integration_tests_rust.rs @@ -2213,6 +2213,22 @@ async fn drop_in_async_context() { node.stop().unwrap(); } +#[test] +fn owned_runtime_electrum_lifecycle() { + let (_bitcoind, electrsd) = setup_bitcoind_and_electrsd(); + let chain_source = TestChainSource::Electrum(&electrsd); + let config = random_config(true); + let node = setup_node(&chain_source, config, Some(vec![42u8; 64])); + + node.sync_wallets().unwrap(); + node.stop().unwrap(); + node.start().unwrap(); + node.sync_wallets().unwrap(); + node.stop().unwrap(); + + drop(node); +} + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn lsps2_client_trusts_lsp() { let (bitcoind, electrsd) = setup_bitcoind_and_electrsd();