From e1357a5a600de9ad3bdf0d10604cf3edda31ad8a Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Fri, 30 May 2025 09:38:08 +0200 Subject: [PATCH 01/24] add a fixme regarding new accounts creation --- mm2src/coins/hd_wallet/mod.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/mm2src/coins/hd_wallet/mod.rs b/mm2src/coins/hd_wallet/mod.rs index 1fedf35fcd..0c7359c870 100644 --- a/mm2src/coins/hd_wallet/mod.rs +++ b/mm2src/coins/hd_wallet/mod.rs @@ -396,7 +396,12 @@ where Some(account_id) => account_id, None => { let accounts = hd_wallet.get_accounts_mut().await; + // Get's the last account ID we have. let last_account_id = accounts.iter().last().map(|(account_id, _account)| *account_id); + // This selects an available account ID, between 0..=last_id, and if all IDs are taken already, we use `last_id + 1`. + // FIXME: This is inherently wrong. If account 0 is used, and account last_id is also used, then there should not exist + // any accounts with ID such that "0 < ID < last_id" and they aren't used. + // It is safe to just use `last_id + 1` but we gotta make sure that `last_id` was used and has some tx history. last_account_id.map_or(INIT_ACCOUNT_ID, |last_id| { (INIT_ACCOUNT_ID..=last_id) .find(|id| !accounts.contains_key(id)) From 66f5bdf01044786c5efe928e1ab20f6ff9f496f5 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Fri, 30 May 2025 10:03:58 +0200 Subject: [PATCH 02/24] PoC fix for hd derivation path conflict basically, we don't now load ALL accounts, but only the accounts matching our XPUB. note that this only returns one account for such a load_ call, but i think this is the correct behaviour. if we wanna really keep loading all accounts at the start, we don't have no way of knowning if an account in the DB should be loaded or not, because we don't know which coin it belongs to (we don't know the m/purpose'/coin', we only know m/purpose'/coin'/account_id' via the xpub) --- mm2src/coins/hd_wallet/mod.rs | 34 +++++++++++++++++++ .../utxo/utxo_builder/utxo_coin_builder.rs | 16 ++++++--- mm2src/crypto/src/global_hd_ctx.rs | 25 +++++++++++++- 3 files changed, 70 insertions(+), 5 deletions(-) diff --git a/mm2src/coins/hd_wallet/mod.rs b/mm2src/coins/hd_wallet/mod.rs index 0c7359c870..1d9f41823d 100644 --- a/mm2src/coins/hd_wallet/mod.rs +++ b/mm2src/coins/hd_wallet/mod.rs @@ -269,6 +269,40 @@ where } } +pub async fn load_hd_accounts_from_storage_with_xpub_specifier( + hd_wallet_storage: &HDWalletCoinStorage, + derivation_path: &HDPathToCoin, + xpub: ExtendedPublicKey, +) -> HDWalletStorageResult>> +where + HDAddress: HDAddressOps + Send, + ExtendedPublicKey: ExtendedPublicKeyOps, + ::Err: Display, +{ + let accounts = hd_wallet_storage.load_all_accounts().await?; + + // Filter in only accounts that match the expected xpub. Surprise! They are only ONE account. + let accounts = accounts + .into_iter() + .filter(|account| account.account_xpub == xpub.to_string(bip32::Prefix::XPUB)); + + let res: HDWalletStorageResult>> = accounts + .map(|account_info| { + let account = HDAccount::try_from_storage_item(derivation_path, &account_info)?; + Ok((account.account_id, account)) + }) + .collect(); + match res { + Ok(accounts) => Ok(accounts), + Err(e) if e.get_inner().is_deserializing_err() => { + warn!("Error loading HD accounts from the storage: '{}'. Clear accounts", e); + hd_wallet_storage.clear_accounts().await?; + Ok(HDAccountsMap::new()) + }, + Err(e) => Err(e), + } +} + /// Represents a Hierarchical Deterministic (HD) wallet for UTXO coins. /// This struct encapsulates all the necessary data for HD wallet operations /// and is initialized whenever a utxo coin is activated in HD wallet mode. diff --git a/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs b/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs index 03ccd6fd8b..f874e64b99 100644 --- a/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs +++ b/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs @@ -1,5 +1,6 @@ -use crate::hd_wallet::{load_hd_accounts_from_storage, HDAccountsMutex, HDWallet, HDWalletCoinStorage, HDWalletOps, - HDWalletStorageError, DEFAULT_GAP_LIMIT}; +use crate::hd_wallet::{load_hd_accounts_from_storage, load_hd_accounts_from_storage_with_xpub_specifier, + HDAccountsMutex, HDWallet, HDWalletCoinStorage, HDWalletOps, HDWalletStorageError, + DEFAULT_GAP_LIMIT}; use crate::utxo::rpc_clients::{ElectrumClient, ElectrumClientSettings, ElectrumConnectionSettings, EstimateFeeMethod, UtxoRpcClientEnum}; use crate::utxo::tx_cache::{UtxoVerboseCacheOps, UtxoVerboseCacheShared}; @@ -11,10 +12,12 @@ use crate::{BlockchainNetwork, CoinTransportMetrics, DerivationMethod, HistorySy PrivKeyBuildPolicy, PrivKeyPolicy, PrivKeyPolicyNotAllowed, RpcClientType, SharableRpcTransportEventHandler, UtxoActivationParams}; use async_trait::async_trait; +use bip32::ChildNumber; use chain::TxHashAlgo; use common::executor::{abortable_queue::AbortableQueue, AbortableSystem, AbortedError}; use common::now_sec; -use crypto::{Bip32DerPathError, CryptoCtx, CryptoCtxError, GlobalHDAccountArc, HwWalletType, StandardHDPathError}; +use crypto::{Bip32DerPathError, Bip32DerPathOps, CryptoCtx, CryptoCtxError, GlobalHDAccountArc, HwWalletType, + StandardHDPathError}; use derive_more::Display; use futures::channel::mpsc::{channel, Receiver as AsyncReceiver}; use futures::compat::Future01CompatExt; @@ -203,7 +206,12 @@ pub trait UtxoFieldsWithGlobalHDBuilder: UtxoCoinBuilderCommonOps { let hd_wallet_rmd160 = *self.ctx().rmd160(); let hd_wallet_storage = HDWalletCoinStorage::init_with_rmd160(self.ctx(), self.ticker().to_owned(), hd_wallet_rmd160).await?; - let accounts = load_hd_accounts_from_storage(&hd_wallet_storage, path_to_coin) + let xpub = { + let mut account_der_path = path_to_coin.to_derivation_path(); + account_der_path.push(ChildNumber::new(path_to_address.account_id, true).unwrap()); + global_hd_ctx.derive_secp256k1_xpub(&account_der_path).unwrap() + }; + let accounts = load_hd_accounts_from_storage_with_xpub_specifier(&hd_wallet_storage, path_to_coin, xpub) .await .mm_err(UtxoCoinBuildError::from)?; let gap_limit = self.gap_limit(); diff --git a/mm2src/crypto/src/global_hd_ctx.rs b/mm2src/crypto/src/global_hd_ctx.rs index ad7c2bc63b..790a89a4cb 100644 --- a/mm2src/crypto/src/global_hd_ctx.rs +++ b/mm2src/crypto/src/global_hd_ctx.rs @@ -1,6 +1,6 @@ use crate::privkey::{bip39_seed_from_passphrase, key_pair_from_secret, PrivKeyError}; use crate::{mm2_internal_der_path, Bip32Error, CryptoInitError, CryptoInitResult}; -use bip32::{DerivationPath, ExtendedPrivateKey}; +use bip32::{DerivationPath, ExtendedPrivateKey, ExtendedPublicKey}; use common::drop_mutability; use keys::{KeyPair, Secret as Secp256k1Secret}; use mm2_err_handle::prelude::*; @@ -74,6 +74,16 @@ impl GlobalHDAccountCtx { pub fn derive_secp256k1_secret(&self, derivation_path: &DerivationPath) -> MmResult { derive_secp256k1_secret(self.bip39_secp_priv_key.clone(), derivation_path) } + + /// Derives an `ExtendedPublicKey` from [`HDAccountCtx::bip39_secp_priv_key`] + /// at the given `m/purpose'/coin_type'/account'` derivation path. + /// Remember, XPub are for accounts. + pub fn derive_secp256k1_xpub( + &self, + derivation_path: &DerivationPath, + ) -> MmResult, Bip32Error> { + derive_secp256k1_xpub(self.bip39_secp_priv_key.clone(), derivation_path) + } } pub fn derive_secp256k1_secret( @@ -89,3 +99,16 @@ pub fn derive_secp256k1_secret( let secret = *priv_key.private_key().as_ref(); Ok(Secp256k1Secret::from(secret)) } + +pub fn derive_secp256k1_xpub( + bip39_secp_priv_key: ExtendedPrivateKey, + derivation_path: &DerivationPath, +) -> MmResult, Bip32Error> { + let mut priv_key = bip39_secp_priv_key; + for child in derivation_path.iter() { + priv_key = priv_key.derive_child(child)?; + } + drop_mutability!(priv_key); + + Ok(priv_key.public_key()) +} From 2d8552ae8d05a1ef21a58976c9b210db9b6c4f76 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Fri, 30 May 2025 10:10:57 +0200 Subject: [PATCH 03/24] add a warning when xpubs don't match --- mm2src/coins/hd_wallet/mod.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/mm2src/coins/hd_wallet/mod.rs b/mm2src/coins/hd_wallet/mod.rs index 1d9f41823d..be7c4decd9 100644 --- a/mm2src/coins/hd_wallet/mod.rs +++ b/mm2src/coins/hd_wallet/mod.rs @@ -282,9 +282,18 @@ where let accounts = hd_wallet_storage.load_all_accounts().await?; // Filter in only accounts that match the expected xpub. Surprise! They are only ONE account. - let accounts = accounts - .into_iter() - .filter(|account| account.account_xpub == xpub.to_string(bip32::Prefix::XPUB)); + let requested_xpub = xpub.to_string(bip32::Prefix::XPUB); + let accounts = accounts.into_iter().filter(|account| { + if account.account_xpub == requested_xpub { + true + } else { + warn!( + "Account with xpub '{}' does not match expected xpub '{}'. Skipping.", + account.account_xpub, requested_xpub + ); + false + } + }); let res: HDWalletStorageResult>> = accounts .map(|account_info| { From f3e327c6d846a77f895a815a3d437ce4e574da56 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Tue, 3 Jun 2025 13:03:51 +0200 Subject: [PATCH 04/24] move the sequential account creation fixme to a todo and add relevant links --- mm2src/coins/hd_wallet/mod.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/mm2src/coins/hd_wallet/mod.rs b/mm2src/coins/hd_wallet/mod.rs index be7c4decd9..ba7bf41e72 100644 --- a/mm2src/coins/hd_wallet/mod.rs +++ b/mm2src/coins/hd_wallet/mod.rs @@ -436,15 +436,19 @@ where { const INIT_ACCOUNT_ID: u32 = 0; let new_account_id = match account_id { + // TODO: We should never let the user choose to create a new account with arbitrary account_id. account_ids must be instantiated + // in order and to instantiate account x+1, account x must have been already instantiated and used (i.e. has tx history). + // ref. https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki#account Some(account_id) => account_id, None => { let accounts = hd_wallet.get_accounts_mut().await; // Get's the last account ID we have. let last_account_id = accounts.iter().last().map(|(account_id, _account)| *account_id); // This selects an available account ID, between 0..=last_id, and if all IDs are taken already, we use `last_id + 1`. - // FIXME: This is inherently wrong. If account 0 is used, and account last_id is also used, then there should not exist - // any accounts with ID such that "0 < ID < last_id" and they aren't used. - // It is safe to just use `last_id + 1` but we gotta make sure that `last_id` was used and has some tx history. + // TODO: This is inherently wrong. If account 0 is used, and account last_id is also used, then there should not exist + // any accounts with ID such that "0 < ID < last_id" and they aren't used. + // It is safe to just use `last_id + 1` but we gotta make sure that `last_id` was used and has some tx history. + // ref. https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki#account last_account_id.map_or(INIT_ACCOUNT_ID, |last_id| { (INIT_ACCOUNT_ID..=last_id) .find(|id| !accounts.contains_key(id)) From d5f511c7dec4fc89b3a3e2994b9de344f64b1a60 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Tue, 3 Jun 2025 15:26:59 +0200 Subject: [PATCH 05/24] clean up un-used methods --- .../coins/hd_wallet/storage/mock_storage.rs | 8 ----- mm2src/coins/hd_wallet/storage/mod.rs | 23 --------------- .../coins/hd_wallet/storage/sqlite_storage.rs | 29 +------------------ .../coins/hd_wallet/storage/wasm_storage.rs | 18 ------------ 4 files changed, 1 insertion(+), 77 deletions(-) diff --git a/mm2src/coins/hd_wallet/storage/mock_storage.rs b/mm2src/coins/hd_wallet/storage/mock_storage.rs index 8086e58be8..1c02d511c3 100644 --- a/mm2src/coins/hd_wallet/storage/mock_storage.rs +++ b/mm2src/coins/hd_wallet/storage/mock_storage.rs @@ -19,14 +19,6 @@ impl HDWalletStorageInternalOps for HDWalletMockStorage { unimplemented!() } - async fn load_account( - &self, - _wallet_id: HDWalletId, - _account_id: u32, - ) -> HDWalletStorageResult> { - unimplemented!() - } - async fn update_external_addresses_number( &self, _wallet_id: HDWalletId, diff --git a/mm2src/coins/hd_wallet/storage/mod.rs b/mm2src/coins/hd_wallet/storage/mod.rs index cb8f11b01d..80936566fb 100644 --- a/mm2src/coins/hd_wallet/storage/mod.rs +++ b/mm2src/coins/hd_wallet/storage/mod.rs @@ -98,12 +98,6 @@ pub(crate) trait HDWalletStorageInternalOps { async fn load_accounts(&self, wallet_id: HDWalletId) -> HDWalletStorageResult>; - async fn load_account( - &self, - wallet_id: HDWalletId, - account_id: u32, - ) -> HDWalletStorageResult>; - async fn update_external_addresses_number( &self, wallet_id: HDWalletId, @@ -133,18 +127,6 @@ pub trait HDWalletStorageOps { /// Getter for the HD wallet storage. fn hd_wallet_storage(&self) -> &HDWalletCoinStorage; - /// Loads all accounts from the HD wallet storage. - async fn load_all_accounts(&self) -> HDWalletStorageResult> { - let storage = self.hd_wallet_storage(); - storage.load_all_accounts().await - } - - /// Loads a specific account from the HD wallet storage. - async fn load_account(&self, account_id: u32) -> HDWalletStorageResult> { - let storage = self.hd_wallet_storage(); - storage.load_account(account_id).await - } - /// Updates the number of external addresses for a specific account. async fn update_external_addresses_number( &self, @@ -260,11 +242,6 @@ impl HDWalletCoinStorage { self.inner.load_accounts(wallet_id).await } - async fn load_account(&self, account_id: u32) -> HDWalletStorageResult> { - let wallet_id = self.wallet_id(); - self.inner.load_account(wallet_id, account_id).await - } - async fn update_external_addresses_number( &self, account_id: u32, diff --git a/mm2src/coins/hd_wallet/storage/sqlite_storage.rs b/mm2src/coins/hd_wallet/storage/sqlite_storage.rs index f430eac042..b8643a7123 100644 --- a/mm2src/coins/hd_wallet/storage/sqlite_storage.rs +++ b/mm2src/coins/hd_wallet/storage/sqlite_storage.rs @@ -6,8 +6,7 @@ use async_trait::async_trait; use common::async_blocking; use db_common::owned_named_params; use db_common::sqlite::rusqlite::{Connection, Error as SqlError, Row}; -use db_common::sqlite::{query_single_row_with_named_params, AsSqlNamedParams, OwnedSqlNamedParams, SqliteConnShared, - SqliteConnWeak}; +use db_common::sqlite::{AsSqlNamedParams, OwnedSqlNamedParams, SqliteConnShared, SqliteConnWeak}; use derive_more::Display; use mm2_core::mm_ctx::MmArc; use mm2_err_handle::prelude::*; @@ -30,10 +29,6 @@ const INSERT_ACCOUNT: &str = "INSERT INTO hd_account const DELETE_ACCOUNTS_BY_WALLET_ID: &str = "DELETE FROM hd_account WHERE coin=:coin AND hd_wallet_rmd160=:hd_wallet_rmd160;"; -const SELECT_ACCOUNT: &str = "SELECT account_id, account_xpub, external_addresses_number, internal_addresses_number - FROM hd_account - WHERE coin=:coin AND hd_wallet_rmd160=:hd_wallet_rmd160 AND account_id=:account_id;"; - const SELECT_ACCOUNTS_BY_WALLET_ID: &str = "SELECT account_id, account_xpub, external_addresses_number, internal_addresses_number FROM hd_account @@ -130,28 +125,6 @@ impl HDWalletStorageInternalOps for HDWalletSqliteStorage { .await } - async fn load_account( - &self, - wallet_id: HDWalletId, - account_id: u32, - ) -> HDWalletStorageResult> { - let selfi = self.clone(); - async_blocking(move || { - let conn_shared = selfi.get_shared_conn()?; - let conn = Self::lock_conn_mutex(&conn_shared)?; - - let mut params = wallet_id.to_sql_params(); - params.extend(owned_named_params! { - ":account_id": account_id, - }); - query_single_row_with_named_params(&conn, SELECT_ACCOUNT, ¶ms.as_sql_named_params(), |row: &Row<'_>| { - HDAccountStorageItem::try_from(row) - }) - .map_to_mm(HDWalletStorageError::from) - }) - .await - } - async fn update_external_addresses_number( &self, wallet_id: HDWalletId, diff --git a/mm2src/coins/hd_wallet/storage/wasm_storage.rs b/mm2src/coins/hd_wallet/storage/wasm_storage.rs index 4654474236..eb9fbe897e 100644 --- a/mm2src/coins/hd_wallet/storage/wasm_storage.rs +++ b/mm2src/coins/hd_wallet/storage/wasm_storage.rs @@ -187,24 +187,6 @@ impl HDWalletStorageInternalOps for HDWalletIndexedDbStorage { .collect()) } - async fn load_account( - &self, - wallet_id: HDWalletId, - account_id: u32, - ) -> HDWalletStorageResult> { - let shared_db = self.get_shared_db()?; - let locked_db = Self::lock_db_mutex(&shared_db).await?; - - let transaction = locked_db.inner.transaction().await?; - let table = transaction.table::().await?; - - let maybe_account = Self::find_account(&table, wallet_id, account_id).await?; - match maybe_account { - Some((_account_item_id, account_item)) => Ok(Some(HDAccountStorageItem::from(account_item))), - None => Ok(None), - } - } - async fn update_external_addresses_number( &self, wallet_id: HDWalletId, From dcc82655579f2888f3c22ae820c543af15a03944 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Tue, 3 Jun 2025 16:35:45 +0200 Subject: [PATCH 06/24] add a fixme regarding get_enabled_address call in trezor activation --- mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs b/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs index f874e64b99..8066a6273e 100644 --- a/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs +++ b/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs @@ -329,6 +329,7 @@ pub trait UtxoFieldsWithHardwareWalletBuilder: UtxoCoinBuilderCommonOps { address_format, }; + // FIXME: This call will fail if no account was found in the DB for this trezor device (e.g. imagine a brand new hd_account DB. It will return zero accounts from `load_hd_accounts_from_storage`). let my_address = hd_wallet .get_enabled_address() .await From e99605cc4eb87627f71706e635b86f02dd6b1f04 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Tue, 3 Jun 2025 20:01:27 +0200 Subject: [PATCH 07/24] use xpubs for db queries that edit hd_account table xpubs and account_ids are pretty much the same thing. the only difference is that xpub is more specific as it also encodes not just the account id, but also the purpose/coin/ sections of the derivaiton. by using xpubs for altering/editing accounts, we make sure we don't mis edit an account that has a different purpose/coin/ fields but with the same ticker (like the confustion that happens when ticker purpose/coin/ changes but the ticker name remains the same). said simply, performing the queries with xpubs is more like performing the queries with purpose/coin/account_id all specified instead of just account_id. --- mm2src/coins/hd_wallet/coin_ops.rs | 5 ++-- .../coins/hd_wallet/storage/mock_storage.rs | 5 ++-- mm2src/coins/hd_wallet/storage/mod.rs | 28 +++++++++---------- .../coins/hd_wallet/storage/sqlite_storage.rs | 15 +++++----- mm2src/coins/utxo/utxo_tests.rs | 14 +++++----- 5 files changed, 35 insertions(+), 32 deletions(-) diff --git a/mm2src/coins/hd_wallet/coin_ops.rs b/mm2src/coins/hd_wallet/coin_ops.rs index 27b92d0aa6..ab7321e485 100644 --- a/mm2src/coins/hd_wallet/coin_ops.rs +++ b/mm2src/coins/hd_wallet/coin_ops.rs @@ -212,15 +212,16 @@ pub trait HDWalletCoinOps { if new_known_addresses_number >= max_addresses_number { return MmError::err(AccountUpdatingError::AddressLimitReached { max_addresses_number }); } + let account_xpub = hd_account.extended_pubkey().to_string(bip32::Prefix::XPUB); match chain { Bip44Chain::External => { hd_wallet - .update_external_addresses_number(hd_account.account_id(), new_known_addresses_number) + .update_external_addresses_number(account_xpub, new_known_addresses_number) .await? }, Bip44Chain::Internal => { hd_wallet - .update_internal_addresses_number(hd_account.account_id(), new_known_addresses_number) + .update_internal_addresses_number(account_xpub, new_known_addresses_number) .await? }, } diff --git a/mm2src/coins/hd_wallet/storage/mock_storage.rs b/mm2src/coins/hd_wallet/storage/mock_storage.rs index 1c02d511c3..a7b3671527 100644 --- a/mm2src/coins/hd_wallet/storage/mock_storage.rs +++ b/mm2src/coins/hd_wallet/storage/mock_storage.rs @@ -1,5 +1,6 @@ use crate::hd_wallet::{HDAccountStorageItem, HDWalletId, HDWalletStorageInternalOps, HDWalletStorageResult}; use async_trait::async_trait; +use crypto::XPub; use mm2_core::mm_ctx::MmArc; #[cfg(test)] use mocktopus::macros::*; @@ -22,7 +23,7 @@ impl HDWalletStorageInternalOps for HDWalletMockStorage { async fn update_external_addresses_number( &self, _wallet_id: HDWalletId, - _account_id: u32, + _account_xpub: XPub, _new_external_addresses_number: u32, ) -> HDWalletStorageResult<()> { unimplemented!() @@ -31,7 +32,7 @@ impl HDWalletStorageInternalOps for HDWalletMockStorage { async fn update_internal_addresses_number( &self, _wallet_id: HDWalletId, - _account_id: u32, + _account_xpub: XPub, _new_internal_addresses_number: u32, ) -> HDWalletStorageResult<()> { unimplemented!() diff --git a/mm2src/coins/hd_wallet/storage/mod.rs b/mm2src/coins/hd_wallet/storage/mod.rs index 80936566fb..514c2fb639 100644 --- a/mm2src/coins/hd_wallet/storage/mod.rs +++ b/mm2src/coins/hd_wallet/storage/mod.rs @@ -33,8 +33,8 @@ type HDWalletStorageBoxed = Box; pub enum HDWalletStorageError { #[display(fmt = "HD wallet not allowed")] HDWalletUnavailable, - #[display(fmt = "HD account '{:?}':{} not found", wallet_id, account_id)] - HDAccountNotFound { wallet_id: HDWalletId, account_id: u32 }, + #[display(fmt = "HD account '{:?}':{} not found", wallet_id, account_xpub)] + HDAccountNotFound { wallet_id: HDWalletId, account_xpub: XPub }, #[display(fmt = "Error saving changes in HD wallet storage: {}", _0)] ErrorSaving(String), #[display(fmt = "Error loading from HD wallet storage: {}", _0)] @@ -101,14 +101,14 @@ pub(crate) trait HDWalletStorageInternalOps { async fn update_external_addresses_number( &self, wallet_id: HDWalletId, - account_id: u32, + account_xpub: XPub, new_external_addresses_number: u32, ) -> HDWalletStorageResult<()>; async fn update_internal_addresses_number( &self, wallet_id: HDWalletId, - account_id: u32, + account_xpub: XPub, new_internal_addresses_number: u32, ) -> HDWalletStorageResult<()>; @@ -130,24 +130,24 @@ pub trait HDWalletStorageOps { /// Updates the number of external addresses for a specific account. async fn update_external_addresses_number( &self, - account_id: u32, + account_xpub: XPub, new_external_addresses_number: u32, ) -> HDWalletStorageResult<()> { let storage = self.hd_wallet_storage(); storage - .update_external_addresses_number(account_id, new_external_addresses_number) + .update_external_addresses_number(account_xpub, new_external_addresses_number) .await } /// Updates the number of internal addresses for a specific account. async fn update_internal_addresses_number( &self, - account_id: u32, + account_xpub: XPub, new_internal_addresses_number: u32, ) -> HDWalletStorageResult<()> { let storage = self.hd_wallet_storage(); storage - .update_internal_addresses_number(account_id, new_internal_addresses_number) + .update_internal_addresses_number(account_xpub, new_internal_addresses_number) .await } @@ -244,23 +244,23 @@ impl HDWalletCoinStorage { async fn update_external_addresses_number( &self, - account_id: u32, + account_xpub: XPub, new_external_addresses_number: u32, ) -> HDWalletStorageResult<()> { let wallet_id = self.wallet_id(); self.inner - .update_external_addresses_number(wallet_id, account_id, new_external_addresses_number) + .update_external_addresses_number(wallet_id, account_xpub, new_external_addresses_number) .await } async fn update_internal_addresses_number( &self, - account_id: u32, + account_xpub: XPub, new_internal_addresses_number: u32, ) -> HDWalletStorageResult<()> { let wallet_id = self.wallet_id(); self.inner - .update_internal_addresses_number(wallet_id, account_id, new_internal_addresses_number) + .update_internal_addresses_number(wallet_id, account_xpub, new_internal_addresses_number) .await } @@ -488,10 +488,10 @@ mod tests { .await .expect("!HDWalletCoinStorage::upload_new_account: RICK wallet=0 account=1"); - db.update_internal_addresses_number(0, 5) + db.update_internal_addresses_number(account0.account_xpub.clone(), 5) .await .expect("!HDWalletCoinStorage::update_internal_addresses_number"); - db.update_external_addresses_number(1, 10) + db.update_external_addresses_number(account1.account_xpub.clone(), 10) .await .expect("!HDWalletCoinStorage::update_external_addresses_number"); diff --git a/mm2src/coins/hd_wallet/storage/sqlite_storage.rs b/mm2src/coins/hd_wallet/storage/sqlite_storage.rs index b8643a7123..2a2f219b8f 100644 --- a/mm2src/coins/hd_wallet/storage/sqlite_storage.rs +++ b/mm2src/coins/hd_wallet/storage/sqlite_storage.rs @@ -4,6 +4,7 @@ use crate::hd_wallet::{HDAccountStorageItem, HDWalletId, HDWalletStorageError, H HDWalletStorageResult}; use async_trait::async_trait; use common::async_blocking; +use crypto::XPub; use db_common::owned_named_params; use db_common::sqlite::rusqlite::{Connection, Error as SqlError, Row}; use db_common::sqlite::{AsSqlNamedParams, OwnedSqlNamedParams, SqliteConnShared, SqliteConnWeak}; @@ -128,13 +129,13 @@ impl HDWalletStorageInternalOps for HDWalletSqliteStorage { async fn update_external_addresses_number( &self, wallet_id: HDWalletId, - account_id: u32, + account_xpub: XPub, new_external_addresses_number: u32, ) -> HDWalletStorageResult<()> { self.update_addresses_number( UpdatingProperty::ExternalAddressesNumber, wallet_id, - account_id, + account_xpub, new_external_addresses_number, ) .await @@ -143,13 +144,13 @@ impl HDWalletStorageInternalOps for HDWalletSqliteStorage { async fn update_internal_addresses_number( &self, wallet_id: HDWalletId, - account_id: u32, + account_xpub: XPub, new_internal_addresses_number: u32, ) -> HDWalletStorageResult<()> { self.update_addresses_number( UpdatingProperty::InternalAddressesNumber, wallet_id, - account_id, + account_xpub, new_internal_addresses_number, ) .await @@ -212,11 +213,11 @@ impl HDWalletSqliteStorage { &self, updating_property: UpdatingProperty, wallet_id: HDWalletId, - account_id: u32, + account_xpub: XPub, new_addresses_number: u32, ) -> HDWalletStorageResult<()> { let sql = format!( - "UPDATE hd_account SET {updating_property}=:new_value WHERE coin=:coin AND hd_wallet_rmd160=:hd_wallet_rmd160 AND account_id=:account_id;", + "UPDATE hd_account SET {updating_property}=:new_value WHERE coin=:coin AND hd_wallet_rmd160=:hd_wallet_rmd160 AND account_xpub=:account_xpub;", ); let selfi = self.clone(); @@ -226,7 +227,7 @@ impl HDWalletSqliteStorage { let mut params = owned_named_params! { ":new_value": new_addresses_number, - ":account_id": account_id, + ":account_xpub": account_xpub, }; params.extend(wallet_id.to_sql_params()); diff --git a/mm2src/coins/utxo/utxo_tests.rs b/mm2src/coins/utxo/utxo_tests.rs index c53d81045d..2a11f139aa 100644 --- a/mm2src/coins/utxo/utxo_tests.rs +++ b/mm2src/coins/utxo/utxo_tests.rs @@ -4492,21 +4492,21 @@ fn test_account_balance_rpc() { #[test] fn test_scan_for_new_addresses() { - static mut ACCOUNT_ID: u32 = 0; + static mut ACCOUNT_XPUB: &str = ""; static mut NEW_EXTERNAL_ADDRESSES_NUMBER: u32 = 0; static mut NEW_INTERNAL_ADDRESSES_NUMBER: u32 = 0; HDWalletMockStorage::update_external_addresses_number.mock_safe( - |_, _, account_id, new_external_addresses_number| { - assert_eq!(account_id, unsafe { ACCOUNT_ID }); + |_, _, account_xpub, new_external_addresses_number| { + assert_eq!(account_xpub, unsafe { ACCOUNT_XPUB }); assert_eq!(new_external_addresses_number, unsafe { NEW_EXTERNAL_ADDRESSES_NUMBER }); MockResult::Return(Box::pin(futures::future::ok(()))) }, ); HDWalletMockStorage::update_internal_addresses_number.mock_safe( - |_, _, account_id, new_internal_addresses_number| { - assert_eq!(account_id, unsafe { ACCOUNT_ID }); + |_, _, account_xpub, new_internal_addresses_number| { + assert_eq!(account_xpub, unsafe { ACCOUNT_XPUB }); assert_eq!(new_internal_addresses_number, unsafe { NEW_INTERNAL_ADDRESSES_NUMBER }); MockResult::Return(Box::pin(futures::future::ok(()))) }, @@ -4648,7 +4648,7 @@ fn test_scan_for_new_addresses() { // Check balance of Account#0 unsafe { - ACCOUNT_ID = 0; + ACCOUNT_XPUB = "xpub6DEHSksajpRPM59RPw7Eg6PKdU7E2ehxJWtYdrfQ6JFmMGBsrR6jA78ANCLgzKYm4s5UqQ4ydLEYPbh3TRVvn5oAZVtWfi4qJLMntpZ8uGJ"; NEW_EXTERNAL_ADDRESSES_NUMBER = 4; NEW_INTERNAL_ADDRESSES_NUMBER = 4; } @@ -4673,7 +4673,7 @@ fn test_scan_for_new_addresses() { // Check balance of Account#1 unsafe { - ACCOUNT_ID = 1; + ACCOUNT_XPUB = "xpub6DEHSksajpRPQq2FdGT6JoieiQZUpTZ3WZn8fcuLJhFVmtCpXbuXxp5aPzaokwcLV2V9LE55Dwt8JYkpuMv7jXKwmyD28WbHYjBH2zhbW2p"; NEW_EXTERNAL_ADDRESSES_NUMBER = 5; NEW_INTERNAL_ADDRESSES_NUMBER = 2; } From 193579d00f68b8ddb72d5c5ea4afb0c5975a6aab Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Tue, 3 Jun 2025 20:39:28 +0200 Subject: [PATCH 08/24] use xpubs for db queries that edit hd_account table (wasm) --- .../coins/hd_wallet/storage/wasm_storage.rs | 73 +++++++++++++------ 1 file changed, 50 insertions(+), 23 deletions(-) diff --git a/mm2src/coins/hd_wallet/storage/wasm_storage.rs b/mm2src/coins/hd_wallet/storage/wasm_storage.rs index eb9fbe897e..d0c20a61ec 100644 --- a/mm2src/coins/hd_wallet/storage/wasm_storage.rs +++ b/mm2src/coins/hd_wallet/storage/wasm_storage.rs @@ -4,13 +4,13 @@ use crate::CoinsContext; use async_trait::async_trait; use crypto::XPub; use mm2_core::mm_ctx::MmArc; -use mm2_db::indexed_db::cursor_prelude::*; +use mm2_db::indexed_db::{cursor_prelude::*, OnUpgradeError}; use mm2_db::indexed_db::{DbIdentifier, DbInstance, DbLocked, DbTable, DbTransactionError, DbUpgrader, IndexedDb, IndexedDbBuilder, InitDbError, InitDbResult, ItemId, MultiIndex, OnUpgradeResult, SharedDb, TableSignature, WeakDb}; use mm2_err_handle::prelude::*; -const DB_VERSION: u32 = 1; +const DB_VERSION: u32 = 2; /// An index of the `HDAccountTable` table that consists of the following properties: /// * coin - coin ticker /// * hd_wallet_rmd160 - RIPEMD160(SHA256(x)) where x is a pubkey extracted from a Hardware Wallet device or passphrase. @@ -18,8 +18,8 @@ const WALLET_ID_INDEX: &str = "wallet_id"; /// A **unique** index of the `HDAccountTable` table that consists of the following properties: /// * coin - coin ticker /// * hd_wallet_rmd160 - RIPEMD160(SHA256(x)) where x is a pubkey extracted from a Hardware Wallet device or passphrase. -/// * account_id - HD account id -const WALLET_ACCOUNT_ID_INDEX: &str = "wallet_account_id"; +/// * account_xpub - HD account xpub +const WALLET_ACCOUNT_XPUB_INDEX: &str = "wallet_account_xpub"; type HDWalletDbLocked<'a> = DbLocked<'a, HDWalletDb>; @@ -96,15 +96,39 @@ struct HDAccountTable { impl TableSignature for HDAccountTable { const TABLE_NAME: &'static str = "hd_account"; - fn on_upgrade_needed(upgrader: &DbUpgrader, old_version: u32, new_version: u32) -> OnUpgradeResult<()> { - if let (0, 1) = (old_version, new_version) { - let table = upgrader.create_table(Self::TABLE_NAME)?; - table.create_multi_index(WALLET_ID_INDEX, &["coin", "hd_wallet_rmd160"], false)?; - table.create_multi_index( - WALLET_ACCOUNT_ID_INDEX, - &["coin", "hd_wallet_rmd160", "account_id"], - true, - )?; + fn on_upgrade_needed(upgrader: &DbUpgrader, mut current_version: u32, new_version: u32) -> OnUpgradeResult<()> { + /// A deprecated index that is now replaced by `WALLET_ACCOUNT_XPUB_INDEX`. + const WALLET_ACCOUNT_ID_INDEX: &str = "wallet_account_id"; + + while current_version < new_version { + match current_version { + 0 => { + let table = upgrader.create_table(Self::TABLE_NAME)?; + table.create_multi_index(WALLET_ID_INDEX, &["coin", "hd_wallet_rmd160"], false)?; + table.create_multi_index( + WALLET_ACCOUNT_ID_INDEX, + &["coin", "hd_wallet_rmd160", "account_id"], + true, + )?; + }, + 1 => { + let table = upgrader.open_table(Self::TABLE_NAME)?; + table.create_multi_index( + WALLET_ACCOUNT_XPUB_INDEX, + &["coin", "hd_wallet_rmd160", "account_xpub"], + true, + )?; + table.delete_index(WALLET_ACCOUNT_ID_INDEX)?; + }, + unsupported_version => { + return MmError::err(OnUpgradeError::UnsupportedVersion { + unsupported_version, + old_version: current_version, + new_version, + }); + }, + }; + current_version += 1; } Ok(()) @@ -190,10 +214,10 @@ impl HDWalletStorageInternalOps for HDWalletIndexedDbStorage { async fn update_external_addresses_number( &self, wallet_id: HDWalletId, - account_id: u32, + account_xpub: XPub, new_external_addresses_number: u32, ) -> HDWalletStorageResult<()> { - self.update_account(wallet_id, account_id, |account| { + self.update_account(wallet_id, account_xpub, |account| { account.external_addresses_number = new_external_addresses_number; }) .await @@ -202,10 +226,10 @@ impl HDWalletStorageInternalOps for HDWalletIndexedDbStorage { async fn update_internal_addresses_number( &self, wallet_id: HDWalletId, - account_id: u32, + account_xpub: XPub, new_internal_addresses_number: u32, ) -> HDWalletStorageResult<()> { - self.update_account(wallet_id, account_id, |account| { + self.update_account(wallet_id, account_xpub, |account| { account.internal_addresses_number = new_internal_addresses_number; }) .await @@ -259,19 +283,19 @@ impl HDWalletIndexedDbStorage { async fn find_account( table: &DbTable<'_, HDAccountTable>, wallet_id: HDWalletId, - account_id: u32, + account_xpub: XPub, ) -> HDWalletStorageResult> { - let index_keys = MultiIndex::new(WALLET_ACCOUNT_ID_INDEX) + let index_keys = MultiIndex::new(WALLET_ACCOUNT_XPUB_INDEX) .with_value(wallet_id.coin)? .with_value(wallet_id.hd_wallet_rmd160)? - .with_value(account_id)?; + .with_value(account_xpub)?; table .get_item_by_unique_multi_index(index_keys) .await .mm_err(HDWalletStorageError::from) } - async fn update_account(&self, wallet_id: HDWalletId, account_id: u32, f: F) -> HDWalletStorageResult<()> + async fn update_account(&self, wallet_id: HDWalletId, account_xpub: XPub, f: F) -> HDWalletStorageResult<()> where F: FnOnce(&mut HDAccountTable), { @@ -281,9 +305,12 @@ impl HDWalletIndexedDbStorage { let transaction = locked_db.inner.transaction().await?; let table = transaction.table::().await?; - let (account_item_id, mut account) = Self::find_account(&table, wallet_id.clone(), account_id) + let (account_item_id, mut account) = Self::find_account(&table, wallet_id.clone(), account_xpub.clone()) .await? - .or_mm_err(|| HDWalletStorageError::HDAccountNotFound { wallet_id, account_id })?; + .or_mm_err(|| HDWalletStorageError::HDAccountNotFound { + wallet_id, + account_xpub, + })?; // Apply `f` to `account` and upload the changes to the storage. f(&mut account); From bdf2f7464aedfe5aa0097bfda03897e1cdff8a3a Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Wed, 4 Jun 2025 11:07:50 +0200 Subject: [PATCH 09/24] shouldn't we rollback tasks the failed running? will clean up this commit before merging --- mm2src/coins_activation/src/utxo_activation/common_impl.rs | 1 + mm2src/rpc_task/src/manager.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/mm2src/coins_activation/src/utxo_activation/common_impl.rs b/mm2src/coins_activation/src/utxo_activation/common_impl.rs index eba196c370..8906c3f4c1 100644 --- a/mm2src/coins_activation/src/utxo_activation/common_impl.rs +++ b/mm2src/coins_activation/src/utxo_activation/common_impl.rs @@ -56,6 +56,7 @@ where None }; task_handle.update_in_progress_status(UtxoStandardInProgressStatus::RequestingWalletBalance)?; + // .e.g: This call will error if the user puts an account number >= `1 << 31`. let wallet_balance = coin .enable_coin_balance( xpub_extractor, diff --git a/mm2src/rpc_task/src/manager.rs b/mm2src/rpc_task/src/manager.rs index b5c43a04b6..29a7b29d0e 100644 --- a/mm2src/rpc_task/src/manager.rs +++ b/mm2src/rpc_task/src/manager.rs @@ -72,6 +72,7 @@ impl RpcTaskManager { // `task.run(&task_handle)`. match task_result { Some(task_result) => { + // FIXME: If the result is an error, shouldn't we cancel the task at this point? (check the e.g. in the same commit). debug!("RPC task '{}' has been finished", task_id); task_handle.finish(task_result); }, From de8ea00bb89a7c158cbd91714e6158f8bcbdd8dd Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Wed, 4 Jun 2025 14:31:56 +0200 Subject: [PATCH 10/24] load all accounts belonging to the activated HD wallet and not just one this is a hacky method to keep the old behaviour (which we might later reconsider designing). that is, we will load all the accounts belonging to the activated HD wallet. we are gauranteed to pick the correct accounts even if the database was mal-formed due to using same tickers names. this gaurantee stems from the fact that account_ids aren't fully trusted, but they are fetche and used to derive the xpub and only if the derived xpub matches the one in the database, it is loaded in next commits, remove get_all_account_ids and merge get_xpubs_for_account_ids and load_all_hd_accounts_from_storage_with_xpubs for simplicity. you might also wanna consider collecting un matching xpubs and deleting them (assuming we always want this effect when we edit the config file, i.e. we are not playing with the config) --- mm2src/coins/eth/v2_activation.rs | 7 +- mm2src/coins/hd_wallet/mod.rs | 77 +++++++++++++++---- .../coins/hd_wallet/storage/mock_storage.rs | 2 + mm2src/coins/hd_wallet/storage/mod.rs | 7 ++ .../coins/hd_wallet/storage/sqlite_storage.rs | 19 +++++ .../utxo/utxo_builder/utxo_coin_builder.rs | 20 ++--- 6 files changed, 104 insertions(+), 28 deletions(-) diff --git a/mm2src/coins/eth/v2_activation.rs b/mm2src/coins/eth/v2_activation.rs index 9bcd2d6c57..10559066f2 100644 --- a/mm2src/coins/eth/v2_activation.rs +++ b/mm2src/coins/eth/v2_activation.rs @@ -1,7 +1,8 @@ use super::*; use crate::eth::erc20::{get_enabled_erc20_by_platform_and_contract, get_token_decimals}; use crate::eth::web3_transport::http_transport::HttpTransport; -use crate::hd_wallet::{load_hd_accounts_from_storage, HDAccountsMutex, HDPathAccountToAddressId, HDWalletCoinStorage, +use crate::hd_wallet::{get_xpubs_for_account_ids, load_all_hd_accounts_from_storage_with_xpubs, + load_hd_accounts_from_storage, HDAccountsMutex, HDPathAccountToAddressId, HDWalletCoinStorage, HDWalletStorageError, DEFAULT_GAP_LIMIT}; use crate::nft::get_nfts_for_activation; use crate::nft::nft_errors::{GetNftInfoError, ParseChainTypeError}; @@ -763,7 +764,9 @@ pub(crate) async fn build_address_and_priv_key_policy( let hd_wallet_storage = HDWalletCoinStorage::init_with_rmd160(ctx, ticker.to_string(), hd_wallet_rmd160) .await .mm_err(EthActivationV2Error::from)?; - let accounts = load_hd_accounts_from_storage(&hd_wallet_storage, &path_to_coin).await?; + let xpub = get_xpubs_for_account_ids(&hd_wallet_storage, &path_to_coin, &global_hd_ctx).await?; + let accounts = + load_all_hd_accounts_from_storage_with_xpubs(&hd_wallet_storage, &path_to_coin, xpub).await?; let gap_limit = gap_limit.unwrap_or(DEFAULT_GAP_LIMIT); let hd_wallet = EthHDWallet { hd_wallet_rmd160, diff --git a/mm2src/coins/hd_wallet/mod.rs b/mm2src/coins/hd_wallet/mod.rs index ba7bf41e72..c49f0a4fc1 100644 --- a/mm2src/coins/hd_wallet/mod.rs +++ b/mm2src/coins/hd_wallet/mod.rs @@ -1,12 +1,13 @@ use async_trait::async_trait; -use common::log::warn; -use crypto::{Bip32DerPathOps, Bip32Error, Bip44Chain, ChildNumber, DerivationPath, HDPathToAccount, HDPathToCoin, - Secp256k1ExtendedPublicKey, StandardHDPath, StandardHDPathError}; +use bip32::ExtendedPublicKey; +use common::log::{error, warn}; +use crypto::{Bip32DerPathOps, Bip32Error, Bip44Chain, ChildNumber, DerivationPath, GlobalHDAccountArc, + HDPathToAccount, HDPathToCoin, Secp256k1ExtendedPublicKey, StandardHDPath, StandardHDPathError}; use futures::lock::{MappedMutexGuard as AsyncMappedMutexGuard, Mutex as AsyncMutex, MutexGuard as AsyncMutexGuard}; use mm2_err_handle::prelude::*; use primitives::hash::H160; use serde::Serialize; -use std::collections::{BTreeMap, HashMap}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::fmt::Display; use std::hash::Hash; use std::str::FromStr; @@ -241,6 +242,46 @@ where } } +/// Derives the account XPubs for account IDs fetched from the database. +/// +/// This method needs the `GlobalHDAccountArc` to derive the XPubs. +pub async fn get_xpubs_for_account_ids( + hd_wallet_storage: &HDWalletCoinStorage, + derivation_path: &HDPathToCoin, + global_hd_ctx: &GlobalHDAccountArc, +) -> HDWalletStorageResult>> { + let account_ids = hd_wallet_storage.get_all_account_ids().await?; + let coin_der_path = derivation_path.to_derivation_path(); + // Map account IDs to account XPubs to use these XPubs + Ok(account_ids + .into_iter() + .filter_map(|acc_id| { + let mut account_der_path = coin_der_path.clone(); + let child_number = match ChildNumber::new(acc_id, true) { + Ok(num) => num, + Err(e) => { + error!( + "Failed to create ChildNumber for account_id={} found in storage: {}", + acc_id, e + ); + return None; + }, + }; + account_der_path.push(child_number); + match global_hd_ctx.derive_secp256k1_xpub(&account_der_path) { + Ok(xpub) => Some(xpub), + Err(e) => { + error!( + "Failed to derive xpub for account_id={} found in storage: {}", + acc_id, e + ); + None + }, + } + }) + .collect()) +} + pub async fn load_hd_accounts_from_storage( hd_wallet_storage: &HDWalletCoinStorage, derivation_path: &HDPathToCoin, @@ -269,30 +310,38 @@ where } } -pub async fn load_hd_accounts_from_storage_with_xpub_specifier( +pub async fn load_all_hd_accounts_from_storage_with_xpubs( hd_wallet_storage: &HDWalletCoinStorage, derivation_path: &HDPathToCoin, - xpub: ExtendedPublicKey, + xpubs: Vec, ) -> HDWalletStorageResult>> where HDAddress: HDAddressOps + Send, ExtendedPublicKey: ExtendedPublicKeyOps, ::Err: Display, { + if xpubs.is_empty() { + return Ok(HDAccountsMap::new()); + } + let xpubs = xpubs + .into_iter() + .map(|xpub| { + // Convert the xpub to a string with the expected prefix. + xpub.to_string(bip32::Prefix::XPUB) + }) + .collect::>(); let accounts = hd_wallet_storage.load_all_accounts().await?; - // Filter in only accounts that match the expected xpub. Surprise! They are only ONE account. - let requested_xpub = xpub.to_string(bip32::Prefix::XPUB); + // Filter in only accounts that match the expected xpubs. let accounts = accounts.into_iter().filter(|account| { - if account.account_xpub == requested_xpub { - true - } else { + if !xpubs.contains(&account.account_xpub) { warn!( - "Account with xpub '{}' does not match expected xpub '{}'. Skipping.", - account.account_xpub, requested_xpub + "Account with id={} has an xpub={} that doesn't match the one derived from the current HD wallet. Skipping.", + account.account_id, account.account_xpub ); - false + return false; } + true }); let res: HDWalletStorageResult>> = accounts diff --git a/mm2src/coins/hd_wallet/storage/mock_storage.rs b/mm2src/coins/hd_wallet/storage/mock_storage.rs index a7b3671527..802dbd0404 100644 --- a/mm2src/coins/hd_wallet/storage/mock_storage.rs +++ b/mm2src/coins/hd_wallet/storage/mock_storage.rs @@ -16,6 +16,8 @@ impl HDWalletStorageInternalOps for HDWalletMockStorage { unimplemented!() } + async fn load_account_ids(&self, _wallet_id: HDWalletId) -> HDWalletStorageResult> { unimplemented!() } + async fn load_accounts(&self, _wallet_id: HDWalletId) -> HDWalletStorageResult> { unimplemented!() } diff --git a/mm2src/coins/hd_wallet/storage/mod.rs b/mm2src/coins/hd_wallet/storage/mod.rs index 514c2fb639..cf512d202c 100644 --- a/mm2src/coins/hd_wallet/storage/mod.rs +++ b/mm2src/coins/hd_wallet/storage/mod.rs @@ -96,6 +96,8 @@ pub(crate) trait HDWalletStorageInternalOps { where Self: Sized; + async fn load_account_ids(&self, wallet_id: HDWalletId) -> HDWalletStorageResult>; + async fn load_accounts(&self, wallet_id: HDWalletId) -> HDWalletStorageResult>; async fn update_external_addresses_number( @@ -237,6 +239,11 @@ impl HDWalletCoinStorage { pub fn wallet_id(&self) -> HDWalletId { HDWalletId::new(self.coin.clone(), &self.hd_wallet_rmd160) } + pub async fn get_all_account_ids(&self) -> HDWalletStorageResult> { + let wallet_id = self.wallet_id(); + self.inner.load_account_ids(wallet_id).await + } + pub async fn load_all_accounts(&self) -> HDWalletStorageResult> { let wallet_id = self.wallet_id(); self.inner.load_accounts(wallet_id).await diff --git a/mm2src/coins/hd_wallet/storage/sqlite_storage.rs b/mm2src/coins/hd_wallet/storage/sqlite_storage.rs index 2a2f219b8f..de503becb7 100644 --- a/mm2src/coins/hd_wallet/storage/sqlite_storage.rs +++ b/mm2src/coins/hd_wallet/storage/sqlite_storage.rs @@ -35,6 +35,9 @@ const SELECT_ACCOUNTS_BY_WALLET_ID: &str = FROM hd_account WHERE coin=:coin AND hd_wallet_rmd160=:hd_wallet_rmd160;"; +const SELECT_ACCOUNT_IDS_BY_WALLET_ID: &str = + "SELECT account_id FROM hd_account WHERE coin=:coin AND hd_wallet_rmd160=:hd_wallet_rmd160;"; + impl From for HDWalletStorageError { fn from(e: SqlError) -> Self { let error = e.to_string(); @@ -107,6 +110,22 @@ impl HDWalletStorageInternalOps for HDWalletSqliteStorage { Ok(storage) } + async fn load_account_ids(&self, wallet_id: HDWalletId) -> HDWalletStorageResult> { + let selfi = self.clone(); + async_blocking(move || { + let conn_shared = selfi.get_shared_conn()?; + let conn = Self::lock_conn_mutex(&conn_shared)?; + + let mut statement = conn.prepare(SELECT_ACCOUNT_IDS_BY_WALLET_ID)?; + let params = wallet_id.to_sql_params(); + let rows = statement + .query_map_named(¶ms.as_sql_named_params(), |row: &Row<'_>| row.get(0))? + .collect::, _>>()?; + Ok(rows) + }) + .await + } + async fn load_accounts(&self, wallet_id: HDWalletId) -> HDWalletStorageResult> { let selfi = self.clone(); async_blocking(move || { diff --git a/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs b/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs index 8066a6273e..cea252a5da 100644 --- a/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs +++ b/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs @@ -1,6 +1,6 @@ -use crate::hd_wallet::{load_hd_accounts_from_storage, load_hd_accounts_from_storage_with_xpub_specifier, - HDAccountsMutex, HDWallet, HDWalletCoinStorage, HDWalletOps, HDWalletStorageError, - DEFAULT_GAP_LIMIT}; +use crate::hd_wallet::{get_xpubs_for_account_ids, load_all_hd_accounts_from_storage_with_xpubs, + load_hd_accounts_from_storage, HDAccountsMutex, HDWallet, HDWalletCoinStorage, HDWalletOps, + HDWalletStorageError, DEFAULT_GAP_LIMIT}; use crate::utxo::rpc_clients::{ElectrumClient, ElectrumClientSettings, ElectrumConnectionSettings, EstimateFeeMethod, UtxoRpcClientEnum}; use crate::utxo::tx_cache::{UtxoVerboseCacheOps, UtxoVerboseCacheShared}; @@ -12,12 +12,10 @@ use crate::{BlockchainNetwork, CoinTransportMetrics, DerivationMethod, HistorySy PrivKeyBuildPolicy, PrivKeyPolicy, PrivKeyPolicyNotAllowed, RpcClientType, SharableRpcTransportEventHandler, UtxoActivationParams}; use async_trait::async_trait; -use bip32::ChildNumber; use chain::TxHashAlgo; use common::executor::{abortable_queue::AbortableQueue, AbortableSystem, AbortedError}; use common::now_sec; -use crypto::{Bip32DerPathError, Bip32DerPathOps, CryptoCtx, CryptoCtxError, GlobalHDAccountArc, HwWalletType, - StandardHDPathError}; +use crypto::{Bip32DerPathError, CryptoCtx, CryptoCtxError, GlobalHDAccountArc, HwWalletType, StandardHDPathError}; use derive_more::Display; use futures::channel::mpsc::{channel, Receiver as AsyncReceiver}; use futures::compat::Future01CompatExt; @@ -206,12 +204,10 @@ pub trait UtxoFieldsWithGlobalHDBuilder: UtxoCoinBuilderCommonOps { let hd_wallet_rmd160 = *self.ctx().rmd160(); let hd_wallet_storage = HDWalletCoinStorage::init_with_rmd160(self.ctx(), self.ticker().to_owned(), hd_wallet_rmd160).await?; - let xpub = { - let mut account_der_path = path_to_coin.to_derivation_path(); - account_der_path.push(ChildNumber::new(path_to_address.account_id, true).unwrap()); - global_hd_ctx.derive_secp256k1_xpub(&account_der_path).unwrap() - }; - let accounts = load_hd_accounts_from_storage_with_xpub_specifier(&hd_wallet_storage, path_to_coin, xpub) + let xpub = get_xpubs_for_account_ids(&hd_wallet_storage, path_to_coin, &global_hd_ctx) + .await + .mm_err(UtxoCoinBuildError::from)?; + let accounts = load_all_hd_accounts_from_storage_with_xpubs(&hd_wallet_storage, path_to_coin, xpub) .await .mm_err(UtxoCoinBuildError::from)?; let gap_limit = self.gap_limit(); From 8a2a8d9df9ef2503e7dc48c7e4f7cb8ab5f76536 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Wed, 4 Jun 2025 15:17:45 +0200 Subject: [PATCH 11/24] remove get_all_account_ids and merge get_xpubs_for_account_ids and load_all_hd_accounts_from_storage_with_xpubs for simplicity --- mm2src/coins/eth/v2_activation.rs | 10 +- mm2src/coins/hd_wallet/mod.rs | 98 +++++++------------ .../coins/hd_wallet/storage/mock_storage.rs | 2 - mm2src/coins/hd_wallet/storage/mod.rs | 7 -- .../coins/hd_wallet/storage/sqlite_storage.rs | 19 ---- .../utxo/utxo_builder/utxo_coin_builder.rs | 16 ++- 6 files changed, 50 insertions(+), 102 deletions(-) diff --git a/mm2src/coins/eth/v2_activation.rs b/mm2src/coins/eth/v2_activation.rs index 10559066f2..be7b114727 100644 --- a/mm2src/coins/eth/v2_activation.rs +++ b/mm2src/coins/eth/v2_activation.rs @@ -1,9 +1,9 @@ use super::*; use crate::eth::erc20::{get_enabled_erc20_by_platform_and_contract, get_token_decimals}; use crate::eth::web3_transport::http_transport::HttpTransport; -use crate::hd_wallet::{get_xpubs_for_account_ids, load_all_hd_accounts_from_storage_with_xpubs, - load_hd_accounts_from_storage, HDAccountsMutex, HDPathAccountToAddressId, HDWalletCoinStorage, - HDWalletStorageError, DEFAULT_GAP_LIMIT}; +use crate::hd_wallet::{load_hd_accounts_from_storage, load_hd_accounts_from_storage_with_matching_xpubs, + HDAccountsMutex, HDPathAccountToAddressId, HDWalletCoinStorage, HDWalletStorageError, + DEFAULT_GAP_LIMIT}; use crate::nft::get_nfts_for_activation; use crate::nft::nft_errors::{GetNftInfoError, ParseChainTypeError}; use crate::nft::nft_structs::Chain; @@ -764,9 +764,9 @@ pub(crate) async fn build_address_and_priv_key_policy( let hd_wallet_storage = HDWalletCoinStorage::init_with_rmd160(ctx, ticker.to_string(), hd_wallet_rmd160) .await .mm_err(EthActivationV2Error::from)?; - let xpub = get_xpubs_for_account_ids(&hd_wallet_storage, &path_to_coin, &global_hd_ctx).await?; let accounts = - load_all_hd_accounts_from_storage_with_xpubs(&hd_wallet_storage, &path_to_coin, xpub).await?; + load_hd_accounts_from_storage_with_matching_xpubs(&hd_wallet_storage, &path_to_coin, &global_hd_ctx) + .await?; let gap_limit = gap_limit.unwrap_or(DEFAULT_GAP_LIMIT); let hd_wallet = EthHDWallet { hd_wallet_rmd160, diff --git a/mm2src/coins/hd_wallet/mod.rs b/mm2src/coins/hd_wallet/mod.rs index c49f0a4fc1..a0941b991c 100644 --- a/mm2src/coins/hd_wallet/mod.rs +++ b/mm2src/coins/hd_wallet/mod.rs @@ -7,7 +7,7 @@ use futures::lock::{MappedMutexGuard as AsyncMappedMutexGuard, Mutex as AsyncMut use mm2_err_handle::prelude::*; use primitives::hash::H160; use serde::Serialize; -use std::collections::{BTreeMap, HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap}; use std::fmt::Display; use std::hash::Hash; use std::str::FromStr; @@ -242,46 +242,6 @@ where } } -/// Derives the account XPubs for account IDs fetched from the database. -/// -/// This method needs the `GlobalHDAccountArc` to derive the XPubs. -pub async fn get_xpubs_for_account_ids( - hd_wallet_storage: &HDWalletCoinStorage, - derivation_path: &HDPathToCoin, - global_hd_ctx: &GlobalHDAccountArc, -) -> HDWalletStorageResult>> { - let account_ids = hd_wallet_storage.get_all_account_ids().await?; - let coin_der_path = derivation_path.to_derivation_path(); - // Map account IDs to account XPubs to use these XPubs - Ok(account_ids - .into_iter() - .filter_map(|acc_id| { - let mut account_der_path = coin_der_path.clone(); - let child_number = match ChildNumber::new(acc_id, true) { - Ok(num) => num, - Err(e) => { - error!( - "Failed to create ChildNumber for account_id={} found in storage: {}", - acc_id, e - ); - return None; - }, - }; - account_der_path.push(child_number); - match global_hd_ctx.derive_secp256k1_xpub(&account_der_path) { - Ok(xpub) => Some(xpub), - Err(e) => { - error!( - "Failed to derive xpub for account_id={} found in storage: {}", - acc_id, e - ); - None - }, - } - }) - .collect()) -} - pub async fn load_hd_accounts_from_storage( hd_wallet_storage: &HDWalletCoinStorage, derivation_path: &HDPathToCoin, @@ -310,32 +270,50 @@ where } } -pub async fn load_all_hd_accounts_from_storage_with_xpubs( +pub async fn load_hd_accounts_from_storage_with_matching_xpubs( hd_wallet_storage: &HDWalletCoinStorage, derivation_path: &HDPathToCoin, - xpubs: Vec, -) -> HDWalletStorageResult>> + global_hd_ctx: &GlobalHDAccountArc, +) -> HDWalletStorageResult>>> where HDAddress: HDAddressOps + Send, - ExtendedPublicKey: ExtendedPublicKeyOps, - ::Err: Display, { - if xpubs.is_empty() { - return Ok(HDAccountsMap::new()); - } - let xpubs = xpubs - .into_iter() - .map(|xpub| { - // Convert the xpub to a string with the expected prefix. - xpub.to_string(bip32::Prefix::XPUB) - }) - .collect::>(); + let coin_der_path = derivation_path.to_derivation_path(); let accounts = hd_wallet_storage.load_all_accounts().await?; - // Filter in only accounts that match the expected xpubs. + // Filter accounts that when derived from the current HD wallet, they yield the correct xpub. + // If they don't yield the correct xpub, this means that account belong to another HD wallet with + // different purpose'/coin_type' fields but same account_id' field. This happens when someone changes + // the purpose'/coin_type' fields in the coins cnofig without changing the ticker. + // TODO: This is a temporary solution that fixes the issue. But we might wanna re-design how accounts + // are loaded from the database (loading only the single account the user asked for and not all accounts). + // FIXME: What about collecting unmatching xpubs and deleting them. So that they never clutter the space of + // the new HD wallet associated with this coin ticker. let accounts = accounts.into_iter().filter(|account| { - if !xpubs.contains(&account.account_xpub) { - warn!( + let mut account_der_path = coin_der_path.clone(); + let child_number = match ChildNumber::new(account.account_id, true) { + Ok(num) => num, + Err(e) => { + error!( + "Failed to create ChildNumber for account_id={} found in storage: {}", + account.account_id, e + ); + return false; + }, + }; + account_der_path.push(child_number); + let derived_xpub = match global_hd_ctx.derive_secp256k1_xpub(&account_der_path) { + Ok(xpub) => xpub.to_string(bip32::Prefix::XPUB), + Err(e) => { + error!( + "Failed to derive xpub for account_id={} found in storage: {}", + account.account_id, e + ); + return false; + }, + }; + if derived_xpub != account.account_xpub { + error!( "Account with id={} has an xpub={} that doesn't match the one derived from the current HD wallet. Skipping.", account.account_id, account.account_xpub ); @@ -344,7 +322,7 @@ where true }); - let res: HDWalletStorageResult>> = accounts + let res: HDWalletStorageResult<_> = accounts .map(|account_info| { let account = HDAccount::try_from_storage_item(derivation_path, &account_info)?; Ok((account.account_id, account)) diff --git a/mm2src/coins/hd_wallet/storage/mock_storage.rs b/mm2src/coins/hd_wallet/storage/mock_storage.rs index 802dbd0404..a7b3671527 100644 --- a/mm2src/coins/hd_wallet/storage/mock_storage.rs +++ b/mm2src/coins/hd_wallet/storage/mock_storage.rs @@ -16,8 +16,6 @@ impl HDWalletStorageInternalOps for HDWalletMockStorage { unimplemented!() } - async fn load_account_ids(&self, _wallet_id: HDWalletId) -> HDWalletStorageResult> { unimplemented!() } - async fn load_accounts(&self, _wallet_id: HDWalletId) -> HDWalletStorageResult> { unimplemented!() } diff --git a/mm2src/coins/hd_wallet/storage/mod.rs b/mm2src/coins/hd_wallet/storage/mod.rs index cf512d202c..514c2fb639 100644 --- a/mm2src/coins/hd_wallet/storage/mod.rs +++ b/mm2src/coins/hd_wallet/storage/mod.rs @@ -96,8 +96,6 @@ pub(crate) trait HDWalletStorageInternalOps { where Self: Sized; - async fn load_account_ids(&self, wallet_id: HDWalletId) -> HDWalletStorageResult>; - async fn load_accounts(&self, wallet_id: HDWalletId) -> HDWalletStorageResult>; async fn update_external_addresses_number( @@ -239,11 +237,6 @@ impl HDWalletCoinStorage { pub fn wallet_id(&self) -> HDWalletId { HDWalletId::new(self.coin.clone(), &self.hd_wallet_rmd160) } - pub async fn get_all_account_ids(&self) -> HDWalletStorageResult> { - let wallet_id = self.wallet_id(); - self.inner.load_account_ids(wallet_id).await - } - pub async fn load_all_accounts(&self) -> HDWalletStorageResult> { let wallet_id = self.wallet_id(); self.inner.load_accounts(wallet_id).await diff --git a/mm2src/coins/hd_wallet/storage/sqlite_storage.rs b/mm2src/coins/hd_wallet/storage/sqlite_storage.rs index de503becb7..2a2f219b8f 100644 --- a/mm2src/coins/hd_wallet/storage/sqlite_storage.rs +++ b/mm2src/coins/hd_wallet/storage/sqlite_storage.rs @@ -35,9 +35,6 @@ const SELECT_ACCOUNTS_BY_WALLET_ID: &str = FROM hd_account WHERE coin=:coin AND hd_wallet_rmd160=:hd_wallet_rmd160;"; -const SELECT_ACCOUNT_IDS_BY_WALLET_ID: &str = - "SELECT account_id FROM hd_account WHERE coin=:coin AND hd_wallet_rmd160=:hd_wallet_rmd160;"; - impl From for HDWalletStorageError { fn from(e: SqlError) -> Self { let error = e.to_string(); @@ -110,22 +107,6 @@ impl HDWalletStorageInternalOps for HDWalletSqliteStorage { Ok(storage) } - async fn load_account_ids(&self, wallet_id: HDWalletId) -> HDWalletStorageResult> { - let selfi = self.clone(); - async_blocking(move || { - let conn_shared = selfi.get_shared_conn()?; - let conn = Self::lock_conn_mutex(&conn_shared)?; - - let mut statement = conn.prepare(SELECT_ACCOUNT_IDS_BY_WALLET_ID)?; - let params = wallet_id.to_sql_params(); - let rows = statement - .query_map_named(¶ms.as_sql_named_params(), |row: &Row<'_>| row.get(0))? - .collect::, _>>()?; - Ok(rows) - }) - .await - } - async fn load_accounts(&self, wallet_id: HDWalletId) -> HDWalletStorageResult> { let selfi = self.clone(); async_blocking(move || { diff --git a/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs b/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs index cea252a5da..7f4989f733 100644 --- a/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs +++ b/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs @@ -1,6 +1,6 @@ -use crate::hd_wallet::{get_xpubs_for_account_ids, load_all_hd_accounts_from_storage_with_xpubs, - load_hd_accounts_from_storage, HDAccountsMutex, HDWallet, HDWalletCoinStorage, HDWalletOps, - HDWalletStorageError, DEFAULT_GAP_LIMIT}; +use crate::hd_wallet::{load_hd_accounts_from_storage, load_hd_accounts_from_storage_with_matching_xpubs, + HDAccountsMutex, HDWallet, HDWalletCoinStorage, HDWalletOps, HDWalletStorageError, + DEFAULT_GAP_LIMIT}; use crate::utxo::rpc_clients::{ElectrumClient, ElectrumClientSettings, ElectrumConnectionSettings, EstimateFeeMethod, UtxoRpcClientEnum}; use crate::utxo::tx_cache::{UtxoVerboseCacheOps, UtxoVerboseCacheShared}; @@ -204,12 +204,10 @@ pub trait UtxoFieldsWithGlobalHDBuilder: UtxoCoinBuilderCommonOps { let hd_wallet_rmd160 = *self.ctx().rmd160(); let hd_wallet_storage = HDWalletCoinStorage::init_with_rmd160(self.ctx(), self.ticker().to_owned(), hd_wallet_rmd160).await?; - let xpub = get_xpubs_for_account_ids(&hd_wallet_storage, path_to_coin, &global_hd_ctx) - .await - .mm_err(UtxoCoinBuildError::from)?; - let accounts = load_all_hd_accounts_from_storage_with_xpubs(&hd_wallet_storage, path_to_coin, xpub) - .await - .mm_err(UtxoCoinBuildError::from)?; + let accounts = + load_hd_accounts_from_storage_with_matching_xpubs(&hd_wallet_storage, path_to_coin, &global_hd_ctx) + .await + .mm_err(UtxoCoinBuildError::from)?; let gap_limit = self.gap_limit(); let hd_wallet = UtxoHDWallet { inner: HDWallet { From 46197d9c57a23a16f02f8fb9ec15e85c48fa92eb Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Thu, 5 Jun 2025 17:46:08 +0200 Subject: [PATCH 12/24] fix wrongfully loaded hd accounts for trezor the fix here relies pretty much on the same technique we did for HD wallets. by only loading the the accounts that has matching xpubs. except that we don't really do that: we can't ask trezor to just derive all the xpubs for all the account_ids we have. this is a very bad ux for the user and they might sus us this way. so the solution right now (with the current hd_account DB design and our view of how HD accounts work) is to just load a single xpub that the user asking for. this happens as if the account isn't created everytime (because it's not loaded anymore) and eventually trezor will hit `create_new_account` which will derive the xpub and insert the account in the DB. when inserting the account in the DB now we account for if the account already existed and not error. the downside to this technique is obviously that the user will not be able to view other accounts until they try to create them (and their trezor will be asked to derive the xpub for that account_id). essentially, the auto work that we did for the HD wallet case is now done via the trezor device and relies on the user keeping creating the accounts they need to view. we can keep it this way till we rework hd_account DB design and include the full derivation path in it --- mm2src/coins/eth/v2_activation.rs | 10 +++---- mm2src/coins/hd_wallet/mod.rs | 28 ------------------- .../coins/hd_wallet/storage/sqlite_storage.rs | 3 +- .../utxo/utxo_builder/utxo_coin_builder.rs | 11 +++----- 4 files changed, 10 insertions(+), 42 deletions(-) diff --git a/mm2src/coins/eth/v2_activation.rs b/mm2src/coins/eth/v2_activation.rs index be7b114727..a18d08b39e 100644 --- a/mm2src/coins/eth/v2_activation.rs +++ b/mm2src/coins/eth/v2_activation.rs @@ -1,9 +1,8 @@ use super::*; use crate::eth::erc20::{get_enabled_erc20_by_platform_and_contract, get_token_decimals}; use crate::eth::web3_transport::http_transport::HttpTransport; -use crate::hd_wallet::{load_hd_accounts_from_storage, load_hd_accounts_from_storage_with_matching_xpubs, - HDAccountsMutex, HDPathAccountToAddressId, HDWalletCoinStorage, HDWalletStorageError, - DEFAULT_GAP_LIMIT}; +use crate::hd_wallet::{load_hd_accounts_from_storage_with_matching_xpubs, HDAccountsMutex, HDPathAccountToAddressId, + HDWalletCoinStorage, HDWalletStorageError, DEFAULT_GAP_LIMIT}; use crate::nft::get_nfts_for_activation; use crate::nft::nft_errors::{GetNftInfoError, ParseChainTypeError}; use crate::nft::nft_structs::Chain; @@ -802,13 +801,12 @@ pub(crate) async fn build_address_and_priv_key_policy( let hd_wallet_storage = HDWalletCoinStorage::init_with_rmd160(ctx, ticker.to_string(), hd_wallet_rmd160) .await .mm_err(EthActivationV2Error::from)?; - let accounts = load_hd_accounts_from_storage(&hd_wallet_storage, &path_to_coin).await?; let gap_limit = gap_limit.unwrap_or(DEFAULT_GAP_LIMIT); let hd_wallet = EthHDWallet { hd_wallet_rmd160, hd_wallet_storage, - derivation_path: path_to_coin.clone(), - accounts: HDAccountsMutex::new(accounts), + derivation_path: path_to_coin, + accounts: HDAccountsMutex::new(Default::default()), enabled_address: *path_to_address, gap_limit, }; diff --git a/mm2src/coins/hd_wallet/mod.rs b/mm2src/coins/hd_wallet/mod.rs index a0941b991c..72dd62e0a9 100644 --- a/mm2src/coins/hd_wallet/mod.rs +++ b/mm2src/coins/hd_wallet/mod.rs @@ -242,34 +242,6 @@ where } } -pub async fn load_hd_accounts_from_storage( - hd_wallet_storage: &HDWalletCoinStorage, - derivation_path: &HDPathToCoin, -) -> HDWalletStorageResult>> -where - HDAddress: HDAddressOps + Send, - ExtendedPublicKey: ExtendedPublicKeyOps, - ::Err: Display, -{ - let accounts = hd_wallet_storage.load_all_accounts().await?; - let res: HDWalletStorageResult>> = accounts - .iter() - .map(|account_info| { - let account = HDAccount::try_from_storage_item(derivation_path, account_info)?; - Ok((account.account_id, account)) - }) - .collect(); - match res { - Ok(accounts) => Ok(accounts), - Err(e) if e.get_inner().is_deserializing_err() => { - warn!("Error loading HD accounts from the storage: '{}'. Clear accounts", e); - hd_wallet_storage.clear_accounts().await?; - Ok(HDAccountsMap::new()) - }, - Err(e) => Err(e), - } -} - pub async fn load_hd_accounts_from_storage_with_matching_xpubs( hd_wallet_storage: &HDWalletCoinStorage, derivation_path: &HDPathToCoin, diff --git a/mm2src/coins/hd_wallet/storage/sqlite_storage.rs b/mm2src/coins/hd_wallet/storage/sqlite_storage.rs index 2a2f219b8f..ed1563da06 100644 --- a/mm2src/coins/hd_wallet/storage/sqlite_storage.rs +++ b/mm2src/coins/hd_wallet/storage/sqlite_storage.rs @@ -25,7 +25,8 @@ const CREATE_HD_ACCOUNT_TABLE: &str = "CREATE TABLE IF NOT EXISTS hd_account ( const INSERT_ACCOUNT: &str = "INSERT INTO hd_account (coin, hd_wallet_rmd160, account_id, account_xpub, external_addresses_number, internal_addresses_number) - VALUES (:coin, :hd_wallet_rmd160, :account_id, :account_xpub, :external_addresses_number, :internal_addresses_number);"; + SELECT :coin, :hd_wallet_rmd160, :account_id, :account_xpub, :external_addresses_number, :internal_addresses_number + WHERE NOT EXISTS (SELECT 1 FROM hd_account WHERE coin=:coin AND hd_wallet_rmd160=:hd_wallet_rmd160 AND account_xpub=:account_xpub);"; const DELETE_ACCOUNTS_BY_WALLET_ID: &str = "DELETE FROM hd_account WHERE coin=:coin AND hd_wallet_rmd160=:hd_wallet_rmd160;"; diff --git a/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs b/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs index 7f4989f733..a2e922aabb 100644 --- a/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs +++ b/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs @@ -1,6 +1,5 @@ -use crate::hd_wallet::{load_hd_accounts_from_storage, load_hd_accounts_from_storage_with_matching_xpubs, - HDAccountsMutex, HDWallet, HDWalletCoinStorage, HDWalletOps, HDWalletStorageError, - DEFAULT_GAP_LIMIT}; +use crate::hd_wallet::{load_hd_accounts_from_storage_with_matching_xpubs, HDAccountsMutex, HDWallet, + HDWalletCoinStorage, HDWalletOps, HDWalletStorageError, DEFAULT_GAP_LIMIT}; use crate::utxo::rpc_clients::{ElectrumClient, ElectrumClientSettings, ElectrumConnectionSettings, EstimateFeeMethod, UtxoRpcClientEnum}; use crate::utxo::tx_cache::{UtxoVerboseCacheOps, UtxoVerboseCacheShared}; @@ -307,16 +306,14 @@ pub trait UtxoFieldsWithHardwareWalletBuilder: UtxoCoinBuilderCommonOps { let hd_wallet_storage = HDWalletCoinStorage::init(self.ctx(), ticker).await?; - let accounts = load_hd_accounts_from_storage(&hd_wallet_storage, &path_to_coin) - .await - .mm_err(UtxoCoinBuildError::from)?; let gap_limit = self.gap_limit(); let hd_wallet = UtxoHDWallet { inner: HDWallet { hd_wallet_rmd160, hd_wallet_storage, derivation_path: path_to_coin, - accounts: HDAccountsMutex::new(accounts), + // FIXME: With this set to no accounts, now surely the fixme below holds and will fail. + accounts: HDAccountsMutex::new(Default::default()), enabled_address: self.activation_params().path_to_address, gap_limit, }, From 387f20f2a15745ac91d297a25310583f6190f495 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Thu, 5 Jun 2025 20:36:30 +0200 Subject: [PATCH 13/24] reactivate the failing test --- mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs b/mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs index 7dfba3395f..e67e8d0091 100644 --- a/mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs +++ b/mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs @@ -5311,9 +5311,7 @@ fn test_sign_verify_message_segwit_with_bip84_derivation_path() { assert!(!verify_response.result.is_valid, "Cross-verification should fail"); } -///NOTE: Should not fail after [issue #2470](https://github.com/KomodoPlatform/komodo-defi-framework/issues/2470) is resolved. #[test] -#[ignore] #[cfg(not(target_arch = "wasm32"))] fn test_hd_address_conflict_across_derivation_paths() { const PASSPHRASE: &str = "tank abandon bind salon remove wisdom net size aspect direct source fossil"; @@ -5335,6 +5333,7 @@ fn test_hd_address_conflict_across_derivation_paths() { let mut conf = Mm2TestConf::seednode_with_hd_account(PASSPHRASE, &coins); let mm_hd = MarketMakerIt::start(conf.conf.clone(), conf.rpc_password.clone(), None).unwrap(); + log!("First kdf log path: {}", mm_hd.log_path.display()); let path_to_address = HDAccountAddressId { account_id: 0, @@ -5357,7 +5356,6 @@ fn test_hd_address_conflict_across_derivation_paths() { log!("Old address: {}", old_address); // Shutdown MM and restart RICK with derivation path m/84'/141' - log!("Conf log path: {}", mm_hd.log_path.display()); conf.conf["dbdir"] = mm_hd.folder.join("DB").to_str().unwrap().into(); block_on(mm_hd.stop()).unwrap(); @@ -5376,6 +5374,7 @@ fn test_hd_address_conflict_across_derivation_paths() { }); conf.conf["coins"] = json!([coin]); let mm_hd = MarketMakerIt::start(conf.conf, conf.rpc_password, None).unwrap(); + log!("Second kdf log path: {}", mm_hd.log_path.display()); // Re-enable RICK, but it will try to reuse address0 stored under old path(m/49'/141') let rick_2 = block_on(enable_utxo_v2_electrum( @@ -5393,17 +5392,11 @@ fn test_hd_address_conflict_across_derivation_paths() { log!("New address: {}", new_address); // KDF has a bug and reuses the same account (and thus the same address) for derivation paths that use different `m/purpose'/coin'` fields. - // This stems from the fact that KDF doesn't differentiate/store the "purpose" & "coin" derivation fields in the database, but it rather stores the whole xpub - // that repsresents `m/purpose'/coin'/account_id'` - // Now, when KDF queries the database for already stored accounts, it specifies the specifies `COIN=ticker` in the SQL query, and since - // we badly mutated the conf by changing the derivation path but not the coin ticker, it returns accounts belonging to the old coin ticker (old derivation path). - // This wouldn't have happened if we gave the conf with `m/84'/141'` ticker="RICK-segwit" and `m/49'/141'` ticker="RICK-legacy", but we don't do that. - assert_ne!( old_address, new_address, "Address from old derivation path(m/49'/141') should not match address from new derivation path(m/84'/141')" From d4a3766a4adac5a3f6827c5367589a0ce478d73c Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Sat, 7 Jun 2025 10:51:03 +0200 Subject: [PATCH 14/24] add another fixme regarding coin balance result for HD wallets --- mm2src/coins/coin_balance.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/mm2src/coins/coin_balance.rs b/mm2src/coins/coin_balance.rs index 3ec047ee80..6ae86aa158 100644 --- a/mm2src/coins/coin_balance.rs +++ b/mm2src/coins/coin_balance.rs @@ -480,6 +480,8 @@ pub mod common_impl { accounts: Vec::with_capacity(accounts.len() + 1), }; + // FIXME: Here if this specific requested account was not found, we create it and `enable_hd_account` for it. But other accounts + // are not taken into account. Note that this check in the preceding commit was `accounts.is_empty()`. if accounts.get(&path_to_address.account_id).is_none() { // Is seems that we couldn't find any HD account from the HD wallet storage. drop(accounts); From 68d0edaaf2b3fc05ba86a1e73882178c4895f2d7 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Wed, 25 Jun 2025 10:03:53 +0200 Subject: [PATCH 15/24] Revert "shouldn't we rollback tasks the failed running?" This reverts commit bdf2f7464aedfe5aa0097bfda03897e1cdff8a3a. --- mm2src/coins_activation/src/utxo_activation/common_impl.rs | 1 - mm2src/rpc_task/src/manager.rs | 1 - 2 files changed, 2 deletions(-) diff --git a/mm2src/coins_activation/src/utxo_activation/common_impl.rs b/mm2src/coins_activation/src/utxo_activation/common_impl.rs index 8906c3f4c1..eba196c370 100644 --- a/mm2src/coins_activation/src/utxo_activation/common_impl.rs +++ b/mm2src/coins_activation/src/utxo_activation/common_impl.rs @@ -56,7 +56,6 @@ where None }; task_handle.update_in_progress_status(UtxoStandardInProgressStatus::RequestingWalletBalance)?; - // .e.g: This call will error if the user puts an account number >= `1 << 31`. let wallet_balance = coin .enable_coin_balance( xpub_extractor, diff --git a/mm2src/rpc_task/src/manager.rs b/mm2src/rpc_task/src/manager.rs index 29a7b29d0e..b5c43a04b6 100644 --- a/mm2src/rpc_task/src/manager.rs +++ b/mm2src/rpc_task/src/manager.rs @@ -72,7 +72,6 @@ impl RpcTaskManager { // `task.run(&task_handle)`. match task_result { Some(task_result) => { - // FIXME: If the result is an error, shouldn't we cancel the task at this point? (check the e.g. in the same commit). debug!("RPC task '{}' has been finished", task_id); task_handle.finish(task_result); }, From 77c6320d5b3a82b3041af219da672dedd3c469a1 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Wed, 25 Jun 2025 10:09:00 +0200 Subject: [PATCH 16/24] leave a not linking to this PR that broke the generic-ability of hd_account loading --- mm2src/coins/hd_wallet/mod.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/mm2src/coins/hd_wallet/mod.rs b/mm2src/coins/hd_wallet/mod.rs index 72dd62e0a9..4efa7bd459 100644 --- a/mm2src/coins/hd_wallet/mod.rs +++ b/mm2src/coins/hd_wallet/mod.rs @@ -242,6 +242,9 @@ where } } +// TODO: Ideally this should be general and not specific for secp256k1 keys only. i.e. we should have this function work with different types +// of extended public and private keys (like Solana's and Sia's ed25519 keys). +// This was generic in the past, but changed here: https://github.com/KomodoPlatform/komodo-defi-framework/pull/2482/ pub async fn load_hd_accounts_from_storage_with_matching_xpubs( hd_wallet_storage: &HDWalletCoinStorage, derivation_path: &HDPathToCoin, From d5bfc9799f6fb40521ca80dccf86bff65fc1902a Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Wed, 25 Jun 2025 10:10:18 +0200 Subject: [PATCH 17/24] remove 'with_matching_xpubs' from func name since that's way too specific --- mm2src/coins/eth/v2_activation.rs | 8 +++----- mm2src/coins/hd_wallet/mod.rs | 2 +- mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs | 11 +++++------ 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/mm2src/coins/eth/v2_activation.rs b/mm2src/coins/eth/v2_activation.rs index a18d08b39e..8ca18cd9f0 100644 --- a/mm2src/coins/eth/v2_activation.rs +++ b/mm2src/coins/eth/v2_activation.rs @@ -1,8 +1,8 @@ use super::*; use crate::eth::erc20::{get_enabled_erc20_by_platform_and_contract, get_token_decimals}; use crate::eth::web3_transport::http_transport::HttpTransport; -use crate::hd_wallet::{load_hd_accounts_from_storage_with_matching_xpubs, HDAccountsMutex, HDPathAccountToAddressId, - HDWalletCoinStorage, HDWalletStorageError, DEFAULT_GAP_LIMIT}; +use crate::hd_wallet::{load_hd_accounts_from_storage, HDAccountsMutex, HDPathAccountToAddressId, HDWalletCoinStorage, + HDWalletStorageError, DEFAULT_GAP_LIMIT}; use crate::nft::get_nfts_for_activation; use crate::nft::nft_errors::{GetNftInfoError, ParseChainTypeError}; use crate::nft::nft_structs::Chain; @@ -763,9 +763,7 @@ pub(crate) async fn build_address_and_priv_key_policy( let hd_wallet_storage = HDWalletCoinStorage::init_with_rmd160(ctx, ticker.to_string(), hd_wallet_rmd160) .await .mm_err(EthActivationV2Error::from)?; - let accounts = - load_hd_accounts_from_storage_with_matching_xpubs(&hd_wallet_storage, &path_to_coin, &global_hd_ctx) - .await?; + let accounts = load_hd_accounts_from_storage(&hd_wallet_storage, &path_to_coin, &global_hd_ctx).await?; let gap_limit = gap_limit.unwrap_or(DEFAULT_GAP_LIMIT); let hd_wallet = EthHDWallet { hd_wallet_rmd160, diff --git a/mm2src/coins/hd_wallet/mod.rs b/mm2src/coins/hd_wallet/mod.rs index 4efa7bd459..5e5437a1d3 100644 --- a/mm2src/coins/hd_wallet/mod.rs +++ b/mm2src/coins/hd_wallet/mod.rs @@ -245,7 +245,7 @@ where // TODO: Ideally this should be general and not specific for secp256k1 keys only. i.e. we should have this function work with different types // of extended public and private keys (like Solana's and Sia's ed25519 keys). // This was generic in the past, but changed here: https://github.com/KomodoPlatform/komodo-defi-framework/pull/2482/ -pub async fn load_hd_accounts_from_storage_with_matching_xpubs( +pub async fn load_hd_accounts_from_storage( hd_wallet_storage: &HDWalletCoinStorage, derivation_path: &HDPathToCoin, global_hd_ctx: &GlobalHDAccountArc, diff --git a/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs b/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs index a2e922aabb..e604ceaf58 100644 --- a/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs +++ b/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs @@ -1,5 +1,5 @@ -use crate::hd_wallet::{load_hd_accounts_from_storage_with_matching_xpubs, HDAccountsMutex, HDWallet, - HDWalletCoinStorage, HDWalletOps, HDWalletStorageError, DEFAULT_GAP_LIMIT}; +use crate::hd_wallet::{load_hd_accounts_from_storage, HDAccountsMutex, HDWallet, HDWalletCoinStorage, HDWalletOps, + HDWalletStorageError, DEFAULT_GAP_LIMIT}; use crate::utxo::rpc_clients::{ElectrumClient, ElectrumClientSettings, ElectrumConnectionSettings, EstimateFeeMethod, UtxoRpcClientEnum}; use crate::utxo::tx_cache::{UtxoVerboseCacheOps, UtxoVerboseCacheShared}; @@ -203,10 +203,9 @@ pub trait UtxoFieldsWithGlobalHDBuilder: UtxoCoinBuilderCommonOps { let hd_wallet_rmd160 = *self.ctx().rmd160(); let hd_wallet_storage = HDWalletCoinStorage::init_with_rmd160(self.ctx(), self.ticker().to_owned(), hd_wallet_rmd160).await?; - let accounts = - load_hd_accounts_from_storage_with_matching_xpubs(&hd_wallet_storage, path_to_coin, &global_hd_ctx) - .await - .mm_err(UtxoCoinBuildError::from)?; + let accounts = load_hd_accounts_from_storage(&hd_wallet_storage, path_to_coin, &global_hd_ctx) + .await + .mm_err(UtxoCoinBuildError::from)?; let gap_limit = self.gap_limit(); let hd_wallet = UtxoHDWallet { inner: HDWallet { From 062a268b25f17f4f7ca0f57ea9f0b1b028aaecea Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Wed, 25 Jun 2025 13:32:03 +0200 Subject: [PATCH 18/24] remove corruprt xpubs found while loading hd accounts --- mm2src/coins/hd_wallet/mod.rs | 14 +++++++-- .../coins/hd_wallet/storage/mock_storage.rs | 4 +++ mm2src/coins/hd_wallet/storage/mod.rs | 7 +++++ .../coins/hd_wallet/storage/sqlite_storage.rs | 30 ++++++++++++++++++- .../coins/hd_wallet/storage/wasm_storage.rs | 17 +++++++++++ 5 files changed, 69 insertions(+), 3 deletions(-) diff --git a/mm2src/coins/hd_wallet/mod.rs b/mm2src/coins/hd_wallet/mod.rs index 5e5437a1d3..3b72dd9e2f 100644 --- a/mm2src/coins/hd_wallet/mod.rs +++ b/mm2src/coins/hd_wallet/mod.rs @@ -1,6 +1,6 @@ use async_trait::async_trait; use bip32::ExtendedPublicKey; -use common::log::{error, warn}; +use common::log::{error, warn, LogOnError}; use crypto::{Bip32DerPathOps, Bip32Error, Bip44Chain, ChildNumber, DerivationPath, GlobalHDAccountArc, HDPathToAccount, HDPathToCoin, Secp256k1ExtendedPublicKey, StandardHDPath, StandardHDPathError}; use futures::lock::{MappedMutexGuard as AsyncMappedMutexGuard, Mutex as AsyncMutex, MutexGuard as AsyncMutexGuard}; @@ -264,6 +264,7 @@ where // are loaded from the database (loading only the single account the user asked for and not all accounts). // FIXME: What about collecting unmatching xpubs and deleting them. So that they never clutter the space of // the new HD wallet associated with this coin ticker. + let mut bad_accounts = Vec::new(); let accounts = accounts.into_iter().filter(|account| { let mut account_der_path = coin_der_path.clone(); let child_number = match ChildNumber::new(account.account_id, true) { @@ -292,6 +293,7 @@ where "Account with id={} has an xpub={} that doesn't match the one derived from the current HD wallet. Skipping.", account.account_id, account.account_xpub ); + bad_accounts.push(account.account_xpub.clone()); return false; } true @@ -303,8 +305,16 @@ where Ok((account.account_id, account)) }) .collect(); + match res { - Ok(accounts) => Ok(accounts), + Ok(accounts) => { + // If there are any bad accounts, delete them from the storage. + if !bad_accounts.is_empty() { + // Ignore and log errors that occur during the deletion of bad accounts. + hd_wallet_storage.delete_accounts(bad_accounts).await.error_log(); + } + Ok(accounts) + }, Err(e) if e.get_inner().is_deserializing_err() => { warn!("Error loading HD accounts from the storage: '{}'. Clear accounts", e); hd_wallet_storage.clear_accounts().await?; diff --git a/mm2src/coins/hd_wallet/storage/mock_storage.rs b/mm2src/coins/hd_wallet/storage/mock_storage.rs index a7b3671527..f3340640d5 100644 --- a/mm2src/coins/hd_wallet/storage/mock_storage.rs +++ b/mm2src/coins/hd_wallet/storage/mock_storage.rs @@ -46,5 +46,9 @@ impl HDWalletStorageInternalOps for HDWalletMockStorage { unimplemented!() } + async fn delete_accounts(&self, _wallet_id: HDWalletId, _account_xpubs: Vec) -> HDWalletStorageResult<()> { + unimplemented!() + } + async fn clear_accounts(&self, _wallet_id: HDWalletId) -> HDWalletStorageResult<()> { unimplemented!() } } diff --git a/mm2src/coins/hd_wallet/storage/mod.rs b/mm2src/coins/hd_wallet/storage/mod.rs index 514c2fb639..72e20237e9 100644 --- a/mm2src/coins/hd_wallet/storage/mod.rs +++ b/mm2src/coins/hd_wallet/storage/mod.rs @@ -118,6 +118,8 @@ pub(crate) trait HDWalletStorageInternalOps { account: HDAccountStorageItem, ) -> HDWalletStorageResult<()>; + async fn delete_accounts(&self, wallet_id: HDWalletId, accounts_xpubs: Vec) -> HDWalletStorageResult<()>; + async fn clear_accounts(&self, wallet_id: HDWalletId) -> HDWalletStorageResult<()>; } @@ -269,6 +271,11 @@ impl HDWalletCoinStorage { self.inner.upload_new_account(wallet_id, account_info).await } + pub async fn delete_accounts(&self, accounts_xpubs: Vec) -> HDWalletStorageResult<()> { + let wallet_id = self.wallet_id(); + self.inner.delete_accounts(wallet_id, accounts_xpubs).await + } + pub async fn clear_accounts(&self) -> HDWalletStorageResult<()> { let wallet_id = self.wallet_id(); self.inner.clear_accounts(wallet_id).await diff --git a/mm2src/coins/hd_wallet/storage/sqlite_storage.rs b/mm2src/coins/hd_wallet/storage/sqlite_storage.rs index ed1563da06..d372c2c9a8 100644 --- a/mm2src/coins/hd_wallet/storage/sqlite_storage.rs +++ b/mm2src/coins/hd_wallet/storage/sqlite_storage.rs @@ -6,7 +6,7 @@ use async_trait::async_trait; use common::async_blocking; use crypto::XPub; use db_common::owned_named_params; -use db_common::sqlite::rusqlite::{Connection, Error as SqlError, Row}; +use db_common::sqlite::rusqlite::{named_params, Connection, Error as SqlError, Row}; use db_common::sqlite::{AsSqlNamedParams, OwnedSqlNamedParams, SqliteConnShared, SqliteConnWeak}; use derive_more::Display; use mm2_core::mm_ctx::MmArc; @@ -31,6 +31,9 @@ const INSERT_ACCOUNT: &str = "INSERT INTO hd_account const DELETE_ACCOUNTS_BY_WALLET_ID: &str = "DELETE FROM hd_account WHERE coin=:coin AND hd_wallet_rmd160=:hd_wallet_rmd160;"; +const DELETE_ACCOUNT_BY_XPUB: &str = + "DELETE FROM hd_account WHERE coin=:coin AND hd_wallet_rmd160=:hd_wallet_rmd160 AND account_xpub=:account_xpub;"; + const SELECT_ACCOUNTS_BY_WALLET_ID: &str = "SELECT account_id, account_xpub, external_addresses_number, internal_addresses_number FROM hd_account @@ -175,6 +178,31 @@ impl HDWalletStorageInternalOps for HDWalletSqliteStorage { .await } + async fn delete_accounts(&self, wallet_id: HDWalletId, account_xpubs: Vec) -> HDWalletStorageResult<()> { + let selfi = self.clone(); + async_blocking(move || { + let conn_shared = selfi.get_shared_conn()?; + let mut conn = Self::lock_conn_mutex(&conn_shared)?; + let tx = conn + .transaction() + .map_to_mm(|e| HDWalletStorageError::Internal(format!("Error starting transaction: {}", e)))?; + + for account_xpub in account_xpubs { + let params = named_params! { + ":coin": wallet_id.coin.clone(), + ":hd_wallet_rmd160": wallet_id.hd_wallet_rmd160.clone(), + ":account_xpub": account_xpub, + }; + tx.execute(DELETE_ACCOUNT_BY_XPUB, params) + .map_to_mm(HDWalletStorageError::from)?; + } + tx.commit() + .map_to_mm(|e| HDWalletStorageError::Internal(format!("Error committing transaction: {}", e)))?; + Ok(()) + }) + .await + } + async fn clear_accounts(&self, wallet_id: HDWalletId) -> HDWalletStorageResult<()> { let selfi = self.clone(); async_blocking(move || { diff --git a/mm2src/coins/hd_wallet/storage/wasm_storage.rs b/mm2src/coins/hd_wallet/storage/wasm_storage.rs index d0c20a61ec..144c341ef3 100644 --- a/mm2src/coins/hd_wallet/storage/wasm_storage.rs +++ b/mm2src/coins/hd_wallet/storage/wasm_storage.rs @@ -254,6 +254,23 @@ impl HDWalletStorageInternalOps for HDWalletIndexedDbStorage { .mm_err(HDWalletStorageError::from) } + async fn delete_accounts(&self, wallet_id: HDWalletId, account_xpubs: Vec) -> HDWalletStorageResult<()> { + let shared_db = self.get_shared_db()?; + let locked_db = Self::lock_db_mutex(&shared_db).await?; + + let transaction = locked_db.inner.transaction().await?; + let table = transaction.table::().await?; + + for account_xpub in account_xpubs { + let index_keys = MultiIndex::new(WALLET_ACCOUNT_XPUB_INDEX) + .with_value(wallet_id.coin.clone())? + .with_value(wallet_id.hd_wallet_rmd160.clone())? + .with_value(account_xpub)?; + table.delete_item_by_unique_multi_index(index_keys).await?; + } + Ok(()) + } + async fn clear_accounts(&self, wallet_id: HDWalletId) -> HDWalletStorageResult<()> { let shared_db = self.get_shared_db()?; let locked_db = Self::lock_db_mutex(&shared_db).await?; From 9d97f4d1bc935ddc3464cdf0850669ebbdfc8d06 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Wed, 25 Jun 2025 13:33:44 +0200 Subject: [PATCH 19/24] rename the old multi index to deprecated_wall... this makes it easy to read the on_upgrade_needed function without confusion --- mm2src/coins/hd_wallet/storage/wasm_storage.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mm2src/coins/hd_wallet/storage/wasm_storage.rs b/mm2src/coins/hd_wallet/storage/wasm_storage.rs index 144c341ef3..c9f449be78 100644 --- a/mm2src/coins/hd_wallet/storage/wasm_storage.rs +++ b/mm2src/coins/hd_wallet/storage/wasm_storage.rs @@ -98,7 +98,7 @@ impl TableSignature for HDAccountTable { fn on_upgrade_needed(upgrader: &DbUpgrader, mut current_version: u32, new_version: u32) -> OnUpgradeResult<()> { /// A deprecated index that is now replaced by `WALLET_ACCOUNT_XPUB_INDEX`. - const WALLET_ACCOUNT_ID_INDEX: &str = "wallet_account_id"; + const DEPRECATED_WALLET_ACCOUNT_ID_INDEX: &str = "wallet_account_id"; while current_version < new_version { match current_version { @@ -106,7 +106,7 @@ impl TableSignature for HDAccountTable { let table = upgrader.create_table(Self::TABLE_NAME)?; table.create_multi_index(WALLET_ID_INDEX, &["coin", "hd_wallet_rmd160"], false)?; table.create_multi_index( - WALLET_ACCOUNT_ID_INDEX, + DEPRECATED_WALLET_ACCOUNT_ID_INDEX, &["coin", "hd_wallet_rmd160", "account_id"], true, )?; @@ -118,7 +118,7 @@ impl TableSignature for HDAccountTable { &["coin", "hd_wallet_rmd160", "account_xpub"], true, )?; - table.delete_index(WALLET_ACCOUNT_ID_INDEX)?; + table.delete_index(DEPRECATED_WALLET_ACCOUNT_ID_INDEX)?; }, unsupported_version => { return MmError::err(OnUpgradeError::UnsupportedVersion { From ba3a4bb634429986d554ec15120850ae0f92975e Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Tue, 1 Jul 2025 11:58:15 +0200 Subject: [PATCH 20/24] append _impl to utility funciton name --- mm2src/crypto/src/global_hd_ctx.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mm2src/crypto/src/global_hd_ctx.rs b/mm2src/crypto/src/global_hd_ctx.rs index 790a89a4cb..59151b940f 100644 --- a/mm2src/crypto/src/global_hd_ctx.rs +++ b/mm2src/crypto/src/global_hd_ctx.rs @@ -82,7 +82,7 @@ impl GlobalHDAccountCtx { &self, derivation_path: &DerivationPath, ) -> MmResult, Bip32Error> { - derive_secp256k1_xpub(self.bip39_secp_priv_key.clone(), derivation_path) + derive_secp256k1_xpub_impl(self.bip39_secp_priv_key.clone(), derivation_path) } } @@ -100,7 +100,7 @@ pub fn derive_secp256k1_secret( Ok(Secp256k1Secret::from(secret)) } -pub fn derive_secp256k1_xpub( +fn derive_secp256k1_xpub_impl( bip39_secp_priv_key: ExtendedPrivateKey, derivation_path: &DerivationPath, ) -> MmResult, Bip32Error> { From e516f393d9aebf695c5e7e9968c84b9907aa0e69 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Wed, 2 Jul 2025 15:54:13 +0200 Subject: [PATCH 21/24] add a unit test for delete_accounts db method --- mm2src/coins/hd_wallet/storage/mod.rs | 82 +++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/mm2src/coins/hd_wallet/storage/mod.rs b/mm2src/coins/hd_wallet/storage/mod.rs index d257c82a1b..146540c124 100644 --- a/mm2src/coins/hd_wallet/storage/mod.rs +++ b/mm2src/coins/hd_wallet/storage/mod.rs @@ -395,6 +395,80 @@ mod tests { } async fn test_delete_accounts_impl() { + let wallet0_account0 = HDAccountStorageItem { + account_id: 0, + account_xpub: "xpub6DEHSksajpRPM59RPw7Eg6PKdU7E2ehxJWtYdrfQ6JFmMGBsrR6jA78ANCLgzKYm4s5UqQ4ydLEYPbh3TRVvn5oAZVtWfi4qJLMntpZ8uGJ".to_owned(), + external_addresses_number: 1, + internal_addresses_number: 2, + }; + let wallet0_account1 = HDAccountStorageItem { + account_id: 1, + account_xpub: "xpub6DEHSksajpRPQq2FdGT6JoieiQZUpTZ3WZn8fcuLJhFVmtCpXbuXxp5aPzaokwcLV2V9LE55Dwt8JYkpuMv7jXKwmyD28WbHYjBH2zhbW2p".to_owned(), + external_addresses_number: 1, + internal_addresses_number: 2, + }; + let wallet0_account2 = HDAccountStorageItem { + account_id: 2, + account_xpub: "xpub6EuV33a2DXxAhoJTRTnr8qnysu81AA4YHpLY6o8NiGkEJ8KADJ35T64eJsStWsmRf1xXkEANVjXFXnaUKbRtFwuSPCLfDdZwYNZToh4LBCd".to_owned(), + external_addresses_number: 3, + internal_addresses_number: 4, + }; + let wallet1_account0 = HDAccountStorageItem { + account_id: 0, + account_xpub: "xpub6CUGRUonZSQ4TWtTMmzXdrXDtypWKiKrhko4egpiMZbpiaQL2jkwSB1icqYh2cfDfVxdx4df189oLKnC5fSwqPfgyP3hooxujYzAu3fDVmz".to_owned(), + external_addresses_number: 5, + internal_addresses_number: 6, + }; + + let ctx = mm_ctx_with_custom_db(); + let device0_rmd160 = H160::from("0000000000000000000000000000000000000010"); + let device1_rmd160 = H160::from("0000000000000000000000000000000000000020"); + + let wallet0_db = HDWalletCoinStorage::init_with_rmd160(&ctx, "RICK".to_owned(), device0_rmd160) + .await + .expect("!HDWalletCoinStorage::new"); + let wallet1_db = HDWalletCoinStorage::init_with_rmd160(&ctx, "RICK".to_owned(), device1_rmd160) + .await + .expect("!HDWalletCoinStorage::new"); + + wallet0_db + .upload_new_account(wallet0_account0.clone()) + .await + .expect("!HDWalletCoinStorage::upload_new_account: RICK wallet=0 account=0"); + wallet0_db + .upload_new_account(wallet0_account1.clone()) + .await + .expect("!HDWalletCoinStorage::upload_new_account: RICK wallet=0 account=1"); + wallet0_db + .upload_new_account(wallet0_account2.clone()) + .await + .expect("!HDWalletCoinStorage::upload_new_account: RICK wallet=0 account=2"); + wallet1_db + .upload_new_account(wallet1_account0.clone()) + .await + .expect("!HDWalletCoinStorage::upload_new_account: RICK wallet=1 account=0"); + + // Delete account 0 and 2 in wallet 0. + wallet0_db + .delete_accounts(vec![ + wallet0_account0.account_xpub.clone(), + wallet0_account2.account_xpub.clone(), + ]) + .await + .expect("HDWalletCoinStorage::delete_accounts: RICK wallet=0"); + + // All accounts must be in the only one database. + // Rows in the database must differ by only `coin`, `hd_wallet_rmd160` and `account_id` values. + let all_accounts: Vec<_> = get_all_storage_items(&ctx) + .await + .into_iter() + .sorted_by(|x, y| x.external_addresses_number.cmp(&y.external_addresses_number)) + .collect(); + // Only account 1 in wallet 0 and account 0 in wallet 1 should exist at this point. + assert_eq!(all_accounts, vec![wallet0_account1, wallet1_account0]); + } + + async fn test_clear_accounts_impl() { let wallet0_account0 = HDAccountStorageItem { account_id: 0, account_xpub: "xpub6DEHSksajpRPM59RPw7Eg6PKdU7E2ehxJWtYdrfQ6JFmMGBsrR6jA78ANCLgzKYm4s5UqQ4ydLEYPbh3TRVvn5oAZVtWfi4qJLMntpZ8uGJ".to_owned(), @@ -531,6 +605,14 @@ mod tests { #[test] fn test_delete_accounts() { block_on(test_delete_accounts_impl()) } + #[cfg(target_arch = "wasm32")] + #[wasm_bindgen_test] + async fn test_clear_accounts() { test_clear_accounts_impl().await } + + #[cfg(not(target_arch = "wasm32"))] + #[test] + fn test_clear_accounts() { block_on(test_clear_accounts_impl()) } + #[cfg(target_arch = "wasm32")] #[wasm_bindgen_test] async fn test_update_account() { test_update_account_impl().await } From 1c313b3f672324479150fd3bb6c9326ea4c2b5f2 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Thu, 3 Jul 2025 10:40:17 +0200 Subject: [PATCH 22/24] remove the fixme regarding corrupt account deletion this was already implemented --- mm2src/coins/hd_wallet/mod.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/mm2src/coins/hd_wallet/mod.rs b/mm2src/coins/hd_wallet/mod.rs index 7e12b64929..d3f5e55418 100644 --- a/mm2src/coins/hd_wallet/mod.rs +++ b/mm2src/coins/hd_wallet/mod.rs @@ -263,8 +263,6 @@ where // the purpose'/coin_type' fields in the coins cnofig without changing the ticker. // TODO: This is a temporary solution that fixes the issue. But we might wanna re-design how accounts // are loaded from the database (loading only the single account the user asked for and not all accounts). - // FIXME: What about collecting unmatching xpubs and deleting them. So that they never clutter the space of - // the new HD wallet associated with this coin ticker. let mut bad_accounts = Vec::new(); let accounts = accounts.into_iter().filter(|account| { let mut account_der_path = coin_der_path.clone(); From 114affc2f42c88c2fe9a58a99360c7913f0cf5b5 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Thu, 3 Jul 2025 10:42:02 +0200 Subject: [PATCH 23/24] remove the fixme regarding hw wallet init error this was fixed in another PR, namely: https://github.com/KomodoPlatform/komodo-defi-framework/pull/2504 --- mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs b/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs index a77de9b295..37fa371ec3 100644 --- a/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs +++ b/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs @@ -321,7 +321,6 @@ pub trait UtxoFieldsWithHardwareWalletBuilder: UtxoCoinBuilderCommonOps { hd_wallet_rmd160, hd_wallet_storage, derivation_path: path_to_coin, - // FIXME: With this set to no accounts, now surely the fixme below holds and will fail. accounts: HDAccountsMutex::new(Default::default()), enabled_address: self.activation_params().path_to_address, gap_limit, From c8282bebc9fe30724106f22a1c3bfd880c3264d3 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Thu, 3 Jul 2025 11:52:05 +0200 Subject: [PATCH 24/24] make sure to add all the known accounts even if the enabled one is brand new previously, we would ditch enabling accounts 0, 1, 2, 3 which were enabled before because the enabled address belonged to account 4 which hasn't been enabled before. this was wrong. --- mm2src/coins/coin_balance.rs | 89 ++++++++++++++---------------------- 1 file changed, 34 insertions(+), 55 deletions(-) diff --git a/mm2src/coins/coin_balance.rs b/mm2src/coins/coin_balance.rs index 56bbc47381..9cae424b3b 100644 --- a/mm2src/coins/coin_balance.rs +++ b/mm2src/coins/coin_balance.rs @@ -482,61 +482,6 @@ pub mod common_impl { accounts: Vec::with_capacity(accounts.len() + 1), }; - // FIXME: Here if this specific requested account was not found, we create it and `enable_hd_account` for it. But other accounts - // are not taken into account. Note that this check in the preceding commit was `accounts.is_empty()`. - if accounts.get(&path_to_address.account_id).is_none() { - // Is seems that we couldn't find any HD account from the HD wallet storage. - drop(accounts); - info!( - "{} HD wallet hasn't been enabled before. Create default HD account", - coin.ticker() - ); - - // Create new HD account. - let mut new_account = create_new_account(coin, hd_wallet, xpub_extractor, Some(path_to_address.account_id)) - .await - .map_mm_err()?; - let scan_new_addresses = matches!( - params.scan_policy, - EnableCoinScanPolicy::ScanIfNewWallet | EnableCoinScanPolicy::Scan - ); - - let account_balance = enable_hd_account( - coin, - hd_wallet, - &mut new_account, - path_to_address.chain, - &address_scanner, - scan_new_addresses, - params.min_addresses_number.max(Some(path_to_address.address_id + 1)), - ) - .await?; - drop(new_account); - - if coin.is_trezor() { - let enabled_address = - hd_wallet - .get_enabled_address() - .await - .ok_or(EnableCoinBalanceError::NewAddressDerivingError( - NewAddressDerivingError::Internal( - "Couldn't find enabled address after it has already been enabled".to_string(), - ), - ))?; - coin.received_enabled_address_from_hw_wallet(enabled_address) - .await - .map_err(|e| { - EnableCoinBalanceError::NewAddressDerivingError(NewAddressDerivingError::Internal(format!( - "Coin rejected the enabled address derived from the hardware wallet: {}", - e - ))) - })?; - } - // Todo: The enabled address should be indicated in the response. - result.accounts.push(account_balance); - return Ok(result); - } - debug!( "{} HD accounts were found on {} coin activation", accounts.len(), @@ -564,8 +509,41 @@ pub mod common_impl { .await?; result.accounts.push(account_balance); } + let account_of_enabled_address_exists = accounts.contains_key(&path_to_address.account_id); drop(accounts); + // Make sure that the account of the enabled address is available, or create it otherwise. + if !account_of_enabled_address_exists { + // Is seems that we couldn't find the account for the enabled address in the HD wallet storage. + info!( + "account={} hasn't been enabled before, enabling it now for coin={}", + path_to_address.account_id, + coin.ticker() + ); + + // Create new HD account. + let mut new_account = create_new_account(coin, hd_wallet, xpub_extractor, Some(path_to_address.account_id)) + .await + .map_mm_err()?; + let scan_new_addresses = matches!( + params.scan_policy, + EnableCoinScanPolicy::ScanIfNewWallet | EnableCoinScanPolicy::Scan + ); + + let account_balance = enable_hd_account( + coin, + hd_wallet, + &mut new_account, + path_to_address.chain, + &address_scanner, + scan_new_addresses, + params.min_addresses_number.max(Some(path_to_address.address_id + 1)), + ) + .await?; + + result.accounts.push(account_balance); + } + if coin.is_trezor() { let enabled_address = hd_wallet @@ -586,6 +564,7 @@ pub mod common_impl { })?; } + // Todo: The enabled address should be indicated in the response. Ok(result) }