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

Use faster Ed25519 impl for gossip crypto #3465

Closed
3 tasks
dapplion opened this issue Nov 29, 2021 · 8 comments
Closed
3 tasks

Use faster Ed25519 impl for gossip crypto #3465

dapplion opened this issue Nov 29, 2021 · 8 comments
Assignees
Labels
prio-medium Resolve this some time soon (tm). scope-performance Performance issue and ideas to improve performance.

Comments

@dapplion
Copy link
Contributor

@achingbrain has identified alternative implementations of Ed25519 crypto. This issue is a tracker to ensure we timely upgrade our deps once a faster impl lands. See benchmarks in libp2p/js-libp2p-crypto#211

I don't have data right now to show but historically gossip crypto has appeared in CPU profiles taking ~5% of total CPU time in the main thread

TODO:

@dapplion dapplion added the scope-performance Performance issue and ideas to improve performance. label Nov 29, 2021
@dapplion dapplion changed the title Use faster Ed25519 impl for lgossip crypto Use faster Ed25519 impl for gossip crypto Nov 29, 2021
@dapplion
Copy link
Contributor Author

@mpetrunic does libp2p-noise must use libp2p-crypto to comply with some interface or can we just import any implementation that we want?

@philknows philknows added the prio-high Resolve issues as soon as possible. label Nov 29, 2021
@paulmillr
Copy link

libp2p decided to drop switch to ed-wasm, see discussion here: libp2p/js-libp2p-crypto#215

@dapplion
Copy link
Contributor Author

dapplion commented Feb 4, 2022

libp2p decided to drop switch to ed-wasm, see discussion here: libp2p/js-libp2p-crypto#215

If I follow correctly the result has been to drop all PRs and stay on the pure-js impl right?

We should count how many ops Lodestar does per second since this has been a bottleneck for us.

@achingbrain
Copy link

For the record I'm not against switching to a WASM ed25519 implementation in the future, it's just that in the testing I did, it didn't seem to be a bottleneck.

If @dapplion is seeing evidence to the contrary then this is certainly something we can re-evaluate if some benchmarks can be created that reflect realistic workflows so we can measure the difference in implementations.

@dapplion
Copy link
Contributor Author

Now we can use a faster implementation thanks to plug-gable crypto here ChainSafe/js-libp2p-noise#133

@dapplion
Copy link
Contributor Author

dapplion commented Apr 3, 2022

Sample impl of native version from @ShogunPanda

If you want, I have sample implementation with sodium-native in https://github.com/web3-storage/ipfs-elastic-provider-bitswap-peer/blob/main/src/noise-crypto.js

@dapplion dapplion added prio-medium Resolve this some time soon (tm). and removed prio-high Resolve issues as soon as possible. labels May 12, 2022
@twoeths
Copy link
Contributor

twoeths commented Jun 20, 2022

the current js implementation is way faster than sodium-native (probably due to the cost of calling native layer)

stablelib decrypt x 268,550 ops/sec ±1.05% (90 runs sampled)
sodium-native decrypt x 45,880 ops/sec ±2.47% (88 runs sampled)
stablelib encrypt x 259,846 ops/sec ±0.70% (92 runs sampled)
sodium-native encrypt x 39,432 ops/sec ±3.09% (82 runs sampled)

see ChainSafe/js-libp2p-noise#161

@dapplion
Copy link
Contributor Author

We are using a dedicated implementation for noise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio-medium Resolve this some time soon (tm). scope-performance Performance issue and ideas to improve performance.
Projects
None yet
Development

No branches or pull requests

5 participants