Fix fuzz 1 failure: incorrect reduction of BigInt #246
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
From the Ethereum Foundation sponsored fuzzing by @guidovranken (linked to #54),
fromBig
can lead to incorrectly reduced field elements.This is because classic Montgomery reduction can reduce inputs from the range [0, 4p²) -> [0, p) but this is not the case with the no-carry optimization from Gnark (https://hackmd.io/@gnark/modular_multiplication and https://eprint.iacr.org/2022/1400.pdf section 2)
In practice
getMont
andfromBig
are internal-only procedures that only deserialized well-formed inputs that have been pre-checked in protocols:constantine/constantine/ethereum_bls_signatures.nim
Lines 284 to 293 in 151f284
constantine/constantine/ethereum_bls_signatures.nim
Lines 339 to 354 in 151f284
constantine/constantine/ethereum_evm_precompiles.nim
Lines 38 to 54 in 151f284
and for large inputs, we use redc2xMont + multMont:
constantine/constantine/hash_to_curve/h2c_hash_to_field.nim
Lines 222 to 235 in 151f284
constantine/constantine/ethereum_eip2333_bls12381_key_derivation.nim
Lines 56 to 71 in 151f284
or full reduction:
constantine/constantine/math_arbitrary_precision/arithmetic/bigints_views.nim
Lines 69 to 79 in 151f284
However there is one protocol case where
getMont
is used, in ECMUL (EIP-198):constantine/constantine/ethereum_evm_precompiles.nim
Lines 189 to 209 in 151f284
Perf
The impact on BN254 is a slowdown from 10ns to 17ns (with Clang, worse with GCC)
The impact on BLS12-381 is a slowdown from 23ns to 39ns (with Clang, worse with GCC)
Most of the slowdown is due to classic Montgomery reduction not having an assembly implementation.
However, that conversion is not in the hot path of any protocol, most protocols that need to deal with a large amount of deserialization allow caching, for example in case of Ethereum validator public keys. And 99% of the time is spend in computing the square root of compressed keys/signatures.