From 0a6c047131ce1e9c1dede92eba8d43354f28a3a0 Mon Sep 17 00:00:00 2001 From: Daniel Date: Thu, 12 Dec 2024 14:52:45 +0100 Subject: [PATCH] WalletInfoCollector: revert more restrictive address scanning of f4b5904f f4b5904f ("Improve syncing/detection during login") changed address scanning to strictly allow an address gap of at most ACCOUNT_MAX_ALLOWED_ADDRESS_GAP, while the implementation we are reverting to is less restrictive by always checking full chunks of size ACCOUNT_MAX_ALLOWED_ADDRESS_GAP, which can result in address gaps between ACCOUNT_MAX_ALLOWED_ADDRESS_GAP and 2*ACCOUNT_MAX_ALLOWED_ADDRESS_GAP-1. This allows to potentially find more of the user's addresses. The more exhaustive scanning is no problem anymore now with the recent perfomance improvements of @nimiq/core. The more detailed status messsages in LoginSuccess of f4b5904f have not been reverted as the only remaining change. The addition of a skip button has already been reverted previously in 1424f5fd. --- src/lib/WalletInfoCollector.ts | 53 ++++++++++++---------------------- src/views/LoginSuccess.vue | 4 +-- 2 files changed, 19 insertions(+), 38 deletions(-) diff --git a/src/lib/WalletInfoCollector.ts b/src/lib/WalletInfoCollector.ts index cbba2fed..206d4c54 100644 --- a/src/lib/WalletInfoCollector.ts +++ b/src/lib/WalletInfoCollector.ts @@ -33,7 +33,6 @@ import { deriveAddressesFromXPub, getBtcNetwork, publicKeyToPayment } from './bi export type BasicAccountInfo = { address: string, path: string, - index: number, }; export type WalletCollectionResultLedger = { @@ -234,8 +233,8 @@ export default class WalletInfoCollector { } // Kick off first round of account derivation - // let startIndex = 0; - let derivedAccountsPromise = WalletInfoCollector._deriveAccounts(0 /*startIndex */, + let startIndex = 0; + let derivedAccountsPromise = WalletInfoCollector._deriveAccounts(startIndex, ACCOUNT_MAX_ALLOWED_ADDRESS_GAP, walletType, keyId, keyguardCookieEncryptionKey); try { @@ -290,26 +289,25 @@ export default class WalletInfoCollector { } let foundAccounts: BasicAccountInfo[]; - let lastActiveAccountIndex = 0; let receiptsError: Error | undefined; do { const derivedAccounts = await derivedAccountsPromise; - // // already start deriving next accounts - // // By always advancing in groups of MAX_ALLOWED_GAP addresses per round, it often happens that more - // // addresses are derived and checked for activity than the BIP44 address gap limit - // // (https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki#address-gap-limit) stipulates, - // // because whenever an active address in a group of addresses is found, the next full group is also - // // derived. Thus the actual gap limit of this implementation is up to (2 x MAX_ALLOWED_GAP) - 1. - // // We argue that this is good UX for users, as potentially more of their active addresses are found, - // // even if they haven't strictly followed to the standard - at only a relatively small cost to the - // // network. For example, if the user adds the accounts derived with indices 0, 19, 39 to his wallet but - // // then only ends up using accounts 0 and 39, the account at index 19 will not be found anymore on - // // reimport. With the current implementation however, at least the account 39 would be found, while an - // // implementation strictly following the specification would stop the search at index 19. - // startIndex += ACCOUNT_MAX_ALLOWED_ADDRESS_GAP; - // derivedAccountsPromise = WalletInfoCollector._deriveAccounts(startIndex, - // ACCOUNT_MAX_ALLOWED_ADDRESS_GAP, walletType, keyId, keyguardCookieEncryptionKey); + // already start deriving next accounts + // By always advancing in groups of MAX_ALLOWED_GAP addresses per round, it often happens that more + // addresses are derived and checked for activity than the BIP44 address gap limit + // (https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki#address-gap-limit) stipulates, + // because whenever an active address in a group of addresses is found, the next full group is also + // derived. Thus the actual gap limit of this implementation is up to (2 x MAX_ALLOWED_GAP) - 1. + // We argue that this is good UX for users, as potentially more of their active addresses are found, + // even if they haven't strictly followed to the standard - at only a relatively small cost to the + // network. For example, if the user adds the accounts derived with indices 0, 19, 39 to his wallet but + // then only ends up using accounts 0 and 39, the account at index 19 will not be found anymore on + // reimport. With the current implementation however, at least the account 39 would be found, while an + // implementation strictly following the specification would stop the search at index 19. + startIndex += ACCOUNT_MAX_ALLOWED_ADDRESS_GAP; + derivedAccountsPromise = WalletInfoCollector._deriveAccounts(startIndex, + ACCOUNT_MAX_ALLOWED_ADDRESS_GAP, walletType, keyId, keyguardCookieEncryptionKey); // Already add addresses that are in the initialAccounts foundAccounts = derivedAccounts.filter((derived) => @@ -325,7 +323,6 @@ export default class WalletInfoCollector { if (!!balance && (balance.balance !== 0 || balance.stake !== 0)) { foundAccounts.push(account); hasActivity = true; - lastActiveAccountIndex = Math.max(lastActiveAccountIndex, account.index); } } @@ -343,7 +340,6 @@ export default class WalletInfoCollector { if (receipts.length > 0) { foundAccounts.push(account); hasActivity = true; - lastActiveAccountIndex = Math.max(lastActiveAccountIndex, account.index); } } catch (error) { if (!(error as Error).message.startsWith(ERROR_TRANSACTION_RECEIPTS)) { @@ -359,14 +355,6 @@ export default class WalletInfoCollector { WalletInfoCollector._addAccounts(walletInfo, foundAccounts, balances); onUpdate(walletInfo, await derivedAccountsPromise); } - - // Check the remaining gap after the last checked account - const lastCheckedIndex = accountsToCheck[accountsToCheck.length - 1].index; - const nextStartIndex = lastCheckedIndex + 1; - const remainingGap = ACCOUNT_MAX_ALLOWED_ADDRESS_GAP - (lastCheckedIndex - lastActiveAccountIndex); - if (!remainingGap) break; - derivedAccountsPromise = WalletInfoCollector._deriveAccounts(nextStartIndex, - remainingGap, walletType, keyId, keyguardCookieEncryptionKey); } while (foundAccounts.length > 0); const releaseKey = walletType === WalletType.BIP39 @@ -465,10 +453,8 @@ export default class WalletInfoCollector { keyguardCookieEncryptionKey?: Uint8Array, ): Promise { const pathsToDerive: string[] = []; - const indices: number[] = []; for (let index = startIndex; index < startIndex + count; ++index) { pathsToDerive.push(`${ACCOUNT_BIP32_BASE_PATH_KEYGUARD}${index}'`); - indices.push(index); } const derivedAddresses = await WalletInfoCollector._keyguardClient!.deriveAddresses(keyId, pathsToDerive, keyguardCookieEncryptionKey); @@ -479,7 +465,6 @@ export default class WalletInfoCollector { accounts.push({ path: pathsToDerive[i], address: userFriendlyAddresses[i], - index: indices[i], }); } return accounts; @@ -487,13 +472,11 @@ export default class WalletInfoCollector { private static async _deriveLedgerAccounts(startIndex: number, count: number): Promise { const pathsToDerive: string[] = []; - const indices: number[] = []; for (let index = startIndex; index < startIndex + count; ++index) { pathsToDerive.push(getBip32Path({ coin: Coin.NIMIQ, addressIndex: index, })); - indices.push(index); } return ( await LedgerApi.Nimiq.deriveAddresses( @@ -501,7 +484,7 @@ export default class WalletInfoCollector { /* expectedWalletId */ undefined, Config.ledgerApiNimiqVersion, ) - ).map((address, i) => ({ path: address.keyPath, address: address.address, index: indices[i] })); + ).map((address) => ({ path: address.keyPath, address: address.address })); } private static async _getBalances( diff --git a/src/views/LoginSuccess.vue b/src/views/LoginSuccess.vue index 6fb8b68d..70695405 100644 --- a/src/views/LoginSuccess.vue +++ b/src/views/LoginSuccess.vue @@ -65,10 +65,9 @@ export default class LoginSuccess extends Vue { await Promise.all( this.keyguardResult.map(async (keyResult) => { // The Keyguard always returns (at least) one derived Address, - const keyguardResultAccounts = keyResult.addresses.map((addressObj, index) => ({ + const keyguardResultAccounts = keyResult.addresses.map((addressObj) => ({ address: new Nimiq.Address(addressObj.address).toUserFriendlyAddress(), path: addressObj.keyPath, - index, })); let tryCount = 0; @@ -187,7 +186,6 @@ export default class LoginSuccess extends Vue { private onUpdate(walletInfo: WalletInfo, currentlyCheckedAccounts: BasicAccountInfo[]) { const count = !walletInfo ? 0 : walletInfo.accounts.size; - if (count <= 1) return; this.status = this.$tc('Imported {count} address so far... | Imported {count} addresses so far...', count); }