Skip to content

fix: check overflow for Pedersen grumpkin scalars#10462

Merged
guipublic merged 6 commits intomasterfrom
gd/pedersen_overflow
Nov 11, 2025
Merged

fix: check overflow for Pedersen grumpkin scalars#10462
guipublic merged 6 commits intomasterfrom
gd/pedersen_overflow

Conversation

@guipublic
Copy link
Contributor

Description

Problem

Resolves a bug (missing constraints) in Pedersen hash.

Summary

When getting the (lo, hi) limbs of a field as Grumpkin scalar for Pedersen hashes, we were relying on the implicit MSM constraints. However MSM does not know that the input is coming from Noir field elements, so it only checks for Grumpkin overflow, not the Noir field overflow.

Additional Context

User Documentation

Check one:

  • No user documentation needed.
  • Changes in docs/ included in this PR.
  • [For Experimental Features] Changes in docs/ to be submitted in a separate PR.

PR Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@guipublic guipublic requested a review from TomAFrench November 10, 2025 22:20
@github-actions
Copy link
Contributor

github-actions bot commented Nov 10, 2025

Changes to number of Brillig opcodes executed

Generated at commit: fbf8ffd71ac976f2117c3199e3d94c13c5a961a4, compared to commit: fd27764583beb61e7c485cf07b2498ba42d3c386

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
import_inliner_min +31 ❌ +91.18%
import_inliner_max +15 ❌ +44.12%
import_inliner_zero +15 ❌ +44.12%
brillig_pedersen_inliner_min +122 ❌ +20.64%
pedersen_check_inliner_min +122 ❌ +20.64%
brillig_pedersen_inliner_zero +122 ❌ +20.64%
pedersen_check_inliner_zero +122 ❌ +20.64%

Full diff report 👇
Program Brillig opcodes (+/-) %
import_inliner_min 65 (+31) +91.18%
import_inliner_max 49 (+15) +44.12%
import_inliner_zero 49 (+15) +44.12%
brillig_pedersen_inliner_min 713 (+122) +20.64%
pedersen_check_inliner_min 713 (+122) +20.64%
brillig_pedersen_inliner_zero 713 (+122) +20.64%
pedersen_check_inliner_zero 713 (+122) +20.64%
pedersen_hash_inliner_min 456 (+59) +14.86%
merkle_insert_inliner_min 2,795 (+347) +14.17%
pedersen_commitment_inliner_max 220 (+26) +13.40%
pedersen_commitment_inliner_zero 220 (+26) +13.40%
brillig_pedersen_inliner_max 653 (+62) +10.49%
pedersen_check_inliner_max 653 (+62) +10.49%
simple_shield_inliner_min 2,019 (+190) +10.39%
simple_shield_inliner_zero 2,301 (+190) +9.00%
simple_shield_inliner_max 2,269 (+158) +7.48%
pedersen_hash_inliner_max 423 (+26) +6.55%
pedersen_hash_inliner_zero 423 (+26) +6.55%
strings_inliner_min 1,645 (+78) +4.98%
merkle_insert_inliner_max 3,091 (+146) +4.96%
merkle_insert_inliner_zero 3,218 (+146) +4.75%
brillig_cow_regression_inliner_min 189,106 (+59) +0.03%
brillig_cow_regression_inliner_max 188,856 (+26) +0.01%
brillig_cow_regression_inliner_zero 188,856 (+26) +0.01%
pedersen_commitment_inliner_min 157 (-37) -19.07%

@github-actions
Copy link
Contributor

github-actions bot commented Nov 10, 2025

Changes to Brillig bytecode sizes

Generated at commit: fbf8ffd71ac976f2117c3199e3d94c13c5a961a4, compared to commit: fd27764583beb61e7c485cf07b2498ba42d3c386

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
import_inliner_min +29 ❌ +72.50%
import_inliner_max +17 ❌ +42.50%
import_inliner_zero +17 ❌ +42.50%
pedersen_commitment_inliner_min -28 ✅ -17.28%

Full diff report 👇
Program Brillig opcodes (+/-) %
import_inliner_min 69 (+29) +72.50%
import_inliner_max 57 (+17) +42.50%
import_inliner_zero 57 (+17) +42.50%
pedersen_check_inliner_max 451 (+44) +10.81%
brillig_pedersen_inliner_max 451 (+44) +10.81%
pedersen_hash_inliner_min 289 (+28) +10.73%
pedersen_commitment_inliner_max 178 (+16) +9.88%
pedersen_commitment_inliner_zero 178 (+16) +9.88%
strings_inliner_min 972 (+78) +8.72%
simple_shield_inliner_max 735 (+58) +8.57%
pedersen_check_inliner_min 438 (+31) +7.62%
brillig_pedersen_inliner_min 438 (+31) +7.62%
pedersen_check_inliner_zero 438 (+31) +7.62%
brillig_pedersen_inliner_zero 438 (+31) +7.62%
merkle_insert_inliner_min 403 (+28) +7.47%
pedersen_hash_inliner_max 277 (+16) +6.13%
pedersen_hash_inliner_zero 277 (+16) +6.13%
merkle_insert_inliner_max 583 (+30) +5.42%
merkle_insert_inliner_zero 400 (+16) +4.17%
brillig_cow_regression_inliner_min 1,234 (+28) +2.32%
brillig_cow_regression_inliner_max 1,214 (+16) +1.34%
brillig_cow_regression_inliner_zero 1,214 (+16) +1.34%
simple_shield_inliner_min 648 (-46) -6.63%
simple_shield_inliner_zero 631 (-46) -6.79%
pedersen_commitment_inliner_min 134 (-28) -17.28%

@github-actions
Copy link
Contributor

github-actions bot commented Nov 10, 2025

Changes to circuit sizes

Generated at commit: fbf8ffd71ac976f2117c3199e3d94c13c5a961a4, compared to commit: fd27764583beb61e7c485cf07b2498ba42d3c386

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
import +29 ❌ +725.00% +2,820 ❌ +4947.37%
merkle_insert +348 ❌ +756.52% 0 ➖ 0.00%
pedersen_check +87 ❌ +580.00% 0 ➖ 0.00%
pedersen_commitment +58 ❌ +725.00% 0 ➖ 0.00%
pedersen_hash +58 ❌ +966.67% 0 ➖ 0.00%
simple_shield +377 ❌ +802.13% 0 ➖ 0.00%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
import 33 (+29) +725.00% 2,877 (+2,820) +4947.37%
merkle_insert 394 (+348) +756.52% 28,688 (0) 0.00%
pedersen_check 102 (+87) +580.00% 28,688 (0) 0.00%
pedersen_commitment 66 (+58) +725.00% 28,688 (0) 0.00%
pedersen_hash 64 (+58) +966.67% 28,688 (0) 0.00%
simple_shield 424 (+377) +802.13% 28,688 (0) 0.00%

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: c0cb5ac Previous: 60bfcf2 Ratio
rollup-block-root-single-tx 0.003 s 0.002 s 1.50
rollup-checkpoint-merge 0.004 s 0.003 s 1.33

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@guipublic
Copy link
Contributor Author

An alternative would be to use the already existing 'safe' function and rely on range-check optimisations in ACIR.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 60269f2 Previous: 60bfcf2 Ratio
test_report_zkpassport_noir-ecdsa_ 2 s 1 s 2

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

LGTM

@TomAFrench
Copy link
Member

Just needs some snapshots updated.

@guipublic guipublic added this pull request to the merge queue Nov 11, 2025
Merged via the queue into master with commit 975ef74 Nov 11, 2025
132 checks passed
@guipublic guipublic deleted the gd/pedersen_overflow branch November 11, 2025 21:14
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Nov 12, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix(frontend)!: Preserve int type when quoting tokens
(noir-lang/noir#10330)
fix: check overflow for Pedersen grumpkin scalars
(noir-lang/noir#10462)
chore(frontend): Various tests in elaborator expressions submodule and
minor refactors (noir-lang/noir#10475)
chore: bump external pinned commits
(noir-lang/noir#10477)
fix: disallow keywords in attributes
(noir-lang/noir#10473)
chore: refactor codegen_control_flow
(noir-lang/noir#10320)
fix: builtin with body now errors instead of crashing
(noir-lang/noir#10474)
fix: handle ambiguous trait methods in assumed traits
(noir-lang/noir#10468)
fix: force_substitute bindings during monomorphization for associated
constants (noir-lang/noir#10467)
fix(brillig): Skip decrementing ref-count in array/vector copy and other
refactors (noir-lang/noir#10335)
fix(ssa): Cast to `u64` when inserting OOB checks in DIE
(noir-lang/noir#10463)
fix: disallow comptime-only types in non-comptime globals
(noir-lang/noir#10458)
chore(fuzzing): fix default artifact for brillig target
(noir-lang/noir#10465)
END_COMMIT_OVERRIDE
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Nov 12, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix(frontend)!: Preserve int type when quoting tokens
(noir-lang/noir#10330)
fix: check overflow for Pedersen grumpkin scalars
(noir-lang/noir#10462)
chore(frontend): Various tests in elaborator expressions submodule and
minor refactors (noir-lang/noir#10475)
chore: bump external pinned commits
(noir-lang/noir#10477)
fix: disallow keywords in attributes
(noir-lang/noir#10473)
chore: refactor codegen_control_flow
(noir-lang/noir#10320)
fix: builtin with body now errors instead of crashing
(noir-lang/noir#10474)
fix: handle ambiguous trait methods in assumed traits
(noir-lang/noir#10468)
fix: force_substitute bindings during monomorphization for associated
constants (noir-lang/noir#10467)
fix(brillig): Skip decrementing ref-count in array/vector copy and other
refactors (noir-lang/noir#10335)
fix(ssa): Cast to `u64` when inserting OOB checks in DIE
(noir-lang/noir#10463)
fix: disallow comptime-only types in non-comptime globals
(noir-lang/noir#10458)
chore(fuzzing): fix default artifact for brillig target
(noir-lang/noir#10465)
END_COMMIT_OVERRIDE
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Nov 12, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix(frontend)!: Preserve int type when quoting tokens
(noir-lang/noir#10330)
fix: check overflow for Pedersen grumpkin scalars
(noir-lang/noir#10462)
chore(frontend): Various tests in elaborator expressions submodule and
minor refactors (noir-lang/noir#10475)
chore: bump external pinned commits
(noir-lang/noir#10477)
fix: disallow keywords in attributes
(noir-lang/noir#10473)
chore: refactor codegen_control_flow
(noir-lang/noir#10320)
fix: builtin with body now errors instead of crashing
(noir-lang/noir#10474)
fix: handle ambiguous trait methods in assumed traits
(noir-lang/noir#10468)
fix: force_substitute bindings during monomorphization for associated
constants (noir-lang/noir#10467)
fix(brillig): Skip decrementing ref-count in array/vector copy and other
refactors (noir-lang/noir#10335)
fix(ssa): Cast to `u64` when inserting OOB checks in DIE
(noir-lang/noir#10463)
fix: disallow comptime-only types in non-comptime globals
(noir-lang/noir#10458)
chore(fuzzing): fix default artifact for brillig target
(noir-lang/noir#10465)
END_COMMIT_OVERRIDE
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