search

MarcoPolo / js-libp2p-noise-wasm

0 stars 0 forks source link

Alpha review #1

Open wemeetagain opened 1 year ago

wemeetagain commented 1 year ago

from @nazarhussain:

  1. The WASM binding is actually a wrapper based on the snow crate. The snow crate seems mature with multiple releases, but they show a warning that it's not formally audited. I am not sure if our implementation was audited or not, or is that component in libp2p is so sensitive to be worried about the audit.

  2. The Rust source of the binding looks good.

  3. There seems to be some dead-code , https://github.com/MarcoPolo/js-libp2p-noise-wasm/blob/6fed8dadc54f83338d2cd4266e08fb6525a252b8/src/lib.rs#L204-L223 That structure is not used and not exported as wasm binding.

  4. The tests are completely missing. There should at least be some unit tests for sanity checks. Having larger test vectors would be a plus.

MarcoPolo commented 1 year ago

Thanks @wemeetagain and @nazarhussain

  1. Yep. This crate is used by rust-libp2p as well.

  2. Removed.

  3. Agreed. Although it does pass the js-libp2p-noise tests, more tests are always better.

Did you notice any perf wins? If I add a bunch of tests and add this to the interop tester, would you run it?

wemeetagain commented 1 year ago

I just ran it locally, it looks promising. Significantly faster generateX25519SharedKey, (possibly) slightly faster chaCha20Poly1305Decrypt.

MarcoPolo commented 1 year ago

In my local throughput benchmarks it 4x the throughput of tcp+noise+yamux.

wemeetagain commented 1 year ago

In lodestar we're using an assemblyscript chacha20poly1305 implementation (https://github.com/ChainSafe/lodestar/blob/unstable/packages/beacon-node/src/network/nodejs/noise.ts#L13), thats why we didn't see such a huge difference. Still seems to be 25% faster in a smaller benchmark I did.