From 67570a04eea971308edcf2f9450d0dd41ca460ca Mon Sep 17 00:00:00 2001 From: Peter Sanderson Date: Mon, 21 Oct 2024 15:23:49 +0200 Subject: [PATCH 1/4] feat: implement random strategy for utxo sorting chore: improve test for composeTx so that random number generotor can be mocked per-case in fixure independently --- .../e2e/__fixtures__/composeTransaction.ts | 16 ++-- .../connect/e2e/tests/device/methods.test.ts | 8 ++ packages/connect/src/constants/utxo.ts | 2 +- .../compose/sorting/randomSortingStrategy.ts | 36 +++++++++ packages/utxo-lib/src/compose/transaction.ts | 2 + packages/utxo-lib/src/types/compose.ts | 2 +- .../utxo-lib/tests/__fixtures__/compose.ts | 74 +++++++++++++++++++ packages/utxo-lib/tests/compose.test.ts | 22 +++++- 8 files changed, 151 insertions(+), 11 deletions(-) create mode 100644 packages/utxo-lib/src/compose/sorting/randomSortingStrategy.ts diff --git a/packages/connect/e2e/__fixtures__/composeTransaction.ts b/packages/connect/e2e/__fixtures__/composeTransaction.ts index 55a8d5f4804..0dad3c0be8a 100644 --- a/packages/connect/e2e/__fixtures__/composeTransaction.ts +++ b/packages/connect/e2e/__fixtures__/composeTransaction.ts @@ -212,8 +212,8 @@ export default { totalSpent: '1142', inputs: [{ amount: '9426', script_type: 'SPENDWITNESS' }], outputs: [ - { amount: '1000', script_type: 'PAYTOADDRESS' }, { amount: '8284', script_type: 'PAYTOWITNESS' }, + { amount: '1000', script_type: 'PAYTOADDRESS' }, ], }, ], @@ -271,13 +271,13 @@ export default { fee: '278', totalSpent: '16678', inputs: [ - { amount: '7086' }, - { amount: '9426' }, { amount: '309896' }, // NOTE: this utxo is used because required utxo is not enough to cover fee + { amount: '9426' }, + { amount: '7086' }, ], outputs: [ - { amount: '16400', script_type: 'PAYTOADDRESS' }, { amount: '309730', script_type: 'PAYTOWITNESS' }, + { amount: '16400', script_type: 'PAYTOADDRESS' }, ], }, ], @@ -356,12 +356,12 @@ export default { totalSpent: '100226000', inputs: [{ script_type: 'SPENDADDRESS' }], outputs: [ + { amount: '399774000', script_type: 'PAYTOADDRESS' }, { address: 'DDn7UV1CrqVefzwrHyw7H2zEZZKqfzR2ZD', amount: '100000000', script_type: 'PAYTOADDRESS', }, - { amount: '399774000', script_type: 'PAYTOADDRESS' }, ], }, ], @@ -388,12 +388,12 @@ export default { totalSpent: '1326000', inputs: [{ script_type: 'SPENDADDRESS' }], outputs: [ + { amount: '498674000', script_type: 'PAYTOADDRESS' }, { address: 'DDn7UV1CrqVefzwrHyw7H2zEZZKqfzR2ZD', amount: '100000', script_type: 'PAYTOADDRESS', }, - { amount: '498674000', script_type: 'PAYTOADDRESS' }, ], }, ], @@ -427,12 +427,12 @@ export default { outputs: [ { address: 'DDn7UV1CrqVefzwrHyw7H2zEZZKqfzR2ZD', - amount: '200000000', + amount: '299000000', script_type: 'PAYTOADDRESS', }, { address: 'DDn7UV1CrqVefzwrHyw7H2zEZZKqfzR2ZD', - amount: '299000000', + amount: '200000000', script_type: 'PAYTOADDRESS', }, ], diff --git a/packages/connect/e2e/tests/device/methods.test.ts b/packages/connect/e2e/tests/device/methods.test.ts index e9d6ced2749..285dfea65bf 100644 --- a/packages/connect/e2e/tests/device/methods.test.ts +++ b/packages/connect/e2e/tests/device/methods.test.ts @@ -11,6 +11,14 @@ import { let controller: ReturnType | undefined; +jest.mock('@trezor/utils', () => ({ + ...jest.requireActual('@trezor/utils'), + + // After the removal bip69, we sort inputs and outputs randomly + // So we need to mock the source of randomness for all tests, so the fixtures are deterministic + getRandomInt: (min: number, max: number) => min + (4 % max), // 4 is truly random number, I rolled the dice +})); + const getFixtures = () => { const includedMethods = process.env.TESTS_INCLUDED_METHODS; const excludedMethods = process.env.TESTS_EXCLUDED_METHODS; diff --git a/packages/connect/src/constants/utxo.ts b/packages/connect/src/constants/utxo.ts index f1d8ad49027..f0f86f46db2 100644 --- a/packages/connect/src/constants/utxo.ts +++ b/packages/connect/src/constants/utxo.ts @@ -1,3 +1,3 @@ import type { TransactionInputOutputSortingStrategy } from '@trezor/utxo-lib'; -export const DEFAULT_SORTING_STRATEGY: TransactionInputOutputSortingStrategy = 'bip69'; +export const DEFAULT_SORTING_STRATEGY: TransactionInputOutputSortingStrategy = 'random'; diff --git a/packages/utxo-lib/src/compose/sorting/randomSortingStrategy.ts b/packages/utxo-lib/src/compose/sorting/randomSortingStrategy.ts new file mode 100644 index 00000000000..63f193ea2a8 --- /dev/null +++ b/packages/utxo-lib/src/compose/sorting/randomSortingStrategy.ts @@ -0,0 +1,36 @@ +import { SortingStrategy } from './sortingStrategy'; +import { convertOutput } from './convertOutput'; +import { arrayShuffle, getRandomInt } from '@trezor/utils'; + +export const randomSortingStrategy: SortingStrategy = ({ result, request, convertedInputs }) => { + const nonChangeOutputPermutation: number[] = []; + const changeOutputPermutation: number[] = []; + + const convertedOutputs = result.outputs.map((output, index) => { + if (request.outputs[index]) { + nonChangeOutputPermutation.push(index); + + return convertOutput(output, request.outputs[index]); + } + + changeOutputPermutation.push(index); + + return convertOutput(output, { type: 'change', ...request.changeAddress }); + }); + + /** + * The goal here is to randomly insert change outputs into the outputs array., + * so you cannot tell what is the change just by the order of the transaction. + */ + const permutation = [...nonChangeOutputPermutation]; + const newPositionOfChange = getRandomInt(0, permutation.length + 1); + permutation.splice(newPositionOfChange, 0, ...changeOutputPermutation); + const sortedOutputs = permutation.map(index => convertedOutputs[index]); + + return { + /** Randomly shuffle inputs to make it harder to fingerprint the Trezor Suite. */ + inputs: arrayShuffle(convertedInputs, { randomInt: getRandomInt }), + outputs: sortedOutputs, + outputsPermutation: permutation, + }; +}; diff --git a/packages/utxo-lib/src/compose/transaction.ts b/packages/utxo-lib/src/compose/transaction.ts index 80af81a6bb9..2596e607c39 100644 --- a/packages/utxo-lib/src/compose/transaction.ts +++ b/packages/utxo-lib/src/compose/transaction.ts @@ -10,10 +10,12 @@ import { import { noneSortingStrategy } from './sorting/noneSortingStrategy'; import { SortingStrategy } from './sorting/sortingStrategy'; import { bip69SortingStrategy } from './sorting/bip69SortingStrategy'; +import { randomSortingStrategy } from './sorting/randomSortingStrategy'; const strategyMap: Record = { bip69: bip69SortingStrategy, none: noneSortingStrategy, + random: randomSortingStrategy, }; export function createTransaction( diff --git a/packages/utxo-lib/src/types/compose.ts b/packages/utxo-lib/src/types/compose.ts index 1060835fe1a..13bb8937459 100644 --- a/packages/utxo-lib/src/types/compose.ts +++ b/packages/utxo-lib/src/types/compose.ts @@ -74,7 +74,7 @@ export type TransactionInputOutputSortingStrategy = // Inputs are randomized, outputs are kept as they were provided in the request, // and change is randomly placed somewhere between outputs - // | 'random' // Todo: will be implemented later in https://github.com/trezor/trezor-suite/issues/10765 + | 'random' // It keeps the inputs and outputs as they were provided in the request. // This is useful for RBF transactions where the order of inputs and outputs must be preserved. diff --git a/packages/utxo-lib/tests/__fixtures__/compose.ts b/packages/utxo-lib/tests/__fixtures__/compose.ts index 5b4424b5464..9c13c109427 100644 --- a/packages/utxo-lib/tests/__fixtures__/compose.ts +++ b/packages/utxo-lib/tests/__fixtures__/compose.ts @@ -30,6 +30,7 @@ type Fixture = { changeAddress: AnyComposeRequest['changeAddress'] & { path?: number[] | string }; }; result: AnyComposeResult; + randomIntSequence?: number[]; }; export const composeTxFixture: Fixture[] = [ @@ -524,6 +525,79 @@ export const composeTxFixture: Fixture[] = [ type: 'final', }, }, + { + description: + 'sorts the inputs randomly and puts change at random place between user-defined outputs' + + 'when sortingStrategy=random', + randomIntSequence: [ + 1, // For outputs (so change output is inserted at index 1 after the first output) + 1, // Shuffling inputs (Fisher-Yates swap last, the 3rd with second) + 0, // Shuffling inputs (Fisher-Yates swap second with the first) + ], + request: { + changeAddress: { address: '1CrwjoKxvdbAnPcGzYjpvZ4no4S71neKXT' }, + dustThreshold: 546, + feeRate: '10', + sortingStrategy: 'random', + outputs: [ + { + address: '1BitcoinEaterAddressDontSendf59kuE', + amount: '100000', + type: 'payment', + }, + { + address: '1LetUsDestroyBitcoinTogether398Nrg', + amount: '150000', + type: 'payment', + }, + ], + utxos: [ + { + ...UTXO, + vout: 3, + }, + { + ...UTXO, + vout: 2, + }, + { + ...UTXO, + vout: 1, + }, + ], + }, + result: { + bytes: 556, + fee: '5560', + feePerByte: '10', + max: undefined, + totalSpent: '255560', + inputs: [ + { ...UTXO, vout: 1 }, + { ...UTXO, vout: 3 }, + { ...UTXO, vout: 2 }, + ], + outputs: [ + { + address: '1BitcoinEaterAddressDontSendf59kuE', + amount: '100000', + type: 'payment', + }, + { + address: '1CrwjoKxvdbAnPcGzYjpvZ4no4S71neKXT', + amount: '50443', + type: 'change', + }, + { + address: '1LetUsDestroyBitcoinTogether398Nrg', + amount: '150000', + type: 'payment', + }, + ], + outputsPermutation: [0, 2, 1], + type: 'final', + }, + }, { description: 'builds a p2sh tx with two same value outputs (mixed p2sh + p2pkh) and change', request: { diff --git a/packages/utxo-lib/tests/compose.test.ts b/packages/utxo-lib/tests/compose.test.ts index 8120000d9a7..14037ef9808 100644 --- a/packages/utxo-lib/tests/compose.test.ts +++ b/packages/utxo-lib/tests/compose.test.ts @@ -5,6 +5,24 @@ import { verifyTxBytes } from './compose.utils'; import { composeTxFixture } from './__fixtures__/compose'; import { fixturesCrossCheck } from './__fixtures__/compose.crosscheck'; +import { getRandomInt } from '@trezor/utils'; + +jest.mock('@trezor/utils', () => ({ + ...jest.requireActual('@trezor/utils'), + getRandomInt: jest.fn(), +})); + +const mockRandomInt = (randomIntSequence: number[] | undefined) => { + let fakeRandomIndex = 0; + (getRandomInt as jest.Mock).mockImplementation(() => { + if (randomIntSequence === undefined || fakeRandomIndex >= randomIntSequence.length) { + throw new Error(`Not enough random numbers provided (i: ${fakeRandomIndex})`); + } + + return randomIntSequence?.[fakeRandomIndex++]; + }); +}; + describe(composeTx.name, () => { composeTxFixture.forEach(f => { const network = f.request.network ?? NETWORKS.bitcoin; @@ -12,6 +30,8 @@ describe(composeTx.name, () => { const result = { ...f.result }; it(f.description, () => { + mockRandomInt(f.randomIntSequence); + const tx = composeTx(request); expect(tx).toEqual(result); @@ -53,7 +73,7 @@ describe('composeTx addresses cross-check', () => { addrKeys.slice(0, offset).forEach(addressType => { const key = `${txType}-${addressType}` as keyof typeof f.result; - it(`${String(key)} ${f.description}`, () => { + it(`${key} ${f.description}`, () => { const tx = composeTx({ ...f.request, network: NETWORKS.bitcoin, From 053bc6b09b41756a4acf41221dc4a8f008d1dcca Mon Sep 17 00:00:00 2001 From: Peter Sanderson Date: Fri, 25 Oct 2024 08:54:26 +0200 Subject: [PATCH 2/4] fix: make reandom mocks work in Karma --- packages/connect/e2e/karma.config.js | 2 +- .../connect/e2e/tests/device/methods.test.ts | 25 ++++++++++++++++++- .../compose/sorting/randomSortingStrategy.ts | 1 + 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/packages/connect/e2e/karma.config.js b/packages/connect/e2e/karma.config.js index eba6f9ccf19..3836c2a0b1e 100644 --- a/packages/connect/e2e/karma.config.js +++ b/packages/connect/e2e/karma.config.js @@ -2,7 +2,7 @@ const path = require('path'); const webpack = require('webpack'); module.exports = config => { - const singleRun = process.env.KARMA_SINGLE_RUN === 'false' ? false : true; + const singleRun = process.env.KARMA_SINGLE_RUN !== 'false'; config.set({ basePath: path.resolve(__dirname, '../..'), // NOTE: "[monorepo-root]/packages", to have access to other packages diff --git a/packages/connect/e2e/tests/device/methods.test.ts b/packages/connect/e2e/tests/device/methods.test.ts index 285dfea65bf..bd62cf655f3 100644 --- a/packages/connect/e2e/tests/device/methods.test.ts +++ b/packages/connect/e2e/tests/device/methods.test.ts @@ -11,7 +11,28 @@ import { let controller: ReturnType | undefined; -jest.mock('@trezor/utils', () => ({ +// After the removal bip69, we sort inputs and outputs randomly +// So we need to mock the source of randomness for all tests, so the fixtures are deterministic. + +// However, we run those test both in Node.js and in browser environment, +// so we need to mock the source of randomness in both environments + +// This is mock of randomnes for Karma (web environment) +if (typeof window !== 'undefined') { + window.crypto.getRandomValues = array => { + if (array instanceof Uint32Array) { + array[0] = 4; + } + + return array; + }; +} + +// In Karma web environment, there is no `jest`, so we fake one +const _jest = typeof jest !== 'undefined' ? jest : { mock: () => undefined }; + +// Jest.mock() MUST be called in global scope, if we put it into condition it won't work. +_jest.mock('@trezor/utils', () => ({ ...jest.requireActual('@trezor/utils'), // After the removal bip69, we sort inputs and outputs randomly @@ -114,6 +135,8 @@ describe(`TrezorConnect methods`, () => { }); } + console.log('window', typeof window); + expect(result).toMatchObject(expected); }, t.customTimeout || 20000, diff --git a/packages/utxo-lib/src/compose/sorting/randomSortingStrategy.ts b/packages/utxo-lib/src/compose/sorting/randomSortingStrategy.ts index 63f193ea2a8..0bffe1c0c74 100644 --- a/packages/utxo-lib/src/compose/sorting/randomSortingStrategy.ts +++ b/packages/utxo-lib/src/compose/sorting/randomSortingStrategy.ts @@ -24,6 +24,7 @@ export const randomSortingStrategy: SortingStrategy = ({ result, request, conver */ const permutation = [...nonChangeOutputPermutation]; const newPositionOfChange = getRandomInt(0, permutation.length + 1); + permutation.splice(newPositionOfChange, 0, ...changeOutputPermutation); const sortedOutputs = permutation.map(index => convertedOutputs[index]); From 957df7e9c23432c1ab4ce23eac761b301843da0a Mon Sep 17 00:00:00 2001 From: Peter Sanderson Date: Fri, 25 Oct 2024 09:00:14 +0200 Subject: [PATCH 3/4] chore: add comments in test to make it clear that the order depends on mocked randomness --- .../connect/e2e/__fixtures__/composeTransaction.ts | 10 ++++++++++ packages/connect/e2e/tests/device/methods.test.ts | 5 ----- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/packages/connect/e2e/__fixtures__/composeTransaction.ts b/packages/connect/e2e/__fixtures__/composeTransaction.ts index 0dad3c0be8a..b1dda10b3e9 100644 --- a/packages/connect/e2e/__fixtures__/composeTransaction.ts +++ b/packages/connect/e2e/__fixtures__/composeTransaction.ts @@ -91,6 +91,7 @@ export default { feePerByte: '1', max: undefined, totalSpent: '6639', + // Order of inputs & outputs is important as we mock randomness in test to make it deterministic inputs: [{ script_type: 'SPENDWITNESS', sequence: 0xffffffff }], outputs: [ { amount: '2787', script_type: 'PAYTOWITNESS' }, @@ -125,6 +126,7 @@ export default { feePerByte: '2.176056338028169', max: undefined, totalSpent: '6806', + // Order of inputs & outputs is important as we mock randomness in test to make it deterministic inputs: [{ script_type: 'SPENDWITNESS', sequence: 1 }], outputs: [ { amount: '6497', script_type: 'PAYTOADDRESS' }, @@ -159,6 +161,7 @@ export default { feePerByte: '2.176056338028169', max: undefined, totalSpent: '6806', + // Order of inputs & outputs is important as we mock randomness in test to make it deterministic inputs: [{ script_type: 'SPENDWITNESS', sequence: 1 }], outputs: [ { amount: '6497', script_type: 'PAYTOADDRESS' }, @@ -210,6 +213,7 @@ export default { bytes: 142, fee: '142', totalSpent: '1142', + // Order of inputs & outputs is important as we mock randomness in test to make it deterministic inputs: [{ amount: '9426', script_type: 'SPENDWITNESS' }], outputs: [ { amount: '8284', script_type: 'PAYTOWITNESS' }, @@ -270,6 +274,7 @@ export default { bytes: 278, fee: '278', totalSpent: '16678', + // Order of inputs & outputs is important as we mock randomness in test to make it deterministic inputs: [ { amount: '309896' }, // NOTE: this utxo is used because required utxo is not enough to cover fee { amount: '9426' }, @@ -329,6 +334,7 @@ export default { feePerByte: '1', max: '9315', totalSpent: '9426', + // Order of inputs & outputs is important as we mock randomness in test to make it deterministic inputs: [{ script_type: 'SPENDWITNESS' }], outputs: [{ amount: '9315', script_type: 'PAYTOADDRESS' }], }, @@ -354,6 +360,7 @@ export default { fee: '226000', max: undefined, totalSpent: '100226000', + // Order of inputs & outputs is important as we mock randomness in test to make it deterministic inputs: [{ script_type: 'SPENDADDRESS' }], outputs: [ { amount: '399774000', script_type: 'PAYTOADDRESS' }, @@ -386,6 +393,7 @@ export default { fee: '1226000', // NOTE: +0.01 DOGE per dust limit output max: undefined, totalSpent: '1326000', + // Order of inputs & outputs is important as we mock randomness in test to make it deterministic inputs: [{ script_type: 'SPENDADDRESS' }], outputs: [ { amount: '498674000', script_type: 'PAYTOADDRESS' }, @@ -423,6 +431,7 @@ export default { fee: '1000000', // NOTE: +0.01 DOGE per dust limit output + ~0.08 DOGE dust limit change max: undefined, totalSpent: '500000000', + // Order of inputs & outputs is important as we mock randomness in test to make it deterministic inputs: [{ script_type: 'SPENDADDRESS' }], outputs: [ { @@ -459,6 +468,7 @@ export default { fee: '192000', max: '499808000', totalSpent: '500000000', + // Order of inputs & outputs is important as we mock randomness in test to make it deterministic inputs: [{ script_type: 'SPENDADDRESS' }], outputs: [{ amount: '499808000', script_type: 'PAYTOADDRESS' }], }, diff --git a/packages/connect/e2e/tests/device/methods.test.ts b/packages/connect/e2e/tests/device/methods.test.ts index bd62cf655f3..f8fed7b0166 100644 --- a/packages/connect/e2e/tests/device/methods.test.ts +++ b/packages/connect/e2e/tests/device/methods.test.ts @@ -34,9 +34,6 @@ const _jest = typeof jest !== 'undefined' ? jest : { mock: () => undefined }; // Jest.mock() MUST be called in global scope, if we put it into condition it won't work. _jest.mock('@trezor/utils', () => ({ ...jest.requireActual('@trezor/utils'), - - // After the removal bip69, we sort inputs and outputs randomly - // So we need to mock the source of randomness for all tests, so the fixtures are deterministic getRandomInt: (min: number, max: number) => min + (4 % max), // 4 is truly random number, I rolled the dice })); @@ -135,8 +132,6 @@ describe(`TrezorConnect methods`, () => { }); } - console.log('window', typeof window); - expect(result).toMatchObject(expected); }, t.customTimeout || 20000, From 7cd98993bec057ccf998f21b2ae3fd85363bb0fc Mon Sep 17 00:00:00 2001 From: Peter Sanderson Date: Fri, 25 Oct 2024 09:55:18 +0200 Subject: [PATCH 4/4] fix: make reandom mocks work in Karma (attempt 2) --- packages/connect/e2e/tests/device/methods.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/connect/e2e/tests/device/methods.test.ts b/packages/connect/e2e/tests/device/methods.test.ts index f8fed7b0166..8491b81269c 100644 --- a/packages/connect/e2e/tests/device/methods.test.ts +++ b/packages/connect/e2e/tests/device/methods.test.ts @@ -29,10 +29,12 @@ if (typeof window !== 'undefined') { } // In Karma web environment, there is no `jest`, so we fake one -const _jest = typeof jest !== 'undefined' ? jest : { mock: () => undefined }; +if (typeof jest === 'undefined') { + globalThis.jest = { mock: () => undefined } as any; +} // Jest.mock() MUST be called in global scope, if we put it into condition it won't work. -_jest.mock('@trezor/utils', () => ({ +jest.mock('@trezor/utils', () => ({ ...jest.requireActual('@trezor/utils'), getRandomInt: (min: number, max: number) => min + (4 % max), // 4 is truly random number, I rolled the dice }));