Fix MultiScalarMul small-n pathology and NewRandomZr allocation regression

driver/gurvy/bls12-377.go

@@ -226,7 +226,20 @@
 	common.CurveBase
 }
 
+// MultiScalarMul computes the sum of the scalar multiplications of the given bases by the
+// given scalars via gnark's bucket-method MultiExp. MultiExp carries a fixed cost (window
+// and chunk setup, goroutine fan-out) that a pairwise Mul2+Add loop does not, so for very
+// small n a caller that knows its sizes may be better served by Mul/Mul2 directly; callers
+// that care make that choice themselves, and this method does not second-guess them beyond
+// the trivial n==0 and n==1 cases.
 func (c *Bls12_377) MultiScalarMul(a []driver.G1, b []driver.Zr) driver.G1 {
+	switch n := len(a); {
+	case n == 0:
+		return &bls12377G1{}
+	case n == 1:
+		return a[0].(*bls12377G1).Mul(b[0])
+	}
+
 	var result bls12377.G1Affine
 	affinePoints := make([]bls12377.G1Affine, len(a))
 	scalars := make([]fr.Element, len(b))

driver/gurvy/bls12381/bls12-381.go

@@ -639,13 +639,29 @@
 	return res
 }
 
+// NewRandomZr draws a uniformly random scalar using rng as the exclusive source of entropy
+// (so the same reader/seed always produces the same scalar), via rejection sampling on a
+// stack buffer - the same strategy fr.Element.SetRandom uses internally, but reading from
+// the caller's reader instead of crypto/rand. Acceptance probability is q/2^255 ~= 0.90,
+// i.e. ~1.1 iterations expected. Allocation-free, unlike a rand.Int-based implementation.
 func (c *Curve) NewRandomZr(rng io.Reader) driver.Zr {
-	bi, err := rand.Int(rng, &c.Modulus)
-	if err != nil {
-		panic(err)
+	var buf [fr.Bytes]byte
+
+	z := &Zr{}
+
+	for {
+		if _, err := io.ReadFull(rng, buf[:]); err != nil {
+			panic(err)
+		}
+
+		buf[0] &= 0x7f // fr's modulus is 255 bits (top byte 0x73)
+
+		if err := z.val.SetBytesCanonical(buf[:]); err == nil {
+			break
+		}
 	}
 
-	return &Zr{val: *new(fr.Element).SetBigInt(bi)}
+	return z
 }
 
 func (c *Curve) HashToZr(data []byte) driver.Zr {
@@ -777,13 +793,30 @@
 	a.rawBigInt = nil
 }
 
+// MultiScalarMul computes the sum of the scalar multiplications of the given bases by the
+// given scalars via gnark's bucket-method MultiExp. MultiExp carries a fixed cost (window
+// and chunk setup, goroutine fan-out) that a pairwise Mul2+Add loop does not, so for very
+// small n a caller that knows its sizes may be better served by Mul/Mul2 directly; callers
+// that care make that choice themselves, and this method does not second-guess them beyond
+// the trivial n==0 and n==1 cases.
 func (c *Curve) MultiScalarMul(a []driver.G1, b []driver.Zr) driver.G1 {
+	switch n := len(a); {
+	case n == 0:
+		return &G1{}
+	case n == 1:
+		return a[0].(*G1).Mul(b[0])
+	}
+
 	affinePoints := make([]bls12381.G1Affine, len(a))
 	scalars := make([]fr.Element, len(b))
 
+	bi := bigIntPool.Get()
+	defer bigIntPool.Put(bi)
+
 	for i := range a {
 		affinePoints[i] = a[i].(*G1).G1Affine
-		scalars[i] = b[i].(*Zr).val // Direct fr.Element copy — no SetBigInt!
+		b[i].(*Zr).toBigInt(bi)
+		scalars[i].SetBigInt(bi)
 	}
 
 	first := G1Jacs.Get()

driver/gurvy/bn254.go

@@ -229,7 +229,20 @@
 	common.CurveBase
 }
 
+// MultiScalarMul computes the sum of the scalar multiplications of the given bases by the
+// given scalars via gnark's bucket-method MultiExp. MultiExp carries a fixed cost (window
+// and chunk setup, goroutine fan-out) that a pairwise Mul2+Add loop does not, so for very
+// small n a caller that knows its sizes may be better served by Mul/Mul2 directly; callers
+// that care make that choice themselves, and this method does not second-guess them beyond
+// the trivial n==0 and n==1 cases.
 func (c *Bn254) MultiScalarMul(a []driver.G1, b []driver.Zr) driver.G1 {
+	switch n := len(a); {
+	case n == 0:
+		return &bn254G1{}
+	case n == 1:
+		return a[0].(*bn254G1).Mul(b[0])
+	}
+
 	var result bn254.G1Affine
 	affinePoints := make([]bn254.G1Affine, len(a))
 	scalars := make([]fr.Element, len(b))

driver/math.go

@@ -151,7 +151,10 @@ type Curve interface {
 	// HashToG2WithDomain hashes data to G2 with domain separation.
 	HashToG2WithDomain(data, domain []byte) G2
 
-	// NewRandomZr generates a random scalar using the provided RNG.
+	// NewRandomZr generates a random scalar using the provided RNG. rng must be the
+	// exclusive source of entropy: the same reader (e.g. a deterministically seeded one)
+	// must always produce the same scalar, so implementations must not draw from any other
+	// randomness source (such as crypto/rand directly) as a shortcut.
 	NewRandomZr(rng io.Reader) Zr
 
 	// Rand returns a cryptographically secure random number generator.

math.go

@@ -748,7 +748,9 @@ func (c *Curve) Rand() (io.Reader, error) {
 }
 
 // NewRandomZr generates a random scalar using the provided random number generator.
-// The scalar is uniformly distributed in the range [0, group order).
+// The scalar is uniformly distributed in the range [0, group order). rng is the exclusive
+// source of entropy: passing a deterministic reader (e.g. seeded from a fixed value) always
+// yields the same scalar, and implementations must not fall back to any other source.
 func (c *Curve) NewRandomZr(rng io.Reader) *Zr {
 	return &Zr{zr: c.c.NewRandomZr(rng), curveID: c.curveID}
 }

math_test.go

@@ -7,6 +7,7 @@ SPDX-License-Identifier: Apache-2.0
 package math
 
 import (
+	"bytes"
 	"crypto/rand"
 	"encoding/json"
 	"fmt"
@@ -326,23 +327,59 @@ func runMultiScalarMul(t *testing.T, c *Curve) {
 	rng, err := c.Rand()
 	require.NoError(t, err)
 
-	n := 10
-	g1s := make([]*G1, n)
-	zrs := make([]*Zr, n)
-	for i := range n {
-		g1s[i] = c.GenG1.Mul(c.NewRandomZr(rng))
-		zrs[i] = c.NewRandomZr(rng)
+	// sweep the trivially special-cased sizes (0, 1) and a spread of sizes that go through
+	// the general MultiExp path.
+	for _, n := range []int{0, 1, 2, 6, 7, 8, 10, 33} {
+		g1s := make([]*G1, n)
+		zrs := make([]*Zr, n)
+		for i := range n {
+			g1s[i] = c.GenG1.Mul(c.NewRandomZr(rng))
+			zrs[i] = c.NewRandomZr(rng)
+		}
+
+		// trivial multi scalar mul
+		g1 := c.NewG1()
+		for i := range n {
+			g1.Add(g1s[i].Mul(zrs[i]))
+		}
+		// single call
+		g2 := c.MultiScalarMul(g1s, zrs)
+
+		assert.True(t, g1.Equals(g2), "curve %s: MultiScalarMul mismatch at n=%d", CurveIDToString(c.curveID), n)
 	}
 
-	// trivial multi scalar mul
+	// a zero scalar and an infinity base must not upset MultiScalarMul.
+	g1s := []*G1{c.GenG1.Mul(c.NewRandomZr(rng)), c.NewG1(), c.GenG1.Mul(c.NewRandomZr(rng))}
+	zrs := []*Zr{c.NewRandomZr(rng), c.NewRandomZr(rng), c.NewZrFromInt(0)}
+
 	g1 := c.NewG1()
-	for i := range n {
+	for i := range g1s {
 		g1.Add(g1s[i].Mul(zrs[i]))
 	}
-	// single call
 	g2 := c.MultiScalarMul(g1s, zrs)
-
-	assert.True(t, g1.Equals(g2))
+	assert.True(t, g1.Equals(g2), "curve %s: MultiScalarMul mismatch with zero scalar/infinity base", CurveIDToString(c.curveID))
+
+	// GroupOrder is a special-cased Zr (see bls12381.Zr's rawBigInt doc comment) whose val
+	// is 0 but whose true value must still be honored by scalar multiplication - pins the
+	// consistency fix between MultiScalarMul's small-n Mul path and its MultiExp path.
+	// Exercised at two sizes so the fix covers both.
+	for _, n := range []int{2, 10} {
+		g1sGO := make([]*G1, n)
+		zrsGO := make([]*Zr, n)
+		g1sGO[0] = c.GenG1.Mul(c.NewRandomZr(rng))
+		zrsGO[0] = c.GroupOrder
+		for i := 1; i < n; i++ {
+			g1sGO[i] = c.GenG1.Mul(c.NewRandomZr(rng))
+			zrsGO[i] = c.NewRandomZr(rng)
+		}
+
+		g1GO := c.NewG1()
+		for i := range g1sGO {
+			g1GO.Add(g1sGO[i].Mul(zrsGO[i]))
+		}
+		g2GO := c.MultiScalarMul(g1sGO, zrsGO)
+		assert.True(t, g1GO.Equals(g2GO), "curve %s: MultiScalarMul mismatch with GroupOrder scalar at n=%d", CurveIDToString(c.curveID), n)
+	}
 }
 
 func runG2Test(t *testing.T, c *Curve) {
@@ -1036,3 +1073,39 @@ func TestNewRandomZrHonorsReader(t *testing.T) {
 			"curve %s: NewRandomZr gave identical scalars for different seeds", name)
 	}
 }
+
+// TestNewRandomZrRejectsBadReader verifies that a reader which cannot supply enough entropy
+// causes NewRandomZr to panic rather than silently fall back to some other randomness source.
+func TestNewRandomZrRejectsBadReader(t *testing.T) {
+	for _, curve := range Curves {
+		name := CurveIDToString(curve.curveID)
+		assert.Panics(t, func() {
+			curve.NewRandomZr(bytes.NewReader(nil))
+		}, "curve %s: NewRandomZr should panic when the reader has no data", name)
+	}
+}
+
+// TestNewRandomZrDistribution is a smoke check (not a rigorous statistical test) that the
+// bls12381 driver's stack-buffer rejection-sampling NewRandomZr does not produce duplicates
+// or an obviously biased high bit over a few thousand draws - the kind of bug an incorrect
+// mask (e.g. zeroing more bits than the 255-bit modulus requires) would introduce.
+func TestNewRandomZrDistribution(t *testing.T) {
+	curve := Curves[BLS12_381_GURVY]
+	rng, err := curve.Rand()
+	require.NoError(t, err)
+
+	const samples = 4096
+	seen := make(map[string]struct{}, samples)
+	secondBitSet := 0
+	for range samples {
+		z := curve.NewRandomZr(rng)
+		b := z.Bytes()
+		seen[string(b)] = struct{}{}
+		if b[0]&0x40 != 0 {
+			secondBitSet++
+		}
+	}
+	assert.Len(t, seen, samples, "NewRandomZr produced a duplicate scalar in %d draws", samples)
+	assert.InDelta(t, samples/2, secondBitSet, float64(samples)/8,
+		"NewRandomZr's second-highest bit looks biased over %d draws", samples)
+}

perf_test.go

@@ -8,6 +8,7 @@ package math
 
 import (
 	"crypto/rand"
+	"fmt"
 	"io"
 	"math/big"
 	"testing"
@@ -281,6 +282,110 @@ func Benchmark_Parallel_BLS(b *testing.B) {
 	}
 }
 
+// gnarkBackedCurveIDs lists the curves whose driver goes through gnark-crypto's
+// bucket-method MultiExp, i.e. the ones the MultiScalarMul small-n dispatch applies to.
+var gnarkBackedCurveIDs = []CurveID{BN254, BLS12_377_GURVY, BLS12_381_GURVY, BLS12_381_BBS_GURVY}
+
+// Benchmark_Sequential_NewRandomZr measures random scalar generation across curves.
+func Benchmark_Sequential_NewRandomZr(b *testing.B) {
+	for _, curve := range Curves {
+		rng, err := curve.Rand()
+		if err != nil {
+			panic(err)
+		}
+
+		b.Run("curve "+CurveIDToString(curve.curveID), func(b *testing.B) {
+			b.ReportAllocs()
+			b.ResetTimer()
+			for range b.N {
+				_ = curve.NewRandomZr(rng)
+			}
+		})
+	}
+}
+
+// Benchmark_Sequential_G1Mul measures a single G1 scalar multiplication, the baseline
+// that MultiScalarMul and Mul2 are compared against at small n.
+func Benchmark_Sequential_G1Mul(b *testing.B) {
+	for _, id := range gnarkBackedCurveIDs {
+		curve := Curves[id]
+		rng, err := curve.Rand()
+		if err != nil {
+			panic(err)
+		}
+
+		p := curve.GenG1.Mul(curve.NewRandomZr(rng))
+		s := curve.NewRandomZr(rng)
+
+		b.Run("curve "+CurveIDToString(id), func(b *testing.B) {
+			b.ReportAllocs()
+			b.ResetTimer()
+			for range b.N {
+				_ = p.Mul(s)
+			}
+		})
+	}
+}
+
+// Benchmark_Sequential_G1Mul2 measures the joint two-scalar multiplication used as the
+// small-n building block for MultiScalarMul.
+func Benchmark_Sequential_G1Mul2(b *testing.B) {
+	for _, id := range gnarkBackedCurveIDs {
+		curve := Curves[id]
+		rng, err := curve.Rand()
+		if err != nil {
+			panic(err)
+		}
+
+		p := curve.GenG1.Mul(curve.NewRandomZr(rng))
+		q := curve.GenG1.Mul(curve.NewRandomZr(rng))
+		s1 := curve.NewRandomZr(rng)
+		s2 := curve.NewRandomZr(rng)
+
+		b.Run("curve "+CurveIDToString(id), func(b *testing.B) {
+			b.ReportAllocs()
+			b.ResetTimer()
+			for range b.N {
+				_ = p.Mul2(s1, q, s2)
+			}
+		})
+	}
+}
+
+// Benchmark_Sequential_MultiScalarMul sweeps the number of (base, scalar) pairs on each
+// gnark-backed curve to find where MultiExp starts to beat a pairwise loop. MultiScalarMul
+// always takes the MultiExp path (beyond the trivial n<=1 cases); this benchmark documents
+// the size range in which a caller that knows its sizes is better off calling Mul/Mul2 itself
+// (see the CSP range proof's smallMSM in Panurus for such a caller).
+func Benchmark_Sequential_MultiScalarMul(b *testing.B) {
+	sizes := []int{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 12, 16, 20, 32, 64}
+
+	for _, id := range gnarkBackedCurveIDs {
+		curve := Curves[id]
+		rng, err := curve.Rand()
+		if err != nil {
+			panic(err)
+		}
+
+		for _, n := range sizes {
+			bases := make([]*G1, n)
+			scalars := make([]*Zr, n)
+			for i := range bases {
+				bases[i] = curve.GenG1.Mul(curve.NewRandomZr(rng))
+				scalars[i] = curve.NewRandomZr(rng)
+			}
+
+			b.Run(fmt.Sprintf("curve %s/n=%d", CurveIDToString(id), n), func(b *testing.B) {
+				b.ReportAllocs()
+				b.ResetTimer()
+				for range b.N {
+					_ = curve.MultiScalarMul(bases, scalars)
+				}
+			})
+		}
+	}
+}
+
 func Benchmark_Parallel_IndividualOpsGurvy(b *testing.B) {
 	curve := Curves[BLS12_381_GURVY]
 	g_gurv, x_gurv := blsInitGurvy(b)