Skip to content

fix(spv): wait for wallet bootstrap before filter scan begins #833

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lklimek wants to merge 2 commits into from
+60 −0
Closed

fix(spv): wait for wallet bootstrap before filter scan begins #833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9c9dea8
fix(spv): wait for wallet bootstrap before filter scan begins
lklimek Apr 20, 2026
8c3c9aa
Update src/context/wallet_lifecycle.rs
lklimek Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/context/wallet_lifecycle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,15 @@ impl AppContext {
.count()
})
.unwrap_or(0);
// Fallback: if no open wallets in memory yet but the database has
// wallets for this network, expect at least 1.
// bootstrap_loaded_wallets will load them shortly and the SPV wait
// loop will block until load completes.
let expected_wallets = if expected_wallets == 0 {
self.db.wallet_count_for_network(&self.network)?.min(1)
} else {
expected_wallets
};
Comment on lines +76 to +80

@thepastaclaw thepastaclaw Apr 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: .min(1) fallback undercounts wallets and counts password-protected ones that cannot auto-unlock

run_spv_loop waits until wm.wallet_count() >= expected_wallet_count before building the client and starting the initial filter scan (src/spv/manager.rs:1045-1075). The DB fallback here has two problems that both cause the race this PR is meant to fix to persist:

  1. Multi-wallet undercount. Clamping the DB count with .min(1) means that when the database contains multiple wallets for this network, SPV only waits for the first one to load. As soon as wallet chore: impl ContextProvider #1 is imported into the WalletManager, wallet_count() >= 1 passes and the compact-filter scan starts, but wallets Fix matrix strategy for ARM and AMD Builds #2..N are still being loaded asynchronously by bootstrap_loaded_wallets, so their addresses are absent from monitored_addresses() when the scan begins. Their historical transactions are skipped for the session, reproducing the original bug for every wallet past the first.

  2. Password-protected wallets are counted but will never load. The in-memory count above filters by is_open() && seed_bytes().is_ok(), meaning only wallets with decrypted seeds. The DB fallback is unfiltered, so password-protected wallets (still WalletSeed::Closed after bootstrap) raise the expected count but handle_wallet_unlocked → wallet_seed_snapshot returns None for them, so queue_spv_wallet_load is never called. wallet_count() stays at 0 and run_spv_loop burns its full 30-second deadline on every startup for users whose wallets are all password-protected.

Both issues share the same root cause: the fallback should count the full set of wallets that can actually auto-load into SPV, not collapse everything to 1 and not include locked wallets that can never satisfy the wait condition.

💡 Suggested change
Suggested change
let expected_wallets = if expected_wallets == 0 {
self.db.wallet_count_for_network(&self.network)?.min(1)
} else {
expected_wallets
};
// Fallback: if no open wallets in memory yet, expect as many wallets
// as the database holds for this network that can auto-unlock on
// bootstrap. Password-protected wallets are excluded because
// bootstrap_loaded_wallets cannot unlock them, so waiting for them
// would burn run_spv_loop's 30s deadline without progress.
let expected_wallets = if expected_wallets == 0 {
self.db
.auto_openable_wallet_count_for_network(&self.network)?
} else {
expected_wallets
};

source: ['claude', 'codex']

🤖 Fix this with AI agents 
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/context/wallet_lifecycle.rs`:
- [BLOCKING] lines 76-80: `.min(1)` fallback undercounts wallets and counts password-protected ones that cannot auto-unlock
  `run_spv_loop` waits until `wm.wallet_count() >= expected_wallet_count` before building the client and starting the initial filter scan (`src/spv/manager.rs:1045-1075`). The DB fallback here has two problems that both cause the race this PR is meant to fix to persist:

1. **Multi-wallet undercount.** Clamping the DB count with `.min(1)` means that when the database contains multiple wallets for this network, SPV only waits for the first one to load. As soon as wallet #1 is imported into the `WalletManager`, `wallet_count() >= 1` passes and the compact-filter scan starts, but wallets #2..N are still being loaded asynchronously by `bootstrap_loaded_wallets`, so their addresses are absent from `monitored_addresses()` when the scan begins. Their historical transactions are skipped for the session, reproducing the original bug for every wallet past the first.

2. **Password-protected wallets are counted but will never load.** The in-memory count above filters by `is_open() && seed_bytes().is_ok()`, meaning only wallets with decrypted seeds. The DB fallback is unfiltered, so password-protected wallets (still `WalletSeed::Closed` after bootstrap) raise the expected count but `handle_wallet_unlocked` → `wallet_seed_snapshot` returns `None` for them, so `queue_spv_wallet_load` is never called. `wallet_count()` stays at 0 and `run_spv_loop` burns its full 30-second deadline on every startup for users whose wallets are all password-protected.

Both issues share the same root cause: the fallback should count the full set of wallets that can actually auto-load into SPV, not collapse everything to 1 and not include locked wallets that can never satisfy the wait condition.

Comment on lines +76 to +80

@thepastaclaw thepastaclaw Apr 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: DB error now aborts start_spv instead of falling back to 0

This commit replaced .unwrap_or(0).min(1) with ?.min(1), converting a previously silent fallback into a hard failure. Any transient rusqlite::Error now propagates as TaskError::Database and prevents SPV from starting at all, even though the wait loop could safely proceed with expected_wallets = 0. The in-memory wallet read immediately above still uses .unwrap_or(0) for lock poisoning, so the two paths treat "couldn't determine count" asymmetrically. This is low-risk in practice because the DB is already open at this point, but it is still a behavior change worth making explicit or softening with a warning + fallback.

💡 Suggested change
Suggested change
let expected_wallets = if expected_wallets == 0 {
self.db.wallet_count_for_network(&self.network)?.min(1)
} else {
expected_wallets
};
let expected_wallets = if expected_wallets == 0 {
self.db
.wallet_count_for_network(&self.network)
.inspect_err(|e| tracing::warn!(?e, "wallet_count_for_network failed, assuming 0"))
.unwrap_or(0)
.min(1)
} else {
expected_wallets
};

source: ['claude']

🤖 Fix this with AI agents 
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/context/wallet_lifecycle.rs`:
- [SUGGESTION] lines 76-80: DB error now aborts `start_spv` instead of falling back to 0
  This commit replaced `.unwrap_or(0).min(1)` with `?.min(1)`, converting a previously silent fallback into a hard failure. Any transient `rusqlite::Error` now propagates as `TaskError::Database` and prevents SPV from starting at all, even though the wait loop could safely proceed with `expected_wallets = 0`. The in-memory wallet read immediately above still uses `.unwrap_or(0)` for lock poisoning, so the two paths treat "couldn't determine count" asymmetrically. This is low-risk in practice because the DB is already open at this point, but it is still a behavior change worth making explicit or softening with a warning + fallback.

Comment on lines +72 to +80

@thepastaclaw thepastaclaw May 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: .min(1) clamps the count at most 1 — comment says "at least 1" and the clamp masks multi-wallet startup

expected_wallets = self.db.wallet_count_for_network(...)?.min(1) returns 0 when the DB count is 0 and 1 for any positive count — i.e. clamps at most 1, not at least 1. Two issues: (1) the inline comment says "expect at least 1," which a future reader (or clippy::redundant_min_max autofix) may "correct" to .max(1), silently regressing the cold-start path (DB empty → expected = 1 → 30s wait, then empty scan); (2) the clamp also limits the multi-wallet startup case: with N>1 not-password-protected wallets in DB and 0 currently in memory, the SPV wait loop releases as soon as the first wallet finishes importing (src/spv/manager.rs:1051), even though wallets 2..N may still be deriving addresses, so they may miss the initial filter scan. If the DB query is filtered to uses_password = 0 (see other finding), the count then represents exactly the loadable set and the .min(1) clamp can be removed entirely — expected_wallets becomes the precise number of wallets bootstrap will populate. If keeping the clamp, expand the comment to capture both (a) why we clamp (avoid stalling on a wallet that fails to decrypt) and (b) why 0 is preserved when the DB is empty.

💡 Suggested change
Suggested change
// Fallback: if no open wallets in memory yet but the database has
// wallets for this network, expect at least 1.
// bootstrap_loaded_wallets will load them shortly and the SPV wait
// loop will block until load completes.
let expected_wallets = if expected_wallets == 0 {
self.db.wallet_count_for_network(&self.network)?.min(1)
} else {
expected_wallets
};
// Fallback: when no wallets are open in memory yet but the database
// has wallets for this network, expect bootstrap to load all of them.
// wallet_count_for_network already excludes password-protected rows
// (which bootstrap cannot decrypt), so the count matches exactly what
// run_spv_loop will see in monitored_addresses() once loads complete.
let expected_wallets = if expected_wallets == 0 {
self.db.wallet_count_for_network(&self.network)?
} else {
expected_wallets
};

source: ['claude', 'codex']

🤖 Fix this with AI agents 
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/context/wallet_lifecycle.rs`:
- [SUGGESTION] lines 72-80: `.min(1)` clamps the count at most 1 — comment says "at least 1" and the clamp masks multi-wallet startup
  `expected_wallets = self.db.wallet_count_for_network(...)?.min(1)` returns 0 when the DB count is 0 and 1 for any positive count — i.e. clamps at most 1, not at least 1. Two issues: (1) the inline comment says "expect at least 1," which a future reader (or `clippy::redundant_min_max` autofix) may "correct" to `.max(1)`, silently regressing the cold-start path (DB empty → `expected = 1` → 30s wait, then empty scan); (2) the clamp also limits the multi-wallet startup case: with N>1 not-password-protected wallets in DB and 0 currently in memory, the SPV wait loop releases as soon as the first wallet finishes importing (src/spv/manager.rs:1051), even though wallets 2..N may still be deriving addresses, so they may miss the initial filter scan. If the DB query is filtered to `uses_password = 0` (see other finding), the count then represents exactly the loadable set and the `.min(1)` clamp can be removed entirely — `expected_wallets` becomes the precise number of wallets bootstrap will populate. If keeping the clamp, expand the comment to capture both (a) why we clamp (avoid stalling on a wallet that fails to decrypt) and (b) why 0 is preserved when the DB is empty.

// Register reconcile channel BEFORE starting SPV so the event handlers
// (spawned inside run_spv_loop) always capture a valid sender.
self.spv_setup_reconcile_listener();
Expand Down
51 changes: 51 additions & 0 deletions src/database/wallet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,16 @@ impl Database {
tx.commit()
}

/// Count how many wallets are stored in the database for a given network.
pub fn wallet_count_for_network(&self, network: &Network) -> rusqlite::Result<usize> {
let conn = self.conn.lock().unwrap();
conn.query_row(
"SELECT COUNT(*) FROM wallet WHERE network = ?",
[network.to_string()],
|row| row.get(0),
)
}
Comment on lines +95 to +103

@thepastaclaw thepastaclaw May 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: DB fallback counts password-locked wallets and forces a 30s startup stall on password-only profiles

wallet_count_for_network counts every row in wallet, but get_wallets() reconstructs password-protected rows as WalletSeed::Closed (src/database/wallet.rs:549-553), and wallet_seed_snapshot() only yields a load when is_open() is true (src/context/wallet_lifecycle.rs:292-305). For a profile whose only wallets are password-protected, the new fallback at src/context/wallet_lifecycle.rs:76-78 sets expected_wallets = 1 while SpvManager::wallet_count() stays at 0 because bootstrap_loaded_wallets cannot decrypt them. run_spv_loop then sleeps for the full 30s deadline at src/spv/manager.rs:1045-1069 before proceeding. Pre-PR this case proceeded immediately (with the empty-scan bug); post-PR it adds a deterministic 30s startup delay on top of still requiring a separate unlock-triggered SPV reload. The fix is to count only wallets bootstrap can actually load — filter the SQL by uses_password = 0. That also lets us drop the .min(1) clamp safely, since locked wallets are no longer in the total.

💡 Suggested change
Suggested change
/// Count how many wallets are stored in the database for a given network.
pub fn wallet_count_for_network(&self, network: &Network) -> rusqlite::Result<usize> {
let conn = self.conn.lock().unwrap();
conn.query_row(
"SELECT COUNT(*) FROM wallet WHERE network = ?",
[network.to_string()],
|row| row.get(0),
)
}
/// Count how many wallets are stored in the database for a given network
/// that will be auto-loaded into SPV at startup (i.e. not password-protected).
pub fn wallet_count_for_network(&self, network: &Network) -> rusqlite::Result<usize> {
let conn = self.conn.lock().unwrap();
conn.query_row(
"SELECT COUNT(*) FROM wallet WHERE network = ? AND uses_password = 0",
[network.to_string()],
|row| row.get(0),
)
}

source: ['claude', 'codex']

🤖 Fix this with AI agents 
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/database/wallet.rs`:
- [BLOCKING] lines 95-103: DB fallback counts password-locked wallets and forces a 30s startup stall on password-only profiles
  `wallet_count_for_network` counts every row in `wallet`, but `get_wallets()` reconstructs password-protected rows as `WalletSeed::Closed` (src/database/wallet.rs:549-553), and `wallet_seed_snapshot()` only yields a load when `is_open()` is true (src/context/wallet_lifecycle.rs:292-305). For a profile whose only wallets are password-protected, the new fallback at src/context/wallet_lifecycle.rs:76-78 sets `expected_wallets = 1` while `SpvManager::wallet_count()` stays at 0 because `bootstrap_loaded_wallets` cannot decrypt them. `run_spv_loop` then sleeps for the full 30s deadline at src/spv/manager.rs:1045-1069 before proceeding. Pre-PR this case proceeded immediately (with the empty-scan bug); post-PR it adds a deterministic 30s startup delay on top of still requiring a separate unlock-triggered SPV reload. The fix is to count only wallets bootstrap can actually load — filter the SQL by `uses_password = 0`. That also lets us drop the `.min(1)` clamp safely, since locked wallets are no longer in the total.


/// Update the Dash Core wallet name for an HD wallet.
///
/// Returns `Ok(true)` if exactly one row was updated, `Ok(false)` if no
Expand Down Expand Up @@ -1884,4 +1894,45 @@ mod tests {
.expect("Failed to query total_received");
assert_eq!(total_received, 10_000_000);
}

fn insert_test_wallet(db: &Database, seed_hash: &[u8; 32], network: Network) {
let network_str = network.to_string();
let conn = db.conn.lock().unwrap();
conn.execute(
"INSERT INTO wallet (seed_hash, encrypted_seed, salt, nonce, master_ecdsa_bip44_account_0_epk, uses_password, network)
VALUES (?, ?, ?, ?, ?, 0, ?)",
rusqlite::params![
seed_hash.as_slice(),
vec![0u8; 64],
vec![0u8; 16],
vec![0u8; 12],
vec![0u8; 78],
network_str,
],
)
.expect("Failed to insert test wallet");
}
Comment on lines +1898 to +1914

@thepastaclaw thepastaclaw Apr 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: insert_test_wallet hand-writes INSERT SQL instead of going through store_wallet

The new test helper covers only part of the wallet table schema and relies on the remaining columns staying NULLable. That creates a second source of truth for the write path: a future schema change can break this helper without exercising the real insertion logic. The surrounding DB tests already use real Wallet fixtures with store_wallet / store_wallet_with_addresses, which both avoids duplicated SQL and exercises a realistic path.

source: ['claude']


#[test]
fn test_wallet_count_for_network_counts_per_network() {
let db = create_test_database().expect("create db");
let mut seed_a = create_test_seed_hash();
let mut seed_b = create_test_seed_hash();
seed_a[0] = 0xA0;
seed_b[0] = 0xB0;

insert_test_wallet(&db, &seed_a, Network::Testnet);
insert_test_wallet(&db, &seed_b, Network::Testnet);

assert_eq!(
db.wallet_count_for_network(&Network::Testnet)
.expect("count"),
2
);
assert_eq!(
db.wallet_count_for_network(&Network::Mainnet)
.expect("count"),
Comment on lines +1898 to +1934

@thepastaclaw thepastaclaw Apr 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Signaling fix has no automated coverage — only the DB helper is tested

test_wallet_count_for_network_counts_per_network verifies that the new helper returns the right per-network count, but the actual bug fixed here — that start_spv passes a non-zero expected_wallet_count when DB wallets exist but memory is still empty — is only exercised by the manual test plan. The original race was a subtle TOCTOU between async flows and the fallback is easy to regress. A unit test that either calls start_spv() in a controlled state, or extracts the expected_wallets computation into a pure helper and tests its branches, would give cheap protection.

source: ['claude']

🤖 Fix this with AI agents 
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/database/wallet.rs`:
- [SUGGESTION] lines 1898-1934: Signaling fix has no automated coverage — only the DB helper is tested
  `test_wallet_count_for_network_counts_per_network` verifies that the new helper returns the right per-network count, but the actual bug fixed here — that `start_spv` passes a non-zero `expected_wallet_count` when DB wallets exist but memory is still empty — is only exercised by the manual test plan. The original race was a subtle TOCTOU between async flows and the fallback is easy to regress. A unit test that either calls `start_spv()` in a controlled state, or extracts the `expected_wallets` computation into a pure helper and tests its branches, would give cheap protection.

0
);
}
Comment on lines +1898 to +1937

@thepastaclaw thepastaclaw May 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: No test exercises the start_spv() fallback branch

test_wallet_count_for_network_counts_per_network covers the new helper directly, which is appropriate at the DB layer. However the actual race-prevention logic — the if expected_wallets == 0 { ... } block in start_spv() — has no automated coverage and is verified only via the manual test plan. Given the bug class (TOCTOU between two parallel async flows during AppContext bring-up), a regression here is exactly what tests should catch. Either extract the small computation into a pure helper that takes (in_memory_count, db_count) -> usize and assert (0,0)→0, (0,N)→N, (M,_)→M, or add a kittest scenario that pre-populates the DB and asserts SPV starts with a non-zero expected_wallets. Optional but cheap insurance against a future .min/.max flip.

source: ['claude']

}
Loading