[kriss39]

Description

When a circuit has two BSB22 commitments that don't depend on each other, groth16 and plonk proofs sometimes fail to verify (proof rejected / algebraic relation does not hold).

The solver computes a level in parallel once it has enough instructions, so both commitment hints can run at the same time. Each hint does Write / Sum / Reset on the same hash-to-field instance (opt.HashToFieldFn in groth16, s.htfFunc in plonk). When the calls interleave, one commitment value is hashed together with the other one, the commitment wire gets a wrong value, and the verifier's recomputation doesn't match. go test -race reports the race on wrappedHashToField.Write/Reset from groth16/bn254.Prove.func1 and plonk/bn254.(*instance).bsb22Hint.

The fix puts the hash calls behind a mutex. It's done in the groth16 and plonk prover templates and in the icicle groth16 template, then regenerated.

Type of change

• Bug fix (non-breaking change which fixes an issue)

How has this been tested?

Added TestIndependentCommitments to backend/groth16 and backend/plonk. The circuit has two independent commitments surrounded by ~400 constraints so both hints land in the same parallel level, and it proves 20 times.

• go test -race -run TestIndependentCommitments ./backend/groth16/ ./backend/plonk/: 8 data races on master, clean with the fix. Without -race master fails too but not on every run (e.g. plonk at proof 18).
• go test -short ./backend/...

I couldn't build the icicle package locally (no icicle library), the change there is the same few lines as in groth16.

Checklist:

• I have performed a self-review of my code
• I have added tests that prove my fix is effective or that my feature works
• I did not modify files generated from templates (templates changed, files regenerated)
• New and existing unit tests pass locally with my changes

---

Note

Medium Risk
Touches core proving paths for BSB22 commitments across groth16/plonk; incorrect locking could still break proofs, but the change is narrowly scoped to shared hasher access and is covered by new concurrency tests.

Overview
Fixes intermittent proof verification failures when a circuit has multiple independent BSB22 commitments: the constraint solver can run commitment hints in parallel, but each hint was sharing one hash-to-field instance (opt.HashToFieldFn in groth16, s.htfFunc in plonk) without synchronization, so interleaved Write/Sum/Reset could corrupt commitment wire values.

The PR serializes hash-to-field use with a mutex around those calls in the BSB22 commitment hint paths for groth16, plonk, and icicle groth16 (via prover templates plus regenerated curve-specific code). Plonk stores htfLock on the prover instance; groth16/icicle use a local htfLock in the hint closure.

Adds TestIndependentCommitments in backend/groth16 and backend/plonk: a circuit with two parallelizable commitments, proved 20 times to stress concurrent hints.

^{Reviewed by Cursor Bugbot for commit c73d789. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Bugbot needs on-demand usage enabled

Bugbot uses usage-based billing for this team and requires on-demand usage to be enabled.

A team admin can enable on-demand usage in the Cursor dashboard.

[ivokub]

Oh, thanks -- I haven't checked deeply yet, but skimmed quickly through and you're right. Perhaps it would make sense to have independent hasher instances instead, in which case we don't synchronize the parallel computation? Would it require significant refactor?

[kriss39]

Thanks! A per-hint instance would be nice, but HashToFieldFn is a user-supplied hash.Hash (via WithProverHashToFieldFunction) and there is no general way to clone one, so we would need a factory option instead of a hasher, which changes the public API. The lock only covers the Write/Sum/Reset of the hash, the pedersen/KZG commitment MSMs still run in parallel, so it should not cost anything noticeable. If you prefer, I can create a fresh hasher per hint for the default hash-to-field and keep the lock only for custom ones.

[ivokub]

Thanks! A per-hint instance would be nice, but HashToFieldFn is a user-supplied hash.Hash (via WithProverHashToFieldFunction) and there is no general way to clone one, so we would need a factory option instead of a hasher, which changes the public API. The lock only covers the Write/Sum/Reset of the hash, the pedersen/KZG commitment MSMs still run in parallel, so it should not cost anything noticeable. If you prefer, I can create a fresh hasher per hint for the default hash-to-field and keep the lock only for custom ones.

Thanks!

Yeah, indeed that we provide instance of the hasher instead of the index, so its difficult to copy. The API compatibility is a bit of a mess (I think at some point we used to overcome the issue by hashing a known value and checking what the digest is to predict the used hash, but its an ugly solution and I'd not have it elsewhere).

Also -- mostly we use only up to a single commitment, so this doesn't really show up, but we should fix for nbCommitments > 1 nevertheless to avoid footguns in the future. And hashing is fast, so synchronizing is fine as well.

You can keep the PR as is for now -- I'll have a look with fresh head soon.

[kriss39]

Hi @ivokub, friendly ping on this one and on my two other small fixes when you have a moment:

• fix(schema): follow pointer fields when building the schema #1846: schema.New skips pointer-to-struct fields, so the JSON witness of such a circuit fails
• fix(conversion): handle a partial top limb in EmulatedToBytes #1847: EmulatedToBytes panics when the top limb is not full (e.g. P-224 with 4x64-bit limbs)

All three are still mergeable on current master. Happy to adjust anything.

[ivokub]

Hi @ivokub, friendly ping on this one and on my two other small fixes when you have a moment:

• fix(schema): follow pointer fields when building the schema #1846: schema.New skips pointer-to-struct fields, so the JSON witness of such a circuit fails
• fix(conversion): handle a partial top limb in EmulatedToBytes #1847: EmulatedToBytes panics when the top limb is not full (e.g. P-224 with 4x64-bit limbs)

All three are still mergeable on current master. Happy to adjust anything.

Please don't spam on all the opened PRs with different alt-accounts. We review and merge when we have time to do so.