Feat : MSM black box implementation

[ocdbytes]

MSM Black Box Support

• Implements black-box Multi-Scalar Multiplication (MSM) in the R1CS compiler with full elliptic
  curve arithmetic over multi-limb field elements.
• Adds a cost model that analytically selects optimal (limb_bits, window_size) parameters,
  following the same pattern as SHA256 spread optimization.
• Implements FakeGLV scalar decomposition for single-point MSM, reducing scalar width by ~50% via
  half-GCD and interleaved two-point windowed scalar multiplication.
• Adds bigint_mod.rs in the prover for 256-bit modular arithmetic (half-GCD, Barrett reduction,
  modular inverse) used in witness solving.
• 18.9% decrease in constraint count as compared to native noir implementation of msm.
• To test MSM blackbox :

  cargo test -p provekit-bench --test compiler -- "embedded_curve_msm"

Performance Comparison :
This is for 2 points call for msm: s_1[p_1] * s_2[p_2]

Metric	Native Noir MSM	Black Box MSM	% Reduction
R1CS Constraints	5,741	4,658	-18.9%
R1CS Witnesses	6,257	5,899	-5.7%
A Entries	6,749	6,813	+0.9%
B Entries	6,364	7,264	+14.1%
C Entries	14,453	9,021	-37.6%
W1 Size	3,354	1,769	-47.3%
W2 Size	3,367	4,130	+22.7%

[Bisht13]

/claude-review

[github-actions]

Code Review

Summary

This PR implements MSM (Multi-Scalar Multiplication) as a black-box opcode in the R1CS compiler, dispatching Noir's embedded_curve_ops::multi_scalar_mul to a custom constraint system rather than the native Noir implementation. The core idea is a two-base FakeGLV decomposition: for each input (P, s), the prover supplies R = s·P via hint, then verifies |s1|·P_eff + |s2|·R_eff = 0 (Phase 2) and the scalar relation ±|s1| ± |s2|·s ≡ 0 (mod n) (Phase 3). Together these two checks soundly establish R = s·P per point. Phase 4 accumulates per-point results into the final MSM output.

---

Review

Correctness & Soundness

The overall proof strategy is sound. Phases 2 + 3 jointly prove R = s·P for each point without directly inverting s2 in-circuit. Specific checks:

• compute_is_zero is correctly implemented using the standard two-constraint gadget (v·v_inv = 1 - is_zero, v·is_zero = 0). The output is uniquely determined and boolean without needing an explicit constrain_boolean. ✓
• inf_flag is boolean-constrained via constrain_boolean in detect_skip. ✓
• neg1/neg2 from FakeGLVHint are implicitly constrained boolean via ops.select(neg1_witness, ...) / ops.select(neg2_witness, ...) in preprocess_points, which internally calls constrain_boolean. ✓
• s2 ≠ 0 is enforced via constrain_zero(ops.compiler, s2_is_zero) in verify_scalar_relation — critical to prevent the degenerate case where the relation degenerates and leaves R unconstrained. ✓
• On-curve checks are applied to both the sanitized input point and the hint-output R. ✓
• The select_unchecked / point_select_unchecked calls all operate on flags that are provably boolean from prior constraints. ✓
• Accumulation uses the offset trick to avoid hitting the identity or requiring a doubling; per-point offset verification is done independently. ✓

One soundness concern to flag:

neg1/neg2 boolean check ordering. emit_fakeglv_hint is called before ops.select(neg1_witness, ...). If any future refactor moves the select call after the scalar_relation::verify_scalar_relation call (which uses neg1/neg2 with select_unchecked-equivalent operations), the boolean constraint on neg1/neg2 would disappear. The existing code is correct, but the boolean constraint is load-bearing and somewhat implicit. Consider adding an explicit constrain_boolean for neg1 and neg2 after emit_fakeglv_hint so the invariant is self-contained and not order-dependent.

Code Quality

Blocking: .expect() in library code — CLAUDE.md prohibits .unwrap() / .expect() in library code. Three violations:

1. provekit/r1cs-compiler/src/msm/ec_points/hints_native.rs — FieldElement::from_bigint(ark_ff::BigInt(params.ec.curve_a_raw)).expect("curve_a must fit in native field") (appears twice: once in point_double_verified_native, once in verify_on_curve_native). These curve parameters are known at compile time for supported curves and will never overflow BN254, but library code must still return anyhow::Result and propagate via ?. Consider encoding the conversion as a static assertion or moving parameter validation to EcFieldParams construction.

2. provekit/r1cs-compiler/src/msm/pipeline.rs in preprocess_points — all_skipped.expect("MSM must have at least one point"). Should use anyhow::ensure!(n_points > 0, "MSM must have at least one point") at the start of process_multi_point instead, propagating the error up.

Minor: Missing /// doc comments on pub methods in multi_limb_ops.rs. The add, sub, and mul methods on MultiLimbOps have #[must_use] but no /// doc comments. CI enforces cargo doc -D warnings for all pub items.

Non-blocking: NoirToR1CSCompiler and add_range_checks visibility widened to pub. Both were pub(crate). The change to pub broadens the public API of the r1cs-compiler crate. This appears intentional (the bench test uses them), but warrants a comment or #[doc(hidden)] if they are not intended as stable API.

Non-blocking: No comment on Grumpkin hardcode. In noir_to_r1cs.rs, MultiScalarMul always dispatches to crate::msm::curve::Grumpkin. A single-line comment // Noir's embedded_curve_ops::multi_scalar_mul targets the Grumpkin curve embedded in BN254 would clarify the intent.

Testing

• Grumpkin (native field, single-limb) path is tested end-to-end via embedded_curve_msm example including several degenerate cases (zero scalar, infinity, near-identity, near-order). ✓
• msm_witness_solving.rs provides detailed unit tests for the FakeGLV decomposition and witness solving. ✓
• The non-native path (NonNativeEcOps, multi-limb, e.g. secp256r1) has no integration test. The hints_non_native.rs file is ~462 lines of schoolbook column equation logic and contributes to a significant portion of added complexity. At minimum a property-based test verifying round-trip (prove + verify) for the secp256r1 curve would be expected before merge.
• No regression test for the .expect() crash paths (e.g., calling add_msm_with_curve with zero points). Once fixed to return anyhow::Result, the error path should be tested.

Suggestions (non-blocking)

• The bigint_mod.rs module in provekit/prover re-implements divmod, mod_pow, half_gcd, etc. entirely from scratch using 4-limb arithmetic. Consider whether num-bigint (already added to workspace deps) or ark-ff's BigInt primitives could replace some of this to reduce the maintenance surface. In particular, half_gcd is a non-trivial algorithm; a reference or proof-of-correctness comment would help future reviewers.
• scripts/verify_offset_points.py is a useful sanity-check script. Consider adding a comment at the top of curve/grumpkin.rs and curve/secp256r1.rs pointing to this script for the precomputed offset/accumulated-offset values.

Verdict

🔄 Request changes — The .expect() calls in library code are a CLAUDE.md violation and must be fixed. The missing integration test for the non-native path is a notable testing gap for safety-critical code. Everything else can be addressed as follow-ups.

[rose2221]

In add_sub_mod_p_core, negate_mod_p_multi, and less_than_p_check_multi, each limb iteration emits two linear constraints: one for the intermediate sum (v_offset/v/v_diff) and one for the remainder (r[i]/ri/d_i). These can be merged into a single constraint per limb.

For example in add_sub_mod_p_core, currently:

C1: (a[i] + b[i] + offset - p[i]*q + carry_prev) * 1 = v_offset
C2: (v_offset - carry * 2^W) * 1 = r[i]

Can become:
CM: (a[i] + b[i] + offset - p[i]*q + carry_prev - carry * 2^W) * 1 = r[i]

[rose2221]

nit: compute_is_zero allocates a value_mul_value_inv witness that never appears in any constraint, it's only used to compute is_zero via the Sum builder. Could be eliminated with a direct WitnessBuilder::IsZero which can be created explicitly

[rose2221]

constant_limbs(offset_x_values) and constant_limbs(offset_y_values) are called inside the per-point loop, allocating fresh pinned witnesses and constraints each iteration for identical values. Limbs is Copy, so these can be hoisted before the loop and reused. Same applies to the duplicate offset_x allocations in accumulate_and_constrain_outputs