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

Vc/io1 2884 improvements #37

Merged
merged 26 commits into from
May 7, 2023
Merged

Vc/io1 2884 improvements #37

merged 26 commits into from
May 7, 2023

Conversation

vadiminc
Copy link
Contributor

@vadiminc vadiminc commented May 3, 2023

Fixes #<issue_number_goes_here>

It's a good idea to open an issue first for discussion.

  • Tests pass
  • Appropriate changes to README are included in PR

@vadiminc vadiminc requested a review from meshin-dev May 3, 2023 11:12
examples/server-worker/index.ts Show resolved Hide resolved
src/commands/actions/KeySharesAction.ts Outdated Show resolved Hide resolved
src/commands/actions/KeySharesAction.ts Show resolved Hide resolved
src/lib/KeyShares/KeyShares.ts Outdated Show resolved Hide resolved
src/lib/KeyShares/KeyShares.ts Outdated Show resolved Hide resolved
src/lib/KeyShares/KeyShares.ts Show resolved Hide resolved
src/lib/KeyShares/KeyShares.ts Show resolved Hide resolved
src/commands/actions/KeySharesAction.ts Show resolved Hide resolved
src/lib/SSVKeys.ts Outdated Show resolved Hide resolved
src/lib/helpers/operator.helper.ts Outdated Show resolved Hide resolved
src/commands/actions/KeySharesAction.ts Show resolved Hide resolved
}
async extractKeys(data: string, password: string): Promise<ExtractedKeys> {
const privateKey = await new EthereumKeyStore(data).getPrivateKey(password);
await bls.init(bls.BLS12_381);
Copy link
Contributor

Choose a reason for hiding this comment

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

I look inside of bls.init method and it's heavy. If we need parallel workers to be fast we should init it only once. Please implement this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@meshin-blox please review

Copy link
Contributor

Choose a reason for hiding this comment

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

@vadiminc I don't see it is initializing only once, did you commit it?

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 did, it ones. extractKeys always works as "entry point". just as exception is that someone will want to work with Threshold directly, on that case we have "try catch" to understand if need to init anyway in low level @meshin-blox

Copy link
Contributor

Choose a reason for hiding this comment

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

If you take a look here:
https://github.com/bloxapp/ssv-keys/pull/37/files#diff-c127a9a3b0b9b65b6bb86b567f9cd4013f3f8b0bb89019301c2846b3882e7157L56

extractKeys is called inside of expressjs route, it means that each time you run this endpoint in parallel nature it will run bls.init, and inside of it it will do that heavy work each time, even more - with the same instance of bls object.

Regarding the fact that ssv-keys strictly works with only one BLS type (12_381), to initialize it only once and bring simple check:

if (!this.initialized) bls.init()

const ssvKeys = new SSVKeys(SSVKeys.VERSION.V3);
const privateKey = await ssvKeys.getPrivateKeyFromKeystoreData(keystore, password)
const encryptedShares = await ssvKeys.buildShares(privateKey, operators_ids, operators_keys);
const ssvKeys = new SSVKeys();
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if you will init bls only once, you still need to extract new SSVKeys into level outside of express route

@vadiminc vadiminc merged commit ace9d6b into v3 May 7, 2023
@vadiminc vadiminc deleted the vc/IO1-2884-improvements branch May 7, 2023 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants