Skip to content

fix(dash-spv): wallet reload pool-state loss past gap limit #759

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

Open
ZocoLini wants to merge 1 commit into
base: v0.42-dev
Choose a base branch
from
Open

fix(dash-spv): wallet reload pool-state loss past gap limit #759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
149 changes: 147 additions & 2 deletions dash-spv/tests/dashd_sync/tests_restart.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ use dash_spv::test_utils::SYNC_TIMEOUT;
use super::setup::{create_and_start_client, TestContext};
use dash_spv::test_utils::{create_test_wallet, TestChain};

use dashcore::Amount;
use key_wallet::managed_account::address_pool::KeySource;
use key_wallet::managed_account::managed_account_trait::ManagedAccountTrait;
use key_wallet::managed_account::managed_account_type::ManagedAccountType;
use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo;
use key_wallet::wallet::Wallet;

/// Verify sync state is identical after stopping and restarting with same storage.
#[tokio::test]
async fn test_sync_restart_consistency() {
Expand Down Expand Up @@ -127,11 +134,11 @@ async fn test_sync_with_multiple_restarts() {
Ok(ref event) if is_progress_event(event) => {
events_seen += 1;
if events_seen % 2 == 0 {
tracing::info!("Restarting on: {}", event.description());
tracing::info!("Restarting on: {}", event);
should_restart = true;
break;
}
tracing::info!("Skipped: {}", event.description());
tracing::info!("Skipped: {}", event);
}
Ok(SyncEvent::SyncComplete { .. }) => break,
Ok(_) => continue,
Expand Down Expand Up @@ -191,3 +198,141 @@ async fn test_sync_with_random_restarts() {
ctx.assert_synced(&client_handle.client.progress().await).await;
tracing::info!("Sync completed after {} random restarts (seed={})", num_restarts, seed);
}

/// When a wallet is rebuilt from xpubs alone (the platform persister
/// flow) and snapshotted UTXOs are re-inserted via [`insert_utxo`],
/// the address pool must reconcile any UTXO whose address lives past
/// the initial gap limit — otherwise signing fails with "no
/// derivation path for input address …".
#[tokio::test]
async fn test_persisted_utxo_outside_initial_gap_limit_resolves_after_reload() {
let Some(ctx) = TestContext::new(TestChain::Minimal).await else {
return;
};
if !ctx.dashd.supports_mining {
eprintln!("Skipping test (dashd RPC miner not available)");
return;
}

let mut client_handle = ctx.spawn_new_client().await;
wait_for_sync(&mut client_handle.progress_receiver, ctx.dashd.initial_height).await;

// Pick an address well past the initial gap limit so the
// rebuild's default pool can't coincidentally cover it.
const TARGET_INDEX: u32 = 50;

let high_index_address: dashcore::Address = {
let mut wallet_write = ctx.wallet.write().await;
let (wallet, info) =
wallet_write.get_wallet_and_info_mut(&ctx.wallet_id).expect("wallet under test exists");

let xpub = wallet
.accounts
.standard_bip44_accounts
.get(&0)
.expect("BIP-44 account 0 on Wallet")
.account_xpub;

let funds_acc = info
.accounts
.standard_bip44_accounts
.get_mut(&0)
.expect("BIP-44 account 0 on ManagedWalletInfo");

let ManagedAccountType::Standard {
external_addresses,
..
} = funds_acc.managed_account_type_mut()
else {
panic!("BIP-44 account 0 is not Standard");
};

let key_source = KeySource::Public(xpub);
let highest = external_addresses.highest_generated.unwrap_or(0);
if TARGET_INDEX > highest {
external_addresses
.generate_addresses(TARGET_INDEX - highest, &key_source, true)
.expect("extend external pool");
}
external_addresses
.info_at_index(TARGET_INDEX)
.expect("address at TARGET_INDEX after extension")
.address
.clone()
};

tracing::info!(
"Faucet target: index={} address={} (initial gap-limit = 30)",
TARGET_INDEX,
high_index_address
);

let miner_address = ctx.dashd.node.get_new_address_from_wallet("default");
let send_amount = Amount::from_sat(100_000_000);
let _txid = ctx.dashd.node.send_to_address(&high_index_address, send_amount);
ctx.dashd.node.generate_blocks(1, &miner_address);
wait_for_sync(&mut client_handle.progress_receiver, ctx.dashd.initial_height + 1).await;
client_handle.stop().await;

{
let wallet_read = ctx.wallet.read().await;
let info = wallet_read.get_wallet_info(&ctx.wallet_id).expect("wallet info");
let funds_acc = info.accounts.standard_bip44_accounts.get(&0).expect("BIP-44 account 0");
assert!(
funds_acc.utxos().values().any(|u| u.address == high_index_address),
"live wallet should have tracked the faucet UTXO"
);
assert!(
funds_acc.address_derivation_path(&high_index_address).is_some(),
"live pool should resolve the address before any reload"
);
}

// Mirror the platform persister: snapshot xpubs + UTXOs, then
// rebuild a wallet from xpubs alone and replay the UTXOs.
let (accounts_snapshot, utxos_snapshot) = {
let wallet_read = ctx.wallet.read().await;
let wallet = wallet_read.get_wallet(&ctx.wallet_id).expect("wallet snapshot");
let info = wallet_read.get_wallet_info(&ctx.wallet_id).expect("wallet info snapshot");
let accounts = wallet.accounts.clone();
let utxos: Vec<_> = info
.accounts
.standard_bip44_accounts
.get(&0)
.expect("BIP-44 account 0")
.utxos()
.values()
.cloned()
.collect();
(accounts, utxos)
};

let rebuilt_wallet =
Wallet::new_external_signable(Network::Regtest, ctx.wallet_id, accounts_snapshot);
let mut rebuilt_info = ManagedWalletInfo::from_wallet(&rebuilt_wallet, 0);
let rebuilt_xpub = rebuilt_wallet
.accounts
.standard_bip44_accounts
.get(&0)
.expect("rebuilt BIP-44 account 0 xpub")
.account_xpub;
let rebuilt_funds = rebuilt_info
.accounts
.standard_bip44_accounts
.get_mut(&0)
.expect("rebuilt BIP-44 account 0");
for utxo in utxos_snapshot {
rebuilt_funds
.insert_utxo(utxo, &KeySource::Public(rebuilt_xpub))
.expect("insert_utxo reconciles the pool past the initial gap limit");
}

assert!(
rebuilt_funds.utxos().values().any(|u| u.address == high_index_address),
"rebuilt funds account should still track the snapshotted UTXO"
);
assert!(
rebuilt_funds.address_derivation_path(&high_index_address).is_some(),
"rebuilt pool should resolve the derivation path for index {TARGET_INDEX}"
);
}
2 changes: 1 addition & 1 deletion key-wallet-ffi/src/managed_account.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -729,7 +729,7 @@ pub unsafe extern "C" fn managed_core_account_get_utxo_count(
}

let account = &*account;
account.as_funds().map_or(0, |f| f.utxos.len() as c_uint)
account.as_funds().map_or(0, |f| f.utxos().len() as c_uint)
}

/// FFI-compatible owning-account descriptor for a [`FFITransactionRecord`].
Expand Down
11 changes: 6 additions & 5 deletions key-wallet-ffi/src/utxo_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
mod utxo_tests {
use super::super::*;
use crate::error::{FFIError, FFIErrorCode};
use key_wallet::managed_account::address_pool::KeySource;
use key_wallet::managed_account::managed_account_type::ManagedAccountType;
use key_wallet::Utxo;
use std::ffi::CStr;
Expand Down Expand Up @@ -226,7 +227,7 @@ mod utxo_tests {
let mut utxo = Utxo::new(outpoint, txout, address, 100 + i as u32, false);
utxo.is_confirmed = true;

bip44_account.utxos.insert(outpoint, utxo);
bip44_account.insert_utxo(utxo, &KeySource::NoKeySource).unwrap();
}

managed_info.accounts.insert(bip44_account).unwrap();
Expand Down Expand Up @@ -311,7 +312,7 @@ mod utxo_tests {

let utxos = Utxo::dummy_batch(0..2, 10000, 100, false, false);
for utxo in utxos {
bip44_account.utxos.insert(utxo.outpoint, utxo);
bip44_account.insert_utxo(utxo, &KeySource::NoKeySource).unwrap();
}
managed_info.accounts.insert(bip44_account).unwrap();

Expand All @@ -336,7 +337,7 @@ mod utxo_tests {

let utxos = Utxo::dummy_batch(10..11, 20000, 200, false, false);
for utxo in utxos {
bip32_account.utxos.insert(utxo.outpoint, utxo);
bip32_account.insert_utxo(utxo, &KeySource::NoKeySource).unwrap();
}
managed_info.accounts.insert(bip32_account).unwrap();

Expand All @@ -354,7 +355,7 @@ mod utxo_tests {

let utxos = Utxo::dummy_batch(20..22, 30000, 300, false, false);
for utxo in utxos {
coinjoin_account.utxos.insert(utxo.outpoint, utxo);
coinjoin_account.insert_utxo(utxo, &KeySource::NoKeySource).unwrap();
}
managed_info.accounts.insert(coinjoin_account).unwrap();

Expand Down Expand Up @@ -412,7 +413,7 @@ mod utxo_tests {

let utxos = Utxo::dummy_batch(1..2, 10000, 100, false, false);
for utxo in utxos {
testnet_account.utxos.insert(utxo.outpoint, utxo);
testnet_account.insert_utxo(utxo, &KeySource::NoKeySource).unwrap();
}
managed_info.accounts.insert(testnet_account).unwrap();

Expand Down
78 changes: 71 additions & 7 deletions key-wallet/src/managed_account/managed_core_funds_account.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use crate::account::BLSAccount;
use crate::account::EdDSAAccount;
use crate::account::TransactionRecord;
use crate::managed_account::address_pool;
use crate::managed_account::address_pool::KeySource;
use crate::managed_account::managed_account_trait::ManagedAccountTrait;
use crate::managed_account::managed_account_type::ManagedAccountType;
use crate::managed_account::managed_core_keys_account::ManagedCoreKeysAccount;
Expand All @@ -41,12 +42,10 @@ use std::collections::{BTreeSet, HashSet};
/// state) and adds the funds-specific bookkeeping used by accounts that hold
/// and spend Dash directly (Standard, CoinJoin, DashPay).
///
/// Most read/write surface comes from [`ManagedAccountTrait`] default methods
/// — which delegate to the inner keys account via the primitive accessors —
/// so this struct only carries the funds-specific inherent methods (transaction
/// recording, the Standard-account receive/change paths, etc.). The
/// funds-specific state (`balance`, `utxos`) is reachable as a public field
/// directly.
/// The UTXO set is private; all insertions must go through
/// [`ManagedCoreFundsAccount::insert_utxo`] so the account can reconcile the
/// owning address into its address pool before tracking the UTXO. Reads are
/// available via [`ManagedCoreFundsAccount::utxos`].
#[derive(Debug, Clone)]
#[cfg_attr(feature = "serde", derive(Serialize))]
pub struct ManagedCoreFundsAccount {
Expand All @@ -56,7 +55,7 @@ pub struct ManagedCoreFundsAccount {
/// Account balance information
pub balance: WalletCoreBalance,
/// UTXO set for this account
pub utxos: BTreeMap<OutPoint, Utxo>,
utxos: BTreeMap<OutPoint, Utxo>,
/// Outpoints spent by recorded transactions.
/// Rebuilt from `transactions` during deserialization.
#[cfg_attr(feature = "serde", serde(skip_serializing))]
Expand Down Expand Up @@ -110,6 +109,71 @@ impl ManagedCoreFundsAccount {
&mut self.keys
}

/// Read-only view of this account's tracked UTXO set.
pub fn utxos(&self) -> &BTreeMap<OutPoint, Utxo> {
&self.utxos
}

/// Track a UTXO, ensuring its address is indexed in one of this account's
/// pools first.
///
/// If the address is already in a pool, the UTXO is recorded directly. If
/// not, the pools are extended (one index at a time, interleaved) using
/// `key_source` until the address is derived. Pass
/// [`KeySource::NoKeySource`] to skip pool reconciliation — used by tests
/// with dummy addresses and by internal flows where the address is known
/// to already be indexed.
pub fn insert_utxo(&mut self, utxo: Utxo, key_source: &KeySource) -> crate::error::Result<()> {
let already_indexed = self
.keys
.managed_account_type()
.address_pools()
.iter()
.any(|pool| pool.contains_address(&utxo.address));

if !already_indexed && !matches!(key_source, KeySource::NoKeySource) {
self.ensure_address_indexed(&utxo.address, key_source)?;
}

self.utxos.insert(utxo.outpoint, utxo);
self.keys.bump_monitor_revision();
Ok(())
}

/// Drop all tracked UTXOs. Test-only helper.
#[cfg(test)]
pub(crate) fn clear_utxos(&mut self) {
if !self.utxos.is_empty() {
self.utxos.clear();
self.keys.bump_monitor_revision();
}
}

/// Walk this account's address pools index by index, deriving new
/// entries until `address` shows up. Bounded so a foreign address
/// doesn't pollute pools indefinitely.
fn ensure_address_indexed(
&mut self,
address: &Address,
key_source: &KeySource,
) -> crate::error::Result<()> {
const MAX_SCAN: u32 = 10_000;
let pool_count = self.keys.managed_account_type().address_pools().len();
for index in 0..MAX_SCAN {
for pool_idx in 0..pool_count {
let pool = &mut self.keys.managed_account_type_mut().address_pools_mut()[pool_idx];
let derived = pool.generate_address_at_index(index, key_source, true)?;
if &derived == address {
return Ok(());
}
}
}
Err(crate::error::Error::InvalidAddress(format!(
"address {} not derivable within scan window for this account",
address
)))
}
Comment on lines +155 to +175
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid mutating address pools while probing for a match.

ensure_address_indexed currently extends pools during scan (generate_address_at_index(..., true)), so a non-derivable address can leave partial pool growth before returning Err. That breaks atomicity and makes failure paths stateful.

💡 Proposed fix (two-phase probe/commit) 
 fn ensure_address_indexed(
     &mut self,
     address: &Address,
     key_source: &KeySource,
 ) -> crate::error::Result<()> {
     const MAX_SCAN: u32 = 10_000;
     let pool_count = self.keys.managed_account_type().address_pools().len();
     for index in 0..MAX_SCAN {
         for pool_idx in 0..pool_count {
-            let pool = &mut self.keys.managed_account_type_mut().address_pools_mut()[pool_idx];
-            let derived = pool.generate_address_at_index(index, key_source, true)?;
-            if &derived == address {
+            let is_match = {
+                let pool = &mut self.keys.managed_account_type_mut().address_pools_mut()[pool_idx];
+                let derived = pool.generate_address_at_index(index, key_source, false)?;
+                &derived == address
+            };
+            if is_match {
+                let pool = &mut self.keys.managed_account_type_mut().address_pools_mut()[pool_idx];
+                let _ = pool.generate_address_at_index(index, key_source, true)?;
                 return Ok(());
             }
         }
     }
     Err(crate::error::Error::InvalidAddress(format!(
         "address {} not derivable within scan window for this account",
         address
     )))
 }

As per coding guidelines: "Implement atomic state updates: when modifying wallet state (transactions, UTXOs, balance, used addresses), update all related state atomically in Rust code".

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fn ensure_address_indexed(
&mut self,
address: &Address,
key_source: &KeySource,
) -> crate::error::Result<()> {
const MAX_SCAN: u32 = 10_000;
let pool_count = self.keys.managed_account_type().address_pools().len();
for index in 0..MAX_SCAN {
for pool_idx in 0..pool_count {
let pool = &mut self.keys.managed_account_type_mut().address_pools_mut()[pool_idx];
let derived = pool.generate_address_at_index(index, key_source, true)?;
if &derived == address {
return Ok(());
}
}
}
Err(crate::error::Error::InvalidAddress(format!(
"address {} not derivable within scan window for this account",
address
)))
}
fn ensure_address_indexed(
&mut self,
address: &Address,
key_source: &KeySource,
) -> crate::error::Result<()> {
const MAX_SCAN: u32 = 10_000;
let pool_count = self.keys.managed_account_type().address_pools().len();
for index in 0..MAX_SCAN {
for pool_idx in 0..pool_count {
let is_match = {
let pool = &mut self.keys.managed_account_type_mut().address_pools_mut()[pool_idx];
let derived = pool.generate_address_at_index(index, key_source, false)?;
&derived == address
};
if is_match {
let pool = &mut self.keys.managed_account_type_mut().address_pools_mut()[pool_idx];
let _ = pool.generate_address_at_index(index, key_source, true)?;
return Ok(());
}
}
}
Err(crate::error::Error::InvalidAddress(format!(
"address {} not derivable within scan window for this account",
address
)))
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@key-wallet/src/managed_account/managed_core_funds_account.rs` around lines
155 - 175, ensure_address_indexed currently mutates address pools during probing
because it calls generate_address_at_index(..., true) on address_pools_mut(),
which can grow pools even when the address is not derivable; change to a
two‑phase approach: probe using non‑mutating reads (use address_pools() and call
generate_address_at_index with the non‑mutating flag or an equivalent no‑grow
path) across MAX_SCAN and pool_count to find a match, and only if a match is
found perform the actual mutation/commit (calling generate_address_at_index with
grow=true or otherwise extending address_pools_mut()) to update state
atomically; refer to ensure_address_indexed, generate_address_at_index,
address_pools(), address_pools_mut(), and MAX_SCAN to locate the code to modify.


/// Check if an outpoint was spent by a previously recorded transaction.
fn is_outpoint_spent(&self, outpoint: &OutPoint) -> bool {
self.spent_outpoints.contains(outpoint)
Expand Down
2 changes: 1 addition & 1 deletion key-wallet/src/test_utils/wallet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ impl TestWalletContext {

/// Returns the first UTXO from the first BIP44 account.
pub fn first_utxo(&self) -> &Utxo {
self.bip44_account().utxos.values().next().expect("Should have UTXO")
self.bip44_account().utxos().values().next().expect("Should have UTXO")
}

/// Processes a transaction: runs `check_core_transaction` with `update_state = true`.
Expand Down
13 changes: 7 additions & 6 deletions key-wallet/src/tests/balance_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Tests for update_balance() UTXO categorization.

use crate::managed_account::address_pool::KeySource;
use crate::managed_account::ManagedCoreFundsAccount;
use crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface;
use crate::wallet::managed_wallet_info::ManagedWalletInfo;
Expand All @@ -12,13 +13,13 @@ fn test_balance_with_mixed_utxo_types() {

// Regular confirmed UTXO
let utxo1 = Utxo::dummy(1, 100_000, 1000, false, true);
account.utxos.insert(utxo1.outpoint, utxo1);
account.insert_utxo(utxo1, &KeySource::NoKeySource).unwrap();
// Mature coinbase (100+ confirmations at height 1100)
let utxo2 = Utxo::dummy(2, 10_000_000, 1000, true, true);
account.utxos.insert(utxo2.outpoint, utxo2);
account.insert_utxo(utxo2, &KeySource::NoKeySource).unwrap();
// Immature coinbase (<100 confirmations at height 1100)
let utxo3 = Utxo::dummy(3, 20_000_000, 1050, true, true);
account.utxos.insert(utxo3.outpoint, utxo3);
account.insert_utxo(utxo3, &KeySource::NoKeySource).unwrap();
wallet_info.accounts.insert(account).unwrap();

assert_eq!(wallet_info.balance, WalletCoreBalance::default());
Expand All @@ -34,7 +35,7 @@ fn test_coinbase_maturity_boundary() {

// Coinbase at height 1000
let utxo = Utxo::dummy(1, 50_000_000, 1000, true, true);
account.utxos.insert(utxo.outpoint, utxo);
account.insert_utxo(utxo, &KeySource::NoKeySource).unwrap();
wallet_info.accounts.insert(account).unwrap();

assert_eq!(wallet_info.balance, WalletCoreBalance::default());
Expand All @@ -56,7 +57,7 @@ fn test_locked_utxos_in_locked_balance() {

let mut utxo = Utxo::dummy(1, 100_000, 1000, false, true);
utxo.is_locked = true;
account.utxos.insert(utxo.outpoint, utxo);
account.insert_utxo(utxo, &KeySource::NoKeySource).unwrap();
wallet_info.accounts.insert(account).unwrap();

assert_eq!(wallet_info.balance, WalletCoreBalance::default());
Expand All @@ -71,7 +72,7 @@ fn test_unconfirmed_utxos_in_unconfirmed_balance() {
let mut account = ManagedCoreFundsAccount::dummy_bip44();

let utxo = Utxo::dummy(1, 100_000, 0, false, false);
account.utxos.insert(utxo.outpoint, utxo);
account.insert_utxo(utxo, &KeySource::NoKeySource).unwrap();
wallet_info.accounts.insert(account).unwrap();

assert_eq!(wallet_info.balance, WalletCoreBalance::default());
Expand Down
Loading
Loading