Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve random int #14828

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/coinjoin/src/client/round/endedRound.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { enumUtils, getRandomNumberInRange } from '@trezor/utils';
import { enumUtils, getWeakRandomNumberInRange } from '@trezor/utils';

import type { CoinjoinRound, CoinjoinRoundOptions } from '../CoinjoinRound';
import { EndRoundState, WabiSabiProtocolErrorCode } from '../../enums';
Expand Down Expand Up @@ -56,7 +56,7 @@ export const ended = (round: CoinjoinRound, { logger, network }: CoinjoinRoundOp
// repeated input-registration will tell if they are really banned,
// make sure that addresses registered in round are recycled (reset Infinity sentence)
const minute = 60 * 1000;
const sentenceEnd = getRandomNumberInRange(5 * minute, 10 * minute);
const sentenceEnd = getWeakRandomNumberInRange(5 * minute, 10 * minute);
[...inputs, ...addresses].forEach(vinvout =>
prison.detain(vinvout, {
sentenceEnd,
Expand Down
4 changes: 2 additions & 2 deletions packages/coinjoin/src/client/round/inputRegistration.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getRandomNumberInRange } from '@trezor/utils';
import { getWeakRandomNumberInRange } from '@trezor/utils';

import * as coordinator from '../coordinator';
import * as middleware from '../middleware';
Expand Down Expand Up @@ -56,7 +56,7 @@ const registerInput = async (
// setup random delay for registration request. we want each input to be registered in different time as different TOR identity
// note that this may cause that the input will not be registered if phase change before expected deadline
const deadline = round.phaseDeadline - Date.now() - ROUND_SELECTION_REGISTRATION_OFFSET;
const delay = deadline > 0 ? getRandomNumberInRange(0, deadline) : 0;
const delay = deadline > 0 ? getWeakRandomNumberInRange(0, deadline) : 0;
logger.info(
`Trying to register ~~${input.outpoint}~~ to ~~${round.id}~~ with delay ${delay}ms and deadline ${round.phaseDeadline}`,
);
Expand Down
4 changes: 2 additions & 2 deletions packages/coinjoin/src/utils/roundUtils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { bufferutils, Transaction, Network } from '@trezor/utxo-lib';
import { getRandomNumberInRange } from '@trezor/utils';
import { getWeakRandomNumberInRange } from '@trezor/utils';

import {
COORDINATOR_FEE_RATE_FALLBACK,
Expand Down Expand Up @@ -88,7 +88,7 @@ export const scheduleDelay = (
// and at most 1 sec before the calculated max (so there's room for randomness)
const min = clamp(minimumDelay, 0, max - 1000);

return getRandomNumberInRange(min, max);
return getWeakRandomNumberInRange(min, max);
};

// NOTE: deadlines are not accurate. phase may change earlier
Expand Down
4 changes: 2 additions & 2 deletions packages/coinjoin/tests/client/CoinjoinRound.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ describe(`CoinjoinRound`, () => {

it('onPhaseChange lock cool off resolved', async () => {
const delayMock = jest
.spyOn(trezorUtils, 'getRandomNumberInRange')
.spyOn(trezorUtils, 'getWeakRandomNumberInRange')
.mockImplementation(() => 800);

const constantsMock = jest
Expand Down Expand Up @@ -396,7 +396,7 @@ describe(`CoinjoinRound`, () => {

it('unregisterAccount when round is locked', async () => {
const delayMock = jest
.spyOn(trezorUtils, 'getRandomNumberInRange')
.spyOn(trezorUtils, 'getWeakRandomNumberInRange')
.mockImplementation(() => 800);

const constantsMock = jest
Expand Down
8 changes: 4 additions & 4 deletions packages/coinjoin/tests/client/transactionSigning.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { networks } from '@trezor/utxo-lib';
import { getRandomNumberInRange } from '@trezor/utils';
import { getWeakRandomNumberInRange } from '@trezor/utils';

import { transactionSigning } from '../../src/client/round/transactionSigning';
import { createServer } from '../mocks/server';
Expand Down Expand Up @@ -529,7 +529,7 @@ describe('transactionSigning signature delay', () => {
);

// signature is sent in range 17-67 sec. (resolve time is less than 50 sec TX_SIGNING_DELAY)
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(17000, 67000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(17000, 67000);
expect(response.isSignedSuccessfully()).toBe(true);
});

Expand Down Expand Up @@ -558,7 +558,7 @@ describe('transactionSigning signature delay', () => {
);

// signature is sent in range 0-46.21 sec. (resolve time is greater than 50 sec of TX_SIGNING_DELAY)
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(0, 46210);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(0, 46210);
expect(response.isSignedSuccessfully()).toBe(true);
});

Expand Down Expand Up @@ -588,7 +588,7 @@ describe('transactionSigning signature delay', () => {
);

// signature is sent in default range 0-1 sec.
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(0, 1000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(0, 1000);
expect(response.isSignedSuccessfully()).toBe(true);
});
});
20 changes: 10 additions & 10 deletions packages/coinjoin/tests/utils/roundUtils.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getRandomNumberInRange } from '@trezor/utils';
import { getWeakRandomNumberInRange } from '@trezor/utils';

import {
getCommitmentData,
Expand Down Expand Up @@ -150,38 +150,38 @@ describe('roundUtils', () => {

// default (no min, no max) range 0-10 sec.
resultInRange(scheduleDelay(60000), 0, 10000);
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(0, 10000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(0, 10000);

// range 3-10sec.
resultInRange(scheduleDelay(20000, 3000), 3000, 10000);
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(3000, 10000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(3000, 10000);

// deadlineOffset < 0, range 0-1 sec.
resultInRange(scheduleDelay(1000, 3000), 0, 1000);
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(0, 1000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(0, 1000);

// deadline < min, range 9-10 sec.
resultInRange(scheduleDelay(60000, 61000), 9000, 10000);
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(9000, 10000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(9000, 10000);

// deadline < min && deadline < max, range 49-50 sec.
resultInRange(scheduleDelay(60000, 61000, 62000), 49000, 50000);
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(49000, 50000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(49000, 50000);

// deadline > min && deadline < max, range 3-20 sec.
resultInRange(scheduleDelay(30000, 3000, 50000), 3000, 20000);
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(3000, 20000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(3000, 20000);

// min < 0 && deadline < max && deadlineOffset > 0, range 0-2.5 sec.
resultInRange(scheduleDelay(12500, -3000, 50000), 0, 2500);
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(0, 2500);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(0, 2500);

// min < 0 && max < 0 && deadlineOffset > 0, range 0-1 sec.
resultInRange(scheduleDelay(12500, -10000, -5000), 0, 1000);
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(0, 1000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(0, 1000);

// min < 0 && max < 0 && deadlineOffset < 0, range 0-1 sec.
resultInRange(scheduleDelay(7500, -10000, -5000), 0, 1000);
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(0, 1000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(0, 1000);
});
});
1 change: 1 addition & 0 deletions packages/connect-popup/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"@trezor/device-utils": "workspace:*",
"@trezor/transport": "workspace:*",
"@trezor/urls": "workspace:*",
"crypto-browserify": "^3.12.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and what about stream-browserify. this is transient?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I used the same approach as was in suite package

"eth-phishing-detect": "^1.2.0",
"react-dom": "18.2.0"
},
Expand Down
5 changes: 5 additions & 0 deletions packages/connect-popup/webpack/prod.webpack.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ const config: webpack.Configuration = {
extensions: ['.ts', '.tsx', '.js', '.jsx'],
modules: ['node_modules'],
mainFields: ['browser', 'module', 'main'],
fallback: {
// Polyfills crypto API for Node.js libraries in the browser. 'crypto' does not run without 'stream'
crypto: require.resolve('crypto-browserify'),
stream: require.resolve('stream-browserify'),
},
},
plugins: [
new DefinePlugin({
Expand Down
3 changes: 2 additions & 1 deletion packages/connect-web/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@
"dependencies": {
"@trezor/connect": "workspace:*",
"@trezor/connect-common": "workspace:*",
"@trezor/utils": "workspace:*"
"@trezor/utils": "workspace:*",
"crypto-browserify": "^3.12.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dependency or devDependency ? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends... 🤣

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should, imho, be only devDependency. because we only need it to produce lib using build:lib. it shouldn't be needed in the runtime of this package at all I believe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

connect-web itself only imports scheduleAction, createDeferred and createDeferredManager

},
"devDependencies": {
"@babel/preset-typescript": "^7.24.7",
Expand Down
7 changes: 7 additions & 0 deletions packages/connect-web/webpack/dev.webpack.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ const dev = {
libraryTarget: 'umd',
libraryExport: 'default',
},
resolve: {
fallback: {
// Polyfills crypto API for NodeJS libraries in the browser. 'crypto' does not run without 'stream'
crypto: require.resolve('crypto-browserify'),
stream: require.resolve('stream-browserify'),
},
},
plugins: [
// connect-web dev needs to be served from https
// to allow injection in 3rd party builds using trezor-connect-src param
Expand Down
4 changes: 4 additions & 0 deletions packages/connect-web/webpack/prod.webpack.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ const config: webpack.Configuration = {
mainFields: ['browser', 'module', 'main'],
extensions: ['.ts', '.js'],
fallback: {
// Polyfills crypto API for Node.js libraries in the browser. 'crypto' does not run without 'stream'
crypto: require.resolve('crypto-browserify'),
stream: require.resolve('stream-browserify'),

fs: false, // ignore "fs" import in markdown-it-imsize
path: false, // ignore "path" import in markdown-it-imsize
},
Expand Down
1 change: 1 addition & 0 deletions packages/connect-webextension/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
"@trezor/connect-common": "workspace:*",
"@trezor/connect-web": "workspace:*",
"@trezor/utils": "workspace:*",
"crypto-browserify": "^3.12.0",
"events": "^3.3.0"
},
"devDependencies": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ const config: webpack.Configuration = {
modules: ['node_modules'],
mainFields: ['browser', 'module', 'main'],
extensions: ['.ts', '.js'],
fallback: {
// Polyfills crypto API for Node.js libraries in the browser. 'crypto' does not run without 'stream'
crypto: require.resolve('crypto-browserify'),
stream: require.resolve('stream-browserify'),
},
},
performance: {
hints: false,
Expand Down
2 changes: 1 addition & 1 deletion packages/suite-build/configs/base.webpack.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ const config: webpack.Configuration = {
src: path.resolve(__dirname, '../../suite/src/'),
},
fallback: {
// Polyfills crypto API for NodeJS libraries in the browser. 'crypto' does not run without 'stream'
// Polyfills crypto API for Node.js libraries in the browser. 'crypto' does not run without 'stream'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I received a notice, that this is the official™ spelling is Node.js. 🤷

crypto: require.resolve('crypto-browserify'),
stream: require.resolve('stream-browserify'),
// Not required
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { useState } from 'react';
import styled from 'styled-components';
import { Card, Column, variables } from '@trezor/components';
import { Translation } from 'src/components/suite';
import { getRandomNumberInRange } from '@trezor/utils';
import { getWeakRandomNumberInRange } from '@trezor/utils';
import { typography } from '@trezor/theme';

const NoResults = styled.div`
Expand Down Expand Up @@ -46,7 +46,7 @@ const getTip = (num: number) => {
};

export const NoSearchResults = () => {
const [tip] = useState(getRandomNumberInRange(1, 10));
const [tip] = useState(getWeakRandomNumberInRange(1, 10));

return (
<Card>
Expand Down
1 change: 1 addition & 0 deletions packages/transport-bridge/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
"@trezor/theme": "workspace:*",
"@trezor/transport": "workspace:*",
"@trezor/utils": "workspace:*",
"crypto-browserify": "^3.12.0",
"json-stable-stringify": "^1.1.1",
"react": "18.2.0",
"react-dom": "18.2.0",
Expand Down
5 changes: 5 additions & 0 deletions packages/transport-bridge/webpack/ui.webpack.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ const config: webpack.Configuration = {
extensions: ['.ts', '.tsx', '.js', '.jsx'],
modules: ['node_modules'],
mainFields: ['browser', 'module', 'main'],
fallback: {
// Polyfills crypto API for Node.js libraries in the browser. 'crypto' does not run without 'stream'
crypto: require.resolve('crypto-browserify'),
stream: require.resolve('stream-browserify'),
},
},
plugins: [
new HtmlWebpackPlugin({
Expand Down
3 changes: 2 additions & 1 deletion packages/transport-test/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"test:e2e:new-bridge:hw": "USE_HW=true USE_NODE_BRIDGE=true yarn test:e2e:bridge",
"test:e2e:new-bridge:emu": "USE_HW=false USE_NODE_BRIDGE=true yarn test:e2e:bridge",
"build:e2e:api:node": "yarn esbuild ./e2e/api/api.test.ts --bundle --outfile=./e2e/dist/api.test.node.js --platform=node --target=node18 --external:usb",
"build:e2e:api:browser": "yarn esbuild ./e2e/api/api.test.ts --bundle --outfile=./e2e/dist/api.test.browser.js --platform=browser --external:usb && cp e2e/ui/api.test.html e2e/dist/index.html",
"build:e2e:api:browser": "yarn esbuild ./e2e/api/api.test.ts --bundle --alias:crypto=crypto-browserify --alias:stream=stream-browserify --outfile=./e2e/dist/api.test.browser.js --platform=browser --external:usb && cp e2e/ui/api.test.html e2e/dist/index.html",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took me a while to figure this out: https://esbuild.github.io/api/#alias

"test:e2e:api:node:hw": "yarn build:e2e:api:node && node ./e2e/dist/api.test.node.js",
"test:e2e:api:browser:hw": "yarn build:e2e:api:browser && npx http-serve ./e2e/dist"
},
Expand All @@ -25,6 +25,7 @@
"@trezor/trezor-user-env-link": "workspace:^",
"@trezor/utils": "workspace:*",
"buffer": "^6.0.3",
"crypto-browserify": "^3.12.0",
"esbuild": "^0.23.1",
"jest": "^29.7.0",
"jest-extended": "^4.0.2",
Expand Down
13 changes: 9 additions & 4 deletions packages/utils/src/arrayShuffle.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
import { getRandomInt } from './getRandomInt';

/**
* Randomly shuffles the elements in an array. This method
* does not mutate the original array.
* Implementation of the Fisher-Yates shuffle algorithm.
* The algorithm produces an unbiased permutation: every permutation is equally likely.
* @link https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle
*
* This method does not mutate the original array.
*/
export const arrayShuffle = <T>(array: readonly T[]) => {
export const arrayShuffle = <T>(array: readonly T[]): T[] => {
const shuffled = array.slice();
for (let i = shuffled.length - 1; i > 0; i--) {
const j = Math.floor(Math.random() * (i + 1));
const j = getRandomInt(0, i);
[shuffled[i], shuffled[j]] = [shuffled[j], shuffled[i]];
}

Expand Down
17 changes: 17 additions & 0 deletions packages/utils/src/getRandomInt.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { randomBytes } from 'crypto';

/**
* Crypto.randomInt() function is not implemented by polyfill 'crypto-browserify'
* @see https://github.com/browserify/crypto-browserify/issues/224
*/
export const getRandomInt = (min: number, max: number) => {
if (min >= max) {
throw new RangeError(
`The value of "max" is out of range. It must be greater than the value of "min" (${min}). Received ${max}`,
);
}

const randomValue = parseInt(randomBytes(4).toString('hex'), 16);

return min + (randomValue % (max - min));
};
2 changes: 0 additions & 2 deletions packages/utils/src/getRandomNumberInRange.ts

This file was deleted.

5 changes: 5 additions & 0 deletions packages/utils/src/getWeakRandomNumberInRange.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
/**
* @deprecated Consider using `getRandomInt` which is cryptographically secure.
*/
export const getWeakRandomNumberInRange = (min: number, max: number) =>
Math.floor(Math.random() * (max - min + 1)) + min;
3 changes: 2 additions & 1 deletion packages/utils/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export * from './createTimeoutPromise';
export * from './getLocaleSeparators';
export * from './getMutex';
export * from './getNumberFromPixelString';
export * from './getRandomNumberInRange';
export * from './getWeakRandomNumberInRange';
export * from './getSynchronize';
export * from './getWeakRandomId';
export * from './hasUppercaseLetter';
Expand All @@ -40,6 +40,7 @@ export * from './topologicalSort';
export * from './truncateMiddle';
export * from './typedEventEmitter';
export * from './urlToOnion';
export * from './getRandomInt';
export * from './logs';
export * from './logsManager';
export * from './bigNumber';
Expand Down
11 changes: 8 additions & 3 deletions packages/utils/tests/arrayShuffle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,24 @@ const SAMPLES = 10000;
const TOLERANCE = 0.1;
const EXPECTED = SAMPLES / KEYS.length;

describe('arrayShuffle', () => {
const LOWER_BOUND = (1 - TOLERANCE) * EXPECTED;
const UPPER_BOUND = (1 + TOLERANCE) * EXPECTED;

describe(arrayShuffle.name, () => {
it('shuffles randomly', () => {
const samples = Object.fromEntries(KEYS.map(key => [key, new Array(KEYS.length).fill(0)]));

for (let sample = 0; sample < SAMPLES; ++sample) {
const shuffled = arrayShuffle(KEYS);
for (let i = 0; i < shuffled.length; ++i) {
samples[shuffled[i]][i]++;
}
}

KEYS.forEach(key =>
samples[key].forEach(count => {
expect(count).toBeGreaterThanOrEqual((1 - TOLERANCE) * EXPECTED);
expect(count).toBeLessThanOrEqual((1 + TOLERANCE) * EXPECTED);
expect(count).toBeGreaterThanOrEqual(LOWER_BOUND);
expect(count).toBeLessThanOrEqual(UPPER_BOUND);
}),
);
});
Expand Down
Loading
Loading