search

ZenGo-X / emerald-city

WASM/HW-friendly lightweight client application for threshold ECDSA
GNU General Public License v3.0
15 stars 6 forks source link

Replaced libsecp256k1 with rust-secp256k1 #10

Open elichai opened 5 years ago

elichai commented 5 years ago

rust-secp256k1 now has wasm support. It has a lot of advantages over the libsecp library:

  1. Promise side channel resistance.
  2. Has multiple writers and gone through a lot of reviewers.
  3. Really fast

https://github.com/rust-bitcoin/rust-secp256k1/pull/125#issuecomment-508598506

vhnatyk commented 5 years ago

Hi - yep

rust-secp256k1 now has wasm support.

due to recent advances I tend to think no point in exotic pure Rust crates no more 👍 Few month's ago I already switched back in my experimental branch just was so focused on playing with MCU that didn't bother to do PR back. Well, it's not that only rust-secp256k1 got WASM support. Ring also got wasm support - so for quite a few weeks since it started to work on MCU I'm wondering with idea maybe it's not worth maintaining emerald city as a fork.

Especially since my slight modification to original repo builds just fine for MCU (though not yet running due to unrelated linking issue) 😄 Don't get me wrong - emerald city served it's purpose very well 👍 Even today it builds amazingly fast ~70 crates vs 200-300+ in original. But I started to note some diverging with original repo and some things, that originally seemed quite a good idea like switching to cryptoxide - now look similar to the situation with libsecp256k1 and rust-secp256k1. I mean for example, cryptoxide seem doesn't compile very well to no_std etc and generally pure rust alternatives have worse performance and - what's even worse - seem lesser maintained and secure due usually to no CT safety etc.

Anyway after linking - original library size is coming down just to few hundred kilobytes - just like emerald-city. The fact that I noted that in curve there is now both ring and cryptoxide bothers me a bit and so on:) @omershlo what do you think?

I'm already playing with gmp enabled with .define("USE_NUM_GMP", Some("1")) But the fact that it went unnoticed how it was creating context each time makes me 😁. To excuse I can say I was profiling and noticed that 99% of cpu time it spends in GMP, and what's more - want to notice that mpecdsa uses neither sign nor verify. So it was easier to miss that it indeed creates verify context each and every time!!. @elichai -Going to try your amazing fix in my experimental branch - and I'm super excited 👍 I already reverted num-bigint to rust-gmp and will play with reverting to ring soon.

vhnatyk commented 5 years ago

Hmm - unfortunately switching to global static context didn't give me anyhow noticeable performance improvement on Sphere 😶 The only explanation I have is that in my custom branch I limited context size to something like 4x4 to bypass RAM crash - so despite initialization happens each time, but so quick, that static context fix doesn't produce expected speedup. Definitely should be merged anyway though. Not sure it's worth pursuing another option to be merged into bitcoin-core/secp256k1 to initialize without any context(would be optimal for the use case - since we use neither signing nor verify).