Skip to content

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

Merged
ivokub merged 2 commits intoConsensys:release/v0.15.0from
HerodotusDev:feat/poseidon2-bn254
Mar 31, 2026
Merged

feat(poseidon2): support BN254 widths t=4,8,12,16; fix matMulExternal indexing#1741
ivokub merged 2 commits intoConsensys:release/v0.15.0from
HerodotusDev:feat/poseidon2-bn254

Conversation

@beeinger
Copy link
Copy Markdown
Contributor

@beeinger beeinger commented Mar 27, 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 cryptographic permutation circuit logic and relies on new precomputed constants from an updated gnark-crypto dependency; mistakes here could silently change hashes/constraints, though the added test coverage reduces regression risk.

Overview
Adds circuit-side support for Poseidon2 on BN254 with widths t=4,8,12,16 by introducing Parameters.DiagM1, wiring it through parameter construction/defaults, and implementing the previously-missing matMulInternalInPlace general case for t>=4.

Fixes the matMulExternalInPlace path for widths 4k (correct zero initialization and per-lane indexing), bumps github.com/consensys/gnark-crypto to v0.20.1, and adds targeted tests: BN254 width coverage for Poseidon2 plus a new Vector aliasing test to ensure in-place ops behave correctly.

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

@socket-security
Copy link
Copy Markdown

socket-security bot commented Mar 27, 2026

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.20.0 ⏵ v0.20.176 +1100100100100

View full report

@beeinger
Copy link
Copy Markdown
Contributor Author

Hey @ivokub I am making this PR to your v0.15.0 release branch as I see you've updated the gnark-crypto to 0.20.0 here, I bumped it to your latest fixed v0.20.1 release. I am adding more T parameter options that are now unlocked by the new gnark-crypto version, it'd be great if you could have a look.
Let me know if you need anything from me!

@ivokub
Copy link
Copy Markdown
Collaborator

ivokub commented Mar 30, 2026

Thanks @beeinger - I'll have a look.

Copy link
Copy Markdown
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! Looks good. I'll check what the CI reports, but otherwise it is good to merge!

@ivokub
Copy link
Copy Markdown
Collaborator

ivokub commented Mar 31, 2026

Thanks for the contribution! Looks good. I'll check what the CI reports, but otherwise it is good to merge!

Ah, I see, with gnark-crypto update we also have update code generation which affects our fuzzing small-field backend (tinyfield). Can you run go generate ./... in the repo and also commit the changes to internal/smallfields/tinyfield/vector_test.go? I don't seem to be able to push directly to PR branch.

@beeinger
Copy link
Copy Markdown
Contributor Author

Sure, on it

@beeinger
Copy link
Copy Markdown
Contributor Author

@ivokub that should be it

@ivokub
Copy link
Copy Markdown
Collaborator

ivokub commented Mar 31, 2026

@ivokub that should be it

Thanks! Seems that the static check passes now, I'll wait for the complete CI run before merge

@ivokub ivokub merged commit 2d72907 into Consensys:release/v0.15.0 Mar 31, 2026
6 checks passed
@ivokub
Copy link
Copy Markdown
Collaborator

ivokub commented Mar 31, 2026

Oh, I just now realized the target branch was release/v0.15 - can you perhaps create a new PR targeting directly master? I'll release the release branch anyway on top of master before merging and then we would have the feature directly on master already.

It will probably take a bit more time before we can release v0.15 due to #1740

@beeinger
Copy link
Copy Markdown
Contributor Author

Yeah sure I can do the same PR to master

beeinger added a commit to HerodotusDev/gnark-poseidon2 that referenced this pull request Mar 31, 2026
… matMulExternalInPlace default; updated code generation; addressing: Consensys#1741 (comment)
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