Skip to content

fix: multiround commits with shared vals - #1814

Open
shramee wants to merge 3 commits into
Consensys-Incorporated:masterfrom
shramee:multiround-commits
Open

fix: multiround commits with shared vals#1814
shramee wants to merge 3 commits into
Consensys-Incorporated:masterfrom
shramee:multiround-commits

Conversation

@shramee

@shramee shramee commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Description

TLDR; privateCommittedSeeker.Seek returns index from original commitment indexes, but code tries to find it in the sliced-in-loop commitment indexes.

builder.Commit (Groth16 R1CS builder) panics with a slice-index-out-of-range error when a circuit commits to a variable, and then later commits again to a variable that is already covered by an earlier commitment (i.e. the "re-commit-to-a-commitment" path).

The bug is in frontend/cs/r1cs/api.go, in (*builder[E]).Commit. The function computes existingCommitmentIndexes := commitments.CommitmentIndexes() and then, over the course of the sort/merge loop, progressively slices this same variable down (existingCommitmentIndexes = existingCommitmentIndexes[1:]) as it consumes committed wires in VID order:

https://github.com/Consensys/gnark/blob/master/frontend/cs/r1cs/api.go#L881

commitments := builder.cs.GetCommitments().(constraint.Groth16Commitments)
existingCommitmentIndexes := commitments.CommitmentIndexes()
privateCommittedSeeker := utils.MultiListSeeker(commitments.GetPrivateCommitted())
...
for len(existingCommitmentIndexes) > 0 && existingCommitmentIndexes[0] < t.VID {
    existingCommitmentIndexes = existingCommitmentIndexes[1:]
}
...
if committer := privateCommittedSeeker.Seek(t.VID); committer != -1 {
    committerWireIndex := existingCommitmentIndexes[committer] // BUG
    ...
}

privateCommittedSeeker.Seek(t.VID) returns committer, an index into the original, unsliced list of commitments (it indexes commitments.GetPrivateCommitted(), which has one entry per commitment, in original commitment order). But by the time the loop reaches a given t.VID, existingCommitmentIndexes has already been sliced down (its front elements dropped) by the existingCommitmentIndexes[0] < t.VID loop above. Indexing the sliced slice with an index meant for the original slice reads the wrong element, or — as soon as the slice has been shortened past committer's original position — panics with an out-of-range index.

Fixes #1813

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How has this been tested?

  • New r1cs test - TestCommitToAlreadyCommittedVariable

How has this been benchmarked?

Not applicable

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I did not modify files generated from templates
  • golangci-lint does not output errors locally
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Note

Medium Risk
Touches multi-round commitment compilation logic used by Groth16 circuits; the change is narrow but incorrect indexing could still affect proof soundness if wrong.

Overview
Fixes a panic / wrong-wire bug in Groth16 R1CS Commit when a later commitment includes a private wire that was already covered by an earlier commitment.

Commit still advances a sliced existingCommitmentIndexes while merging wires in VID order, but the “already privately committed” path now resolves the commitment wire via allCommitmentIndexes[committer], because privateCommittedSeeker.Seek returns an index into the full commitment list—not the shortened slice.

Adds TestCommitToAlreadyCommittedVariable: two commits that privately cover v1, then a third Commit(v1) that should redirect to the existing commitment wire; compile and solve must succeed.

Reviewed by Cursor Bugbot for commit 76aca44. Bugbot is set up for automated code reviews on this repo. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Commit panics with "index out of range" when re-committing a variable that is already part of an earlier commitment

1 participant