chore: replace asserts with runtime errors. by johnathan79717 · Pull Request #15671 · AztecProtocol/aztec-packages

johnathan79717 · 2025-07-11T11:06:21Z

Asserts now throw runtime errors in non-debug builds.
BB_ASSERT_* now throw_or_abort in non-debug builds.
Replace ASSERT in tests with ASSERT_TRUE or other gtest functions.
Replace EXPECT_DEATH and ASSERT_DEATH in tests with EXPECT_THROW_OR_ABORT or ASSERT_THROW_OR_ABORT.
Remove #ifdef NDEBUG around the above in tests since they work in non-debug builds now.
Asserts in constexprs uses info instead of sstream.

johnathan79717 · 2025-07-11T11:14:06Z

Once the change is ready, we should also benchmark it to make sure that its effect on performance is negligible.

johnathan79717 · 2025-07-16T14:18:07Z

        , current_max(current_max)
    {
-        ASSERT(safety == IS_UNSAFE);
-        if (current_max > MAX_VALUE) // For optimal efficiency this should only be checked while testing a circuit


Not sure what it means by "testing a circuit". Looks like it's already checked in release builds anyway?

johnathan79717 · 2025-07-16T15:30:54Z

-        BB_ASSERT_EQ(challenge_poly.evaluate(opening_pair.challenge), opening_pair.evaluation, "Opening claim does not hold for challenge polynomial.");
+        BB_ASSERT_EQ_DEBUG_ONLY(challenge_poly.evaluate(opening_pair.challenge), opening_pair.evaluation, "Opening claim does not hold for challenge polynomial.");

        IPA<NativeCurve, log_poly_length>::compute_opening_proof(ck, { challenge_poly, opening_pair }, prover_transcript);
-        BB_ASSERT_EQ(challenge_poly.evaluate(fq(output_claim.opening_pair.challenge.get_value())), fq(output_claim.opening_pair.evaluation.get_value()), "Opening claim does not hold for challenge polynomial.");
+        BB_ASSERT_EQ_DEBUG_ONLY(challenge_poly.evaluate(fq(output_claim.opening_pair.challenge.get_value())), fq(output_claim.opening_pair.evaluation.get_value()), "Opening claim does not hold for challenge polynomial.");


Are these evaluations potentially heavy?

the debugging gains might outstrip how long it takes - tbh we can probably just add TODOs around anything we really think is heavy, I don't think I saw anything egregious stand out

Sign in to view

+            current_max, MAX_VALUE, "exceeded modulus in safe_uint class"); // For optimal efficiency this should only
+                                                                            // be checked while testing a circuit


johnathan79717 · 2025-07-17T15:25:55Z

Performance on ClientIVCBench/Full/6.

Merge Train

--------------------------------------------------------------------------------
Benchmark                      Time             CPU   Iterations UserCounters...
--------------------------------------------------------------------------------
ClientIVCBench/Full/6      27302 ms        23096 ms            1

Branch

--------------------------------------------------------------------------------
Benchmark                      Time             CPU   Iterations UserCounters...
--------------------------------------------------------------------------------
ClientIVCBench/Full/6      27651 ms        23527 ms            1

ludamad · 2025-07-17T19:35:48Z

LGTM, thanks for doing this. I think I still would prefer just calling it ASSERT as it was before and having an ASSERT_DEBUG. for slow asserts. it can be in a follow-on if desired

up to nullifier_tree.cpp up to bitvector.hpp constexpr noexcept up to evaluation_domain.cpp up to part of bigfield_impl.hpp up to part of safe_uint.hpp BB_ASSERT_EQ(poly.evalueate()) remove debug only BB_ASSERT except EQ fix build always include throw_or_abort in assert.hpp fix blake3 test fix tests remove debug checks in tests small fixes Remove unnecessary includes assert in constexpr remove BB_ASSERT_EQ_DEBUG_ONLY

…DEBUG

johnathan79717 · 2025-07-18T10:35:37Z

LGTM, thanks for doing this. I think I still would prefer just calling it ASSERT as it was before and having an ASSERT_DEBUG. for slow asserts. it can be in a follow-on if desired

The original names are fine. I messaged the crypto-dev channel to make sure people are aware of the change.

johnathan79717 · 2025-07-18T10:36:51Z

I meant just calling it ASSERT is fine by me as long as people know that it's run in release. They've been renamed to ASSERT and ASSERT_DEBUG.

See [merge-train-readme.md](https://github.com/AztecProtocol/aztec-packages/blob/next/.github/workflows/merge-train-readme.md). BEGIN_COMMIT_OVERRIDE chore!: Remove circuit_size from all VKs (#15747) chore: Introduce data structures to hold inputs/outputs of the Merge verification (#15735) chore: replace asserts with runtime errors. (#15671) chore: kernels start with eq and reset (#15734) chore: Introduce Native IO mechanism (#15820) fix: Update Cargo.lock in avm-transpiler (#15837) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: ledwards2225 <98505400+ledwards2225@users.noreply.github.com> Co-authored-by: federicobarbacovi <171914500+federicobarbacovi@users.noreply.github.com> Co-authored-by: sergei iakovenko <105737703+iakovenkos@users.noreply.github.com> Co-authored-by: ludamad <adam.domurad@gmail.com> Co-authored-by: Suyash Bagad <suyash@aztecprotocol.com> Co-authored-by: Jonathan Hao <jonathan@aztec-labs.com> Co-authored-by: Raju Krishnamoorthy <krishnamoorthy@gmail.com> Co-authored-by: notnotraju <raju@aztec-labs.com> Co-authored-by: Lucas Xia <lucasxia01@gmail.com> Co-authored-by: Khashayar Barooti <khashayar@aztecprotocol.com> Co-authored-by: Jean M <132435771+jeanmon@users.noreply.github.com> Co-authored-by: Alex Gherghisan <alexghr@users.noreply.github.com> Co-authored-by: Santiago Palladino <spalladino@users.noreply.github.com> Co-authored-by: Santiago Palladino <santiago@aztec-labs.com> Co-authored-by: ludamad <domuradical@gmail.com> Co-authored-by: maramihali <mara@aztecprotocol.com> Co-authored-by: Sarkoxed <75146596+Sarkoxed@users.noreply.github.com>

johnathan79717 · 2025-07-23T12:52:11Z

Adding some more benchmark results. It looks like on native the runtime is ~1% slower and on wasm it's ~5.5% slower.

Benchmark 1: WASM 16 threads

Ran on the benchmark machine via scripts/benchmark_remote.sh client_ivc_bench '/bin/time -v ../scripts/wasmtime.sh client_ivc_bench --benchmark_filter=ClientIVCBench/Full/6' wasm-threads build-wasm-threads

This branch:

Runtime	Peak Memory
83,676 ms	1,742,168 kbs
83,946 ms	1,741,616 kbs
84,322 ms	1,742,404 kbs

Base branch (79ffdbc):

Runtime	Peak Memory
79,096 ms	1,741,356 kbs
79,612 ms	1,741,176 kbs
79,799 ms	1,741,568 kbs

Benchmark 2: Native 192 threads

Ran on the benchmark machine via scripts/benchmark_remote.sh client_ivc_bench './client_ivc_bench --benchmark_filter=ClientIVCBench/Full/6' clang16 build

This branch:

Runtime	Peak Memory
27,276 ms	1665.14 MiB
27,222 ms	1663.86 MiB
27293 ms	1665.48 MiB

Base branch (79ffdbc):

Runtime	Peak Memory
26,996 ms	1664.47 MiB
27,061 ms	1664.34 MiB
27,043 ms	1664.72 MiB

#15671 regressed wasm performance by 5.5%, so this PR disables them in wasm. Ran on the benchmark machine via `scripts/benchmark_remote.sh client_ivc_bench '/bin/time -v ../scripts/wasmtime.sh client_ivc_bench --benchmark_filter=ClientIVCBench/Full/6' wasm-threads build-wasm-threads` This branch: | Runtime | Peak Memory | | --- | --- | | 80,501 ms | 2,868,200 kbs | | 80,092 ms | 2,290,660 kbs | | 79,490 ms | 2,290,560 kbs | Merge train: | Runtime | Peak Memory | | --- | --- | | 85,167 ms | 2,839,172 kbs | | 85,105 ms | 2,291,284 kbs | | 84,510 ms | 2,291,548 kbs |

johnathan79717 requested a review from ludamad July 11, 2025 11:06

johnathan79717 self-assigned this Jul 11, 2025

johnathan79717 commented Jul 11, 2025

View reviewed changes

Comment thread barretenberg/cpp/src/barretenberg/commitment_schemes/shplonk/shplonk.test.cpp Outdated

johnathan79717 commented Jul 14, 2025

View reviewed changes

Comment thread barretenberg/cpp/src/barretenberg/ecc/scalar_multiplication/bitvector.hpp Outdated

johnathan79717 commented Jul 14, 2025

View reviewed changes

Comment thread barretenberg/cpp/src/barretenberg/ecc/fields/field_declarations.hpp Outdated

ludamad reviewed Jul 14, 2025

View reviewed changes

Comment thread barretenberg/cpp/src/barretenberg/benchmark/ipa_bench/ipa.bench.cpp Outdated

ludamad reviewed Jul 14, 2025

View reviewed changes

Comment thread barretenberg/cpp/src/barretenberg/ecc/groups/group_impl_asm.tcc Outdated

Base automatically changed from merge-train/barretenberg to next July 14, 2025 14:40

johnathan79717 changed the base branch from next to merge-train/barretenberg July 16, 2025 12:13

johnathan79717 force-pushed the jh/assert branch from eee30d8 to 40c665a Compare July 16, 2025 12:14

johnathan79717 commented Jul 16, 2025

View reviewed changes

Base automatically changed from merge-train/barretenberg to next July 16, 2025 16:20

johnathan79717 force-pushed the jh/assert branch from 04d3335 to 66b1003 Compare July 16, 2025 16:56

johnathan79717 changed the base branch from next to merge-train/barretenberg July 16, 2025 16:56

johnathan79717 commented Jul 17, 2025

View reviewed changes

Comment thread barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/circuit_builder_base.hpp

johnathan79717 marked this pull request as ready for review July 17, 2025 13:59

johnathan79717 requested review from IlyasRidhuan, fcarreiro and jeanmon as code owners July 17, 2025 13:59

johnathan79717 marked this pull request as draft July 17, 2025 14:11

johnathan79717 marked this pull request as ready for review July 17, 2025 14:18

ludamad approved these changes Jul 17, 2025

View reviewed changes

Base automatically changed from merge-train/barretenberg to next July 17, 2025 20:46

johnathan79717 added 2 commits July 18, 2025 09:49

Rename ASSERT_RELEASE back to ASSERT and ASSERT_DEBUG_ONLY to ASSERT_…

92fe1c3

…DEBUG

johnathan79717 force-pushed the jh/assert branch from 0856a8b to 92fe1c3 Compare July 18, 2025 09:54

johnathan79717 changed the base branch from next to merge-train/barretenberg July 18, 2025 10:27

johnathan79717 merged commit 35a201e into merge-train/barretenberg Jul 18, 2025
5 checks passed

johnathan79717 deleted the jh/assert branch July 18, 2025 10:27

AztecBot mentioned this pull request Jul 18, 2025

feat: merge-train/barretenberg #15809

Merged

johnathan79717 mentioned this pull request Jul 22, 2025

replace ASSERT calls with runtime errors, have ASSERT be only for slow operations AztecProtocol/barretenberg#1460

Closed

johnathan79717 mentioned this pull request Jul 23, 2025

fix: disable asserts in wasm builds #15922

Merged

		current_max, MAX_VALUE, "exceeded modulus in safe_uint class"); // For optimal efficiency this should only
		// be checked while testing a circuit

Conversation

johnathan79717 commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

johnathan79717 commented Jul 11, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

johnathan79717 Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

johnathan79717 Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

ludamad Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

johnathan79717 commented Jul 17, 2025

Merge Train

Branch

Uh oh!

ludamad commented Jul 17, 2025

Uh oh!

Uh oh!

johnathan79717 commented Jul 18, 2025

Uh oh!

johnathan79717 commented Jul 18, 2025

Uh oh!

johnathan79717 commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmark 1: WASM 16 threads

Benchmark 2: Native 192 threads

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

johnathan79717 commented Jul 11, 2025 •

edited

Loading

johnathan79717 commented Jul 23, 2025 •

edited

Loading