Skip to content

fix(ssa): Validate field to integer cast#8799

Merged
vezenovm merged 15 commits intomasterfrom
mv/validate-field-to-int-cast
Jun 11, 2025
Merged

fix(ssa): Validate field to integer cast#8799
vezenovm merged 15 commits intomasterfrom
mv/validate-field-to-int-cast

Conversation

@vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Jun 4, 2025

Description

Problem*

Resolves #8087

Builds upon #8798 as I was originally worried about merge conflicts for building off of #8765.

Summary*

This simply adds validation for Field -> integer casting. Casting involving signed integer requires a much more involved validation check.

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation 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.

@vezenovm vezenovm changed the title validate field to integer cast fix(ssa): Validate field to integer cast Jun 4, 2025
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 'ACVM Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: ec007e6 Previous: 9d44ab7 Ratio
perfectly_parallel_batch_inversion_opcodes 3573493 ns/iter (± 1553) 2778522 ns/iter (± 7503) 1.29

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

CC: @TomAFrench

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 Memory'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 1625b68 Previous: 6f1b46f Ratio
private-kernel-inner 292.98 MB 202.09 MB 1.45

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

CC: @TomAFrench

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: c8a7b93 Previous: 9580a12 Ratio
rollup-merge 0.004 s 0.003 s 1.33

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

CC: @TomAFrench

@vezenovm vezenovm marked this pull request as draft June 5, 2025 02:12
Base automatically changed from mv/validate-signed-mul-overflow to mv/validation-module June 5, 2025 19:20
Base automatically changed from mv/validation-module to master June 9, 2025 15:10
@vezenovm vezenovm force-pushed the mv/validate-field-to-int-cast branch from 60a5bff to e94a44f Compare June 9, 2025 16:09
@vezenovm vezenovm added the bench-show Display benchmark results on PR label Jun 10, 2025
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: c8a7b93 Previous: 9580a12 Ratio
test_report_zkpassport_noir_rsa_ 2 s 1 s 2

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

CC: @TomAFrench

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.

ACVM Benchmarks

Details
Benchmark suite Current: bd45216 Previous: 12bf0db Ratio
purely_sequential_opcodes 247842 ns/iter (± 520) 254138 ns/iter (± 1529) 0.98
perfectly_parallel_opcodes 221797 ns/iter (± 2635) 225984 ns/iter (± 1376) 0.98
perfectly_parallel_batch_inversion_opcodes 2784994 ns/iter (± 2173) 2791262 ns/iter (± 10832) 1.00

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

@vezenovm vezenovm requested a review from guipublic June 10, 2025 16:16
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.

Execution Time

Details
Benchmark suite Current: bd45216 Previous: 12bf0db Ratio
private-kernel-inner 0.027 s 0.027 s 1
private-kernel-reset 0.158 s 0.155 s 1.02
private-kernel-tail 0.011 s 0.011 s 1
rollup-base-private 0.29 s 0.293 s 0.99
rollup-base-public 0.186 s 0.191 s 0.97
rollup-block-root 10.8 s 10.7 s 1.01
rollup-merge 0.004 s 0.004 s 1
rollup-root 0.009 s 0.009 s 1
semaphore-depth-10 0.02 s 0.02 s 1

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

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.

Artifact Size

Details
Benchmark suite Current: bd45216 Previous: 12bf0db Ratio
private-kernel-inner 1134.4 KB 1134.4 KB 1
private-kernel-reset 2071.4 KB 2071.4 KB 1
private-kernel-tail 588.5 KB 588.5 KB 1
rollup-base-private 4944.6 KB 4944.6 KB 1
rollup-base-public 3986.2 KB 3986.2 KB 1
rollup-block-root-empty 240.3 KB 240.3 KB 1
rollup-block-root-single-tx 25691.2 KB 25691.2 KB 1
rollup-block-root 25717.7 KB 25717.7 KB 1
rollup-merge 184.7 KB 184.7 KB 1
rollup-root 420.8 KB 420.8 KB 1
semaphore-depth-10 636.4 KB 636.4 KB 1

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

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.

Compilation Time

Details
Benchmark suite Current: bd45216 Previous: 12bf0db Ratio
private-kernel-inner 2.386 s 2.354 s 1.01
private-kernel-reset 7.364 s 7.536 s 0.98
private-kernel-tail 1.09 s 1.108 s 0.98
rollup-base-private 15.52 s 16.14 s 0.96
rollup-base-public 12.98 s 13.28 s 0.98
rollup-block-root-empty 1.194 s 1.332 s 0.90
rollup-block-root-single-tx 124 s 127 s 0.98
rollup-block-root 131 s 127 s 1.03
rollup-merge 1.104 s 1.094 s 1.01
rollup-root 1.652 s 1.65 s 1.00
semaphore-depth-10 0.794 s 0.755 s 1.05

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

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.

Test Suite Duration

Details
Benchmark suite Current: bd45216 Previous: 12bf0db Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr 68 s 70 s 0.97
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts 109 s 110 s 0.99
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob 43 s 42 s 1.02
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib 210 s 203 s 1.03
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib 263 s 272 s 0.97
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types 70 s 75 s 0.93
test_report_noir-lang_noir-bignum_ 421 s 372 s 1.13
test_report_noir-lang_noir_bigcurve_ 244 s 219 s 1.11
test_report_noir-lang_sha512_ 31 s 28 s 1.11
test_report_zkpassport_noir_rsa_ 1 s 1 s 1

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

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.

Compilation Memory

Details
Benchmark suite Current: bd45216 Previous: 12bf0db Ratio
private-kernel-inner 292.98 MB 292.94 MB 1.00
private-kernel-reset 534.02 MB 534.02 MB 1
private-kernel-tail 191.54 MB 191.56 MB 1.00
rollup-base-private 1380 MB 1380 MB 1
rollup-base-public 1510 MB 1510 MB 1
rollup-block-root-empty 339.39 MB 339.39 MB 1
rollup-block-root-single-tx 7830 MB 7830 MB 1
rollup-block-root 7830 MB 7830 MB 1
rollup-merge 327.16 MB 327.17 MB 1.00
rollup-root 381.46 MB 381.44 MB 1.00
semaphore_depth_10 106.39 MB 106.39 MB 1

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

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.

Execution Memory

Details
Benchmark suite Current: bd45216 Previous: 12bf0db Ratio
private-kernel-inner 202.09 MB 202.09 MB 1
private-kernel-reset 225.81 MB 225.81 MB 1
private-kernel-tail 177.35 MB 177.35 MB 1
rollup-base-private 489.89 MB 489.89 MB 1
rollup-base-public 423.19 MB 423.19 MB 1
rollup-block-root 1400 MB 1400 MB 1
rollup-merge 312.02 MB 312.02 MB 1
rollup-root 317.7 MB 317.7 MB 1
semaphore_depth_10 70.96 MB 70.96 MB 1

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

Copy link
Contributor

@guipublic guipublic left a comment

Choose a reason for hiding this comment

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

I think the added truncates in remove_bit_shifts can be avoided.

@vezenovm vezenovm marked this pull request as ready for review June 10, 2025 20:12
@vezenovm vezenovm marked this pull request as draft June 10, 2025 20:12
@vezenovm
Copy link
Contributor Author

I think the added truncates in remove_bit_shifts can be avoided.

I ultimately removed all of them @guipublic.

I have moved to only running validation after initial SSA gen for the time being. It used to live in FunctionBuilder::finish which was also used by the inlining passes. Making validation more generally aware of value ranges is overly complex for this PR and should be done as part of an entirely separate value range analysis work. The validator can then reference the map constructed by a separate value range analysis to determine whether a Cast is safe. As the validator is currently written it can only be run on the initial SSA (which in itself required adding the truncate simplification invariants).

@vezenovm vezenovm marked this pull request as ready for review June 11, 2025 16:11
@guipublic
Copy link
Contributor

Could you also fix the wrong 'cast' in remove_bit_shits.rs?
Line 133, remove the casting: let pow = self.insert_cast(pow, typ);, and return a pow of type 'Field' instead, and implement the casting after:
if max_bit <= bit_size then do the casting here, we know it is safe
'else': the result is already 'cast' after the truncate, so there is nothing to do. (just remove the casting of 'pow' to Field, as it is already a Field now).

…ve_bit_shifts, and add field to integer cast tests in validation pass
@vezenovm
Copy link
Contributor Author

(just remove the casting of 'pow' to Field, as it is already a Field now).

I also ended up casting the overflow predicate to a Field so that the predicate * pow multiplication had matching types.

@vezenovm vezenovm requested a review from guipublic June 11, 2025 16:53
Copy link
Contributor

@guipublic guipublic left a comment

Choose a reason for hiding this comment

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

LGTM

@vezenovm vezenovm enabled auto-merge June 11, 2025 17:11
@vezenovm vezenovm added this pull request to the merge queue Jun 11, 2025
Merged via the queue into master with commit 229d57b Jun 11, 2025
117 checks passed
@vezenovm vezenovm deleted the mv/validate-field-to-int-cast branch June 11, 2025 17:30
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jun 12, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
chore: interpret all execution_success programs
(noir-lang/noir#8887)
fix: always error if integer literal doesn't fit its type on the fron…
(noir-lang/noir#8885)
fix: Count array copies for slice functions
(noir-lang/noir#8867)
chore(ssa): Simplify truncate when the bit size we are truncating to is
greater than the value's max bit size
(noir-lang/noir#8875)
fix(ssa): Validate field to integer cast
(noir-lang/noir#8799)
feat: SSA interpreter checks return value
(noir-lang/noir#8883)
chore: add return value to every execution_success program that produces
an output (noir-lang/noir#8882)
chore(fuzz): Run fuzzing deterministically on CI
(noir-lang/noir#8868)
fix: (SSA interpreter) check requires_acir_gen_predicate for
enable_side_effects (noir-lang/noir#8869)
feat: allow printing each SSA interpreter definition
(noir-lang/noir#8865)
fix: handle unconditional break during SSA codegen in all cases
(noir-lang/noir#8855)
fix: update external lib commit tdd.nr
(noir-lang/noir#8823)
chore: bump dependencies (noir-lang/noir#8838)
chore(fuzz): Enable SSA Interpreter fuzzing on all passes
(noir-lang/noir#8610)
chore: Prune changelog older than ~1 year (<0.32.0)
(noir-lang/noir#8856)
chore(docs): Add links to awesome-noir in sidebar
(noir-lang/noir#8854)
chore: add a regression test for #8727
(noir-lang/noir#8851)
chore(fuzz): Handle overflows in comptime fuzzing
(noir-lang/noir#8847)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jun 12, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
chore: interpret all execution_success programs
(noir-lang/noir#8887)
fix: always error if integer literal doesn't fit its type on the fron…
(noir-lang/noir#8885)
fix: Count array copies for slice functions
(noir-lang/noir#8867)
chore(ssa): Simplify truncate when the bit size we are truncating to is
greater than the value's max bit size
(noir-lang/noir#8875)
fix(ssa): Validate field to integer cast
(noir-lang/noir#8799)
feat: SSA interpreter checks return value
(noir-lang/noir#8883)
chore: add return value to every execution_success program that produces
an output (noir-lang/noir#8882)
chore(fuzz): Run fuzzing deterministically on CI
(noir-lang/noir#8868)
fix: (SSA interpreter) check requires_acir_gen_predicate for
enable_side_effects (noir-lang/noir#8869)
feat: allow printing each SSA interpreter definition
(noir-lang/noir#8865)
fix: handle unconditional break during SSA codegen in all cases
(noir-lang/noir#8855)
fix: update external lib commit tdd.nr
(noir-lang/noir#8823)
chore: bump dependencies (noir-lang/noir#8838)
chore(fuzz): Enable SSA Interpreter fuzzing on all passes
(noir-lang/noir#8610)
chore: Prune changelog older than ~1 year (<0.32.0)
(noir-lang/noir#8856)
chore(docs): Add links to awesome-noir in sidebar
(noir-lang/noir#8854)
chore: add a regression test for #8727
(noir-lang/noir#8851)
chore(fuzz): Handle overflows in comptime fuzzing
(noir-lang/noir#8847)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
danielntmd pushed a commit to danielntmd/aztec-packages that referenced this pull request Jul 16, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
chore: interpret all execution_success programs
(noir-lang/noir#8887)
fix: always error if integer literal doesn't fit its type on the fron…
(noir-lang/noir#8885)
fix: Count array copies for slice functions
(noir-lang/noir#8867)
chore(ssa): Simplify truncate when the bit size we are truncating to is
greater than the value's max bit size
(noir-lang/noir#8875)
fix(ssa): Validate field to integer cast
(noir-lang/noir#8799)
feat: SSA interpreter checks return value
(noir-lang/noir#8883)
chore: add return value to every execution_success program that produces
an output (noir-lang/noir#8882)
chore(fuzz): Run fuzzing deterministically on CI
(noir-lang/noir#8868)
fix: (SSA interpreter) check requires_acir_gen_predicate for
enable_side_effects (noir-lang/noir#8869)
feat: allow printing each SSA interpreter definition
(noir-lang/noir#8865)
fix: handle unconditional break during SSA codegen in all cases
(noir-lang/noir#8855)
fix: update external lib commit tdd.nr
(noir-lang/noir#8823)
chore: bump dependencies (noir-lang/noir#8838)
chore(fuzz): Enable SSA Interpreter fuzzing on all passes
(noir-lang/noir#8610)
chore: Prune changelog older than ~1 year (<0.32.0)
(noir-lang/noir#8856)
chore(docs): Add links to awesome-noir in sidebar
(noir-lang/noir#8854)
chore: add a regression test for AztecProtocol#8727
(noir-lang/noir#8851)
chore(fuzz): Handle overflows in comptime fuzzing
(noir-lang/noir#8847)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bench-show Display benchmark results on PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Casting without truncation causes differences between ACIR and Brillig

2 participants