Fix MultiScalarMul small-n pathology and NewRandomZr allocation regression - #55
Open
adecaro wants to merge 2 commits into
Open
Fix MultiScalarMul small-n pathology and NewRandomZr allocation regression#55adecaro wants to merge 2 commits into
adecaro wants to merge 2 commits into
Conversation
…ssion MultiScalarMul dispatched every call to gnark's bucket-method MultiExp, whose large fixed setup cost (window/chunk sizing, goroutine fan-out) made it ~4x slower than a single Mul at n=1 and dominates small inputs generally. Add a pairwise Mul2+Add fallback below a measured n=7 crossover on the three gnark-backed drivers (bls12381, bn254, bls12-377), and route the MultiExp scalar conversion through Zr.toBigInt so GroupOrder scalars are no longer silently read as zero. NewRandomZr on bls12381 regressed to a big.Int-based rejection sampler when 0148cd9 fixed it to honor the caller's rng; restore an allocation-free stack-buffer implementation using fr.Element.SetBytesCanonical while keeping rng as the sole entropy source. Also measured (no fix needed): the kilic->gnark hash-to-curve swap in 4952af1 is ~1.9x faster with gnark, not a regression.
6 tasks
The small-n pairwise dispatch added to the gurvy drivers' MultiScalarMul was tuned on single-threaded microbenchmarks (go test -bench -cpu 1), where the fixed cost of gnark's bucket-method MultiExp - window/chunk setup plus a goroutine fan-out sized for runtime.NumCPU() - dominates for a handful of bases. Under real concurrent load that reasoning inverts: the fan-out only pays off when spare cores exist, and a saturated process has none. It also silently overrode callers that had already made a deliberate choice for their own sizes. Panurus' CSP range proof, for instance, special-cases n=1 and n=2 with Mul/Mul2 and hands everything from n=3 up to MultiScalarMul on the strength of its own measurements; the threshold of 7 rerouted n=3..6 back to a pairwise loop behind its back. Drop the threshold and both pairwise helpers. MultiScalarMul now keeps only the trivial n==0 and n==1 guards and otherwise goes straight to MultiExp, so a caller that knows its sizes can pick Mul/Mul2 itself and a caller that asks for a multi-scalar multiplication gets one. The doc comments point at that division of labor, and the benchmark/test comments that described the now-removed dispatch boundary are updated to match. No behavioral change: both paths compute the same sum, and the GroupOrder consistency test still covers a small-n and a large-n size. Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
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
1 participant
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.
Summary
MultiScalarMulon the gnark-backed drivers (bls12381, bn254, bls12-377) always dispatched to gnark's bucket-methodMultiExp, whose fixed setup cost (window/chunk sizing, goroutine fan-out) is tuned for thousands of points. At n=1 this is ~4x slower than a plainMul(280µs vs 68µs, 144 vs 13 allocs). Add a pairwiseMul2+Addfallback below a measured n=7 crossover point on all three drivers, keepingMultiExpunchanged at/above the threshold (no regression at large n).MultiScalarMulreadZr.valdirectly instead of going throughtoBigInt, silently treatingGroupOrderscalars as zero. Both the new pairwise branch and theMultiExpbranch now route throughtoBigInt, matching whatMul/Mul2already did.NewRandomZron bls12381 regressed in 0148cd9 (which correctly fixed it to honor the caller'srnginstead ofcrypto/rand) by reaching for abig.Int-based rejection sampler (5 allocs/184B). Restored an allocation-free implementation using stack-buffer rejection sampling +fr.Element.SetBytesCanonical, the same strategy gnark's ownSetRandomuses internally, while still reading exclusively fromrng(2 allocs/80B).NewRandomZrdoc contracts (math.go,driver/math.go) to state thatrngmust be the exclusive entropy source, so this isn't silently "fixed" back.4952af1^(kilic vs gnark in the same binary) shows gnark'sHashToG1is actually ~1.9x faster (76µs vs 141µs) — not a regression, no action needed.Test plan
go build ./...,go vet ./...,gofmt -l -s .cleango test ./...andgo test -race -cover ./...passgolangci-lint run— 0 issuesaddlicense -check— cleango fix -diff ./...— emits nothingMultiScalarMuldispatch-boundary sweep (n=0,1,2,6,7,8,10,33) across all curves, zero-scalar/infinity-base case,GroupOrderscalar correctness at both the pairwise and MultiExp branch;NewRandomZrbad-reader panic and distribution sanity checkbenchstatbefore/after:MultiScalarMuln=1 now within noise ofBenchmarkG1Mul, n=2-6 show large allocation/time wins, n≥7 unchanged;NewRandomZrback to ~1 allocationgo mod edit -replace):BenchmarkSignerSign/EidNymRhNymunchanged (~3.5ms), and idemix's own MSM-vs-pairwise crossover benchmark shows the gap closed at small n — confirming this subsumes idemix's client-side workaround inbbs/bbs12381g2pub.go