Conversation
The GMP-to-custom-mp migration dropped the canOptimizeConsensys branching from three Montgomery multiplication functions in fr_raw_generic.cpp.ejs: rawMMul, rawMMul1, and rawFromMontgomery. canOptimizeConsensys is true when the top limb of the prime q is below 2^63, meaning intermediate Montgomery reduction products cannot overflow their allocated limb space. When false (top bit of the top limb is set), mp_addmul can produce a non-zero carry that must be tracked and checked during the final reduction step. The new code always followed the optimized (no-carry) path, which produced wrong results for any prime with a large top limb. The Goldilocks field (q = 0xFFFFFFFF00000001, n64=1) was the first to fail: add, sub, mul, square, eq, neq, and div all returned incorrect values. Larger primes like bn128 were unaffected because their top limbs have bit 63 clear. The fix re-adds canOptimizeConsensys branching in all three functions. When the flag is false: - Intermediate loop: carry from mp_addmul accumulates into product[i+1][n64] (not the fixed index [1] used in the optimized path), and carry from mp_add is captured. - Final step: carry from the last mp_addmul is accumulated. - Final reduction: checks both carry and mp_cmp, not just mp_cmp alone. Additional fixes discovered during review: - mp.cpp.ejs: added Knuth Algorithm D saturation guard in the __SIZEOF_INT128__ branch of mp_mod_2n_n. When unorm[j+n] >= v1 the 128-bit division can produce a result >= 2^64, and the uint64_t cast truncates it. The existing mp_div already had this guard; mp_mod_2n_n was missing it. - mp.cpp.ejs: removed stray closing brace in the #else branch of mp_addmul that would cause compilation errors on platforms without __SIZEOF_INT128__. - buildzqfield.js: the CLI now renders and writes mp.hpp/mp.cpp alongside the other generated files. Without this, users running the CLI tool got a broken build. - tasksfile.js: restored fr_generic.cpp and fq_generic.cpp in the ARM64 benchmark source list. These files define _rawMSquare and _rawToMontgomery which are not provided by the ARM64 assembly templates. - tester.cpp: changed throw new std::runtime_error(...) to throw std::runtime_error(...). The former throws a pointer, which is uncatchable by the standard catch-by-reference pattern. - alt_bn128_test.cpp: fixed TEST macro indentation, removed duplicate perf utility functions (perf_clock2, bench_ms2, ns_per_op2), uncommented includes needed by perf tests. - curve.cpp: removed two blocks of commented-out dead code (old copy/neg implementations superseded by batch versions). - README.md: removed GMP references, updated generated file list to include mp.cpp/mp.hpp, fixed compile command. - package.json: added mocha to devDependencies (was referenced in the test script but never declared). All 184 field tests pass across gl, bn128q, bn128r, bls12_381q, secp256k1q, secp256k1r, mnt6753q, and mnt6753r.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
No description provided.