Fix bbs MultiScalarMul small-n regression and cache hash-to-G1 generators - #90
Open
adecaro wants to merge 2 commits into
Open
Fix bbs MultiScalarMul small-n regression and cache hash-to-G1 generators#90adecaro wants to merge 2 commits into
adecaro wants to merge 2 commits into
Conversation
…s, bump mathlib BenchmarkSignerSign regressed after mathlib started unconditionally dispatching sumOfG1Products to curve.MultiScalarMul: gnark's bucket-method MultiExp has a large fixed setup cost that a handful of bases (the common case for per-attribute nym/signature proofs) cannot amortize, costing ~4x a plain Mul at n=1. Add a pairwise Mul2+Add fallback below a measured n=7 crossover (bbs/bbs12381g2pub.go), backed by a new BenchmarkSumOfG1ProductsCrossover. Also cache hash-to-G1 generator points across *BBSLib instances (bbs/keys.go): every sign/verify/proof call constructed a fresh *BBSLib and redid the ~69us hash-to-G1 work for h0 and every message generator, since the cache was previously owned per-instance rather than shared. Add .github/workflows/bench.yml to catch a regression like this one in review via a non-blocking benchstat PR comment. Bump github.com/IBM/mathlib to a commit that fixes both of these regressions upstream (IBM/mathlib#55) so the pairwise workaround above can eventually be dropped in favor of mathlib's own MultiScalarMul dispatch; kept for now since it is still measurably faster below the crossover and mathlib's fix has not shipped in a tagged release yet.
Benchmark comparison (base vs. this PR)Informational only — noisy GitHub runners, not a merge gate. |
sumOfG1Products dispatched to curve.MultiScalarMul from 7 bases up, on the strength of single-threaded microbenchmarks. That win does not survive concurrent load: gnark's bucket-method MultiExp is faster in wall-clock terms only because it fans out over runtime.NumCPU() goroutines and allocates buckets to do it, and a process already saturated with concurrent verifications has no spare cores to fan out onto - it just pays the extra allocations. Measured end to end on Panurus' zkatdlog transfer validation (10 workers, Apple M1 Max, BLS12_381_BBS_GURVY): this package's verification path only ever calls sumOfG1Products with 2, 3, 7 or 8 bases, straddling the threshold, and removing the dispatch cuts allocated bytes per verification by ~11-12% (bulletproof 618KB -> 546KB, CSP 541KB -> 477KB) while improving CSP throughput ~7% (426 -> 438 ops/s). The cost is a ~4% single-worker latency win given up, which is the case that matters least. So fold sumOfG1ProductsPairwise back into sumOfG1Products and drop msmThreshold. The FrToRepr calls the original loop made are omitted since FrToRepr is the identity function (see fr.go). Behavior is unchanged: the loop's odd tail handles a single base with Mul exactly as the old n==1 case did, and an empty input still yields nil. The companion mathlib change (IBM/mathlib) removes the same threshold from the gurvy drivers' MultiScalarMul, so bump to it. Signed-off-by: Angelo De Caro <adc@zurich.ibm.com>
Benchmark comparison (base vs. this PR)Informational only — noisy GitHub runners, not a merge gate. |
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
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
sumOfG1Products(bbs/bbs12381g2pub.go) unconditionally calledcurve.MultiScalarMul, which regressed on the gnark-backed curves: gnark's bucket-methodMultiExphas a large fixed setup cost tuned for thousands of points, costing ~4x a plainMulat a single base. Most of this package's hot-path call sites (per-attribute nym/signature proofs) sum well under a dozen bases. Add a pairwiseMul2+Addfallback below a measured n=7 crossover, keepingMultiScalarMulfor larger n.publicKeyGeneratorCache(bbs/keys.go) was owned per-*BBSLibinstance, but everybccsp/schemes/ariescall site constructs a fresh*BBSLibper operation — so the ~69µs hash-to-G1 work forh0and every message generator was redone on every sign/verify/proof call instead of once per (public key, message count) pair. Made the cache package-wide, keyed by curve ID + data..github/workflows/bench.yml: a non-blocking benchstat comparison posted as a PR comment, so a regression shaped like this one shows up in review.github.com/IBM/mathlibto a commit (IBM/mathlib#55) that fixes both root causes upstream — the sameMultiScalarMulsmall-n dispatch problem and aNewRandomZrallocation regression. This package's pairwise workaround insumOfG1Productsis kept for now (it's still measurably faster below the crossover, and mathlib's fix hasn't shipped in a tagged release), but can be revisited once mathlib#55 lands in a release.Test plan
go build ./...,go vet ./...,gofmt -l -scleango test ./...passes with the new mathlib pseudo-versionaddlicense -checkcleanBenchmarkSignerSign/EidNymRhNymback down to ~3.5ms (was regressed)BenchmarkSumOfG1ProductsCrossoverbacks themsmThreshold=7constantgo.modoff the pseudo-version and revisit whethersumOfG1Products's pairwise fallback can be dropped in favor of mathlib's own dispatch (follow-up, not this PR)