Skip to content

fix: optimize reading and decrypting keystores in batches - #5274

Closed
maschad wants to merge 6 commits into
ChainSafe:unstablefrom
maschad:decrypt-keystores-in-batches
Closed

fix: optimize reading and decrypting keystores in batches#5274
maschad wants to merge 6 commits into
ChainSafe:unstablefrom
maschad:decrypt-keystores-in-batches

Conversation

@maschad

@maschad maschad commented Mar 16, 2023

Copy link
Copy Markdown
Contributor

Closes #4179

last task is to process import of validator keystores in batches, we need to find a way to determine this batchSize @nflaig had suggested os.availableParallelism() but that API is not in stable Node as of now.

@maschad maschad changed the title fix: optimize reading and decrypting keystores in batches for better … fix: optimize reading and decrypting keystores in batches Mar 16, 2023
path: string,
batch: string[],
resolve: (value: LocalKeystoreDefinition[]) => void,
reject: (err: Error) => void

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm very confused by this pattern, why are the Promise callbacks being passed as arguments?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The idea was break the file down into batches, and then read from the file in batches of 5-10. I think using worker threads may make a better alternative though.

@nflaig nflaig Mar 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think using worker threads may make a better alternative though

isn't most of the work of BLS Keystore decrypt executed in a separate thread already?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes but the process of reading the files itself is not multithreaded, nor is the decryption of these multiple keys, which is what I assumed was the rationale behind

Instead of decrypting 100 keystores at once, decrypting 5-10 is fine, which provides a significant speed up. A good number would be to decrypt thread count at once

and perhaps the bottleneck for situations like this https://twitter.com/Stefa2kEth/status/1628407293325807623?s=20

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nor is the decryption of these multiple keys

would have to dig into how bls-keystore implements this but I just assumed this was not executed in the main thread

and perhaps the bottleneck for situations like this

the main issue is that we decrypt just one at a time, and running all in sequence caused a OOM error

Would be curious if running this in a worker actually makes a noticeable difference

@maschad maschad Mar 20, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

would have to dig into how bls-keystore implements this but I just assumed this was not executed in the main thread

The decryption itself happens within a thread you may be referring to the verification which split large signature sets into smaller sets and then sends them to different workers, I imagine a similar strategy could be employed here for decrypting the keys

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

would be interesting to see what acutally takes up the time in decryptKeystoreDefinitions

export async function decryptKeystoreDefinitions(

I just assumed as of now that keystore.decrypt takes the vast majority of the time and it should not be executed in the main thread, I would be really surprised if it was

const secretKeyBytes = await keystore.decrypt(password);

Comment thread packages/cli/src/cmds/validator/signers/index.ts
@dapplion

Copy link
Copy Markdown
Contributor

Massive diff, there have been some issue on git rebase or merge please double check

@wemeetagain

Copy link
Copy Markdown
Member

Closing in favor of #5357

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.

Optimize UX loading +100 keystores on vc

4 participants