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

Replaced libsecp256k1 with rust-secp256k1 #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

elichai
Copy link

@elichai elichai commented Jul 23, 2019

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

rust-bitcoin/rust-secp256k1#125 (comment)

@vhnatyk
Copy link
Contributor

vhnatyk commented Jul 23, 2019

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

vhnatyk commented Jul 26, 2019

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).

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.

2 participants