Add on-by-default fast crate feature for gating basepoint tables

[tarcieri]

NOTE: depends on dalek-cryptography/curve25519-dalek#489 which needs to be merged first Merged!

NOTE: now depends on #260 which needs to be merged first Merged!

Disabling the feature reduces overall code size at the cost of performance, which is useful for e.g. embedded users.

This feature transitively enables the basepoint-tables feature in curve25519-dalek.

TODO: fallback support for using variable-base scalar multiplication by the basepoint when the basepoint-tables feature is disabled. Added in 8f56c5c

[tarcieri]

It might make more sense to give this feature a simpler name like fast which makes its function and code-size-vs-performance tradeoff more readily apparent to non-expert end-users.

Embedded users who start with --no-default-features can experiment with turning it back on and seeing if the larger code size blows up their program (something I've seen happen in person with M0/M4 users trying to use ed25519-dalek at OxidizeConf's hack days)

Edit: went ahead and renamed the feature to fast

[tarcieri]

Not sure why the build is failing. I split out #260 to handle updating to the latest upstream curve25519-dalek changes as an intermediate debugging step.

[tarcieri]

@rozbb this is ready now

[rozbb]

Just ran some benchmarks on M1 and x86_64. It looks like we get massive (64%) speedups in keygen and signing. But we get a 0-4% slowdown in batch verification, and no effect at all for single verification. Do you wanna rename to fast-sign?

[tarcieri]

Haha what? That's surprising.

If that's really the case then batch verification should always use ... * ED25519_BASEPOINT_POINT if using mul_base has a negative impact on performance when the basepoint tables feature is enabled

[rozbb]

Yeah makes sense. First I'll test how the feature interacts with the simd backends.

Update: yup, same slowdown even with AVX2 SIMD

[rozbb]

Yup same slowdown even with AVX2 SIMD. And actually, single signature verification is 5-9% slower with fast enabled. So there's no situation wherein verification benefits from the tables. This is really surprising.

I'll do the following: rename the feature to fast-sign, explain it in the README (maybe after merging the other README PR), and make all verification use table-less scalar mult.

[tarcieri]

I still think we should remove mul_base anywhere it isn’t helping and keep the feature name as fast.

If there are slowdowns occurring in places where mul_base isn’t used, that is a real mystery

[tarcieri]

Perhaps we should open an issue on curve25519-dalek?

Tracing the code I'm perplexed how this feature would have any effect on verification, either single signature or batch verification, as as far as I can tell neither use the basepoint tables? (although there are many backends and maybe I haven't grepped/traced properly)

Batch verification calls EdwardsPoint::optional_multiscalar_mul which starts with the basepoint constant (i.e. point, not table). That calls either Straus or Pippinger backends for multiscalar multiplication, which receive all their scalar/point inputs explicitly regardless of if one of them is the basepoint.

Single signature verification calls EdwardsPoint::vartime_double_scalar_mul_basepoint which calls scalar_mul::vartime_double_base::mul which uses a completely different basepoint table AFFINE_ODD_MULTIPLES_OF_BASEPOINT located in backend::serial::u64::constants (which, sidebar, should probably also be gated by the precomputed tables feature, whatever we end up calling it)

[rozbb]

Yeah I ran into the same thing when tracing this down. I have no idea what's causing this. I'll spend a little more time tracing things

[rozbb]

Note: this will likely return a CompressedEdwardsY in the process of fixing #267 . Documenting the comment here:

Note that this returns the compressed form of R and the caller does a byte comparison.
This means that all our verification functions do not accept non-canonically encoded R values.
See the validation criteria blog post for more details:
    https://hdevalence.ca/blog/2020-10-04-its-25519am

[tarcieri]

@rozbb let me update curve25519-dalek in this PR now that the feature name has changed to precomputed-tables

[tarcieri]

Updated and it's green again