Skip to content
This repository has been archived by the owner on Apr 18, 2024. It is now read-only.

feature: Use curve25519-voi #66

Closed
wants to merge 6 commits into from
Closed

Conversation

Yawning
Copy link

@Yawning Yawning commented Apr 30, 2021

Can't be bothered disabling turboboost, or running multiple iterations.

name                                                  old time/op    new time/op    delta
ValidatorSet_VerifyCommit_Ed25519/valset_size_1-8       58.3µs ± 0%    28.3µs ± 0%  -51.38%
ValidatorSet_VerifyCommit_Ed25519/valset_size_8-8       32.5µs ± 0%    19.9µs ± 0%  -38.58%
ValidatorSet_VerifyCommit_Ed25519/valset_size_64-8      27.5µs ± 0%    17.0µs ± 0%  -38.02%
ValidatorSet_VerifyCommit_Ed25519/valset_size_1024-8    31.7µs ± 0%    13.5µs ± 0%  -57.50%

name                                                  old alloc/op   new alloc/op   delta
ValidatorSet_VerifyCommit_Ed25519/valset_size_1-8       1.00kB ± 0%    1.10kB ± 0%   +9.60%
ValidatorSet_VerifyCommit_Ed25519/valset_size_8-8       4.94kB ± 0%    3.75kB ± 0%  -24.15%
ValidatorSet_VerifyCommit_Ed25519/valset_size_64-8      5.01kB ± 0%    3.66kB ± 0%  -26.86%
ValidatorSet_VerifyCommit_Ed25519/valset_size_1024-8    4.78kB ± 0%    2.25kB ± 0%  -52.90%

name                                                  old allocs/op  new allocs/op  delta
ValidatorSet_VerifyCommit_Ed25519/valset_size_1-8         15.0 ± 0%      18.0 ± 0%  +20.00%
ValidatorSet_VerifyCommit_Ed25519/valset_size_8-8         17.0 ± 0%      17.0 ± 0%    0.00%
ValidatorSet_VerifyCommit_Ed25519/valset_size_64-8        16.0 ± 0%      15.0 ± 0%   -6.25%
ValidatorSet_VerifyCommit_Ed25519/valset_size_1024-8      19.0 ± 0%      16.0 ± 0%  -15.79%

TODO:

@Yawning Yawning force-pushed the yawning/wip/curve25519-voi branch from 53b761d to 2838c38 Compare May 11, 2021 14:16
@Yawning Yawning marked this pull request as ready for review May 11, 2021 14:17
@Yawning Yawning requested a review from kostko May 11, 2021 14:17
@Yawning Yawning force-pushed the yawning/wip/curve25519-voi branch 3 times, most recently from 611b14c to 699d768 Compare May 11, 2021 14:23
@Yawning Yawning force-pushed the yawning/wip/curve25519-voi branch from 699d768 to fde9315 Compare May 20, 2021 15:52
@Yawning Yawning force-pushed the yawning/wip/curve25519-voi branch 5 times, most recently from cf56be3 to aaa66e0 Compare June 2, 2021 11:25
@Yawning Yawning force-pushed the yawning/wip/curve25519-voi branch 3 times, most recently from 8710280 to f415212 Compare June 9, 2021 09:13
@Yawning Yawning force-pushed the yawning/wip/curve25519-voi branch 4 times, most recently from 7a6e3a0 to 1b5655f Compare June 25, 2021 09:34
Yawning added 5 commits June 26, 2021 03:54
The old benchmark measured:
 * `sigsCount` GenPrivKey + Sign
 * `sigsCount` BatchVerifier.Add
 * `b.N/sigsCount` BatchVerifier.Verify (majority of the runtime).

This is all sorts of nonsensical, especially give that there can be a
non-trivial amount of overhead in `BatchVerifier.Add`.

The rewritten benchmark measures:
 * `b.N/sigsCount` NewBatchVerifier
 * `b.N/sigsCount * sigsCount` BatchVerifier.Add
 * `b.N/sigsCount` BatchVerifier.Verify

This is far more sensible as it better reflects the per-signature
combined cost of instantiating the batch verifier, adding a signature
to the batch, and the verify.
Having to redo quite a bit of computation in the event of a batch
failure if the caller is actually interested in which signature failed,
is rather sub-optimal.  Change the batch verification interface to
expose a one-shot batch verify + fallback call, so that sensible
libraries can handle this case faster.

This commit also leverages the failure information so that validation
will only ever call one of `verifyCommitBatch` or `verifyCommitSingle`.
Switch the sr25519 implementation to curve25519-voi for better
performance, and code quality.

Performance comparisions should still be taken with a grain of salt
for the following reasons:

 * curve25519-voi's sr25519 support can use more optimization.
 * go-schnorrkel cuts corners in places by:
   * Not doing delinearization at all when verifying batches
   * Not using the secret key nonce at all when signing.
   * Not sampling random scalars at all when verifying batches,
     unless the import is bumped.

WARNING: This is a breaking change as the original tendermint sr25519
support expands the MiniSecretKey twice, while this implementation
only does it once.
@Yawning Yawning force-pushed the yawning/wip/curve25519-voi branch from 1b5655f to c5e0263 Compare June 26, 2021 03:54
Since I had to fork gtank's to add a transcript RNG, and ended up
rewriting the STROBE implementation for my fork, the code might as well
use it for every use of merlin as there are a number of improvements,
the most notable being a dramatic reduction in the number of allocations
done for the various STROBE calls.
@Yawning Yawning force-pushed the yawning/wip/curve25519-voi branch from c5e0263 to 744d486 Compare June 26, 2021 16:44
@Yawning
Copy link
Author

Yawning commented Jun 28, 2021

This has been merged into upstream as tendermint@c5cc3c8 so we will get this for free. I'll deal adding our specific domain separation flavor when we rebase the rest of our patches.

@Yawning Yawning closed this Jun 28, 2021
@Yawning Yawning deleted the yawning/wip/curve25519-voi branch January 26, 2022 20:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant