Merged
Conversation
Changes: - Updated code comments in prover_instance.hpp and verifier_instance.hpp to use generic folding terminology and reference Hypernova - Changed Noir circuit comments from "protogalaxy proof" to "folding proof" - Updated documentation in chonk.md to reference Hypernova paper (eprint.iacr.org/2023/573) instead of ProtoGalaxy - Added explicit BB_BENCH_NAME() calls to hypernova_prover.cpp and hypernova_verifier.cpp for better benchmarking - Updated bench_hardware_concurrency.sh to process HypernovaProver/Verifier metrics - Updated acir_format.cpp comments to use "HN" instead of "pg" - Added "hypernova" to cspell.json dictionary Note: Versioned documentation, CHANGELOGs, and security descriptions kept as-is for historical accuracy. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Please read [contributing guidelines](CONTRIBUTING.md) and remove this line. For audit-related pull requests, please use the [audit PR template](?expand=1&template=audit.md). --------- Co-authored-by: Claude <noreply@anthropic.com>
Collaborator
Author
Flakey Tests🤖 says: This CI run detected 1 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
## Summary Replace the boolean `--update_inputs` flag with a new `--vk_policy=rewrite` option to provide more flexibility in handling verification keys. ## Changes - Add `REWRITE` policy to `VkPolicy` enum - Update CLI to accept 'rewrite' as a valid vk_policy value - Remove `update_inputs` field from `API::Flags` - Update `check_precomputed_vks` to use `vk_policy` instead of `update_inputs` - Update tests and scripts to use `--vk_policy=rewrite` ## Behavior The new `--vk_policy=rewrite` provides the same functionality as the old `--update_inputs` flag: when a VK check fails, it rewrites the input file with the correct VK. This change provides better consistency with the existing vk_policy options (default, check, recompute). ## Testing - Unit tests pass (`CheckPrecomputedVks` and `CheckPrecomputedVksMismatch`) - Verified CLI accepts `--vk_policy=rewrite` and rejects old `--update_inputs` flag - Scripts updated to use new flag ## Migration Guide Replace `--update_inputs` with `--vk_policy=rewrite`: **Before:** ```bash bb check --update_inputs --scheme chonk --ivc_inputs_path inputs.msgpack ``` **After:** ```bash bb check --vk_policy=rewrite --scheme chonk --ivc_inputs_path inputs.msgpack ``` Co-authored-by: Claude <noreply@anthropic.com>
## Critical Changes ### Issues 1 & 2: Prevent Unconstrained Value Propagation **Root Cause**: Constructors allowed creation of byte_arrays with unconstrained field_t values, enabling malicious provers to inject invalid "bytes" (>255) into hash function lookup tables. **Fixes**: - **1** (6585762): Removed `byte_array(Builder*, size_t)` constructor entirely - **2** (de5b87e): Made `byte_array(Builder*, bytes_t const&)` and `byte_array(Builder*, bytes_t&&)` constructors **private** **Impact**: Establishes security boundary where: - External code must use public constructors that add 8-bit range constraints - Internal methods (slice, reverse) safely use private constructors only on already-constrained data - Unconstrained values cannot propagate to hash functions **Cascading Benefits**: Issues 5 and 8 automatically prevented at source. --- ## 📝 byte_array API Security & Correctness ### Issue 3: Allow Valid 32-Byte ↔ field_t Round-Trips - **Commit**: f899a23 - **Fix**: Removed overly restrictive `BB_ASSERT(bytes < 32)` from `operator field_t()` - **Result**: Enables testing of critical modulus boundary cases ### Issue 4: Proper Bounds Checking & Prevent Mutation - **Commits**: bf58e85, e897223 - **Fix**: Changed const `operator[]` from `assert(size > 0)` to `BB_ASSERT_LT(index, size)`; removed non-const `operator[]` - **Result**: Proper bounds validation; no direct mutation possible ### Issue 5: write_at() Cannot Accept Unconstrained Arrays - **Commit**: de5b87e (architectural fix) - **Fix**: With private bytes_t constructors, only constrained byte_arrays can exist - **Result**: write_at() validated at construction time, not call time ### Issue 6: Handle Empty Array Edge Case in slice() - **Commit**: 5f916a6 - **Fix**: Changed `BB_ASSERT_LT(offset, size)` to `BB_ASSERT_LTE(offset, size)` - **Result**: Allows valid `slice(0)` on empty arrays ### Issue 7: Document Division Safety Invariant - **Commit**: 8d9f4e1 - **Fix**: Added SAFETY comment explaining `reconstructed_hi / 2^128` divisibility invariant - **Result**: Critical assumption now documented for maintainers and auditors ### Issue 8: slice() Cannot Propagate Unconstrained Fields - **Commit**: de5b87e (architectural fix) - **Fix**: slice() uses private constructor only on already-constrained internal data - **Result**: No external bypass possible; constraints always preserved ### Issue 9: No Checks/Documentation About field_t Conversion Not Adding Constraints - **Commits**: de5b87e + 6585762 (architectural fixes) - **Fix**: The operator field_t() conversion doesn't need to check constraints because the type system now guarantees all byte_arrays contain constrained values: - **Result**: The conversion's "trust model" is valid - it's not trusting user input, it's relying on a type-level invariant enforced at construction ### Issue 10: Remove Undefined Behavior in get_string() - **Commit**: 2cf61e6 - **Fix**: Removed `get_string()` method entirely - **Result**: No signed char UB with values 128-255 --- ## 🔧 bool_t Security & Correctness ### Issue 11: Validate Builder Context in Operators - **Commit**: a9f6608 - **Fix**: Use `validate_context<Builder>()` instead of manual `context ? context : other.context` in &, |, ^, == operators - **Result**: Ensures both operands belong to same builder ### Issue 12: Use Canonical Zero Witness - **Commit**: 1fb4b6e - **Fix**: Use `context->zero_idx()` instead of same witness with q_r=0 in normalize() - **Result**: Cleaner gate construction following best practices --- ## 🚀 Performance & Correctness ### Issue 13: Honor Move Semantics in Rvalue Constructor - **Commit**: eaa34b7 - **Fix**: Changed to `values(std::move(input))` in rvalue constructor - **Result**: Avoids unnecessary copies for large arrays ### Issue 14: Prevent Underflow in reverse() on Empty Array - **Commit**: 6cf505d - **Fix**: Added early return: `if (values.empty()) { return *this; }` - **Result**: Prevents underflow from `size() - 1` computation ### Issue 15: Fix Selector Order in Boolean Equality Gate - **Commit**: cf791df - **Fix**: Swapped q_l and q_r to match canonical (q_m, q_l, q_r, q_o, q_c) order - **Result**: Conforms to documented layout; prevents future bugs if coefficients diverge ### `bool_t`: Always normalize `conditional_assign` Output - **Commit**: [d0032b2](d0032b2) - **Fix**: Fixed concept leakage in `bool_t::conditional_assign` that could return un-normalized `bool_t` and cause in APIs at higher level - **Note**: Minimal gate overhead - only 0-1 gates added when values are actually inverted. ## Summary The key insight: Making internal `bytes_t` constructors private creates a security boundary that prevents unconstrained field_t values from ever entering byte_arrays. Combined with removing unsafe constructors and the non-const operator[], this ensures that only properly range-constrained bytes can reach hash function lookup tables throughout the circuit stack. --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: federicobarbacovi <171914500+federicobarbacovi@users.noreply.github.com>
Update the naming in `bb_proof_verification` lib so that we guide users towards ZK by default (`verify_honk_proof`) and non-ZK is more opt-in (`verify_honk_proof_non_zk`)
Minor safety/docs updates to `cycle_scalar`, no logic changes - Add "unsafe" syntax to `validate_split_in_field_unsafe` to highlight that it only does what it claims when range constraints are applied on the lo/hi limbs (as is done when a cycle_scalar is used in batch_mul) - Make limbs `_lo`, `_hi` private members - Remove `skip_validation` bool from constructor; public constructor just always does it, option to not is private/internal use only --------- Co-authored-by: Suyash Bagad <suyash@aztecprotocol.com>
Its been noted in a few locations that use of field_t::conditional_assign with a non-constant predicate on the individual coordinates of a cycle_group element prior to construction can result in mixed constancy. E.g. if trying to conditionally assign between two points whose coordinates are constant but agree in only one or the other coordinate. In this case the coordinate that agrees will produce a constant, while the two that disagree will produce a witness. To avoid the need to handle this in disparate locations, we opt to handle it once and for all at the single interface for cycle_group. In particular, if we encounter mixed constancy, we convert the constant coordinate to a fixed witness. Closes #17514
Collaborator
Author
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BEGIN_COMMIT_OVERRIDE
chore: remove more pg references and leftovers (#18260)
refactor: Migrate --update_inputs flag to --vk_policy option (#18237)
chore: stdlib byte_array and bool external audit fixes (#17838)
chore: update naming in bb_proof_verification lib (#18269)
chore: cycle group 13 (#18200)
chore: universal handling for coord constancy in cycle_group (#18253)
END_COMMIT_OVERRIDE