chore: graph tooling audit 0 #19691
chore: graph tooling audit 0 #19691
Conversation
Introduces a pattern-based approach for extracting gate variables in the
static analyzer. Key changes:
- Add gate_patterns.hpp with declarative patterns for all gate types:
ARITHMETIC, ELLIPTIC, DELTA_RANGE, LOOKUP, POSEIDON2_INTERNAL/EXTERNAL,
NON_NATIVE_FIELD, MEMORY, DATABUS, and ECC_OP
- Add extract_gate_variables() template method that uses patterns to
extract wire indices based on selector conditions
- Convert most gate extraction methods to use pattern-based extraction:
- ARITHMETIC: Uses pattern with special handling for fix_witness gates
- ELLIPTIC, DELTA_RANGE, LOOKUP, POSEIDON2, NON_NATIVE_FIELD, DATABUS:
Fully converted to pattern-based extraction
- MEMORY: Uses pattern for non-access gates; access gates still handled
by transcript-based methods due to tau-tag constraints
- Add perturbation tests (gate_patterns.test.cpp) that verify patterns
match actual relation constraints by checking that perturbing constrained
wires changes relation output
All 82 existing tests pass. The pattern-based approach provides:
- Automated verification via perturbation testing
- Self-documenting constraint specifications
- Easier maintenance when relations change
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| fixed_variables.insert(this->to_real(left_idx)); | ||
| } else if (!q_m.is_zero() || q_1 != FF::one() || !q_2.is_zero() || !q_3.is_zero() || !q_4.is_zero()) { | ||
| // this is not the gate for fix_witness, so we have to process this gate | ||
| if (!q_m.is_zero()) { |
There was a problem hiding this comment.
Here we unconditionally add w_r to the set of constrained wires, but since the mul term is disabled for q_m == 3, we technically need to check whether q_2.is_zero()
| // ram constitency check | ||
| if (!q_3.is_zero()) { | ||
| if (index < block.size() - 1) { | ||
| gate_variables.insert(gate_variables.end(), |
There was a problem hiding this comment.
This should technically contain w_l and w_r as well but maybe this was intentional due to those being handled by the record based methods below.
| std::vector<uint32_t> second_row_variables; | ||
| auto w1 = blk.w_l()[index]; // get opcode of operation, because function get_ecc_op_idx returns type | ||
| // uint32_t and it adds as w1 | ||
| if (w1 != circuit_builder.zero_idx()) { |
There was a problem hiding this comment.
This code was previously executed on all block so this if condition captures almost all gates. Later we filter on gates with wires matching circuit_builder.equality_op_idx (which in most cases only appear in the ecc_op block but could appear anytime the constant value 3 is used). E.g. this value always appears in the arithmetic block from the gate that fixes the constant to begin with. Then, since this code also connects the gate at the next row (intended to account for the two-row ecc_op block structure), it erroneously marks as constrained whatever gate comes after the arithmetic gate that fixes the equality_op_idx. This was hiding some witnesses appearing in one gate (see EXPECTED_UNCONSTRAINED_INFINITY_FLAGS in test suite).
| gate_variables.emplace_back(vc2_witness); | ||
| } | ||
| gate_variables.emplace_back(record_witness); | ||
| auto& memory_block = circuit_builder.blocks.memory; |
There was a problem hiding this comment.
misleadingly large diff here. I'm just executing this on the memory block explicitly which removes the outer conditional scope
| gate_variables.emplace_back(value_witness); | ||
| } | ||
| gate_variables.emplace_back(record_witness); | ||
| auto& memory_block = circuit_builder.blocks.memory; |
DanielKotov
left a comment
DanielKotov
left a comment
There was a problem hiding this comment.
The tool is more readable right now. I like it!
Primary changes: - Replace methods of the form `get_*_gate_connected_component` with `GatePattern` structs that specify the conditions under which each wire is constrained for each gate type - Test correctness of `GatePattern`'s by perturbing relation inputs to empirically check which wires are constrained (`gate_patterns.test.cpp`) - Resolves a few bugs/errors identified by the aforementioned tests (see PR comments) - Use `update_used_witnesses` in `fix_witness` to avoid need for ad-hoc handling in the tooling Cleanup: - Replace use of `block_idx` with reference to `block` in several places for improved clarity --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
BEGIN_COMMIT_OVERRIDE cherchore: graph tooling audit 0 (#19691) chore: opt sol scope renaming (#20123) feat: add download functionality to barretenberg-rs build.rs (#20105) fix: set BB_LIB_DIR in build function (ffi feature is on by default) Revert barretenberg-rs download functionality feat: re-land barretenberg-rs download functionality with fixes (#20128) fix: skip FFI feature in PipeBackend tests fix: skip FFI feature in PipeBackend tests END_COMMIT_OVERRIDE
Primary changes:
get_*_gate_connected_componentwithGatePatternstructs that specify the conditions under which each wire is constrained for each gate typeGatePattern's by perturbing relation inputs to empirically check which wires are constrained (gate_patterns.test.cpp)update_used_witnessesinfix_witnessto avoid need for ad-hoc handling in the toolingCleanup:
block_idxwith reference toblockin several places for improved clarity