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

feat: Bye bye bcrypto #302

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

acolytec3
Copy link
Contributor

This is a redo of #197 and fixes #301.

Problem: discv5 uses bcrypto dependency for performant secp256k1 and sha256 functions. This library causes occasional CI failures in ultralight due to in explicable OOMs and other indecipherable C++ errors

Solution: Replace bcrypto usage with a discv5Crypto interface similar to the v4Crypto interface exposed by the enr package. As currently stands, this PR implements a defaultCrypto object using noble/hashes and noble/secp256k1 that are pure javascript implementation and are somewhat slower than the bcrypto equivalents but can easily be replaced with WASM compiled alternatives such as as-sha256 or polkadot/wasm-crypto.

Sidequest 1: Replace bcrypto AES cipher functionality with nodejs builtins that are overall 5-10% faster.
Sidequest 2: Replace bcrypto randomBytes function with noble equivalent

@acolytec3 acolytec3 requested a review from a team as a code owner January 3, 2025 20:07
@acolytec3
Copy link
Contributor Author

@wemeetagain Here's the initial draft. I haven't implemented the wasm-crypto alternative to noble yet (that is much more performant), but this basically what I'm thinking.

@acolytec3
Copy link
Contributor Author

Note, the CI is failing because of a quirk in noble/hashes related to Node 18 and older versions. Would you be open to just bumping the nodejs version to 20 in CI to eliminate this issue? If not, I'll do the required shim but seems kind of silly given node 18 is EOL at this point.

@acolytec3
Copy link
Contributor Author

Note, the CI is failing because of a quirk in noble/hashes related to Node 18 and older versions. Would you be open to just bumping the nodejs version to 20 in CI to eliminate this issue? If not, I'll do the required shim but seems kind of silly given node 18 is EOL at this point.

@wemeetagain would you be open to bumping CI to run on node 20 or do you want me to apply the shim that noble says is required to run on node 18? Note, ultralight is running on node 20 and above only so not an issue for us.

@acolytec3
Copy link
Contributor Author

Other than the CI nodejs environment question, this should be ready for review.

@acolytec3 acolytec3 mentioned this pull request Jan 8, 2025
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.

Drop bcrypto
2 participants