Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

upgrade schnorrkel to 0.10 #10048

Open
Tracked by #9965
gilescope opened this issue Oct 18, 2021 · 5 comments
Open
Tracked by #9965

upgrade schnorrkel to 0.10 #10048

gilescope opened this issue Oct 18, 2021 · 5 comments

Comments

@gilescope
Copy link
Contributor

we are a few versions behind and this gets us to rand 0.8.

linked to upgrade substrate-bip39 crate.

@gilescope
Copy link
Contributor Author

Previous upgrade notes: #3243

@nazar-pc
Copy link
Contributor

I saw #10025, is there a good reason schnorrkel wasn't updated yet?

@burdges
Copy link

burdges commented May 25, 2022

It's likely some computational dependency that caused the regression, so curve2551-dalek or merlin or rand, since not much else changed. It could be tested vs schnorrkel 10.1 to detect when the regression happened.

A priori, we do not use rand enough to have much impact, except I switched from ThreadRng or OsRng in w3f/schnorrkel@c333370 and w3f/schnorrkel@f155c2a which likely slows down signing. Any idea if the regression comes from verification or signing? We could test with this change reverted easily I guess.

I always prefer transcript-like hash functions like merlin when implementing Fiat-Shamir protocols, but merlin is based upon Keccak STROBE and slower than blake2b. In substrate, we typically pass big messages through blake2b before signing, but perhaps somewhere we pass some large-ish message into merlin directly, so then a merlin regression could trigger a noticeable substrate regression. I donno..

It's more likely some curve25519-dalek regression caused this regression somehow, because elliptic curve arithmetic runs way slower than hashing. I've merged everything from @gilescope into 10.2 except w3f/schnorrkel@5334cb6 but since then we've decided to drop the -ng versions, since not even zcash uses them, so we should see if this commit improves or worsens things relative to 9.2.

We'll definitely observe some speedup from non -ng versions because others besides HdV and Isis contributes patches like dalek-cryptography/curve25519-dalek#379 although that specific one never got merged. We therefore risk some mixed bag here regardless.

We should maybe recheck that no dependency uses SigningContext::hash512 due to w3f/schnorrkel@5527059 but anything like that should not be in the substrate ecosystem

@nazar-pc
Copy link
Contributor

I see there is a verification bench in schnorrkel, did you use that to compare 0.9 to 0.10? Maybe other benches like signing should be added as well?

@burdges
Copy link

burdges commented May 25, 2022

Yes likely so, but verification is by far the most important benchmark.

I donno how big the substrate regression was either.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants