fix(HD-Wallet): load correct HD Accounts that truly belong to the HD Wallet #2482

mariocynicys · 2025-06-08T11:30:40Z

This fixes a problem were a coin could be activated with an incorrect HD Account that doesn't actually represent/correspond to the activated one. Most of the coin's interactions (e.g. balance response in enable request) will work on the wrong HD account and thus also the wrong address(es).

This bug shows up when either (or both of) the purpose or coin sections of the derivation paths are changed while the coin name stays the same. If that coin had some accounts in the hd_account table in MM2-shared.db (which holds info about HD wallets corresponding to KDF's activated seed), and we changed either of the two mentioned sections of the derivation path, when we load accounts the next time it will load with the old purpose'/coin' sections of the derivation paths. This is because the SQL query that loads the accounts is based only on the coin ticker which now corresponds to a new derivation path (new purpose'/coin').

The gist of this fix here is that we now load accounts based on xpubs. We only load the accounts that their xpubs are within the tree of the coin's HD Wallet.

For native HD wallets this goes as follows:

We load all the accounts (corresponding to the coin's ticker) and derive the xpub for each one.
If the derived xpub matches the account's xpub (one from the DB) then we can load this account. Otherwise we don't load it (but the same account_id might be created afterwards if it hold the enabled address).

For HW HD wallets:

There is no way to derive an xpub without interacting with the hardware wallet. So we just don't load any accounts whatsoever.
On coin enables, the user will be prompted to derive the xpub for the account every time. i.e. next enable request will still ask them to derive the xpub nonetheless.
For the user to view more accounts, they need to call init_create_new_account_rpc. These activated accounts will not autoload the next time KDF is booted up and they must be re-inited again.

I initially thought that this fix design is good enough as it keep the same UX with minimal changes, but found out later that it doesn't work very well with HW wallets. We probably better-off fix the root issue by re-working the hd_account DB schema to include the complete derivation path purpose'/coin'/account' so that we are sure we don't load the incorrect account/xpub. Putting this PR up for discussion/review nonetheless (in case it's an acceptable fix for the time-being).

I have left some fixmes of things I think we should discuss. Please open discussion threads on them.
Fixes #2470

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)

and add relevant links

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.

will clean up this commit before merging

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)

…ad_all_hd_accounts_from_storage_with_xpubs for simplicity

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

onur-ozkan

There are lots of FIXMEs which should be replaced with TODO IMO.

mm2src/rpc_task/src/manager.rs

mm2src/crypto/src/global_hd_ctx.rs

mm2src/coins/hd_wallet/mod.rs

mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs

mm2src/rpc_task/src/manager.rs

This reverts commit bdf2f74.

…ccount loading

…ific

this makes it easy to read the on_upgrade_needed function without confusion

onur-ozkan

LGTM once conflicts are resolved and with/without the nits I shared in my previous review.

and resolve merge conflicts

shamardy · 2025-07-02T10:40:46Z

@mariocynicys We have to cover new db methods with tests, please cover the new delete_accounts method

mm2src/coins/coin_balance.rs

this was already implemented

this was fixed in another PR, namely: #2504

…and 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.

mariocynicys · 2025-07-03T09:56:24Z

all the fixmes are now resolved.

shamardy · 2025-07-08T11:49:06Z

As discussed with @mariocynicys on DM, we might change the approach in another PR instead of this

…-corruption

mariocynicys added 14 commits June 3, 2025 20:44

add a fixme regarding new accounts creation

e1357a5

add a warning when xpubs don't match

2d8552a

move the sequential account creation fixme to a todo

f3e327c

and add relevant links

clean up un-used methods

d5f511c

add a fixme regarding get_enabled_address call in trezor activation

dcc8265

use xpubs for db queries that edit hd_account table (wasm)

193579d

shouldn't we rollback tasks the failed running?

bdf2f74

will clean up this commit before merging

remove get_all_account_ids and merge get_xpubs_for_account_ids and lo…

8a2a8d9

…ad_all_hd_accounts_from_storage_with_xpubs for simplicity

reactivate the failing test

387f20f

add another fixme regarding coin balance result for HD wallets

d4a3766

mariocynicys added status: pending review priority: urgent Critical tasks requiring immediate action. bug: hd-wallet labels Jun 8, 2025

onur-ozkan reviewed Jun 9, 2025

View reviewed changes

mm2src/rpc_task/src/manager.rs Outdated Show resolved Hide resolved

mm2src/crypto/src/global_hd_ctx.rs Outdated Show resolved Hide resolved

mm2src/coins/hd_wallet/mod.rs Outdated Show resolved Hide resolved

shamardy added the 2.6.0-beta label Jun 16, 2025