Add API for Random Module - Closes #6779 #6860

ishantiw · 2021-10-22T00:38:13Z

What was the problem?

This PR resolves #6779

How was it solved?

🌱 Add random module utils cd1b814

Add isSeedRevealValid util
randomByte util
bitwiseXOR

🌱 Add randomModule API 51d3bcb

isSeedRevealValid
getRandomBytes

✅ Add unit test for randomModule API c734169

How was it tested?

Run npm run test:unit random/api.spec under framework

- Add isSeedRevealValid util - randomByte util - bitwiseXOR

- isSeedRevealValid - getRandomBytes

…add-api-rand-module

mehmetegemen · 2021-10-22T14:01:17Z

framework/src/modules/random/utils.ts

+	return false;
+};
+
+export const randomBytesUtil = (


maybe getRandomSeed as name?

mehmetegemen · 2021-10-22T14:01:49Z

framework/src/modules/random/utils.ts

+import { SEED_REVEAL_HASH_SIZE } from './constants';
+import { ValidatorSeedReveal } from './types';
+
+export const isSeedRevealValidUtil = (


maybe getSeedRevealValidity or checkSeedRevealValidity as name?

mehmetegemen · 2021-10-22T14:02:57Z

framework/src/modules/random/utils.ts

+	const initRandomBuffer = Buffer.allocUnsafe(4);
+	initRandomBuffer.writeInt32BE(height + numberOfSeeds, 0);


what do you think about using intToBuffer from lisk-cryptography here with signed parameter true?

true I forgot about this func in crypto lib, but I think we need unsigned in this case

there is write**U**Int32BE for unsigned, since you didn't use it I thought you don't need unsigned.

mehmetegemen · 2021-10-22T14:16:15Z

framework/src/modules/random/utils.ts

+	const bufferSizes = new Set(bufferArray.map(buffer => buffer.length));
+	if (bufferSizes.size > 1) {
+		throw new Error('All input for XOR should be same size');
+	}
+	const outputSize = [...bufferSizes][0];


What do you think about this? (maybe with better naming)

const isBufferLengthSame = bufferArray.every((elem, _, arr) => elem.length === arr[0].length); if (!isBufferLengthSame) { throw new Error('All input for XOR should be same size'); } const outputSize = bufferArray[0].length;

I think we can improve this later, I copied this function from other place, it better to resolve it in a separate issue. Also, we can move to common utils if we find more usage of this func

mehmetegemen · 2021-10-22T14:24:13Z

framework/test/unit/modules/random/api.spec.ts

+			const seed = genesisDelegates.delegates[4].hashOnion.hashes[0];
+			const hashes = cryptography.hashOnion(
+				Buffer.from(seed, 'hex'),
+				genesisDelegates.delegates[0].hashOnion.distance,


Shouldn't we correspond the same index even if they are same value? genesisDelegates.delegates[4].hashOnion.distance

mehmetegemen · 2021-10-22T14:26:23Z

framework/test/unit/modules/random/api.spec.ts

+			const initRandomBuffer = Buffer.allocUnsafe(4);
+			initRandomBuffer.writeInt32BE(height + numberOfSeeds, 0);


same intToBuffer comment for here

mehmetegemen · 2021-10-22T14:46:30Z

And also I think you should test failure edge cases of util functions but maybe you are saving them for another issue?

- Rename util isSeedRevealValidUtil->getSeedRevealValidity - Rename util randomBytesUtil->getRandomSeed - Use intToBuffer - Use same delegate fixture for seed reveal validity

…lisk-sdk into 6779-add-api-rand-module * '6779-add-api-rand-module' of https://github.com/LiskHQ/lisk-sdk: 🔥 Remove constant RANDOM_SEED_BYTE_SIZE

shuse2

Don't we have protocol spec for random seed?

shuse2 · 2021-10-22T12:55:21Z

framework/src/modules/random/api.ts

+	public async isSeedRevealValid(
+		apiContext: ImmutableAPIContext,
+		generatorAddress: Buffer,
+		seedReveal: Buffer,


i think i missed this on LIP as well, but this should be blockAssets: BlockAssets, so that dpos module or any other module don't need to know how to decode the asset

shuse2 · 2021-10-22T15:26:21Z

framework/src/modules/random/utils.ts

+	const largestSeedHeight = Math.max(
+		...validatorsReveal
+			.filter(sr => sr.generatorAddress.equals(generatorAddress))
+			.map(s => s.height),
+	);
+
+	const lastSeed = validatorsReveal.find(
+		(seedObject: ValidatorSeedReveal) =>
+			seedObject.height === largestSeedHeight &&
+			seedObject.generatorAddress.equals(generatorAddress),
+	);


instead of looping multiple times, i think simple one for loop with outside let can solve the problem?

- Improve getSeedRevealValidity func

framework/src/modules/random/api.ts

ishantiw · 2021-10-22T16:29:46Z

Update test to use protocol-specs in Issue #6780

mehmetegemen

Great job Ishan 🎉

ishantiw added 3 commits October 22, 2021 02:31

🌱 Add random module utils

cd1b814

- Add isSeedRevealValid util - randomByte util - bitwiseXOR

🌱 Add randomModule API

51d3bcb

- isSeedRevealValid - getRandomBytes

✅ Add unit test for randomModule API

c734169

ishantiw requested a review from shuse2 October 22, 2021 00:38

ishantiw self-assigned this Oct 22, 2021

ishantiw requested a review from mehmetegemen October 22, 2021 07:47

ishantiw added 3 commits October 22, 2021 12:10

✅ Fix test for isSeedRevealValid

c7a5b5b

Merge branch 'feature/6554-improve-framework-architecture' into 6779-…

7863630

…add-api-rand-module

🔥 Remove constant RANDOM_SEED_BYTE_SIZE

46a3206

mehmetegemen reviewed Oct 22, 2021

View reviewed changes

ishantiw added 3 commits October 22, 2021 17:14

💅 Rename util funcs

57838e4

- Rename util isSeedRevealValidUtil->getSeedRevealValidity - Rename util randomBytesUtil->getRandomSeed - Use intToBuffer - Use same delegate fixture for seed reveal validity

Merge branch '6779-add-api-rand-module' of https://github.com/LiskHQ/…

4ccb5ea

…lisk-sdk into 6779-add-api-rand-module * '6779-add-api-rand-module' of https://github.com/LiskHQ/lisk-sdk: 🔥 Remove constant RANDOM_SEED_BYTE_SIZE

♻️ Reject negative height and numberOfSeeds value for getRandomSeed

de2a46e

shuse2 suggested changes Oct 22, 2021

View reviewed changes

ishantiw requested review from mehmetegemen and shuse2 October 22, 2021 16:06

♻️ Pass BlockAsset to isSeedRevealValid API

6bba550

- Improve getSeedRevealValidity func

ishantiw force-pushed the 6779-add-api-rand-module branch from 8419e1f to 6bba550 Compare October 22, 2021 16:08

shuse2 approved these changes Oct 22, 2021

View reviewed changes

framework/src/modules/random/api.ts Outdated Show resolved Hide resolved

✅ Add test when block asset is undefined

b369b74

ishantiw force-pushed the 6779-add-api-rand-module branch from d05cf36 to b369b74 Compare October 22, 2021 16:24

🔥 Remove BIG_ENDIAN as its by default

189a847

mehmetegemen approved these changes Oct 25, 2021

View reviewed changes

shuse2 merged commit 6b16cc9 into feature/6554-improve-framework-architecture Oct 25, 2021

shuse2 deleted the 6779-add-api-rand-module branch October 25, 2021 11:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API for Random Module - Closes #6779 #6860

Add API for Random Module - Closes #6779 #6860

ishantiw commented Oct 22, 2021

mehmetegemen Oct 22, 2021

mehmetegemen Oct 22, 2021

mehmetegemen Oct 22, 2021

ishantiw Oct 22, 2021

mehmetegemen Oct 22, 2021

mehmetegemen Oct 22, 2021

ishantiw Oct 22, 2021

mehmetegemen Oct 22, 2021

mehmetegemen Oct 22, 2021

mehmetegemen commented Oct 22, 2021

shuse2 left a comment

shuse2 Oct 22, 2021

shuse2 Oct 22, 2021

ishantiw commented Oct 22, 2021

mehmetegemen left a comment

		const initRandomBuffer = Buffer.allocUnsafe(4);
		initRandomBuffer.writeInt32BE(height + numberOfSeeds, 0);

Add API for Random Module - Closes #6779 #6860

Add API for Random Module - Closes #6779 #6860

Conversation

ishantiw commented Oct 22, 2021

What was the problem?

How was it solved?

How was it tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehmetegemen commented Oct 22, 2021

shuse2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishantiw commented Oct 22, 2021

mehmetegemen left a comment

Choose a reason for hiding this comment