Skip to content

feat(poseidon2): support BN254 widths t=4,8,12,16; fix matMulExternal indexing#1743

Merged
ivokub merged 1 commit intoConsensys:masterfrom
HerodotusDev:feat/poseidon2-bn254-master
Mar 31, 2026
Merged

feat(poseidon2): support BN254 widths t=4,8,12,16; fix matMulExternal indexing#1743
ivokub merged 1 commit intoConsensys:masterfrom
HerodotusDev:feat/poseidon2-bn254-master

Conversation

@beeinger
Copy link
Copy Markdown
Contributor

@beeinger beeinger commented Mar 31, 2026

Description

Adds support for BN254 Poseidon2 permutation widths t=4, 8, 12, and 16 in the circuit-side implementation, now that gnark-crypto provides precomputed constants (DiagM1 + round keys) for these widths via gnark-crypto#783.

Previously, matMulInternalInPlace panicked for any width beyond 2 or 3:

// TODO: we don't have general case implemented in gnark-crypto side.
panic("only T=2,3 is supported")

This PR:

  • bumps gnark-crypto to v0.20.1 (also picks up the IFMA vector mul correctness fix from gnark-crypto#816)
  • adds DiagM1 to the circuit Parameters struct and propagates it from gnark-crypto for BN254
  • implements the general-case internal matrix multiplication for width >= 4 using the DiagM1 diagonal, mirroring gnark-crypto's matMulInternalInPlace
  • fixes two bugs in matMulExternalInPlace for the default (width = 4k) branch:
    • accumulator was nil-initialized via make([]frontend.Variable, 4) instead of zero-valued
    • copy-paste error: input[4*i+1], input[4*i+2], input[4*i+3] were all incorrectly reading from input[4*i]

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How has this been tested?

How has this been benchmarked?

  • Not applicable (no performance-sensitive changes; the new code paths mirror gnark-crypto's existing matrix operations)

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 Poseidon2 permutation arithmetic used in circuits and changes matrix multiplication behavior for wider states, which can impact proof correctness if wrong. Coverage is improved with new width-specific and aliasing tests, reducing but not eliminating risk.

Overview
Adds Poseidon2 circuit support for BN254 widths t=4,8,12,16 by extending parameter handling with Parameters.DiagM1 and implementing the width >=4 internal matrix multiplication path (previously panicked).

Fixes a correctness bug in matMulExternalInPlace for widths 4k (bad accumulator init and wrong indexing), bumps github.com/consensys/gnark-crypto to v0.20.1, and adds regression tests: TestPoseidon2_BN254_Widths plus TestVectorAliasing to ensure vector ops are safe when result aliases inputs.

Written by Cursor Bugbot for commit 5d800c7. This will update automatically on new commits. Configure here.

… matMulExternalInPlace default; updated code generation; addressing: Consensys#1741 (comment)
@beeinger
Copy link
Copy Markdown
Contributor Author

@ivokub here is the PR, only the changes, with the go generate
As requested in: #1741 (comment)

@socket-security
Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Updatedgolang/​github.com/​consensys/​gnark-crypto@​v0.19.3-0.20260210233638-4abc1c162a65 ⏵ v0.20.176 +1100100100100

View full report

@ivokub ivokub merged commit 17b079f into Consensys:master Mar 31, 2026
13 checks passed
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.

2 participants