Skip to content

feat: Make crypto implementation pluggable. #133

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

Merged
merged 1 commit into from
Mar 24, 2022
Merged

feat: Make crypto implementation pluggable. #133

merged 1 commit into from
Mar 24, 2022

Conversation

ShogunPanda
Copy link
Contributor

Hello.
This PR extracts all low level crypto interface in a specific file and makes the crypto interface pluggable in Noise.
Hope this helps!

@CLAassistant
Copy link

CLAassistant commented Mar 11, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@mpetrunic mpetrunic left a comment

Choose a reason for hiding this comment

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

Tnx for your contribution. Not a bad idea, will review in more detail next week

Copy link
Member

@mpetrunic mpetrunic left a comment

Choose a reason for hiding this comment

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

lgtm, you should probably put latest master as we are converting to esm

@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2022

Codecov Report

Merging #133 (590facf) into master (75116be) will decrease coverage by 2.67%.
The diff coverage is 92.81%.

@@            Coverage Diff             @@
##           master     #133      +/-   ##
==========================================
- Coverage   88.59%   85.92%   -2.68%     
==========================================
  Files          16       18       +2     
  Lines        1841     1918      +77     
  Branches      248      242       -6     
==========================================
+ Hits         1631     1648      +17     
- Misses        210      270      +60     
Impacted Files Coverage Δ
src/noise.ts 73.96% <60.00%> (-2.56%) ⬇️
src/crypto/streaming.ts 97.67% <97.67%> (ø)
src/crypto.ts 100.00% <100.00%> (+2.32%) ⬆️
src/crypto/stablelib.ts 100.00% <100.00%> (ø)
src/handshake-ik.ts 61.39% <100.00%> (-30.92%) ⬇️
src/handshake-xx-fallback.ts 93.10% <100.00%> (+0.16%) ⬆️
src/handshake-xx.ts 97.15% <100.00%> (+0.03%) ⬆️
src/handshakes/abstract-handshake.ts 92.11% <100.00%> (+0.03%) ⬆️
src/handshakes/ik.ts 89.03% <100.00%> (ø)
src/handshakes/xx.ts 90.21% <100.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75116be...590facf. Read the comment docs.

@ShogunPanda
Copy link
Contributor Author

@mpetrunic Rebased and updated!

Copy link
Contributor

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Looks good to me, nice abstraction! Are there any performance tests to ensure equal throughput? Results should be the same, but just to be sure

Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

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

LGTM from an organizational perspective

@mpetrunic
Copy link
Member

Looks good to me, nice abstraction! Are there any performance tests to ensure equal throughput? Results should be the same, but just to be sure

master
$ node benchmarks/benchmark.js
Initializing handshake benchmark
Init complete, running benchmark
handshake x 40.53 ops/sec ±17.26% (72 runs sampled)
Done in 6.54s.

PR#133
$ node benchmarks/benchmark.js
Initializing handshake benchmark
Init complete, running benchmark
handshake x 41.62 ops/sec ±15.59% (72 runs sampled)
Done in 6.39s.

It varies on each run but it's identical

@mpetrunic mpetrunic merged commit a1cc6b0 into ChainSafe:master Mar 24, 2022
@mpetrunic
Copy link
Member

@dapplion This potentially allows you in lodestar to use some native library that might be faster than stablelib (purejs).

@dapplion
Copy link
Contributor

@dapplion This potentially allows you in lodestar to use some native library that might be faster than stablelib (purejs).

yeah looking forward to testing multiple impl

@ShogunPanda
Copy link
Contributor Author

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
Copy link
Contributor

dapplion commented Apr 3, 2022

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

Let's goo! 🔥 If you wanna contribute bringing this live in Lodestar I may airdrop you some $LODE tokens in the future

@MarcoPolo
Copy link
Contributor

I've implemented this interface with WASM using Rust and the snow crate: https://github.com/MarcoPolo/js-libp2p-noise-wasm

Here's some benchmarks:

Throughput:

❮ node dist/benchmark/src/index.js
TCP+mplex+noise
Download throughput is (mbits/s) 369n
Upload throughput is (mbits/s) 272n
TCP+yamux+noise
Download throughput is (mbits/s) 193n
Upload throughput is (mbits/s) 195n
TCP+mplex+wasm_noise
Download throughput is (mbits/s) 1047n
Upload throughput is (mbits/s) 1055n
TCP+yamux+wasm_noise
Download throughput is (mbits/s) 869n
Upload throughput is (mbits/s) 867n

Handshakes per second:

❮ node benchmarks/benchmark.js
Initializing handshake benchmark
Init complete, running benchmark
handshake x 632 ops/sec ±0.58% (89 runs sampled)

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.

7 participants