-
Notifications
You must be signed in to change notification settings - Fork 585
feat: merge-train/barretenberg #19887
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
## Summary Adds support for reading JSON-formatted proof, public inputs, and VK files in the `bb verify` command. This complements the existing `--output_format json` feature by allowing the JSON files to be consumed for verification. This enables users who receive JSON proof artifacts from third parties to verify the proof before using it in their circuits (e.g., for recursive verification). ### Format Detection The format is **auto-detected by attempting to parse as JSON**. If parsing succeeds, the file is treated as JSON; otherwise it falls back to binary format. This is robust and works regardless of file extension. ### Usage ```bash # Verify using JSON files bb verify --scheme ultra_honk -p proof.json -i public_inputs.json -k vk.json # Works even without .json extension (content-based detection) bb verify --scheme ultra_honk -p my_proof -i my_inputs -k my_vk # Mix and match: JSON proof with binary VK bb verify --scheme ultra_honk -p proof.json -i public_inputs.json -k vk ``` ## Test plan - [x] Build passes - [x] Tested verify with JSON files - [x] Tested verify with binary files (no false positive JSON detection) - [x] Tested verify with mixed JSON and binary files - [x] acir_tests bb_prove tests pass
…es must always be set in tests (#19884) Update `index.js` so that `HAS_ZK` and `PUBLIC_INPUTS` variables must always be set in tests. These code paths were leftovers from Plonk. Closes AztecProtocol/barretenberg#1316
clean up + docs+ a couple of edge case tests Closes AztecProtocol/barretenberg#486 --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Montgomery batch inversion trick was implemented in 3 different places, now all of them are using generic impl that has a policy abstraction allowing to support different memory requirements (e.g. pippenger vs scaling a vector of commitments by the same field element) Closes AztecProtocol/barretenberg#826 --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
### Summary This PR addresses findings from the external audit of the Transcript and Poseidon2 components. The changes ensure consistent behavior between native and in-circuit verification, particularly around field element serialization/deserialization. #### 1. Field Element Serialization/Deserialization (Issues 5, 6, 9, 10, 11) Problem: Inconsistent handling of field elements during serialization, particularly around edge cases involving the field modulus and aliased values. - `validate_split_in_field_unsafe` now properly rejects `hi||lo == field_modulus` (Issue 5) - Replaced hardcoded $254$ with generically calculated `max_bits` (Issue 6) - Added proper borrow boolean check even when `lo` is constant (Issue 10) - Unified deserialization behavior: both `FrCodec` (native) and `StdlibCodec` (in-circuit) now use `assert_is_in_field()` to reject unreduced values (Issues 9 & 11) - For `Mega` arithmetization, validation is deferred to `Translator+ECCVM`; for `Ultra`, strict validation happens immediately via `bigfield` #### 2. Transcript Round Tracking (Issues 3, 4) - Removed redundant `round_number` field, now using only `round_index` (Issue 4) - Consecutive challenges with no data between them now stay in the same round - `prover_init_empty` and `verifier_init_empty` are now properly marked as testing methods (Issue 3) #### 3. Poseidon2 Cleanup (Issues 1, 2, 7, 8) Strategy: - Not marking the first element of the sponge state as `used`. - Added documentation for $-1$ offset in `internal_matrix_diagonal` (Issue 2) - Removed unnecessary normalization assert in `matrix_multiplication_external` (Issue 7) - Fixed transcript initialization in circuit failure tests (Issue 8) --- #### Commits: - Issue 1 (68011cc): First element of of `FieldSponge::hash_internal` is **not** marked as used - Issue 2 (2dd4197): Missing annotation about $-1$ offset in poseidon2 `internal_matrix_diagonal` - Issue 3 (1dddf54): `prover_init_empty` and `verifier_init_empty` are testing methods - Issue 4 (ced1a88): `round_number` is redundant with `round_index` - Issues 5 & 6 (e4fa3c5): `validate_split_in_field_unsafe` accepts `hi||lo == modulus` + hardcoded $254$ - Issue 7 (dd1f644): Unnecessary assert for matrix_multiplication_external output normalization - Issue 8 (d064598): Verification in poseidon2.circuit.failure.test.cpp fails on valid input - Issues 9 & 11 (7f1cb65): `deserialize_from_fields` rejects unreduced `fq`s + (fq_modulus, fq_modulus) handling - Issue 10 (98178c5): `validate_split_in_field_unsafe` skips borrow check when `lo` is constant --- #### Documentation Added barretenberg/cpp/src/barretenberg/stdlib/primitives/field/CODEC_README.md documenting: - Codec architecture (FrCodec, StdlibCodec, U256Codec) - Field element encoding (2-limb representation for fq) - Canonical representation requirements and point at infinity handling - Ultra vs Mega arithmetization differences - Threat model: why native/recursive consistency matters - Mega/Goblin deferred validation flow and ECCVM↔Translator translation check --- #### New Tests field_utils.test.cpp (new file): - ValidateSplitRejectsModulus — Issue 5: split rejects hi||lo == modulus - ValidateSplitAcceptsModulusMinusOne — edge case: modulus - 1 is valid - SplitUniqueRejectsModulus — Issue 5: unique split rejects modulus - SplitUniqueMaxValue — edge case handling - ValidateSplitConstantLoWitnessHiRejectsModulus — Issue 10: constant lo with witness hi - ValidateSplitWitnessLoConstantHiRejectsModulus — Issue 10: witness lo with constant hi field_conversion.test.cpp: - BigfieldDeserializationFailsOnLimbOverflow — limb bounds validation - BothCodecsRejectPointAtInfinityAlias — Issue 11: aliased infinity rejected - BothCodecsAcceptCanonicalRejectAlias — Issues 9, 11: native/circuit consistency - AcceptCanonicalPointAtInfinity — canonical infinity accepted - RejectPointNotOnCurve — off-curve points rejected poseidon2.test.cpp: - PointCoordinatesVsAliasProduceDifferentHashes — aliased coords produce different hashes poseidon2.circuit.failure.test.cpp: - ValidCircuitVerifies — Issue 8: baseline validity check
Changes include: - Add explicit range constraints so that all inputs are constrained within bb (possibly redundant with noir-applied constraints) - Add explicit range constraints so that intermediate witnesses are constrained to be unique where possible - Get rid of `apply_32_bit_range_constraint_via_lookup` in favor of explicit range constraints since the former is only cheaper if the latter is not already in use (which in practice won't occur)
ludamad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 Auto-approved
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
1 similar comment
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
This reverts commit 8b88095.
Flakey Tests🤖 says: This CI run detected 1 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
BEGIN_COMMIT_OVERRIDE
feat: support JSON input files for bb verify command (#19800)
fix: update bootstrap.sh to use new JSON field names
chore: Update
index.jsso thatHAS_ZKandPUBLIC_INPUTSvariables must always be set in tests (#19884)chore: pippenger int audit (#19302)
chore: deduplicate batch affine addition trick (#19788)
chore: transcript+codec+poseidon2 fixes (#19419)
chore!: explicitly constrain inputs and intermediate witnesses (#19826)
fix: exclude nlohmann/json from WASM builds in json_output.hpp
chore: translator circuit builder and flavor audit (#19798)
Revert "fix: exclude nlohmann/json from WASM builds in json_output.hpp"
Revert "feat: support JSON input files for bb verify command (#19800)"
Revert "fix: update bootstrap.sh to use new JSON field names"
END_COMMIT_OVERRIDE