From d01033b40fdd5358422998b9097ad0e5d2e7d11e Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Sat, 18 Jul 2026 09:42:55 +0000 Subject: [PATCH 1/6] feat(migration): auto-refresh DAPI nodes during pre-1.0 migration --- CHANGELOG.md | 5 + docs/user-stories.md | 3 + src/backend_task/migration/finish_unwire.rs | 364 +++++++++++++++++++- 3 files changed, 367 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1eddbec28..0a47c3a6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added +- **Automatic Platform node refresh during upgrades**: the first launch that + moves data from a pre-1.0 installation now fetches fresh Mainnet or Testnet + node addresses, saves them, and reconnects automatically, so upgrading users + do not need to find the manual refresh action when old addresses stop working. + - **Search tags in the "Send to" field**: type `type:core`, `type:platform`, `type:shielded`, or `wallet:` to narrow the address suggestions instead of scrolling through everything; plain words still search like diff --git a/docs/user-stories.md b/docs/user-stories.md index c2fccf397..6bbb2a95e 100644 --- a/docs/user-stories.md +++ b/docs/user-stories.md @@ -1261,6 +1261,9 @@ As a user, I want to fetch a fresh list of Platform (DAPI) node addresses from D - "Refresh DAPI endpoints" action available on Mainnet and Testnet. - Confirmation prompt before replacing an existing configured address set. - New addresses are persisted to config and the SDK reinitialized without an app restart. +- The first launch that detects a pre-1.0 migration silently refreshes Mainnet + or Testnet addresses once; the manual action keeps its existing success + message. ### NET-017: View live connection status (indicator and Platform endpoints) [Implemented] **Persona:** Alex, Priya, Jordan diff --git a/src/backend_task/migration/finish_unwire.rs b/src/backend_task/migration/finish_unwire.rs index db7582f82..71c4b0412 100644 --- a/src/backend_task/migration/finish_unwire.rs +++ b/src/backend_task/migration/finish_unwire.rs @@ -7,6 +7,7 @@ //! launches **on the same network**. use std::collections::BTreeSet; +use std::future::Future; use std::sync::Arc; use dash_sdk::dpp::dashcore::Network; @@ -16,6 +17,7 @@ use rusqlite::Connection; use serde::{Deserialize, Serialize}; use crate::backend_task::error::TaskError; +use crate::config::Config; use crate::context::AppContext; use crate::context::migration_status::{MigrationState, MigrationStep}; use crate::model::qualified_identity::QualifiedIdentity; @@ -43,6 +45,15 @@ pub fn sentinel_key_for(network: Network) -> String { ) } +/// Per-network completion key for automatic DAPI refresh during an upgrade. +/// +/// This pass cannot share the wallet-drain sentinel: a prior launch may finish +/// moving wallets while node discovery is temporarily unavailable. Keeping a +/// separate key lets discovery retry without rerunning or gating fund recovery. +pub fn dapi_refresh_sentinel_key_for(network: Network) -> String { + format!("det:migration:dapi_refresh:{}:v1", network_prefix(network)) +} + /// Tables sniffed during detection. Any non-empty row count flips the /// migration into the `Running` state. Ordered so the cheapest check /// (the single-row `wallet` table) runs first. @@ -373,6 +384,149 @@ fn validate_saved_data_for_migration(app_context: &AppContext) -> Result<(), Mig validate_legacy_database_version(version) } +async fn refresh_dapi_nodes_once(app_context: &Arc) { + refresh_dapi_nodes_once_with(app_context, |network| async move { + crate::backend_task::dapi_discovery::discover_and_format(network, None).await + }) + .await; +} + +async fn refresh_dapi_nodes_once_with(app_context: &Arc, discover: D) +where + D: FnOnce(Network) -> F, + F: Future< + Output = Result<(usize, String), crate::backend_task::dapi_discovery::DapiDiscoveryError>, + >, +{ + let network = app_context.network; + let app_kv = app_context.app_kv(); + let sentinel_key = dapi_refresh_sentinel_key_for(network); + + match app_kv.get::(DetScope::Global, &sentinel_key) { + Ok(Some(_)) => return, + Ok(None) => {} + Err(error) => { + tracing::warn!( + target = "migration::finish_unwire", + ?network, + ?error, + "Could not read the automatic DAPI refresh sentinel; node discovery will retry on the next launch", + ); + return; + } + } + + match detect_legacy_rows(app_context) { + Ok(true) => {} + Ok(false) => { + write_dapi_refresh_completion(&app_kv, &sentinel_key, network, 0); + return; + } + Err(error) => { + tracing::warn!( + target = "migration::finish_unwire", + ?network, + ?error, + "Could not inspect the previous version's data for automatic DAPI refresh; recording the refresh as unnecessary", + ); + write_dapi_refresh_completion(&app_kv, &sentinel_key, network, 0); + return; + } + } + + if !matches!(network, Network::Mainnet | Network::Testnet) { + write_dapi_refresh_completion(&app_kv, &sentinel_key, network, 0); + return; + } + + let (count, addresses_csv) = match discover(network).await { + Ok(result) => result, + Err(error) => { + tracing::warn!( + target = "migration::finish_unwire", + ?network, + ?error, + "Automatic DAPI node discovery failed during migration; it will retry on the next launch", + ); + return; + } + }; + + let mut config = match Config::load_from(&app_context.data_dir) { + Ok(config) => config, + Err(error) => { + tracing::warn!( + target = "migration::finish_unwire", + ?network, + ?error, + "Could not load the network configuration after automatic DAPI discovery; the refresh will retry on the next launch", + ); + return; + } + }; + let mut network_config = config + .config_for_network(network) + .clone() + .unwrap_or_default(); + network_config.dapi_addresses = Some(addresses_csv); + config.update_config_for_network(network, network_config.clone()); + if let Err(error) = config.save(&app_context.data_dir) { + tracing::warn!( + target = "migration::finish_unwire", + ?network, + ?error, + "Could not save automatically discovered DAPI nodes; the refresh will retry on the next launch", + ); + return; + } + + match app_context.config.write() { + Ok(mut live_config) => *live_config = network_config, + Err(error) => { + tracing::warn!( + target = "migration::finish_unwire", + ?network, + ?error, + "Could not update the live network configuration with discovered DAPI nodes; the refresh will retry on the next launch", + ); + return; + } + } + + if let Err(error) = Arc::clone(app_context).reinit_core_client_and_sdk() { + tracing::warn!( + target = "migration::finish_unwire", + ?network, + ?error, + "Could not reinitialize network clients after automatic DAPI refresh; the saved addresses will be used on the next launch", + ); + } + + tracing::info!( + target = "migration::finish_unwire", + ?network, + count, + "Automatically refreshed DAPI nodes during migration", + ); + write_dapi_refresh_completion(&app_kv, &sentinel_key, network, 1); +} + +fn write_dapi_refresh_completion( + app_kv: &crate::wallet_backend::DetKv, + sentinel_key: &str, + network: Network, + network_count: u32, +) { + if let Err(error) = write_completion_sentinel(app_kv, sentinel_key, network_count) { + tracing::warn!( + target = "migration::finish_unwire", + ?network, + ?error, + "Could not write the automatic DAPI refresh sentinel; the pass may retry on the next launch", + ); + } +} + /// Run the FinishUnwire migration. Idempotent — completes a no-op when /// the sentinels are already present. /// @@ -382,7 +536,12 @@ fn validate_saved_data_for_migration(app_context: &AppContext) -> Result<(), Mig /// decide whether to surface a "storage update complete" banner — a no-op /// launch must not show one. /// -/// Three independent passes, each under its own sentinel, in this order: +/// Before the recovery passes, a best-effort DAPI refresh runs under its own +/// sentinel. It never changes migration state or propagates an error, so node +/// discovery cannot affect the precedence below or delay access to recovered +/// funds and identities. +/// +/// Three independent recovery passes, each under its own sentinel, in this order: /// /// 1. **App data** (scheduled votes, top-up history) — DET-owned rows the /// wallet drain never touched. @@ -479,8 +638,20 @@ pub async fn run(app_context: &Arc) -> Result { } async fn run_under_guard(app_context: &Arc) -> Result { + run_under_guard_with_dapi_refresh(app_context, refresh_dapi_nodes_once(app_context)).await +} + +async fn run_under_guard_with_dapi_refresh( + app_context: &Arc, + dapi_refresh: F, +) -> Result +where + F: Future, +{ validate_saved_data_for_migration(app_context)?; + dapi_refresh.await; + let status = app_context.migration_status(); // Scheduled votes and top-up history carry their own sentinel and run @@ -1240,8 +1411,9 @@ fn migrate_app_data(app_context: &Arc) -> Result = tokio::sync::Mutex::const_new(()); fn kv() -> DetKv { DetKv::from_store(Arc::new(InMemoryKv::default())) } + fn dapi_refresh_is_complete(app_context: &AppContext) -> bool { + app_context + .app_kv() + .get::( + DetScope::Global, + &dapi_refresh_sentinel_key_for(app_context.network), + ) + .expect("read DAPI refresh sentinel") + .is_some() + } + + fn seed_legacy_single_key(app_context: &AppContext) { + let path = app_context.db.db_file_path().expect("file-backed database"); + let conn = Connection::open(path).expect("open legacy database"); + seed_legacy_row( + &conn, + &[7u8; 32], + &[1u8; 32], + &[], + &[], + "addr", + None, + false, + app_context.network, + ); + } + + #[tokio::test] + async fn dapi_refresh_fresh_install_skips_discovery_and_preserves_config() { + let _env_guard = CONFIG_ENV_LOCK.lock().await; + let tmp = tempfile::tempdir().expect("tempdir"); + let ctx = fresh_app_context(tmp.path()); + let original_env = std::fs::read(tmp.path().join(".env")).expect("read original config"); + let original_live_addresses = ctx + .config + .read() + .expect("read original in-memory config") + .dapi_addresses + .clone(); + let calls = AtomicUsize::new(0); + + refresh_dapi_nodes_once_with(&ctx, |_| async { + calls.fetch_add(1, Ordering::SeqCst); + Ok((1, "https://unused.example:443".to_string())) + }) + .await; + + assert_eq!(calls.load(Ordering::SeqCst), 0); + assert!(dapi_refresh_is_complete(&ctx)); + assert_eq!( + std::fs::read(tmp.path().join(".env")).expect("read final config"), + original_env, + ); + assert_eq!( + ctx.config + .read() + .expect("read in-memory config") + .dapi_addresses + .as_ref(), + original_live_addresses.as_ref(), + ); + } + + #[tokio::test] + async fn dapi_refresh_legacy_install_updates_disk_and_live_config() { + let _env_guard = CONFIG_ENV_LOCK.lock().await; + let tmp = tempfile::tempdir().expect("tempdir"); + let ctx = fresh_app_context(tmp.path()); + seed_legacy_single_key(&ctx); + let calls = AtomicUsize::new(0); + let calls_ref = &calls; + let addresses = "https://one.example:443,https://two.example:443"; + + refresh_dapi_nodes_once_with(&ctx, |network| async move { + assert_eq!(network, Network::Testnet); + calls_ref.fetch_add(1, Ordering::SeqCst); + Ok((2, addresses.to_string())) + }) + .await; + + assert_eq!(calls.load(Ordering::SeqCst), 1); + assert!(dapi_refresh_is_complete(&ctx)); + let saved = crate::config::Config::load_from(tmp.path()).expect("reload saved config"); + assert_eq!( + saved + .config_for_network(Network::Testnet) + .as_ref() + .and_then(|config| config.dapi_addresses.as_deref()), + Some(addresses), + ); + assert_eq!( + ctx.config + .read() + .expect("read in-memory config") + .dapi_addresses + .as_deref(), + Some(addresses), + ); + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn dapi_refresh_discovery_failure_retries_without_failing_migration() { + let _env_guard = CONFIG_ENV_LOCK.lock().await; + let tmp = tempfile::tempdir().expect("tempdir"); + let ctx = fresh_app_context(tmp.path()); + seed_legacy_wallet(&ctx, &[0x81u8; 64], "funds", ctx.network); + wire_backend(&ctx).await; + let backend = ctx.wallet_backend().expect("backend wired"); + let calls = AtomicUsize::new(0); + + let refresh = refresh_dapi_nodes_once_with(&ctx, |_| async { + calls.fetch_add(1, Ordering::SeqCst); + Err(crate::backend_task::dapi_discovery::DapiDiscoveryError::Timeout) + }); + let did_work = run_under_guard_with_dapi_refresh(&ctx, refresh) + .await + .expect("DAPI discovery must not fail the migration"); + + assert!(did_work, "the independent wallet pass still moved data"); + assert_eq!(calls.load(Ordering::SeqCst), 1); + assert!(!dapi_refresh_is_complete(&ctx)); + + refresh_dapi_nodes_once_with(&ctx, |_| async { + calls.fetch_add(1, Ordering::SeqCst); + Err(crate::backend_task::dapi_discovery::DapiDiscoveryError::Timeout) + }) + .await; + + assert_eq!(calls.load(Ordering::SeqCst), 2, "the next launch retries"); + assert!(!dapi_refresh_is_complete(&ctx)); + backend.shutdown().await; + } + + #[tokio::test] + async fn dapi_refresh_devnet_legacy_install_skips_discovery() { + let _env_guard = CONFIG_ENV_LOCK.lock().await; + let tmp = tempfile::tempdir().expect("tempdir"); + let ctx = app_context_for_network(tmp.path(), Network::Devnet); + seed_legacy_single_key(&ctx); + let calls = AtomicUsize::new(0); + + refresh_dapi_nodes_once_with(&ctx, |_| async { + calls.fetch_add(1, Ordering::SeqCst); + Ok((1, "https://unused.example:443".to_string())) + }) + .await; + + assert_eq!(calls.load(Ordering::SeqCst), 0); + assert!(dapi_refresh_is_complete(&ctx)); + } + + #[tokio::test] + async fn dapi_refresh_existing_sentinel_skips_discovery() { + let _env_guard = CONFIG_ENV_LOCK.lock().await; + let tmp = tempfile::tempdir().expect("tempdir"); + let ctx = fresh_app_context(tmp.path()); + seed_legacy_single_key(&ctx); + let calls = AtomicUsize::new(0); + + refresh_dapi_nodes_once_with(&ctx, |_| async { + calls.fetch_add(1, Ordering::SeqCst); + Ok((1, "https://first.example:443".to_string())) + }) + .await; + assert_eq!(calls.load(Ordering::SeqCst), 1); + assert!(dapi_refresh_is_complete(&ctx)); + + refresh_dapi_nodes_once_with(&ctx, |_| async { + calls.fetch_add(1, Ordering::SeqCst); + Ok((1, "https://unused.example:443".to_string())) + }) + .await; + + assert_eq!(calls.load(Ordering::SeqCst), 1); + assert!(dapi_refresh_is_complete(&ctx)); + } + #[test] fn unreadable_top_up_warning_is_durable_and_acknowledgeable() { let app_kv = kv(); @@ -4381,8 +4733,10 @@ mod tests { /// two `run()` no-op paths, which return before touching the wallet /// backend. fn fresh_app_context(dir: &std::path::Path) -> Arc { - use dash_sdk::dpp::dashcore::Network; + app_context_for_network(dir, Network::Testnet) + } + fn app_context_for_network(dir: &std::path::Path, network: Network) -> Arc { crate::app_dir::ensure_env_file(dir); let db_file = dir.join("data.db"); let db = Arc::new(crate::database::Database::new(&db_file).expect("db")); @@ -4393,7 +4747,7 @@ mod tests { let secret_store = AppContext::open_secret_store(dir).expect("open secret store"); AppContext::new( dir.to_path_buf(), - Network::Testnet, + network, db, Default::default(), Default::default(), From c52389f15e222e7df3218c174e010f64cc192cd3 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Sat, 18 Jul 2026 10:32:34 +0000 Subject: [PATCH 2/6] fix(migration): keep DAPI refresh off recovery paths Keep test migration runs offline, detach automatic discovery from storage recovery, and retry after transient legacy-data read failures. Co-Authored-By: OpenAI Codex GPT-5 --- src/backend_task/migration/finish_unwire.rs | 82 +++++++++++++++++++-- 1 file changed, 74 insertions(+), 8 deletions(-) diff --git a/src/backend_task/migration/finish_unwire.rs b/src/backend_task/migration/finish_unwire.rs index 71c4b0412..6024f8ffe 100644 --- a/src/backend_task/migration/finish_unwire.rs +++ b/src/backend_task/migration/finish_unwire.rs @@ -384,6 +384,7 @@ fn validate_saved_data_for_migration(app_context: &AppContext) -> Result<(), Mig validate_legacy_database_version(version) } +#[cfg(not(test))] async fn refresh_dapi_nodes_once(app_context: &Arc) { refresh_dapi_nodes_once_with(app_context, |network| async move { crate::backend_task::dapi_discovery::discover_and_format(network, None).await @@ -391,12 +392,38 @@ async fn refresh_dapi_nodes_once(app_context: &Arc) { .await; } +#[cfg(test)] +async fn refresh_dapi_nodes_once(app_context: &Arc) { + refresh_dapi_nodes_once_with(app_context, |_| async { + Err(crate::backend_task::dapi_discovery::DapiDiscoveryError::Timeout) + }) + .await; +} + +/// Runs one best-effort refresh with an injected discovery operation. +/// +/// Config persistence intentionally shares the manual refresh's unlocked load-mutate-save; +/// concurrent settings saves may overwrite either snapshot in this accepted narrow window. async fn refresh_dapi_nodes_once_with(app_context: &Arc, discover: D) where D: FnOnce(Network) -> F, F: Future< Output = Result<(usize, String), crate::backend_task::dapi_discovery::DapiDiscoveryError>, >, +{ + refresh_dapi_nodes_once_with_legacy_check(app_context, discover, detect_legacy_rows).await; +} + +async fn refresh_dapi_nodes_once_with_legacy_check( + app_context: &Arc, + discover: D, + detect_legacy: L, +) where + D: FnOnce(Network) -> F, + F: Future< + Output = Result<(usize, String), crate::backend_task::dapi_discovery::DapiDiscoveryError>, + >, + L: FnOnce(&AppContext) -> Result, { let network = app_context.network; let app_kv = app_context.app_kv(); @@ -416,7 +443,7 @@ where } } - match detect_legacy_rows(app_context) { + match detect_legacy(app_context) { Ok(true) => {} Ok(false) => { write_dapi_refresh_completion(&app_kv, &sentinel_key, network, 0); @@ -427,9 +454,8 @@ where target = "migration::finish_unwire", ?network, ?error, - "Could not inspect the previous version's data for automatic DAPI refresh; recording the refresh as unnecessary", + "Could not inspect the previous version's data for automatic DAPI refresh; node discovery will retry on the next launch", ); - write_dapi_refresh_completion(&app_kv, &sentinel_key, network, 0); return; } } @@ -536,10 +562,10 @@ fn write_dapi_refresh_completion( /// decide whether to surface a "storage update complete" banner — a no-op /// launch must not show one. /// -/// Before the recovery passes, a best-effort DAPI refresh runs under its own -/// sentinel. It never changes migration state or propagates an error, so node -/// discovery cannot affect the precedence below or delay access to recovered -/// funds and identities. +/// A best-effort DAPI refresh starts concurrently under its own sentinel. It +/// never changes migration state or propagates an error, so node discovery +/// cannot affect the precedence below or delay access to recovered funds and +/// identities. /// /// Three independent recovery passes, each under its own sentinel, in this order: /// @@ -638,7 +664,11 @@ pub async fn run(app_context: &Arc) -> Result { } async fn run_under_guard(app_context: &Arc) -> Result { - run_under_guard_with_dapi_refresh(app_context, refresh_dapi_nodes_once(app_context)).await + let ctx = Arc::clone(app_context); + std::mem::drop(tokio::spawn(async move { + refresh_dapi_nodes_once(&ctx).await; + })); + run_under_guard_with_dapi_refresh(app_context, std::future::ready(())).await } async fn run_under_guard_with_dapi_refresh( @@ -2708,6 +2738,42 @@ mod tests { ); } + #[tokio::test] + async fn dapi_refresh_legacy_detection_failure_retries_then_completes() { + let _env_guard = CONFIG_ENV_LOCK.lock().await; + let tmp = tempfile::tempdir().expect("tempdir"); + let ctx = fresh_app_context(tmp.path()); + seed_legacy_single_key(&ctx); + let calls = AtomicUsize::new(0); + + refresh_dapi_nodes_once_with_legacy_check( + &ctx, + |_| async { + calls.fetch_add(1, Ordering::SeqCst); + Ok((1, "https://unused.example:443".to_string())) + }, + |_| { + Err(MigrationError::LegacyDbRead { + table: "wallet", + source: rusqlite::Error::InvalidQuery, + }) + }, + ) + .await; + + assert_eq!(calls.load(Ordering::SeqCst), 0); + assert!(!dapi_refresh_is_complete(&ctx)); + + refresh_dapi_nodes_once_with(&ctx, |_| async { + calls.fetch_add(1, Ordering::SeqCst); + Ok((1, "https://retry.example:443".to_string())) + }) + .await; + + assert_eq!(calls.load(Ordering::SeqCst), 1); + assert!(dapi_refresh_is_complete(&ctx)); + } + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn dapi_refresh_discovery_failure_retries_without_failing_migration() { let _env_guard = CONFIG_ENV_LOCK.lock().await; From 93797f4d5dcfe3bfde1af28691fa8948fc2c7ac3 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Sat, 18 Jul 2026 13:19:35 +0000 Subject: [PATCH 3/6] fix(migration): serialize DAPI refresh against retries Guard detached refreshes with the migration mutex, complete unsupported networks before legacy detection, and clarify best-effort retry semantics. Co-Authored-By: Codex GPT-5 --- CHANGELOG.md | 8 +- docs/user-stories.md | 6 +- src/backend_task/migration/finish_unwire.rs | 103 +++++++++++++++++--- 3 files changed, 96 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a47c3a6a..5908e95f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,10 +8,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added -- **Automatic Platform node refresh during upgrades**: the first launch that - moves data from a pre-1.0 installation now fetches fresh Mainnet or Testnet - node addresses, saves them, and reconnects automatically, so upgrading users - do not need to find the manual refresh action when old addresses stop working. +- **Automatic Platform node refresh during upgrades**: migrating a pre-1.0 + installation now triggers a best-effort Mainnet or Testnet node refresh. + Failed attempts retry on later launches until fresh addresses are saved and + the app reconnects, so upgrading users do not need to find the manual action. - **Search tags in the "Send to" field**: type `type:core`, `type:platform`, `type:shielded`, or `wallet:` to narrow the address suggestions diff --git a/docs/user-stories.md b/docs/user-stories.md index 6bbb2a95e..d731a6ea9 100644 --- a/docs/user-stories.md +++ b/docs/user-stories.md @@ -1261,9 +1261,9 @@ As a user, I want to fetch a fresh list of Platform (DAPI) node addresses from D - "Refresh DAPI endpoints" action available on Mainnet and Testnet. - Confirmation prompt before replacing an existing configured address set. - New addresses are persisted to config and the SDK reinitialized without an app restart. -- The first launch that detects a pre-1.0 migration silently refreshes Mainnet - or Testnet addresses once; the manual action keeps its existing success - message. +- A pre-1.0 migration triggers a silent, best-effort Mainnet or Testnet address + refresh; failures retry on later launches until addresses are saved, while + the manual action keeps its existing success message. ### NET-017: View live connection status (indicator and Platform endpoints) [Implemented] **Persona:** Alex, Priya, Jordan diff --git a/src/backend_task/migration/finish_unwire.rs b/src/backend_task/migration/finish_unwire.rs index 6024f8ffe..9c5155974 100644 --- a/src/backend_task/migration/finish_unwire.rs +++ b/src/backend_task/migration/finish_unwire.rs @@ -402,8 +402,9 @@ async fn refresh_dapi_nodes_once(app_context: &Arc) { /// Runs one best-effort refresh with an injected discovery operation. /// -/// Config persistence intentionally shares the manual refresh's unlocked load-mutate-save; -/// concurrent settings saves may overwrite either snapshot in this accepted narrow window. +/// Run-triggered passes hold `migration_run` from sentinel read through completion, so they cannot race each other. +/// The independent manual "Refresh DAPI endpoints" path in `network_chooser_screen.rs` takes no such guard, +/// leaving its load-save sequence as the accepted narrow-window race. async fn refresh_dapi_nodes_once_with(app_context: &Arc, discover: D) where D: FnOnce(Network) -> F, @@ -443,6 +444,11 @@ async fn refresh_dapi_nodes_once_with_legacy_check( } } + if !matches!(network, Network::Mainnet | Network::Testnet) { + write_dapi_refresh_completion(&app_kv, &sentinel_key, network, 0); + return; + } + match detect_legacy(app_context) { Ok(true) => {} Ok(false) => { @@ -460,11 +466,6 @@ async fn refresh_dapi_nodes_once_with_legacy_check( } } - if !matches!(network, Network::Mainnet | Network::Testnet) { - write_dapi_refresh_completion(&app_kv, &sentinel_key, network, 0); - return; - } - let (count, addresses_csv) = match discover(network).await { Ok(result) => result, Err(error) => { @@ -663,9 +664,22 @@ pub async fn run(app_context: &Arc) -> Result { } } +/// Owns `migration_run` for the full detached refresh after the launching pass releases it. +/// The launching pass never awaits this handle, so waiting for its guard cannot deadlock. +fn spawn_dapi_refresh(app_context: &Arc, refresh: F) -> tokio::task::JoinHandle<()> +where + F: Future + Send + 'static, +{ + let ctx = Arc::clone(app_context); + tokio::spawn(async move { + let _refresh_guard = ctx.migration_run.lock().await; + refresh.await; + }) +} + async fn run_under_guard(app_context: &Arc) -> Result { let ctx = Arc::clone(app_context); - std::mem::drop(tokio::spawn(async move { + std::mem::drop(spawn_dapi_refresh(app_context, async move { refresh_dapi_nodes_once(&ctx).await; })); run_under_guard_with_dapi_refresh(app_context, std::future::ready(())).await @@ -2649,6 +2663,14 @@ mod tests { .is_some() } + async fn wait_for_dapi_refresh(app_context: &Arc) { + // `run` deliberately detaches this work; yield so it queues for the guard, + // then acquire the same guard after the refresh releases it. + tokio::task::yield_now().await; + let guard = app_context.migration_run.lock().await; + drop(guard); + } + fn seed_legacy_single_key(app_context: &AppContext) { let path = app_context.db.db_file_path().expect("file-backed database"); let conn = Connection::open(path).expect("open legacy database"); @@ -2807,18 +2829,69 @@ mod tests { backend.shutdown().await; } + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn dapi_refresh_launches_are_serialized() { + let tmp = tempfile::tempdir().expect("tempdir"); + let ctx = fresh_app_context(tmp.path()); + let attempts = Arc::new(AtomicUsize::new(0)); + let first_started = Arc::new(tokio::sync::Notify::new()); + let release_first = Arc::new(tokio::sync::Notify::new()); + + let first_attempts = Arc::clone(&attempts); + let first_started_task = Arc::clone(&first_started); + let release_first_task = Arc::clone(&release_first); + let first = spawn_dapi_refresh(&ctx, async move { + first_attempts.fetch_add(1, Ordering::SeqCst); + first_started_task.notify_one(); + release_first_task.notified().await; + }); + first_started.notified().await; + + let second_attempts = Arc::clone(&attempts); + let second_started = Arc::new(tokio::sync::Notify::new()); + let second_started_task = Arc::clone(&second_started); + let second = spawn_dapi_refresh(&ctx, async move { + second_attempts.fetch_add(1, Ordering::SeqCst); + second_started_task.notify_one(); + }); + + assert!( + tokio::time::timeout( + std::time::Duration::from_millis(50), + second_started.notified(), + ) + .await + .is_err(), + "a second refresh must wait for the first refresh's migration guard", + ); + assert_eq!(attempts.load(Ordering::SeqCst), 1); + + release_first.notify_one(); + first.await.expect("join first refresh"); + second.await.expect("join second refresh"); + assert_eq!(attempts.load(Ordering::SeqCst), 2); + } + #[tokio::test] - async fn dapi_refresh_devnet_legacy_install_skips_discovery() { + async fn dapi_refresh_devnet_detection_failure_completes_without_discovery() { let _env_guard = CONFIG_ENV_LOCK.lock().await; let tmp = tempfile::tempdir().expect("tempdir"); let ctx = app_context_for_network(tmp.path(), Network::Devnet); - seed_legacy_single_key(&ctx); let calls = AtomicUsize::new(0); - refresh_dapi_nodes_once_with(&ctx, |_| async { - calls.fetch_add(1, Ordering::SeqCst); - Ok((1, "https://unused.example:443".to_string())) - }) + refresh_dapi_nodes_once_with_legacy_check( + &ctx, + |_| async { + calls.fetch_add(1, Ordering::SeqCst); + Ok((1, "https://unused.example:443".to_string())) + }, + |_| { + Err(MigrationError::LegacyDbRead { + table: "wallet", + source: rusqlite::Error::InvalidQuery, + }) + }, + ) .await; assert_eq!(calls.load(Ordering::SeqCst), 0); @@ -5717,6 +5790,7 @@ mod tests { run(&ctx).await.expect("first run"), "the first launch moves legacy data", ); + wait_for_dapi_refresh(&ctx).await; assert!(backend.is_wallet_registered(&seed_hash)); assert_eq!(ctx.get_scheduled_votes().expect("read votes").len(), 1); let sentinel_after_first = read_sentinel(&ctx.app_kv(), network) @@ -6170,6 +6244,7 @@ mod tests { let backend = ctx.wallet_backend().expect("backend wired"); run(&ctx).await.expect("first launch"); + wait_for_dapi_refresh(&ctx).await; let deleted_id = Identifier::from(deleted); assert!( From f665677c745f58608ee6127ebe501366cbc323f3 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Sat, 18 Jul 2026 23:14:58 +0000 Subject: [PATCH 4/6] fix(migration): serialize DAPI config persistence across networks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3's migration_run guard only serializes DAPI refresh retries within a single AppContext. thepastaclaw and CodeRabbit both flagged (BLOCKING) that this doesn't protect two different network contexts (e.g. Mainnet + Testnet, which can coexist in one process) racing on the same shared .env file via Config::load_from/save. Add a process-wide CONFIG_PERSISTENCE_LOCK (plain std::sync::Mutex, no .await held across it) in config.rs, and acquire it around the load->mutate->save->live-update span in both the migration refresh path (finish_unwire.rs) and the manual "Refresh DAPI endpoints" button handler (network_chooser_screen.rs) — the only two writers of this file. New test dapi_config_persistence_serializes_across_networks proves actual serialization (not just lock presence) using real OS threads: a Mainnet section pauses mid-critical-section while a Testnet section attempts to enter, and the test asserts the second section cannot enter or save until the first releases, then confirms both networks' addresses land correctly on disk with no clobbering. Co-Authored-By: Codex Sol --- src/backend_task/migration/finish_unwire.rs | 218 +++++++++++++++++--- src/config.rs | 2 + src/ui/network_chooser_screen.rs | 48 +++-- 3 files changed, 216 insertions(+), 52 deletions(-) diff --git a/src/backend_task/migration/finish_unwire.rs b/src/backend_task/migration/finish_unwire.rs index 9c5155974..7f7a19d9e 100644 --- a/src/backend_task/migration/finish_unwire.rs +++ b/src/backend_task/migration/finish_unwire.rs @@ -17,7 +17,7 @@ use rusqlite::Connection; use serde::{Deserialize, Serialize}; use crate::backend_task::error::TaskError; -use crate::config::Config; +use crate::config::{CONFIG_PERSISTENCE_LOCK, Config}; use crate::context::AppContext; use crate::context::migration_status::{MigrationState, MigrationStep}; use crate::model::qualified_identity::QualifiedIdentity; @@ -402,9 +402,8 @@ async fn refresh_dapi_nodes_once(app_context: &Arc) { /// Runs one best-effort refresh with an injected discovery operation. /// -/// Run-triggered passes hold `migration_run` from sentinel read through completion, so they cannot race each other. -/// The independent manual "Refresh DAPI endpoints" path in `network_chooser_screen.rs` takes no such guard, -/// leaving its load-save sequence as the accepted narrow-window race. +/// Run-triggered passes still hold their per-context `migration_run`, while migration and manual refresh +/// share a process-wide guard across whole-file config persistence, including different network contexts. async fn refresh_dapi_nodes_once_with(app_context: &Arc, discover: D) where D: FnOnce(Network) -> F, @@ -479,44 +478,49 @@ async fn refresh_dapi_nodes_once_with_legacy_check( } }; - let mut config = match Config::load_from(&app_context.data_dir) { - Ok(config) => config, - Err(error) => { + { + let _persistence_guard = CONFIG_PERSISTENCE_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let mut config = match Config::load_from(&app_context.data_dir) { + Ok(config) => config, + Err(error) => { + tracing::warn!( + target = "migration::finish_unwire", + ?network, + ?error, + "Could not load the network configuration after automatic DAPI discovery; the refresh will retry on the next launch", + ); + return; + } + }; + let mut network_config = config + .config_for_network(network) + .clone() + .unwrap_or_default(); + network_config.dapi_addresses = Some(addresses_csv); + config.update_config_for_network(network, network_config.clone()); + if let Err(error) = config.save(&app_context.data_dir) { tracing::warn!( target = "migration::finish_unwire", ?network, ?error, - "Could not load the network configuration after automatic DAPI discovery; the refresh will retry on the next launch", + "Could not save automatically discovered DAPI nodes; the refresh will retry on the next launch", ); return; } - }; - let mut network_config = config - .config_for_network(network) - .clone() - .unwrap_or_default(); - network_config.dapi_addresses = Some(addresses_csv); - config.update_config_for_network(network, network_config.clone()); - if let Err(error) = config.save(&app_context.data_dir) { - tracing::warn!( - target = "migration::finish_unwire", - ?network, - ?error, - "Could not save automatically discovered DAPI nodes; the refresh will retry on the next launch", - ); - return; - } - match app_context.config.write() { - Ok(mut live_config) => *live_config = network_config, - Err(error) => { - tracing::warn!( - target = "migration::finish_unwire", - ?network, - ?error, - "Could not update the live network configuration with discovered DAPI nodes; the refresh will retry on the next launch", - ); - return; + match app_context.config.write() { + Ok(mut live_config) => *live_config = network_config, + Err(error) => { + tracing::warn!( + target = "migration::finish_unwire", + ?network, + ?error, + "Could not update the live network configuration with discovered DAPI nodes; the refresh will retry on the next launch", + ); + return; + } } } @@ -2642,6 +2646,7 @@ impl From for TaskError { #[cfg(test)] mod tests { use super::*; + use crate::config::NetworkConfig; use crate::wallet_backend::DetKv; use crate::wallet_backend::kv_test_support::InMemoryKv; use std::sync::atomic::{AtomicUsize, Ordering}; @@ -2760,6 +2765,151 @@ mod tests { ); } + #[derive(Debug, Clone, Copy, PartialEq, Eq)] + enum ConfigPersistenceEvent { + FirstEntered, + FirstPausedBeforeSave, + SecondAttempting, + SecondEntered, + SecondSaved, + FirstSaved, + } + + #[tokio::test] + async fn dapi_config_persistence_serializes_across_networks() { + let _env_guard = CONFIG_ENV_LOCK.lock().await; + let tmp = tempfile::tempdir().expect("tempdir"); + Config { + mainnet_config: Some(NetworkConfig { + dapi_addresses: Some("https://mainnet-old.example:443".to_string()), + ..Default::default() + }), + testnet_config: Some(NetworkConfig { + dapi_addresses: Some("https://testnet-old.example:443".to_string()), + ..Default::default() + }), + devnet_config: None, + local_config: None, + } + .save(tmp.path()) + .expect("write initial config"); + + let (event_tx, event_rx) = std::sync::mpsc::channel(); + let (release_first_tx, release_first_rx) = std::sync::mpsc::channel(); + let first_data_dir = tmp.path().to_path_buf(); + let first_event_tx = event_tx.clone(); + let first = std::thread::spawn(move || { + let _persistence_guard = CONFIG_PERSISTENCE_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + first_event_tx + .send(ConfigPersistenceEvent::FirstEntered) + .expect("report first entry"); + let mut config = Config::load_from(&first_data_dir).expect("first load"); + let mut network_config = config + .config_for_network(Network::Mainnet) + .clone() + .expect("mainnet config"); + network_config.dapi_addresses = Some("https://mainnet-new.example:443".to_string()); + config.update_config_for_network(Network::Mainnet, network_config); + first_event_tx + .send(ConfigPersistenceEvent::FirstPausedBeforeSave) + .expect("report first pause"); + release_first_rx.recv().expect("release first save"); + config.save(&first_data_dir).expect("first save"); + first_event_tx + .send(ConfigPersistenceEvent::FirstSaved) + .expect("report first save"); + }); + + assert_eq!( + event_rx.recv().expect("first entry event"), + ConfigPersistenceEvent::FirstEntered, + ); + assert_eq!( + event_rx.recv().expect("first pause event"), + ConfigPersistenceEvent::FirstPausedBeforeSave, + ); + + let second_data_dir = tmp.path().to_path_buf(); + let second_event_tx = event_tx.clone(); + let second = std::thread::spawn(move || { + second_event_tx + .send(ConfigPersistenceEvent::SecondAttempting) + .expect("report second attempt"); + let _persistence_guard = CONFIG_PERSISTENCE_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + second_event_tx + .send(ConfigPersistenceEvent::SecondEntered) + .expect("report second entry"); + let mut config = Config::load_from(&second_data_dir).expect("second load"); + let mut network_config = config + .config_for_network(Network::Testnet) + .clone() + .expect("testnet config"); + network_config.dapi_addresses = Some("https://testnet-new.example:443".to_string()); + config.update_config_for_network(Network::Testnet, network_config); + config.save(&second_data_dir).expect("second save"); + second_event_tx + .send(ConfigPersistenceEvent::SecondSaved) + .expect("report second save"); + }); + + assert_eq!( + event_rx.recv().expect("second attempt event"), + ConfigPersistenceEvent::SecondAttempting, + ); + let premature_entry = event_rx.recv_timeout(std::time::Duration::from_millis(250)); + let premature_save = if premature_entry.is_ok() { + Some( + event_rx + .recv_timeout(std::time::Duration::from_secs(1)) + .expect("second save before first release"), + ) + } else { + None + }; + + release_first_tx.send(()).expect("release first"); + first.join().expect("join first persistence section"); + second.join().expect("join second persistence section"); + + assert_eq!( + premature_entry, + Err(std::sync::mpsc::RecvTimeoutError::Timeout), + "the second persistence section entered before the first released", + ); + assert!( + premature_save.is_none(), + "the second persistence section saved before the first released: {premature_save:?}", + ); + assert_eq!( + event_rx.try_iter().collect::>(), + [ + ConfigPersistenceEvent::FirstSaved, + ConfigPersistenceEvent::SecondEntered, + ConfigPersistenceEvent::SecondSaved, + ], + ); + + let saved = Config::load_from(tmp.path()).expect("load final config"); + assert_eq!( + saved + .config_for_network(Network::Mainnet) + .as_ref() + .and_then(|config| config.dapi_addresses.as_deref()), + Some("https://mainnet-new.example:443"), + ); + assert_eq!( + saved + .config_for_network(Network::Testnet) + .as_ref() + .and_then(|config| config.dapi_addresses.as_deref()), + Some("https://testnet-new.example:443"), + ); + } + #[tokio::test] async fn dapi_refresh_legacy_detection_failure_retries_then_completes() { let _env_guard = CONFIG_ENV_LOCK.lock().await; diff --git a/src/config.rs b/src/config.rs index b1f2c4178..543f144dd 100644 --- a/src/config.rs +++ b/src/config.rs @@ -8,6 +8,8 @@ use dash_sdk::dpp::dashcore::Network; use serde::Deserialize; use tempfile::NamedTempFile; +pub(crate) static CONFIG_PERSISTENCE_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + #[derive(Debug, Deserialize, Clone)] pub struct Config { pub mainnet_config: Option, diff --git a/src/ui/network_chooser_screen.rs b/src/ui/network_chooser_screen.rs index 822b45d88..33e6af87f 100644 --- a/src/ui/network_chooser_screen.rs +++ b/src/ui/network_chooser_screen.rs @@ -1473,28 +1473,40 @@ impl ScreenLike for NetworkChooserScreen { { self.discovery_in_progress = false; - // Update config with new addresses - let data_dir = &self.current_app_context().data_dir; - if let Ok(mut config) = Config::load_from(data_dir) { - let mut network_cfg = config - .config_for_network(network) - .clone() - .unwrap_or_default(); - network_cfg.dapi_addresses = Some(addresses_csv); - config.update_config_for_network(network, network_cfg.clone()); - - if let Err(e) = config.save(data_dir) { - tracing::error!("Failed to save config after DAPI discovery: {e}"); - } + let config_loaded = { + let _persistence_guard = crate::config::CONFIG_PERSISTENCE_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + + // Update config with new addresses + let data_dir = &self.current_app_context().data_dir; + if let Ok(mut config) = Config::load_from(data_dir) { + let mut network_cfg = config + .config_for_network(network) + .clone() + .unwrap_or_default(); + network_cfg.dapi_addresses = Some(addresses_csv); + config.update_config_for_network(network, network_cfg.clone()); + + if let Err(e) = config.save(data_dir) { + tracing::error!("Failed to save config after DAPI discovery: {e}"); + } - // Update in-memory config and schedule async SDK reinit - if let Some(app_context) = self.context_for_network(network) { - if let Ok(mut cfg_lock) = app_context.config.write() { - *cfg_lock = network_cfg; + // Update in-memory config and schedule async SDK reinit + if let Some(app_context) = self.context_for_network(network) { + if let Ok(mut cfg_lock) = app_context.config.write() { + *cfg_lock = network_cfg; + } + self.pending_reinit_after_discovery = true; } - self.pending_reinit_after_discovery = true; + + true + } else { + false } + }; + if config_loaded { MessageBanner::set_global( self.current_app_context().egui_ctx(), format!("Updated to {count} node addresses."), From ca0d15b2b5607a4c3752d5fbdb17fd13446da7d5 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Mon, 20 Jul 2026 14:57:44 +0000 Subject: [PATCH 5/6] fix(migration): preserve unmodeled .env keys, report save failures honestly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the still-open review threads on PR #908: - Config::save previously rewrote .env from only its four modeled network blocks, silently destroying anything else in the file (MCP_API_KEY, MCP_LISTEN, RUST_LOG, operator comments, ...) on every save — including the unattended automatic DAPI refresh on first launch. save() now reads the existing file, preserves every line it doesn't own, and only rewrites modeled keys in place; load() now fails closed (typed error) on a malformed modeled value instead of silently dropping it via .ok(). - Extracted the lock->load->mutate->save->live-update sequence, previously hand-duplicated across the migration path, the manual UI refresh, and the concurrency test, into one shared persist_dapi_addresses() helper (backend_task::dapi_discovery). Both writers and the test now go through it, so a future change to the lock discipline can't silently drift out of sync between copies. - network_chooser_screen's display_task_result no longer shows a success banner when the save actually failed — it now surfaces a proper MessageBanner error with details instead of only tracing::error!-logging and falling through to the happy path. - Documented the migration_run mutex policy as intentional: holding it across the detached DAPI refresh (not just core migration steps) is by design, per maintainer direction. Fixed the spawn_dapi_refresh docstring that previously claimed discovery "cannot delay" identity operations — it does, on purpose, and WalletStorageNotReady's doc comment now says so. Co-Authored-By: Codex Sol Co-Authored-By: Claude Sonnet 5 --- src/backend_task/dapi_discovery.rs | 43 ++ src/backend_task/error.rs | 12 +- src/backend_task/migration/finish_unwire.rs | 209 +++++---- src/config.rs | 475 +++++++++++++++----- src/context/mod.rs | 7 +- src/ui/network_chooser_screen.rs | 62 +-- 6 files changed, 564 insertions(+), 244 deletions(-) diff --git a/src/backend_task/dapi_discovery.rs b/src/backend_task/dapi_discovery.rs index 7a0b0c81a..07e5b7441 100644 --- a/src/backend_task/dapi_discovery.rs +++ b/src/backend_task/dapi_discovery.rs @@ -27,6 +27,10 @@ use std::num::NonZeroUsize; use std::str::FromStr; use std::time::Duration; +use crate::backend_task::error::TaskError; +use crate::config::{CONFIG_PERSISTENCE_LOCK, Config}; +use crate::context::AppContext; + /// Errors from DAPI address resolution and discovery. #[derive(Debug, thiserror::Error)] pub enum DapiDiscoveryError { @@ -133,3 +137,42 @@ pub async fn discover_and_format( let csv = urls.join(","); Ok((count, csv)) } + +/// Persist DAPI addresses and publish them to the active network configuration. +pub(crate) fn persist_dapi_addresses( + app_context: &AppContext, + addresses_csv: String, +) -> Result<(), TaskError> { + persist_dapi_addresses_inner(app_context, addresses_csv, || {}) +} + +fn persist_dapi_addresses_inner( + app_context: &AppContext, + addresses_csv: String, + before_save: impl FnOnce(), +) -> Result<(), TaskError> { + let _persistence_guard = CONFIG_PERSISTENCE_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let network = app_context.network(); + let mut config = Config::load_from(app_context.data_dir())?; + let mut network_config = config + .config_for_network(network) + .clone() + .unwrap_or_default(); + network_config.dapi_addresses = Some(addresses_csv); + config.update_config_for_network(network, network_config.clone()); + before_save(); + config.save(app_context.data_dir())?; + *app_context.config.write()? = network_config; + Ok(()) +} + +#[cfg(test)] +pub(crate) fn persist_dapi_addresses_with_hook( + app_context: &AppContext, + addresses_csv: String, + before_save: impl FnOnce(), +) -> Result<(), TaskError> { + persist_dapi_addresses_inner(app_context, addresses_csv, before_save) +} diff --git a/src/backend_task/error.rs b/src/backend_task/error.rs index 50c343d57..99416d1f4 100644 --- a/src/backend_task/error.rs +++ b/src/backend_task/error.rs @@ -2125,13 +2125,17 @@ pub enum TaskError { #[error("Could not connect to {network}. Check your network configuration and retry.")] NetworkContextCreationFailed { network: Network }, + /// A DAPI refresh completed after its network context was removed. + #[error( + "The node addresses could not be applied because the selected network changed. Select the network and retry." + )] + DapiConfigContextUnavailable { network: Network }, + // ────────────────────────────────────────────────────────────────────────── // Migration errors // ────────────────────────────────────────────────────────────────────────── - /// Surfaced when wallet/identity/DashPay storage is being upgraded - /// from the legacy `data.db` and a task tried to touch it before - /// the migration finished. The user can retry once the migration - /// banner clears. + /// Surfaced while the legacy-data upgrade or its best-effort DAPI refresh + /// still owns the migration guard. The user can retry after a short wait. #[error("The storage update is still running. Please wait a moment and try again.")] WalletStorageNotReady, diff --git a/src/backend_task/migration/finish_unwire.rs b/src/backend_task/migration/finish_unwire.rs index 7f7a19d9e..e585eb5ae 100644 --- a/src/backend_task/migration/finish_unwire.rs +++ b/src/backend_task/migration/finish_unwire.rs @@ -16,8 +16,8 @@ use dash_sdk::platform::Identifier; use rusqlite::Connection; use serde::{Deserialize, Serialize}; +use crate::backend_task::dapi_discovery::persist_dapi_addresses; use crate::backend_task::error::TaskError; -use crate::config::{CONFIG_PERSISTENCE_LOCK, Config}; use crate::context::AppContext; use crate::context::migration_status::{MigrationState, MigrationStep}; use crate::model::qualified_identity::QualifiedIdentity; @@ -478,50 +478,14 @@ async fn refresh_dapi_nodes_once_with_legacy_check( } }; - { - let _persistence_guard = CONFIG_PERSISTENCE_LOCK - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); - let mut config = match Config::load_from(&app_context.data_dir) { - Ok(config) => config, - Err(error) => { - tracing::warn!( - target = "migration::finish_unwire", - ?network, - ?error, - "Could not load the network configuration after automatic DAPI discovery; the refresh will retry on the next launch", - ); - return; - } - }; - let mut network_config = config - .config_for_network(network) - .clone() - .unwrap_or_default(); - network_config.dapi_addresses = Some(addresses_csv); - config.update_config_for_network(network, network_config.clone()); - if let Err(error) = config.save(&app_context.data_dir) { - tracing::warn!( - target = "migration::finish_unwire", - ?network, - ?error, - "Could not save automatically discovered DAPI nodes; the refresh will retry on the next launch", - ); - return; - } - - match app_context.config.write() { - Ok(mut live_config) => *live_config = network_config, - Err(error) => { - tracing::warn!( - target = "migration::finish_unwire", - ?network, - ?error, - "Could not update the live network configuration with discovered DAPI nodes; the refresh will retry on the next launch", - ); - return; - } - } + if let Err(error) = persist_dapi_addresses(app_context, addresses_csv) { + tracing::warn!( + target = "migration::finish_unwire", + ?network, + ?error, + "Could not persist automatically discovered DAPI nodes; the refresh will retry on the next launch", + ); + return; } if let Err(error) = Arc::clone(app_context).reinit_core_client_and_sdk() { @@ -567,10 +531,10 @@ fn write_dapi_refresh_completion( /// decide whether to surface a "storage update complete" banner — a no-op /// launch must not show one. /// -/// A best-effort DAPI refresh starts concurrently under its own sentinel. It -/// never changes migration state or propagates an error, so node discovery -/// cannot affect the precedence below or delay access to recovered funds and -/// identities. +/// A best-effort DAPI refresh is queued before the migration passes. After the +/// run publishes terminal status and releases its guard, refresh acquires and +/// holds that guard until it finishes. Operations that claim the same guard, +/// including local identity deletion, remain unavailable during that interval. /// /// Three independent recovery passes, each under its own sentinel, in this order: /// @@ -668,8 +632,8 @@ pub async fn run(app_context: &Arc) -> Result { } } -/// Owns `migration_run` for the full detached refresh after the launching pass releases it. -/// The launching pass never awaits this handle, so waiting for its guard cannot deadlock. +/// Waits for the launching pass, then holds `migration_run` through refresh. +/// Operations that claim this guard stay gated until the detached work completes. fn spawn_dapi_refresh(app_context: &Arc, refresh: F) -> tokio::task::JoinHandle<()> where F: Future + Send + 'static, @@ -681,6 +645,7 @@ where }) } +/// Queues refresh before migration so its waiter acquires the guard at completion. async fn run_under_guard(app_context: &Arc) -> Result { let ctx = Arc::clone(app_context); std::mem::drop(spawn_dapi_refresh(app_context, async move { @@ -2646,13 +2611,12 @@ impl From for TaskError { #[cfg(test)] mod tests { use super::*; - use crate::config::NetworkConfig; + use crate::backend_task::dapi_discovery::persist_dapi_addresses_with_hook; + use crate::config::{CONFIG_ENV_LOCK, Config, NetworkConfig}; use crate::wallet_backend::DetKv; use crate::wallet_backend::kv_test_support::InMemoryKv; use std::sync::atomic::{AtomicUsize, Ordering}; - static CONFIG_ENV_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); - fn kv() -> DetKv { DetKv::from_store(Arc::new(InMemoryKv::default())) } @@ -2793,30 +2757,32 @@ mod tests { } .save(tmp.path()) .expect("write initial config"); + let mainnet_context = app_context_for_network(tmp.path(), Network::Mainnet); + let testnet_context = app_context_for_network_with_shared_storage( + tmp.path(), + Network::Testnet, + &mainnet_context, + ); let (event_tx, event_rx) = std::sync::mpsc::channel(); let (release_first_tx, release_first_rx) = std::sync::mpsc::channel(); - let first_data_dir = tmp.path().to_path_buf(); + let first_context = Arc::clone(&mainnet_context); let first_event_tx = event_tx.clone(); let first = std::thread::spawn(move || { - let _persistence_guard = CONFIG_PERSISTENCE_LOCK - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); - first_event_tx - .send(ConfigPersistenceEvent::FirstEntered) - .expect("report first entry"); - let mut config = Config::load_from(&first_data_dir).expect("first load"); - let mut network_config = config - .config_for_network(Network::Mainnet) - .clone() - .expect("mainnet config"); - network_config.dapi_addresses = Some("https://mainnet-new.example:443".to_string()); - config.update_config_for_network(Network::Mainnet, network_config); - first_event_tx - .send(ConfigPersistenceEvent::FirstPausedBeforeSave) - .expect("report first pause"); - release_first_rx.recv().expect("release first save"); - config.save(&first_data_dir).expect("first save"); + persist_dapi_addresses_with_hook( + &first_context, + "https://mainnet-new.example:443".to_string(), + || { + first_event_tx + .send(ConfigPersistenceEvent::FirstEntered) + .expect("report first entry"); + first_event_tx + .send(ConfigPersistenceEvent::FirstPausedBeforeSave) + .expect("report first pause"); + release_first_rx.recv().expect("release first save"); + }, + ) + .expect("first persistence"); first_event_tx .send(ConfigPersistenceEvent::FirstSaved) .expect("report first save"); @@ -2831,26 +2797,22 @@ mod tests { ConfigPersistenceEvent::FirstPausedBeforeSave, ); - let second_data_dir = tmp.path().to_path_buf(); + let second_context = Arc::clone(&testnet_context); let second_event_tx = event_tx.clone(); let second = std::thread::spawn(move || { second_event_tx .send(ConfigPersistenceEvent::SecondAttempting) .expect("report second attempt"); - let _persistence_guard = CONFIG_PERSISTENCE_LOCK - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); - second_event_tx - .send(ConfigPersistenceEvent::SecondEntered) - .expect("report second entry"); - let mut config = Config::load_from(&second_data_dir).expect("second load"); - let mut network_config = config - .config_for_network(Network::Testnet) - .clone() - .expect("testnet config"); - network_config.dapi_addresses = Some("https://testnet-new.example:443".to_string()); - config.update_config_for_network(Network::Testnet, network_config); - config.save(&second_data_dir).expect("second save"); + persist_dapi_addresses_with_hook( + &second_context, + "https://testnet-new.example:443".to_string(), + || { + second_event_tx + .send(ConfigPersistenceEvent::SecondEntered) + .expect("report second entry"); + }, + ) + .expect("second persistence"); second_event_tx .send(ConfigPersistenceEvent::SecondSaved) .expect("report second save"); @@ -2908,6 +2870,58 @@ mod tests { .and_then(|config| config.dapi_addresses.as_deref()), Some("https://testnet-new.example:443"), ); + assert_eq!( + mainnet_context + .config + .read() + .expect("read mainnet live config") + .dapi_addresses + .as_deref(), + Some("https://mainnet-new.example:443"), + ); + assert_eq!( + testnet_context + .config + .read() + .expect("read testnet live config") + .dapi_addresses + .as_deref(), + Some("https://testnet-new.example:443"), + ); + } + + #[tokio::test] + async fn dapi_config_persistence_reports_save_failure() { + let _env_guard = CONFIG_ENV_LOCK.lock().await; + let tmp = tempfile::tempdir().expect("tempdir"); + Config { + mainnet_config: Some(NetworkConfig { + dapi_addresses: Some("https://mainnet-old.example:443".to_string()), + ..Default::default() + }), + testnet_config: None, + devnet_config: None, + local_config: None, + } + .save(tmp.path()) + .expect("write initial config"); + let app_context = app_context_for_network(tmp.path(), Network::Mainnet); + let env_path = tmp.path().join(".env"); + + let error = persist_dapi_addresses_with_hook( + &app_context, + "https://mainnet-new.example:443".to_string(), + || { + std::fs::remove_file(&env_path).expect("remove config file"); + std::fs::create_dir(&env_path).expect("replace config file with directory"); + }, + ) + .expect_err("persistence must report the save failure"); + + assert!(matches!( + error, + TaskError::Config(crate::config::ConfigError::SaveError { .. }) + )); } #[tokio::test] @@ -5048,6 +5062,25 @@ mod tests { .expect("AppContext") } + fn app_context_for_network_with_shared_storage( + dir: &std::path::Path, + network: Network, + shared: &AppContext, + ) -> Arc { + AppContext::new( + dir.to_path_buf(), + network, + Arc::clone(&shared.db), + Default::default(), + Default::default(), + egui::Context::default(), + shared.app_kv(), + shared.secret_store(), + crate::model::user_role::UserRoleCell::default(), + ) + .expect("AppContext") + } + #[test] fn identity_deletion_is_rejected_while_migration_is_running() { let tmp = tempfile::tempdir().expect("tempdir"); diff --git a/src/config.rs b/src/config.rs index 543f144dd..3a4642a36 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,3 +1,5 @@ +use std::collections::HashSet; +use std::fs; use std::io::{self, Write}; use std::path::Path; use std::str::FromStr; @@ -9,6 +11,20 @@ use serde::Deserialize; use tempfile::NamedTempFile; pub(crate) static CONFIG_PERSISTENCE_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); +#[cfg(test)] +pub(crate) static CONFIG_ENV_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); + +const NETWORK_PREFIXES: [&str; 4] = ["MAINNET_", "TESTNET_", "DEVNET_", "LOCAL_"]; +const NETWORK_CONFIG_FIELDS: [&str; 8] = [ + "dapi_addresses", + "core_host", + "core_rpc_port", + "core_rpc_user", + "core_rpc_password", + "core_zmq_endpoint", + "devnet_name", + "wallet_private_key", +]; #[derive(Debug, Deserialize, Clone)] pub struct Config { @@ -21,8 +37,28 @@ pub struct Config { #[derive(Debug, thiserror::Error)] pub enum ConfigError { /// Failed to load configuration from disk or environment. - #[error("{0}")] - LoadError(String), + #[error("Could not load settings. Check that the application folder is readable and retry.")] + LoadError { + #[source] + source: std::io::Error, + }, + + /// The existing `.env` file could not be parsed safely. + #[error("The saved settings contain an invalid entry. Correct the settings file and retry.")] + InvalidEnvFile { + #[source] + source: dotenvy::Error, + }, + + /// A network-specific value could not be parsed into its modeled type. + #[error( + "The saved {network} settings contain an invalid value. Correct the settings file and retry." + )] + InvalidNetworkConfig { + network: &'static str, + #[source] + source: envy::Error, + }, /// Failed to save configuration to disk. #[error("Could not save settings. Check that the application folder is writable and retry.")] @@ -103,11 +139,17 @@ impl Config { /// Write the current configuration back to the `.env` file so that /// subsequent calls to `Config::load()` will reflect changes. /// - /// Uses atomic write (write to temp file, then rename) to prevent - /// config corruption if a write fails partway through. + /// Preserves lines not modeled by `Config` and replaces the modeled fields. + /// Uses a temporary file and atomic rename to prevent partial writes. pub fn save(&self, data_dir: &Path) -> Result<(), ConfigError> { let env_file_path = data_file_path(data_dir, ".env").map_err(|e| ConfigError::SaveError { source: e })?; + let existing_contents = match fs::read_to_string(&env_file_path) { + Ok(contents) => contents, + Err(error) if error.kind() == io::ErrorKind::NotFound => String::new(), + Err(source) => return Err(ConfigError::SaveError { source }), + }; + validate_env_contents(&existing_contents)?; // Write to a temporary file in the same directory first, then // atomically replace. This prevents corruption if the write fails @@ -124,87 +166,28 @@ impl Config { })?; let mut env_file = NamedTempFile::new_in(parent_dir).map_err(|e| ConfigError::SaveError { source: e })?; - - // Helper function to write a single network config to the `.env` file - let mut write_network_config = |prefix: &str, config: &NetworkConfig| { - // Each line becomes e.g. MAINNET_dapi_addresses=... - // For "local" (regtest), you'll see LOCAL_dapi_addresses=... - // - // Use the environment variable scheme you prefer. Make sure it - // matches what `load()` expects (i.e. `envy::prefixed("MAINNET_")`, - // etc.). - - if let Some(ref addrs) = config.dapi_addresses - && !addrs.is_empty() - { - writeln!(env_file, "{}dapi_addresses={}", prefix, addrs) - .map_err(|e| ConfigError::SaveError { source: e })?; - } - if let Some(ref host) = config.core_host { - writeln!(env_file, "{}core_host={}", prefix, host) - .map_err(|e| ConfigError::SaveError { source: e })?; - } - if let Some(port) = config.core_rpc_port { - writeln!(env_file, "{}core_rpc_port={}", prefix, port) - .map_err(|e| ConfigError::SaveError { source: e })?; - } - if let Some(ref user) = config.core_rpc_user { - writeln!(env_file, "{}core_rpc_user={}", prefix, user) - .map_err(|e| ConfigError::SaveError { source: e })?; - } - if let Some(ref password) = config.core_rpc_password { - writeln!(env_file, "{}core_rpc_password={}", prefix, password) - .map_err(|e| ConfigError::SaveError { source: e })?; - } - if let Some(core_zmq_endpoint) = &config.core_zmq_endpoint { - writeln!( - env_file, - "{}core_zmq_endpoint={}", - prefix, core_zmq_endpoint - ) - .map_err(|e| ConfigError::SaveError { source: e })?; + let modeled_entries = self.modeled_entries(); + let mut written_keys = HashSet::new(); + for line in existing_contents.lines() { + if let Some(key) = owned_config_key(line) { + if let Some((_, value)) = modeled_entries + .iter() + .find(|(candidate, _)| candidate == &key) + && written_keys.insert(key.clone()) + { + let assignment = line.split_once('=').map_or(key.as_str(), |(left, _)| left); + writeln!(env_file, "{assignment}={value}") + .map_err(|source| ConfigError::SaveError { source })?; + } + } else { + writeln!(env_file, "{line}").map_err(|source| ConfigError::SaveError { source })?; } - - if let Some(devnet_name) = &config.devnet_name { - // Only write devnet name if it exists - writeln!(env_file, "{}devnet_name={}", prefix, devnet_name) - .map_err(|e| ConfigError::SaveError { source: e })?; - } - if let Some(wallet_private_key) = &config.wallet_private_key { - writeln!( - env_file, - "{}wallet_private_key={}", - prefix, wallet_private_key - ) - .map_err(|e| ConfigError::SaveError { source: e })?; - } - - // Add a blank line after each config block - writeln!(env_file).map_err(|e| ConfigError::SaveError { source: e })?; - - Ok(()) - }; - - // Mainnet - if let Some(ref mainnet_config) = self.mainnet_config { - // `envy::prefixed("MAINNET_")` expects these lines to start with "MAINNET_" - write_network_config("MAINNET_", mainnet_config)?; - } - - // Testnet - if let Some(ref testnet_config) = self.testnet_config { - write_network_config("TESTNET_", testnet_config)?; } - - // Devnet - if let Some(ref devnet_config) = self.devnet_config { - write_network_config("DEVNET_", devnet_config)?; - } - - // Local (Regtest) - if let Some(ref local_config) = self.local_config { - // `envy::prefixed("LOCAL_")` expects "LOCAL_..." - write_network_config("LOCAL_", local_config)?; + for (key, value) in modeled_entries { + if written_keys.insert(key.clone()) { + writeln!(env_file, "{key}={value}") + .map_err(|source| ConfigError::SaveError { source })?; + } } // Sync all data to disk before renaming to ensure crash-safety @@ -245,38 +228,43 @@ impl Config { /// located in the given data directory. pub fn load_from(data_dir: &Path) -> Result { let env_file_path = - data_file_path(data_dir, ".env").map_err(|e| ConfigError::LoadError(e.to_string()))?; + data_file_path(data_dir, ".env").map_err(|source| ConfigError::LoadError { source })?; Self::load_from_env_path(env_file_path) } fn load_from_env_path(env_file_path: std::path::PathBuf) -> Result { - if let Err(err) = dotenvy::from_path_override(env_file_path) { - tracing::warn!( - ?err, - "Failed to load .env file. Continuing with environment variables." - ); - } else { - tracing::info!("Successfully loaded .env file"); + let file_entries = match fs::read_to_string(&env_file_path) { + Ok(contents) => dotenvy::Iter::new( + contents + .strip_prefix('\u{feff}') + .unwrap_or(&contents) + .as_bytes(), + ) + .collect::, _>>() + .map_err(|source| ConfigError::InvalidEnvFile { source })?, + Err(error) if error.kind() == io::ErrorKind::NotFound => Vec::new(), + Err(source) => return Err(ConfigError::LoadError { source }), + }; + match dotenvy::from_path_override(&env_file_path) { + Ok(()) => tracing::info!("Successfully loaded .env file"), + Err(error) if error.not_found() => { + tracing::warn!( + ?error, + "No .env file was found. Continuing with environment variables." + ); + } + Err(source) => return Err(ConfigError::InvalidEnvFile { source }), } - // Load each network config. Missing configs are normal — not every - // user configures all networks. Only fail if nothing is configured at all. - let mainnet_config = envy::prefixed("MAINNET_") - .from_env::() - .inspect_err(|e| tracing::debug!("Failed to parse mainnet config: {e}")) - .ok(); - let testnet_config = envy::prefixed("TESTNET_") - .from_env::() - .inspect_err(|e| tracing::debug!("Failed to parse testnet config: {e}")) - .ok(); - let devnet_config = envy::prefixed("DEVNET_") - .from_env::() - .inspect_err(|e| tracing::debug!("Failed to parse devnet config: {e}")) - .ok(); - let local_config = envy::prefixed("LOCAL_") - .from_env::() - .inspect_err(|e| tracing::debug!("Failed to parse local config: {e}")) - .ok(); + let process_entries = std::env::vars().collect::>(); + let mainnet_config = + load_network_config("MAINNET_", "Mainnet", &process_entries, &file_entries)?; + let testnet_config = + load_network_config("TESTNET_", "Testnet", &process_entries, &file_entries)?; + let devnet_config = + load_network_config("DEVNET_", "Devnet", &process_entries, &file_entries)?; + let local_config = + load_network_config("LOCAL_", "local network", &process_entries, &file_entries)?; if mainnet_config.is_none() && testnet_config.is_none() @@ -303,6 +291,134 @@ impl Config { Network::Regtest => self.local_config = Some(new_config), } } + + fn modeled_entries(&self) -> Vec<(String, String)> { + let mut entries = Vec::new(); + for (prefix, config) in [ + ("MAINNET_", self.mainnet_config.as_ref()), + ("TESTNET_", self.testnet_config.as_ref()), + ("DEVNET_", self.devnet_config.as_ref()), + ("LOCAL_", self.local_config.as_ref()), + ] { + if let Some(config) = config { + append_network_entries(&mut entries, prefix, config); + } + } + entries + } +} + +fn load_network_config( + prefix: &'static str, + network: &'static str, + process_entries: &[(String, String)], + file_entries: &[(String, String)], +) -> Result, ConfigError> { + let mut entries = std::collections::HashMap::new(); + for (key, value) in process_entries.iter().chain(file_entries) { + if let Some(field) = key.strip_prefix(prefix) { + entries.insert( + format!("{prefix}{}", field.to_ascii_lowercase()), + value.clone(), + ); + } + } + if entries.is_empty() { + return Ok(None); + } + envy::prefixed(prefix) + .from_iter::<_, NetworkConfig>(entries) + .map(Some) + .map_err(|source| ConfigError::InvalidNetworkConfig { network, source }) +} + +fn validate_env_contents(contents: &str) -> Result<(), ConfigError> { + let contents = contents.strip_prefix('\u{feff}').unwrap_or(contents); + let entries = dotenvy::Iter::new(contents.as_bytes()) + .collect::, _>>() + .map_err(|source| ConfigError::InvalidEnvFile { source })?; + for (prefix, network) in [ + ("MAINNET_", "Mainnet"), + ("TESTNET_", "Testnet"), + ("DEVNET_", "Devnet"), + ("LOCAL_", "local network"), + ] { + if entries.iter().any(|(key, _)| key.starts_with(prefix)) { + envy::prefixed(prefix) + .from_iter::<_, NetworkConfig>(entries.iter().cloned()) + .map_err(|source| ConfigError::InvalidNetworkConfig { network, source })?; + } + } + Ok(()) +} + +fn append_network_entries( + entries: &mut Vec<(String, String)>, + prefix: &str, + config: &NetworkConfig, +) { + let mut push = |field: &str, value: String| { + entries.push((format!("{prefix}{field}"), value)); + }; + if let Some(addresses) = &config.dapi_addresses + && !addresses.is_empty() + { + push("dapi_addresses", addresses.clone()); + } + if let Some(host) = &config.core_host { + push("core_host", host.clone()); + } + if let Some(port) = config.core_rpc_port { + push("core_rpc_port", port.to_string()); + } + if let Some(user) = &config.core_rpc_user { + push("core_rpc_user", user.clone()); + } + if let Some(password) = &config.core_rpc_password { + push("core_rpc_password", password.clone()); + } + if let Some(endpoint) = &config.core_zmq_endpoint { + push("core_zmq_endpoint", endpoint.clone()); + } + if let Some(name) = &config.devnet_name { + push("devnet_name", name.clone()); + } + if let Some(key) = &config.wallet_private_key { + push("wallet_private_key", key.clone()); + } +} + +fn owned_config_key(line: &str) -> Option { + let line = line.strip_prefix('\u{feff}').unwrap_or(line).trim_start(); + let (mut key, mut rest) = take_env_key(line)?; + rest = rest.trim_start(); + if key == "export" && !rest.starts_with('=') { + (key, rest) = take_env_key(rest)?; + rest = rest.trim_start(); + } + if !rest.starts_with('=') { + return None; + } + NETWORK_PREFIXES.iter().find_map(|prefix| { + let field = key.strip_prefix(prefix)?; + NETWORK_CONFIG_FIELDS + .iter() + .find(|owned| field.eq_ignore_ascii_case(owned)) + .map(|owned| format!("{prefix}{owned}")) + }) +} + +fn take_env_key(input: &str) -> Option<(&str, &str)> { + let first = *input.as_bytes().first()?; + if !first.is_ascii_alphabetic() && first != b'_' { + return None; + } + let end = input + .as_bytes() + .iter() + .position(|byte| !byte.is_ascii_alphanumeric() && *byte != b'_' && *byte != b'.') + .unwrap_or(input.len()); + Some((&input[..end], &input[end..])) } impl NetworkConfig { @@ -333,6 +449,21 @@ impl NetworkConfig { mod tests { use super::*; + fn run_config_test_in_subprocess(test_name: &str) -> bool { + const CHILD_MARKER: &str = "DET_CONFIG_TEST_CHILD"; + if std::env::var_os(CHILD_MARKER).is_some() { + return false; + } + let status = std::process::Command::new(std::env::current_exe().expect("test executable")) + .arg(test_name) + .arg("--exact") + .env(CHILD_MARKER, "1") + .status() + .expect("run isolated config test"); + assert!(status.success(), "isolated config test failed"); + true + } + /// Helper to create a minimal valid NetworkConfig for testing fn make_network_config(dapi_addresses: &str, port: u16) -> NetworkConfig { let dapi = if dapi_addresses.is_empty() { @@ -559,6 +690,132 @@ mod tests { assert!(output.contains("MAINNET_core_zmq_endpoint=tcp://127.0.0.1:23708")); } + #[test] + fn save_preserves_unmodeled_env_keys() { + if run_config_test_in_subprocess("config::tests::save_preserves_unmodeled_env_keys") { + return; + } + let tmp = tempfile::tempdir().expect("tempdir"); + let env_path = tmp.path().join(".env"); + std::fs::write( + &env_path, + "# Operator settings\nMCP_API_KEY=some-fake-test-value\nMAINNET_dapi_addresses=https://old.example:443\nRUST_LOG=info\n", + ) + .expect("write test config"); + + let mut config = Config::load_from(tmp.path()).expect("load test config"); + config + .mainnet_config + .as_mut() + .expect("mainnet config") + .dapi_addresses = Some("https://new.example:443".to_string()); + config.save(tmp.path()).expect("save test config"); + + let saved = std::fs::read_to_string(env_path).expect("read saved config"); + assert!(saved.contains("# Operator settings\n")); + assert!(saved.contains("MCP_API_KEY=some-fake-test-value\n")); + assert!(saved.contains("RUST_LOG=info\n")); + assert!(saved.contains("MAINNET_dapi_addresses=https://new.example:443\n")); + assert!(!saved.contains("MAINNET_dapi_addresses=https://old.example:443\n")); + } + + #[test] + fn save_replaces_modeled_keys_case_insensitively() { + if run_config_test_in_subprocess( + "config::tests::save_replaces_modeled_keys_case_insensitively", + ) { + return; + } + let tmp = tempfile::tempdir().expect("tempdir"); + let env_path = tmp.path().join(".env"); + std::fs::write( + &env_path, + "\u{feff}export\tMAINNET_DAPI_ADDRESSES=https://old.example:443\nmainnet_dapi_addresses=keep-this-line\n", + ) + .expect("write test config"); + + let mut config = Config::load_from(tmp.path()).expect("load test config"); + config + .mainnet_config + .as_mut() + .expect("mainnet config") + .dapi_addresses = Some("https://new.example:443".to_string()); + config.save(tmp.path()).expect("save test config"); + + let saved = std::fs::read_to_string(&env_path).expect("read saved config"); + assert!(!saved.contains("MAINNET_DAPI_ADDRESSES=https://old.example:443\n")); + assert!(saved.contains("mainnet_dapi_addresses=keep-this-line\n")); + let reloaded = Config::load_from(tmp.path()).expect("reload test config"); + assert_eq!( + reloaded + .mainnet_config + .as_ref() + .and_then(|network| network.dapi_addresses.as_deref()), + Some("https://new.example:443"), + ); + } + + #[test] + fn save_preserves_unmodeled_line_order() { + if run_config_test_in_subprocess("config::tests::save_preserves_unmodeled_line_order") { + return; + } + let tmp = tempfile::tempdir().expect("tempdir"); + let env_path = tmp.path().join(".env"); + std::fs::write( + &env_path, + "MAINNET_core_host=127.0.0.1\nEXTRA_HOST=${MAINNET_core_host}\nMAINNET_dapi_addresses=https://old.example:443\n", + ) + .expect("write test config"); + + let mut config = Config::load_from(tmp.path()).expect("load test config"); + config + .mainnet_config + .as_mut() + .expect("mainnet config") + .dapi_addresses = Some("https://new.example:443".to_string()); + config.save(tmp.path()).expect("save test config"); + + let saved = std::fs::read_to_string(env_path).expect("read saved config"); + let host_position = saved.find("MAINNET_core_host=").expect("modeled host"); + let extra_position = saved.find("EXTRA_HOST=").expect("unmodeled reference"); + let dapi_position = saved + .find("MAINNET_dapi_addresses=") + .expect("modeled addresses"); + assert!(host_position < extra_position); + assert!(extra_position < dapi_position); + } + + #[test] + fn save_rejects_invalid_modeled_values_without_rewriting() { + let tmp = tempfile::tempdir().expect("tempdir"); + let env_path = tmp.path().join(".env"); + let original = "MAINNET_dapi_addresses=https://mainnet.example:443\nTESTNET_core_rpc_port=not-a-number\nTESTNET_core_rpc_password=fake-test-password\n"; + std::fs::write(&env_path, original).expect("write test config"); + let config = Config { + mainnet_config: Some(make_network_config("https://mainnet-new.example:443", 9998)), + testnet_config: None, + devnet_config: None, + local_config: None, + }; + + let error = config + .save(tmp.path()) + .expect_err("invalid modeled value must fail closed"); + + assert!(matches!( + error, + ConfigError::InvalidNetworkConfig { + network: "Testnet", + .. + } + )); + assert_eq!( + std::fs::read_to_string(env_path).expect("read unchanged config"), + original, + ); + } + // ── envy parsing roundtrip ────────────────────────────────────── #[test] diff --git a/src/context/mod.rs b/src/context/mod.rs index 70e215fd4..8078bb45b 100644 --- a/src/context/mod.rs +++ b/src/context/mod.rs @@ -149,10 +149,9 @@ pub struct AppContext { /// frame from the UI. Always present and idle on fresh installs; /// driven by [`MigrationTask::FinishUnwire`](crate::backend_task::migration::MigrationTask). pub(crate) migration_status: Arc, - /// Serializes complete storage-update runs. This prevents a GUI dispatch - /// and a shared MCP request from creating two password waiters for the same - /// wallet; a follower waits here and returns the leader's terminal result - /// without rerunning the update. + /// Serializes complete storage-update runs, including the detached automatic + /// DAPI refresh that continues after migration publishes terminal status. + /// This also prevents duplicate password waiters for the same wallet. pub(crate) migration_run: tokio::sync::Mutex<()>, /// Process-local claim shared by every UI surface before a paid DashPay /// request action enters its backend flow. diff --git a/src/ui/network_chooser_screen.rs b/src/ui/network_chooser_screen.rs index 33e6af87f..c2c07cdaf 100644 --- a/src/ui/network_chooser_screen.rs +++ b/src/ui/network_chooser_screen.rs @@ -1,9 +1,9 @@ use crate::app::AppAction; use crate::backend_task::core::CoreTask; +use crate::backend_task::dapi_discovery::persist_dapi_addresses; use crate::backend_task::error::TaskError; use crate::backend_task::system_task::SystemTask; use crate::backend_task::{BackendTask, BackendTaskContext, BackendTaskSuccessResult}; -use crate::config::Config; use crate::context::AppContext; use crate::context::connection_status::OverallConnectionState; use crate::model::spv_status::{SpvStatus, SpvStatusSnapshot}; @@ -1473,45 +1473,29 @@ impl ScreenLike for NetworkChooserScreen { { self.discovery_in_progress = false; - let config_loaded = { - let _persistence_guard = crate::config::CONFIG_PERSISTENCE_LOCK - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); - - // Update config with new addresses - let data_dir = &self.current_app_context().data_dir; - if let Ok(mut config) = Config::load_from(data_dir) { - let mut network_cfg = config - .config_for_network(network) - .clone() - .unwrap_or_default(); - network_cfg.dapi_addresses = Some(addresses_csv); - config.update_config_for_network(network, network_cfg.clone()); - - if let Err(e) = config.save(data_dir) { - tracing::error!("Failed to save config after DAPI discovery: {e}"); - } - - // Update in-memory config and schedule async SDK reinit - if let Some(app_context) = self.context_for_network(network) { - if let Ok(mut cfg_lock) = app_context.config.write() { - *cfg_lock = network_cfg; - } - self.pending_reinit_after_discovery = true; - } - - true - } else { - false + let persistence_result = self + .context_for_network(network) + .cloned() + .ok_or(TaskError::DapiConfigContextUnavailable { network }) + .and_then(|app_context| persist_dapi_addresses(&app_context, addresses_csv)); + + match persistence_result { + Ok(()) => { + self.pending_reinit_after_discovery = true; + MessageBanner::set_global( + self.current_app_context().egui_ctx(), + format!("Updated to {count} node addresses."), + MessageType::Success, + ); + } + Err(error) => { + MessageBanner::set_global( + self.current_app_context().egui_ctx(), + error.to_string(), + MessageType::Error, + ) + .with_details(error); } - }; - - if config_loaded { - MessageBanner::set_global( - self.current_app_context().egui_ctx(), - format!("Updated to {count} node addresses."), - MessageType::Success, - ); } } } From 1acf308592e91d6cd86b5cb4f3e6f0594044a444 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Mon, 20 Jul 2026 21:20:58 +0000 Subject: [PATCH 6/6] fix(config): isolate network persistence failures Keep valid network configs usable when another network is malformed. Persist DAPI endpoints as a targeted locked update and retry refresh after reinitialization fails. Co-Authored-By: Codex GPT-5 --- src/backend_task/dapi_discovery.rs | 18 +- src/backend_task/migration/finish_unwire.rs | 116 +++++++- src/config.rs | 290 +++++++++++++++----- 3 files changed, 329 insertions(+), 95 deletions(-) diff --git a/src/backend_task/dapi_discovery.rs b/src/backend_task/dapi_discovery.rs index 07e5b7441..3100e87a6 100644 --- a/src/backend_task/dapi_discovery.rs +++ b/src/backend_task/dapi_discovery.rs @@ -143,28 +143,23 @@ pub(crate) fn persist_dapi_addresses( app_context: &AppContext, addresses_csv: String, ) -> Result<(), TaskError> { - persist_dapi_addresses_inner(app_context, addresses_csv, || {}) + persist_dapi_addresses_inner(app_context, addresses_csv, || {}, || {}) } fn persist_dapi_addresses_inner( app_context: &AppContext, addresses_csv: String, before_save: impl FnOnce(), + after_save: impl FnOnce(), ) -> Result<(), TaskError> { let _persistence_guard = CONFIG_PERSISTENCE_LOCK .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); let network = app_context.network(); - let mut config = Config::load_from(app_context.data_dir())?; - let mut network_config = config - .config_for_network(network) - .clone() - .unwrap_or_default(); - network_config.dapi_addresses = Some(addresses_csv); - config.update_config_for_network(network, network_config.clone()); before_save(); - config.save(app_context.data_dir())?; - *app_context.config.write()? = network_config; + Config::save_dapi_addresses(app_context.data_dir(), network, &addresses_csv)?; + app_context.config.write()?.dapi_addresses = Some(addresses_csv); + after_save(); Ok(()) } @@ -173,6 +168,7 @@ pub(crate) fn persist_dapi_addresses_with_hook( app_context: &AppContext, addresses_csv: String, before_save: impl FnOnce(), + after_save: impl FnOnce(), ) -> Result<(), TaskError> { - persist_dapi_addresses_inner(app_context, addresses_csv, before_save) + persist_dapi_addresses_inner(app_context, addresses_csv, before_save, after_save) } diff --git a/src/backend_task/migration/finish_unwire.rs b/src/backend_task/migration/finish_unwire.rs index e585eb5ae..1a9ef72a9 100644 --- a/src/backend_task/migration/finish_unwire.rs +++ b/src/backend_task/migration/finish_unwire.rs @@ -495,6 +495,7 @@ async fn refresh_dapi_nodes_once_with_legacy_check( ?error, "Could not reinitialize network clients after automatic DAPI refresh; the saved addresses will be used on the next launch", ); + return; } tracing::info!( @@ -2729,6 +2730,77 @@ mod tests { ); } + #[test] + fn dapi_config_persistence_updates_only_selected_key() { + const CHILD_MARKER: &str = "DET_DAPI_PERSISTENCE_TEST_CHILD"; + if std::env::var_os(CHILD_MARKER).is_none() { + let status = std::process::Command::new( + std::env::current_exe().expect("test executable"), + ) + .arg( + "backend_task::migration::finish_unwire::tests::dapi_config_persistence_updates_only_selected_key", + ) + .arg("--exact") + .env(CHILD_MARKER, "1") + .env_remove("MAINNET_core_rpc_password") + .env_remove("TESTNET_wallet_private_key") + .status() + .expect("run isolated persistence test"); + assert!(status.success(), "isolated persistence test failed"); + return; + } + + let tmp = tempfile::tempdir().expect("tempdir"); + let env_path = tmp.path().join(".env"); + let app_context = app_context_for_network(tmp.path(), Network::Mainnet); + let mainnet_password_before = std::env::var_os("MAINNET_core_rpc_password"); + let testnet_key_before = std::env::var_os("TESTNET_wallet_private_key"); + std::fs::write( + &env_path, + "# Preserve operator formatting\n\ + MAINNET_dapi_addresses=https://mainnet-old.example:443\n\ + MAINNET_core_rpc_password='[REDACTED]' # unchanged\n\ + TESTNET_wallet_private_key='[NOT_A_KEY]' # unchanged\n", + ) + .expect("write initial config"); + + persist_dapi_addresses_with_hook( + &app_context, + "https://mainnet-new.example:443".to_string(), + || { + app_context + .config + .write() + .expect("update live config during persistence") + .core_rpc_password = Some("[LIVE_UPDATE]".to_string()); + }, + || {}, + ) + .expect("persist selected DAPI addresses"); + + let saved = std::fs::read_to_string(env_path).expect("read updated config"); + assert!(saved.contains("MAINNET_dapi_addresses=https://mainnet-new.example:443\n")); + assert!(saved.contains("MAINNET_core_rpc_password='[REDACTED]' # unchanged\n")); + assert!(saved.contains("TESTNET_wallet_private_key='[NOT_A_KEY]' # unchanged\n")); + assert_eq!( + std::env::var_os("MAINNET_core_rpc_password"), + mainnet_password_before, + ); + assert_eq!( + std::env::var_os("TESTNET_wallet_private_key"), + testnet_key_before, + ); + assert_eq!( + app_context + .config + .read() + .expect("read live config after persistence") + .core_rpc_password + .as_deref(), + Some("[LIVE_UPDATE]"), + ); + } + #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum ConfigPersistenceEvent { FirstEntered, @@ -2767,25 +2839,28 @@ mod tests { let (event_tx, event_rx) = std::sync::mpsc::channel(); let (release_first_tx, release_first_rx) = std::sync::mpsc::channel(); let first_context = Arc::clone(&mainnet_context); - let first_event_tx = event_tx.clone(); + let first_before_save_tx = event_tx.clone(); + let first_after_save_tx = event_tx.clone(); let first = std::thread::spawn(move || { persist_dapi_addresses_with_hook( &first_context, "https://mainnet-new.example:443".to_string(), || { - first_event_tx + first_before_save_tx .send(ConfigPersistenceEvent::FirstEntered) .expect("report first entry"); - first_event_tx + first_before_save_tx .send(ConfigPersistenceEvent::FirstPausedBeforeSave) .expect("report first pause"); release_first_rx.recv().expect("release first save"); }, + || { + first_after_save_tx + .send(ConfigPersistenceEvent::FirstSaved) + .expect("report first save"); + }, ) .expect("first persistence"); - first_event_tx - .send(ConfigPersistenceEvent::FirstSaved) - .expect("report first save"); }); assert_eq!( @@ -2798,24 +2873,28 @@ mod tests { ); let second_context = Arc::clone(&testnet_context); - let second_event_tx = event_tx.clone(); + let second_attempt_tx = event_tx.clone(); + let second_before_save_tx = event_tx.clone(); + let second_after_save_tx = event_tx.clone(); let second = std::thread::spawn(move || { - second_event_tx + second_attempt_tx .send(ConfigPersistenceEvent::SecondAttempting) .expect("report second attempt"); persist_dapi_addresses_with_hook( &second_context, "https://testnet-new.example:443".to_string(), || { - second_event_tx + second_before_save_tx .send(ConfigPersistenceEvent::SecondEntered) .expect("report second entry"); }, + || { + second_after_save_tx + .send(ConfigPersistenceEvent::SecondSaved) + .expect("report second save"); + }, ) .expect("second persistence"); - second_event_tx - .send(ConfigPersistenceEvent::SecondSaved) - .expect("report second save"); }); assert_eq!( @@ -2915,6 +2994,7 @@ mod tests { std::fs::remove_file(&env_path).expect("remove config file"); std::fs::create_dir(&env_path).expect("replace config file with directory"); }, + || {}, ) .expect_err("persistence must report the save failure"); @@ -2924,6 +3004,18 @@ mod tests { )); } + #[tokio::test] + async fn dapi_refresh_reinit_failure_withholds_completion_sentinel() { + let _env_guard = CONFIG_ENV_LOCK.lock().await; + let tmp = tempfile::tempdir().expect("tempdir"); + let ctx = fresh_app_context(tmp.path()); + seed_legacy_single_key(&ctx); + + refresh_dapi_nodes_once_with(&ctx, |_| async { Ok((1, String::new())) }).await; + + assert!(!dapi_refresh_is_complete(&ctx)); + } + #[tokio::test] async fn dapi_refresh_legacy_detection_failure_retries_then_completes() { let _env_guard = CONFIG_ENV_LOCK.lock().await; diff --git a/src/config.rs b/src/config.rs index 3a4642a36..1042f4d6f 100644 --- a/src/config.rs +++ b/src/config.rs @@ -144,79 +144,62 @@ impl Config { pub fn save(&self, data_dir: &Path) -> Result<(), ConfigError> { let env_file_path = data_file_path(data_dir, ".env").map_err(|e| ConfigError::SaveError { source: e })?; - let existing_contents = match fs::read_to_string(&env_file_path) { - Ok(contents) => contents, - Err(error) if error.kind() == io::ErrorKind::NotFound => String::new(), - Err(source) => return Err(ConfigError::SaveError { source }), - }; + let existing_contents = read_env_contents(&env_file_path)?; validate_env_contents(&existing_contents)?; - - // Write to a temporary file in the same directory first, then - // atomically replace. This prevents corruption if the write fails - // partway through. NamedTempFile::persist() closes the handle before - // renaming and uses MoveFileEx with MOVEFILE_REPLACE_EXISTING on - // Windows for atomic replacement. - let parent_dir = env_file_path - .parent() - .ok_or_else(|| ConfigError::SaveError { - source: io::Error::new( - io::ErrorKind::NotFound, - "config file path has no parent directory", - ), - })?; - let mut env_file = - NamedTempFile::new_in(parent_dir).map_err(|e| ConfigError::SaveError { source: e })?; let modeled_entries = self.modeled_entries(); - let mut written_keys = HashSet::new(); - for line in existing_contents.lines() { - if let Some(key) = owned_config_key(line) { - if let Some((_, value)) = modeled_entries - .iter() - .find(|(candidate, _)| candidate == &key) - && written_keys.insert(key.clone()) - { - let assignment = line.split_once('=').map_or(key.as_str(), |(left, _)| left); - writeln!(env_file, "{assignment}={value}") + atomic_replace_env_file(&env_file_path, |env_file| { + let mut written_keys = HashSet::new(); + for line in existing_contents.lines() { + if let Some(key) = owned_config_key(line) { + if let Some((_, value)) = modeled_entries + .iter() + .find(|(candidate, _)| candidate == &key) + && written_keys.insert(key.clone()) + { + let assignment = + line.split_once('=').map_or(key.as_str(), |(left, _)| left); + writeln!(env_file, "{assignment}={value}") + .map_err(|source| ConfigError::SaveError { source })?; + } + } else { + writeln!(env_file, "{line}") .map_err(|source| ConfigError::SaveError { source })?; } - } else { - writeln!(env_file, "{line}").map_err(|source| ConfigError::SaveError { source })?; - } - } - for (key, value) in modeled_entries { - if written_keys.insert(key.clone()) { - writeln!(env_file, "{key}={value}") - .map_err(|source| ConfigError::SaveError { source })?; } - } - - // Sync all data to disk before renaming to ensure crash-safety - env_file - .as_file() - .sync_all() - .map_err(|e| ConfigError::SaveError { source: e })?; - - // Atomically replace the old config with the new one. - // persist() closes the file handle and uses platform-safe rename - // (MoveFileEx with MOVEFILE_REPLACE_EXISTING on Windows). - env_file - .persist(&env_file_path) - .map_err(|e| ConfigError::SaveError { source: e.error })?; - - // Restrict file permissions on Unix (config contains RPC credentials). - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let perms = std::fs::Permissions::from_mode(0o600); - if let Err(e) = std::fs::set_permissions(&env_file_path, perms) { - tracing::warn!("Could not set config file permissions to 0600: {e}"); + for (key, value) in modeled_entries { + if written_keys.insert(key.clone()) { + writeln!(env_file, "{key}={value}") + .map_err(|source| ConfigError::SaveError { source })?; + } } - } + Ok(()) + })?; tracing::info!("Successfully saved configuration to {:?}", env_file_path); Ok(()) } + /// Persist one network's DAPI addresses without parsing or reserializing other values. + pub(crate) fn save_dapi_addresses( + data_dir: &Path, + network: Network, + addresses: &str, + ) -> Result<(), ConfigError> { + let env_file_path = + data_file_path(data_dir, ".env").map_err(|source| ConfigError::SaveError { source })?; + let existing_contents = read_env_contents(&env_file_path)?; + let key = format!("{}dapi_addresses", dotenv_network_prefix(network)); + atomic_replace_env_file(&env_file_path, |env_file| { + write_single_env_update(env_file, &existing_contents, &key, addresses) + })?; + tracing::info!( + ?network, + path = ?env_file_path, + "Saved DAPI addresses for one network" + ); + Ok(()) + } + /// Loads the configuration for all networks from environment variables and `.env` file /// located in the default app data directory. pub fn load() -> Result { @@ -258,13 +241,13 @@ impl Config { let process_entries = std::env::vars().collect::>(); let mainnet_config = - load_network_config("MAINNET_", "Mainnet", &process_entries, &file_entries)?; + load_network_config_or_none("MAINNET_", "Mainnet", &process_entries, &file_entries); let testnet_config = - load_network_config("TESTNET_", "Testnet", &process_entries, &file_entries)?; + load_network_config_or_none("TESTNET_", "Testnet", &process_entries, &file_entries); let devnet_config = - load_network_config("DEVNET_", "Devnet", &process_entries, &file_entries)?; + load_network_config_or_none("DEVNET_", "Devnet", &process_entries, &file_entries); let local_config = - load_network_config("LOCAL_", "local network", &process_entries, &file_entries)?; + load_network_config_or_none("LOCAL_", "local network", &process_entries, &file_entries); if mainnet_config.is_none() && testnet_config.is_none() @@ -308,6 +291,118 @@ impl Config { } } +fn read_env_contents(env_file_path: &Path) -> Result { + match fs::read_to_string(env_file_path) { + Ok(contents) => Ok(contents), + Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(String::new()), + Err(source) => Err(ConfigError::SaveError { source }), + } +} + +fn atomic_replace_env_file( + env_file_path: &Path, + write_contents: impl FnOnce(&mut NamedTempFile) -> Result<(), ConfigError>, +) -> Result<(), ConfigError> { + let parent_dir = env_file_path + .parent() + .ok_or_else(|| ConfigError::SaveError { + source: io::Error::new( + io::ErrorKind::NotFound, + "config file path has no parent directory", + ), + })?; + let mut env_file = + NamedTempFile::new_in(parent_dir).map_err(|source| ConfigError::SaveError { source })?; + write_contents(&mut env_file)?; + env_file + .as_file() + .sync_all() + .map_err(|source| ConfigError::SaveError { source })?; + env_file + .persist(env_file_path) + .map_err(|error| ConfigError::SaveError { + source: error.error, + })?; + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let permissions = std::fs::Permissions::from_mode(0o600); + if let Err(error) = std::fs::set_permissions(env_file_path, permissions) { + tracing::warn!( + ?error, + path = ?env_file_path, + "Could not restrict config file permissions to 0600" + ); + } + } + Ok(()) +} + +fn write_single_env_update( + env_file: &mut NamedTempFile, + existing_contents: &str, + target_key: &str, + value: &str, +) -> Result<(), ConfigError> { + let mut updated = false; + for line in existing_contents.split_inclusive('\n') { + let (body, line_ending) = if let Some(body) = line.strip_suffix("\r\n") { + (body, "\r\n") + } else if let Some(body) = line.strip_suffix('\n') { + (body, "\n") + } else { + (line, "") + }; + if owned_config_key(body).as_deref() == Some(target_key) { + if updated { + continue; + } + let assignment = body.split_once('=').map_or(target_key, |(left, _)| left); + write!(env_file, "{assignment}={value}{line_ending}") + .map_err(|source| ConfigError::SaveError { source })?; + updated = true; + } else { + write!(env_file, "{line}").map_err(|source| ConfigError::SaveError { source })?; + } + } + if !updated { + if !existing_contents.is_empty() && !existing_contents.ends_with('\n') { + writeln!(env_file).map_err(|source| ConfigError::SaveError { source })?; + } + writeln!(env_file, "{target_key}={value}") + .map_err(|source| ConfigError::SaveError { source })?; + } + Ok(()) +} + +fn dotenv_network_prefix(network: Network) -> &'static str { + match network { + Network::Mainnet => "MAINNET_", + Network::Testnet => "TESTNET_", + Network::Devnet => "DEVNET_", + Network::Regtest => "LOCAL_", + } +} + +fn load_network_config_or_none( + prefix: &'static str, + network: &'static str, + process_entries: &[(String, String)], + file_entries: &[(String, String)], +) -> Option { + match load_network_config(prefix, network, process_entries, file_entries) { + Ok(config) => config, + Err(_) => { + tracing::warn!( + network, + "Ignoring invalid network settings while loading other networks" + ); + None + } + } +} + fn load_network_config( prefix: &'static str, network: &'static str, @@ -454,12 +549,15 @@ mod tests { if std::env::var_os(CHILD_MARKER).is_some() { return false; } - let status = std::process::Command::new(std::env::current_exe().expect("test executable")) - .arg(test_name) - .arg("--exact") - .env(CHILD_MARKER, "1") - .status() - .expect("run isolated config test"); + let mut command = + std::process::Command::new(std::env::current_exe().expect("test executable")); + command.arg(test_name).arg("--exact").env(CHILD_MARKER, "1"); + for prefix in NETWORK_PREFIXES { + for field in NETWORK_CONFIG_FIELDS { + command.env_remove(format!("{prefix}{field}")); + } + } + let status = command.status().expect("run isolated config test"); assert!(status.success(), "isolated config test failed"); true } @@ -816,6 +914,54 @@ mod tests { ); } + #[test] + fn load_ignores_invalid_network_when_another_network_is_valid() { + if run_config_test_in_subprocess( + "config::tests::load_ignores_invalid_network_when_another_network_is_valid", + ) { + return; + } + let tmp = tempfile::tempdir().expect("tempdir"); + std::fs::write( + tmp.path().join(".env"), + "MAINNET_core_rpc_port=invalid\nTESTNET_core_rpc_port=19998\n", + ) + .expect("write test config"); + + let config = Config::load_from(tmp.path()).expect("load the valid network"); + + assert!(config.mainnet_config.is_none()); + assert_eq!( + config + .testnet_config + .as_ref() + .and_then(|network| network.core_rpc_port), + Some(19998), + ); + } + + #[test] + fn load_returns_no_valid_configs_when_all_networks_are_invalid() { + if run_config_test_in_subprocess( + "config::tests::load_returns_no_valid_configs_when_all_networks_are_invalid", + ) { + return; + } + let tmp = tempfile::tempdir().expect("tempdir"); + std::fs::write( + tmp.path().join(".env"), + "MAINNET_core_rpc_port=invalid\n\ + TESTNET_core_rpc_port=invalid\n\ + DEVNET_core_rpc_port=invalid\n\ + LOCAL_core_rpc_port=invalid\n", + ) + .expect("write test config"); + + let error = Config::load_from(tmp.path()).expect_err("all networks are invalid"); + + assert!(matches!(error, ConfigError::NoValidConfigs)); + } + // ── envy parsing roundtrip ────────────────────────────────────── #[test]