From 68e82a7ebe33bf407ace270330b9cbe6ebef528c Mon Sep 17 00:00:00 2001 From: Alessandro Rezzi Date: Thu, 21 Nov 2024 16:58:17 +0100 Subject: [PATCH 1/2] Fix address rotation (#470) * feat: Update addressIndices after sync * feat: Introduce getNewChangeAddress and correctly select the address to undelegate * feat: Avoid address re-use * refactor: Make private every wallet method that can be private * more private methods * test: Add integration test to check address rotation --- scripts/composables/use_wallet.js | 6 +- scripts/global.js | 11 +-- scripts/stake/Stake.vue | 2 +- scripts/wallet.js | 134 +++++++++++++++----------- tests/integration/wallet/sync.spec.js | 37 ++++++- 5 files changed, 122 insertions(+), 68 deletions(-) diff --git a/scripts/composables/use_wallet.js b/scripts/composables/use_wallet.js index 15f1a5fa6..4c5aa27f4 100644 --- a/scripts/composables/use_wallet.js +++ b/scripts/composables/use_wallet.js @@ -52,7 +52,6 @@ export const useWallet = defineStore('wallet', () => { const getKeyToBackup = async () => await wallet.getKeyToBackup(); const getKeyToExport = () => wallet.getKeyToExport(); const isEncrypted = ref(true); - const loadFromDisk = () => wallet.loadFromDisk(); const hasShield = ref(wallet.hasShield()); const setMasterKey = async ({ mk, extsk }) => { @@ -71,7 +70,7 @@ export const useWallet = defineStore('wallet', () => { wallet.setShield(shield); hasShield.value = wallet.hasShield(); }; - const getAddress = () => wallet.getAddress(); + const getNewChangeAddress = () => wallet.getNewChangeAddress(); const isHardwareWallet = ref(wallet.isHardwareWallet()); const isHD = ref(wallet.isHD()); const checkDecryptPassword = async (passwd) => @@ -157,7 +156,7 @@ export const useWallet = defineStore('wallet', () => { isHardwareWallet, checkDecryptPassword, encrypt, - getAddress, + getNewChangeAddress, wipePrivateData: () => { wallet.wipePrivateData(); isViewOnly.value = wallet.isViewOnly(); @@ -173,7 +172,6 @@ export const useWallet = defineStore('wallet', () => { price, sync, createAndSendTransaction, - loadFromDisk, coldBalance, }; }); diff --git a/scripts/global.js b/scripts/global.js index 1c296ccf3..f3108e56c 100644 --- a/scripts/global.js +++ b/scripts/global.js @@ -393,12 +393,11 @@ export function optimiseCurrencyLocale(nAmount) { */ export async function openExplorer(strAddress = '') { const network = useNetwork(); + const toExport = wallet.getKeyToExport(); if (wallet.isLoaded() && wallet.isHD() && !strAddress) { - const xpub = wallet.getXPub(); - window.open(network.explorerUrl + '/xpub/' + xpub, '_blank'); + window.open(network.explorerUrl + '/xpub/' + toExport, '_blank'); } else { - const address = strAddress || wallet.getAddress(); - window.open(network.explorerUrl + '/address/' + address, '_blank'); + window.open(network.explorerUrl + '/address/' + toExport, '_blank'); } } @@ -745,7 +744,7 @@ export async function sweepAddress(arrUTXOs, sweepingMasterKey, nFixedFee) { const txBuilder = TransactionBuilder.create().addUTXOs(arrUTXOs); const outputValue = txBuilder.valueIn - (nFixedFee || txBuilder.getFee()); - const [address] = wallet.getNewAddress(1); + const address = wallet.getNewChangeAddress(); const tx = txBuilder .addOutput({ address, @@ -1677,7 +1676,7 @@ export async function createProposal() { // If Advanced Mode is enabled and an address is given, use the provided address, otherwise, generate a new one const strAddress = document.getElementById('proposalAddress').value.trim() || - wallet.getNewAddress(1)[0]; + wallet.getNewChangeAddress(); const nextSuperblock = await getNetwork().getNextSuperblock(); const proposal = { name: strTitle, diff --git a/scripts/stake/Stake.vue b/scripts/stake/Stake.vue index 3e5c29edc..9b3da89ca 100644 --- a/scripts/stake/Stake.vue +++ b/scripts/stake/Stake.vue @@ -80,7 +80,7 @@ async function unstake(value) { } const res = await wallet.createAndSendTransaction( getNetwork(), - wallet.getAddress(1), + wallet.getNewChangeAddress(), value, { useDelegatedInputs: true, diff --git a/scripts/wallet.js b/scripts/wallet.js index e06e9d2b5..25eec7e09 100644 --- a/scripts/wallet.js +++ b/scripts/wallet.js @@ -135,7 +135,7 @@ export class Wallet { this.#highestUsedIndices.set(i, 0); this.#loadedIndexes.set(i, 0); } - this.subscribeToNetworkEvents(); + this.#subscribeToNetworkEvents(); } /** @@ -232,7 +232,7 @@ export class Wallet { if (extsk) await this.setExtsk(extsk); if (isNewAcc) { this.reset(); - for (let i = 0; i < Wallet.chains; i++) this.loadAddresses(i); + for (let i = 0; i < Wallet.chains; i++) this.#loadAddresses(i); } } @@ -283,15 +283,27 @@ export class Wallet { * */ getCurrentAddress() { - return this.getAddress(0, this.#addressIndices.get(0)); + return this.#getAddress(0, this.#addressIndices.get(0)); + } + + /** + * Update the current address. + */ + #updateCurrentAddress() { + // No need to update the change address, as it is only handled internally by the wallet. + const last = this.#highestUsedIndices.get(0); + const curr = this.#addressIndices.get(0); + if (curr <= last) { + this.#addressIndices.set(0, last + 1); + } } /** * Derive a generic address (given nReceiving and nIndex) * @return {string} Address */ - getAddress(nReceiving = 0, nIndex = 0) { - const path = this.getDerivationPath(nReceiving, nIndex); + #getAddress(nReceiving = 0, nIndex = 0) { + const path = this.#getDerivationPath(nReceiving, nIndex); return this.#masterKey.getAddress(path); } @@ -309,7 +321,7 @@ export class Wallet { */ getXPub(nReceiving = 0, nIndex = 0) { // Get our current wallet XPub - const derivationPath = this.getDerivationPath(nReceiving, nIndex) + const derivationPath = this.#getDerivationPath(nReceiving, nIndex) .split('/') .slice(0, 4) .join('/'); @@ -385,27 +397,31 @@ export class Wallet { */ getNewAddress(nReceiving = 0) { const last = this.#highestUsedIndices.get(nReceiving); - this.#addressIndices.set( - nReceiving, - (this.#addressIndices.get(nReceiving) > last - ? this.#addressIndices.get(nReceiving) - : last) + 1 - ); + const curr = this.#addressIndices.get(nReceiving); + this.#addressIndices.set(nReceiving, Math.max(curr, last) + 1); if (this.#addressIndices.get(nReceiving) - last > MAX_ACCOUNT_GAP) { // If the user creates more than ${MAX_ACCOUNT_GAP} empty wallets we will not be able to sync them! this.#addressIndices.set(nReceiving, last); } - const path = this.getDerivationPath( + const path = this.#getDerivationPath( nReceiving, this.#addressIndices.get(nReceiving) ); - const address = this.getAddress( + const address = this.#getAddress( nReceiving, this.#addressIndices.get(nReceiving) ); return [address, path]; } + /** + * Generates a new change address + * @returns {string} + */ + getNewChangeAddress() { + return this.getNewAddress(1)[0]; + } + /** * @returns {Promise} new shield address */ @@ -421,10 +437,10 @@ export class Wallet { * Check if the vout is owned and in case update highestUsedIdex * @param {CTxOut} vout */ - updateHighestUsedIndex(vout) { + #updateHighestUsedIndex(vout) { const dataBytes = hexToBytes(vout.script); const iStart = isP2PKH(dataBytes) ? P2PK_START_INDEX : COLD_START_INDEX; - const address = this.getAddressFromHashCache( + const address = this.#getAddressFromHashCache( bytesToHex(dataBytes.slice(iStart, iStart + 20)) ); const path = this.isOwnAddress(address); @@ -441,7 +457,7 @@ export class Wallet { this.#highestUsedIndices.get(nReceiving) + MAX_ACCOUNT_GAP >= this.#loadedIndexes.get(nReceiving) ) { - this.loadAddresses(nReceiving); + this.#loadAddresses(nReceiving); } } } @@ -450,12 +466,12 @@ export class Wallet { * Load MAX_ACCOUNT_GAP inside #ownAddresses map. * @param {number} chain - Chain to load */ - loadAddresses(chain) { + #loadAddresses(chain) { if (this.isHD()) { const start = this.#loadedIndexes.get(chain); const end = start + MAX_ACCOUNT_GAP; for (let i = start; i <= end; i++) { - const path = this.getDerivationPath(chain, i); + const path = this.#getDerivationPath(chain, i); const address = this.#masterKey.getAddress(path); this.#ownAddresses.set(address, path); } @@ -478,7 +494,7 @@ export class Wallet { /** * @return {String} BIP32 path or null if it's not your address */ - getDerivationPath(nReceiving = 0, nIndex = 0) { + #getDerivationPath(nReceiving = 0, nIndex = 0) { return this.#masterKey.getDerivationPath( this.#nAccount, nReceiving, @@ -516,7 +532,7 @@ export class Wallet { const dataBytes = hexToBytes(script); // At the moment we support only P2PKH and P2CS const iStart = isP2PKH(dataBytes) ? P2PK_START_INDEX : COLD_START_INDEX; - const address = this.getAddressFromHashCache( + const address = this.#getAddressFromHashCache( bytesToHex(dataBytes.slice(iStart, iStart + 20)) ); return this.isOwnAddress(address); @@ -528,7 +544,7 @@ export class Wallet { * It doesn't know about LOCK, IMMATURE or SPENT statuses, for that * it's necessary to interrogate the mempool */ - getScriptType(script) { + #getScriptType(script) { const { type, addresses } = this.getAddressesFromScript(script); let status = 0; const isOurs = addresses.some((s) => this.isOwnAddress(s)); @@ -547,7 +563,7 @@ export class Wallet { getAddressesFromScript(script) { const dataBytes = hexToBytes(script); if (isP2PKH(dataBytes)) { - const address = this.getAddressFromHashCache( + const address = this.#getAddressFromHashCache( bytesToHex( dataBytes.slice(P2PK_START_INDEX, P2PK_START_INDEX + 20) ) @@ -561,7 +577,7 @@ export class Wallet { for (let i = 0; i < 2; i++) { const iStart = i === 0 ? OWNER_START_INDEX : COLD_START_INDEX; addresses.push( - this.getAddressFromHashCache( + this.#getAddressFromHashCache( bytesToHex(dataBytes.slice(iStart, iStart + 20)), iStart === OWNER_START_INDEX ? 'coldaddress' @@ -571,7 +587,7 @@ export class Wallet { } return { type: 'p2cs', addresses }; } else if (isP2EXC(dataBytes)) { - const address = this.getAddressFromHashCache( + const address = this.#getAddressFromHashCache( bytesToHex( dataBytes.slice( P2PK_START_INDEX + 1, @@ -590,7 +606,7 @@ export class Wallet { } // Avoid calculating over and over the same getAddressFromHash by saving the result in a map - getAddressFromHashCache(pkh_hex, type) { + #getAddressFromHashCache(pkh_hex, type) { if (!this.#knownPKH.has(pkh_hex)) { this.#knownPKH.set( pkh_hex, @@ -604,7 +620,7 @@ export class Wallet { * Return true if the transaction contains undelegations regarding the given wallet * @param {import('./transaction.js').Transaction} tx */ - checkForUndelegations(tx) { + #checkForUndelegations(tx) { for (const vin of tx.vin) { const status = this.#mempool.getOutpointStatus(vin.outpoint); if (status & OutpointState.P2CS) { @@ -618,7 +634,7 @@ export class Wallet { * Return true if the transaction contains delegations regarding the given wallet * @param {import('./transaction.js').Transaction} tx */ - checkForDelegations(tx) { + #checkForDelegations(tx) { const txid = tx.txid; for (let i = 0; i < tx.vout.length; i++) { const outpoint = new COutpoint({ @@ -638,7 +654,7 @@ export class Wallet { * Return the output addresses for a given transaction * @param {import('./transaction.js').Transaction} tx */ - getOutAddress(tx) { + #getOutAddress(tx) { return tx.vout.reduce( (acc, vout) => [ ...acc, @@ -653,7 +669,7 @@ export class Wallet { * @param {import('./transaction.js').Transaction[]} arrTXs - An array of the Blockbook TXs * @returns {Promise>} - A new array of `HistoricalTx`-formatted transactions */ - async toHistoricalTXs(arrTXs) { + async #toHistoricalTXs(arrTXs) { let histTXs = []; for (const tx of arrTXs) { const { credit, ownAllVout } = this.#mempool.getCredit(tx); @@ -663,11 +679,11 @@ export class Wallet { // Shielded data const { shieldCredit, shieldDebit, arrShieldReceivers } = - await this.extractSaplingAmounts(tx); + await this.#extractSaplingAmounts(tx); const nShieldAmount = (shieldCredit - shieldDebit) / COIN; const ownAllShield = shieldDebit - shieldCredit === tx.valueBalance; // The receiver addresses, if any - let arrReceivers = this.getOutAddress(tx); + let arrReceivers = this.#getOutAddress(tx); const getFilteredCredit = (filter) => { return tx.vout .filter((_, i) => { @@ -688,10 +704,10 @@ export class Wallet { type = HistoricalTxType.STAKE; } else if (tx.isProposalFee()) { type = HistoricalTxType.PROPOSAL_FEE; - } else if (this.checkForUndelegations(tx)) { + } else if (this.#checkForUndelegations(tx)) { type = HistoricalTxType.UNDELEGATION; nAmount = getFilteredCredit(OutpointState.P2PKH) / COIN; - } else if (this.checkForDelegations(tx)) { + } else if (this.#checkForDelegations(tx)) { type = HistoricalTxType.DELEGATION; arrReceivers = arrReceivers.filter((addr) => { return addr[0] === cChainParams.current.STAKING_PREFIX; @@ -724,7 +740,7 @@ export class Wallet { * Extract the sapling spent, received and shield addressed, regarding the wallet, from a tx * @param {import('./transaction.js').Transaction} tx - a Transaction object */ - async extractSaplingAmounts(tx) { + async #extractSaplingAmounts(tx) { let shieldCredit = 0; let shieldDebit = 0; let arrShieldReceivers = []; @@ -752,7 +768,7 @@ export class Wallet { * @param {Transaction} tx */ async #pushToHistoricalTx(tx) { - const hTx = (await this.toHistoricalTXs([tx]))[0]; + const hTx = (await this.#toHistoricalTXs([tx]))[0]; this.#historicalTxs.insert(hTx); } @@ -771,8 +787,8 @@ export class Wallet { getEventEmitter().disableEvent('balance-update'); getEventEmitter().disableEvent('new-tx'); - await this.loadShieldFromDisk(); - await this.loadFromDisk(); + await this.#loadShieldFromDisk(); + await this.#loadFromDisk(); // Let's set the last processed block 5 blocks behind the actual chain tip // This is just to be sure since blockbook (as we know) // usually does not return txs of the actual last block. @@ -785,7 +801,10 @@ export class Wallet { } this.#isSynced = true; // At this point download the last missing blocks in the range (blockCount -5, blockCount] - await this.getLatestBlocks(blockCount); + await this.#getLatestBlocks(blockCount); + + // Finally avoid address re-use by updating the map #addressIndices + this.#updateCurrentAddress(); // Update both activities post sync getEventEmitter().enableEvent('balance-update'); @@ -916,7 +935,7 @@ export class Wallet { `syncShield rust internal ${handleBlocksTime} ms` ); // At this point it should be safe to assume that shield is ready to use - await this.saveShieldOnDisk(); + await this.#saveShieldOnDisk(); } catch (e) { debugError(DebugTopics.WALLET, e); } @@ -958,16 +977,16 @@ export class Wallet { }); } - subscribeToNetworkEvents() { + #subscribeToNetworkEvents() { getEventEmitter().on('new-block', async (block) => { if (this.#isSynced) { - await this.getLatestBlocks(block); + await this.#getLatestBlocks(block); // Invalidate the balance cache to keep immature balance updated this.#mempool.invalidateBalanceCache(); } }); } - getLatestBlocks = lockableFunction( + #getLatestBlocks = lockableFunction( /** * Update the shield object with the latest blocks * @param{number} blockCount - block count @@ -998,7 +1017,7 @@ export class Wallet { !(await this.#checkShieldSaplingRoot(saplingRoot)) ) return; - await this.saveShieldOnDisk(); + await this.#saveShieldOnDisk(); } } ); @@ -1023,7 +1042,7 @@ export class Wallet { /** * Save shield data on database */ - async saveShieldOnDisk() { + async #saveShieldOnDisk() { const cDB = await Database.getInstance(); const cAccount = await cDB.getAccount(); // If the account has not been created yet (for example no encryption) return @@ -1036,7 +1055,7 @@ export class Wallet { /** * Load shield data from database */ - async loadShieldFromDisk() { + async #loadShieldFromDisk() { if (this.#shield) { return; } @@ -1065,7 +1084,7 @@ export class Wallet { async #resetShield() { // TODO: take the wallet creation height in input from users await this.#shield.reloadFromCheckpoint(4200000); - await this.saveShieldOnDisk(); + await this.#saveShieldOnDisk(); } /** @@ -1130,7 +1149,7 @@ export class Wallet { // Add primary output if (isDelegation) { - if (!returnAddress) [returnAddress] = this.getNewAddress(1); + if (!returnAddress) returnAddress = this.getNewChangeAddress(); transactionBuilder.addColdStakeOutput({ address: returnAddress, addressColdStake: address, @@ -1173,7 +1192,7 @@ export class Wallet { transactionBuilder.equallySubtractAmt(Math.abs(changeValue)); } else if (changeValue > 0) { // TransactionBuilder will internally add the change only if it is not dust - if (!changeAddress) [changeAddress] = this.getNewAddress(1); + if (!changeAddress) changeAddress = this.getNewChangeAddress(); if (delegateChange && changeValue >= 1 * COIN) { transactionBuilder.addColdStakeOutput({ address: changeAddress, @@ -1229,7 +1248,7 @@ export class Wallet { blockHeight: blockCount + 1, useShieldInputs: transaction.vin.length === 0, utxos: this.#getUTXOsForShield(value), - transparentChangeAddress: this.getNewAddress(1)[0], + transparentChangeAddress: this.getNewChangeAddress(), }); return transaction.fromHex(hex); } catch (e) { @@ -1280,8 +1299,8 @@ export class Wallet { this.#mempool.addTransaction(transaction); let i = 0; for (const out of transaction.vout) { - this.updateHighestUsedIndex(out); - const status = this.getScriptType(out.script); + this.#updateHighestUsedIndex(out); + const status = this.#getScriptType(out.script); if (status & OutpointState.OURS) { this.#mempool.addOutpointStatus( new COutpoint({ @@ -1305,6 +1324,9 @@ export class Wallet { if (tx) { this.#historicalTxs.remove((hTx) => hTx.id === tx.txid); } + if (this.#isSynced) { + this.#updateCurrentAddress(); + } await this.#pushToHistoricalTx(transaction); getEventEmitter().emit('new-tx'); } @@ -1328,7 +1350,7 @@ export class Wallet { parsed.blockHeight = blockHeight; parsed.blockTime = block.time; // Avoid wasting memory on txs that do not regard our wallet - const isOwned = allowOwn ? this.ownTransaction(parsed) : false; + const isOwned = allowOwn ? this.#ownTransaction(parsed) : false; if (isOwned || shieldTxs.includes(tx.hex)) { await this.addTransaction(parsed); } @@ -1339,10 +1361,10 @@ export class Wallet { * Check if any vin or vout of the transaction belong to the wallet * @param {import('./transaction.js').Transaction} transaction */ - ownTransaction(transaction) { + #ownTransaction(transaction) { const ownVout = transaction.vout.filter((out) => { - return this.getScriptType(out.script) & OutpointState.OURS; + return this.#getScriptType(out.script) & OutpointState.OURS; }).length > 0; const ownVin = transaction.vin.filter((input) => { @@ -1397,7 +1419,7 @@ export class Wallet { return this.#mempool.outpointToUTXO(outpoint); } - async loadFromDisk() { + async #loadFromDisk() { const db = await Database.getInstance(); if ((await db.getAccount())?.publicKey !== this.getKeyToExport()) { await db.removeAllTxs(); diff --git a/tests/integration/wallet/sync.spec.js b/tests/integration/wallet/sync.spec.js index f8b646769..541b491b7 100644 --- a/tests/integration/wallet/sync.spec.js +++ b/tests/integration/wallet/sync.spec.js @@ -50,7 +50,7 @@ async function mineBlocks(nBlocks) { * This is implementation-depended, so it's not ideal. Increase this number * If tests don't pass */ - for (let i = 0; i < 4; i++) await flushPromises(); + for (let i = 0; i < 10; i++) await flushPromises(); } describe('Wallet sync tests', () => { @@ -187,6 +187,41 @@ describe('Wallet sync tests', () => { coldStakeProfit ); }); + it('correctly rotates addresses', async () => { + let rotatedAddresses = [walletHD.getCurrentAddress()]; + // 1) walletHD receives a tx to his current address + await createAndSendTransaction( + walletLegacy, + rotatedAddresses[0], + 0.01 * 10 ** 8 + ); + // Transaction sent but not received. current address must remain the same. + expect(walletHD.getCurrentAddress()).toBe(rotatedAddresses[0]); + // Mine the block + await mineBlocks(1); + // Current address now must be different. + let newAddress = walletHD.getCurrentAddress(); + expect(rotatedAddresses.includes(newAddress)).toBeFalsy(); + rotatedAddresses.push(newAddress); + + // 2) The owner of the wallet sends a transaction to himself + await createAndSendTransaction(walletHD, newAddress, 0.01 * 10 ** 8); + // Since the transaction is to self, rotation already happened! + newAddress = walletHD.getCurrentAddress(); + expect(rotatedAddresses.includes(newAddress)).toBeFalsy(); + rotatedAddresses.push(newAddress); + await mineBlocks(1); + expect(walletHD.getCurrentAddress()).toBe(newAddress); + + // 3) Send a tx to a previous current address and verify that rotation doesn't happen + await createAndSendTransaction( + walletLegacy, + rotatedAddresses[0], + 0.01 * 10 ** 8 + ); + await mineBlocks(1); + expect(walletHD.getCurrentAddress()).toBe(newAddress); + }); afterAll(() => { vi.clearAllMocks(); }); From 7bfb6cedbc808890692080f8d17d7f486c6130b8 Mon Sep 17 00:00:00 2001 From: Alessandro Rezzi Date: Sat, 23 Nov 2024 21:32:06 +0100 Subject: [PATCH 2/2] fix unconfirmed activity bug (#476) --- scripts/wallet.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/wallet.js b/scripts/wallet.js index 25eec7e09..ff9a6086a 100644 --- a/scripts/wallet.js +++ b/scripts/wallet.js @@ -983,6 +983,9 @@ export class Wallet { await this.#getLatestBlocks(block); // Invalidate the balance cache to keep immature balance updated this.#mempool.invalidateBalanceCache(); + // Emit a new-tx signal to update the Activity. + // Otherwise, unconfirmed txs would not get updated + getEventEmitter().emit('new-tx'); } }); }