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

Replace cryptographically secure random with regular random #1088

Merged
merged 1 commit into from
Aug 5, 2023

Conversation

FireMasterK
Copy link
Member

  • I carefully read the contribution guidelines and agree to them.
  • I have tested the API against NewPipe.
  • I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.

There's no need for a cryptographically secure random, even though the JavaScript code uses one.

@Stypox
Copy link
Member

Stypox commented Aug 4, 2023

Out of curiosity, did you notice this because of performance hiccups?

@FireMasterK
Copy link
Member Author

Out of curiosity, did you notice this because of performance hiccups?

Yes, I did notice it in my profiler. It wasn't necessarily a hiccup, but an improvement I saw could be helpful. SecureRandom seems to do blocking IO according to my profiler. (not sure why)

@Stypox
Copy link
Member

Stypox commented Aug 4, 2023

I guess the underlying implementation uses /dev/urandom, which is based on an entropy pool, which needs to fill up from various noise sources and sensors

@Stypox Stypox merged commit de0a9bb into TeamNewPipe:dev Aug 5, 2023
3 of 4 checks passed
@FireMasterK FireMasterK deleted the pseudo-rng branch August 5, 2023 11:37
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.

None yet

2 participants