chore: resolve field_t issues from external audit#16210
Conversation
| { | ||
| if (other.is_constant()) { | ||
| additive_constant = (other.witness_bool ^ other.witness_inverted) ? bb::fr::one() : bb::fr::zero(); | ||
| additive_constant = other.get_value(); |
There was a problem hiding this comment.
issue 3
use get_value() on a bool_t
| true, | ||
| "Attempting to create a bool_t from a witness_t not satisfying x^2 - x = 0"); | ||
| result.witness_bool = (witness == bb::fr::one()); | ||
| bool_t result(context, witness == bb::fr::one()); |
There was a problem hiding this comment.
issue 4
use an appropriate field_t constructor
| } | ||
|
|
||
| // Variadic version: compare first with the rest | ||
| template <typename T, typename... Ts> T* validate_context(T* first, Ts*... rest) |
There was a problem hiding this comment.
issue 10
the contexts are validated for all basic arithmetic ops
| return first; | ||
| } | ||
|
|
||
| template <typename T, typename Container> T* validate_context(const Container& elements) |
There was a problem hiding this comment.
issue 10
the contexts are validated for all entries of an accumulate input
|
|
||
| // Let q = (a < b) | ||
| // Assume both a and b are < K where K = 2^{num_bits} - 1 | ||
| // Assume both a and b are < K where K = 2^{num_bits} |
|
|
||
| context->create_add_gate({ .a = witness_index, | ||
| .b = witness_index, | ||
| .b = context->zero_idx, |
|
|
||
| // Step 2: compute output value | ||
| size_t num_elements = accumulator.size(); | ||
| // If `input` contains non-constant `field_t` elements, add the accumulated constant value to the output value, |
There was a problem hiding this comment.
issue 9
removed confusing comment
| field_t left = lhs.normalize(); | ||
| field_t right = rhs.normalize(); | ||
| ctx->assert_equal(left.witness_index, right.witness_index, msg); | ||
| if (lhs.is_normalized() || rhs.is_normalized()) { |
There was a problem hiding this comment.
issue 8
although the optimization can be applied in other cases, I think it only makes sense when both arguments are non-normalized witnesses, cause in other cases it would only remove a copy constraint when a witness argument is not normalized
| template <typename Builder> field_t<Builder> field_t<Builder>::add_two(const field_t& add_b, const field_t& add_c) const | ||
| { | ||
| if ((add_b.is_constant()) && (add_c.is_constant()) && (is_constant())) { | ||
| int num_const = |
There was a problem hiding this comment.
issue 6
optimized add_two: it does not create gates when at least two inputs are constant
| }; | ||
| uint32_t set_public() const | ||
| { | ||
| ASSERT(!is_constant()); |
| // = a.v * b.v * [a.mul * b.mul] + a.v * [a.mul * b.add] + b.v * [b.mul + a.add] + c.v * [c.mul] + | ||
| // [a.add * b.add + c.add] | ||
| // = a.v * b.v * [ q_m ] + a.v * [ q_1 ] + b.v * [ q_2 ] + c.v * [ q_3 ] + [ q_c ] | ||
| // = a.v * b.v * [a.mul * b.mul] + a.v * [a.mul * b.add] + b.v * [b.mul + a.add] + c.v * [c.mul] |
There was a problem hiding this comment.
issue 5
made the docs consistent with the structs used in circuit builder
| // q_2 := 0; | ||
| // q_3 := -1; | ||
| // q_c := this.add; | ||
| // a_scaling := this.mul; |
There was a problem hiding this comment.
issue 5
made the docs consistent with the structs used in circuit builder
| // constraints 1) and 2) above. | ||
| // More precisely, to check that `a * I - 1 + is_zero = 0`, it creates a `big_mul_gate` given by the equation: | ||
| // a.v * I.v * q_m + a.v * q_1 + I.v * q_2 + is_zero.v * q_3 + (-1) * q_4 + q_c = 0 | ||
| // a.v * I.v * mul_scaling + a.v * a_scaling + I.v * b_scaling + is_zero.v * c_scaling + (-1) * d_scaling + |
There was a problem hiding this comment.
issue 5
made the docs consistent with the structs used in circuit builder
| bb::fr q_3 = c.multiplicative_constant; | ||
| bb::fr q_4 = d.multiplicative_constant; | ||
| bb::fr q_c = a.additive_constant + b.additive_constant + c.additive_constant + d.additive_constant; | ||
| bb::fr const_scaling = a.additive_constant + b.additive_constant + c.additive_constant + d.additive_constant; |
There was a problem hiding this comment.
issue 5
made the docs consistent with the structs used in circuit builder
| bb::fr q_3 = c.multiplicative_constant; | ||
| bb::fr q_4 = d.multiplicative_constant; | ||
| bb::fr q_c = a.additive_constant * b.additive_constant + c.additive_constant + d.additive_constant; | ||
| bb::fr mul_scaling = a.multiplicative_constant * b.multiplicative_constant; |
There was a problem hiding this comment.
issue 5
made the docs consistent with the structs used in circuit builder
| for (size_t i = 0; i < last_gate_idx; ++i) { | ||
| // For i < last_gate_idx, we create a `big_add_gate` constraint | ||
| // a_i.v * q_l + b_i.v* q_r + c_i.v * q_o + d_i.v * q_4 + q_c + w_4_omega = 0 | ||
| // a_i.v * a_scaling + b_i.v * b_scaling + c_i.v * c_scaling + d_i.v * d_scaling + const_scaling + |
There was a problem hiding this comment.
issue 5
made the docs consistent with the structs used in circuit builder
| field_t invert() const { return field_t(fr::one()) / *this; } | ||
| field_t invert() const | ||
| { | ||
| auto* ctx = get_context(); |
There was a problem hiding this comment.
issue 15
invert calls divide_no_zero_check directly. checks that the denominator is not zero in the constant case. the change is reflected in the corresponding test
| /** | ||
| * @brief Return (a < b) as bool circuit type. | ||
| * This method *assumes* that both a and b are < 2^{num_bits} - 1 | ||
| * This method *assumes* that both a and b are < 2^{num_bits} |
There was a problem hiding this comment.
issue 1
corrected the inequality
| field_t<Builder> result(ctx); | ||
|
|
||
| // If at least one of the arguments is a constant 0, the output is a constant 0 | ||
| if ((is_constant() && is_zero().get_value()) || (other.is_constant() && other.is_zero().get_value())) { |
There was a problem hiding this comment.
issue 7
multiplication by a constant 0 results in a constant 0
field_t issues from external audit
✅ Deploy Preview for aztec-docs-temp-you-can-delete canceled.
|
### `field_t` Issues and Resolutions Tracker For more details, see the comments below. | ID | Title | Status | | --- | -------------------------------------------------------------------------------------------- | ------ | | 17 | `test_slice_random` not covering the entire input space | `slice` is replaced with a simpler method `split_at` | | 16 | Unexpected random value in `test_slice_random` | `slice` is replaced with a simpler method `split_at` | | 15 | \[opt] Duplicate constraint in `invert` | ✅ | | 14 | `set_public` on constant `field_t` passes `IS_CONSTANT` as a wire index | added the assertion that the input is not constant | | 13 | \[opt] Multiplication by zero in `field_t` not canonicalized to constant | ❌ (breaks bigfield assertions + no efficiency gains in real circuits) | | 12 | \[opt] `decompose_into_bits` with `num_bits = 253` does unnecessary aliasing check | deleted | | 11 | \[doc] Incorrect documentation of `slice` method | `slice` is replaced with a simpler method `split_at` | | 10 | Missing builder context validation in field operations | ✅ | | 9 | \[doc] Confusing comment in `accumulate` | ✅ | | 8 | \[opt] `assert_equal` optimization | ✅ | | 7 | Unnecessary witness multiplied by 0 in `normalize` | ✅ | | 6 | \[opt] `madd` could be optimized when 2 inputs are constant | ✅ | | 5 | Inconsistent naming of PLONK gate terms | Made the comments consistent with the naming in gate-related structs | | 4 | `bool_t` has a constructor that takes context and value | ✅ | | 3 | Use `get_value` to get a boolean value | ✅ | | 2 | No restriction on `num_bits` in `ranged_less_than` | ✅ | | 1 | \[doc] Assumption over `ranged_less_than` is overly restrictive, method comment has mistakes | ✅ |
### `field_t` Issues and Resolutions Tracker For more details, see the comments below. | ID | Title | Status | | --- | -------------------------------------------------------------------------------------------- | ------ | | 17 | `test_slice_random` not covering the entire input space | `slice` is replaced with a simpler method `split_at` | | 16 | Unexpected random value in `test_slice_random` | `slice` is replaced with a simpler method `split_at` | | 15 | \[opt] Duplicate constraint in `invert` | ✅ | | 14 | `set_public` on constant `field_t` passes `IS_CONSTANT` as a wire index | added the assertion that the input is not constant | | 13 | \[opt] Multiplication by zero in `field_t` not canonicalized to constant | ❌ (breaks bigfield assertions + no efficiency gains in real circuits) | | 12 | \[opt] `decompose_into_bits` with `num_bits = 253` does unnecessary aliasing check | deleted | | 11 | \[doc] Incorrect documentation of `slice` method | `slice` is replaced with a simpler method `split_at` | | 10 | Missing builder context validation in field operations | ✅ | | 9 | \[doc] Confusing comment in `accumulate` | ✅ | | 8 | \[opt] `assert_equal` optimization | ✅ | | 7 | Unnecessary witness multiplied by 0 in `normalize` | ✅ | | 6 | \[opt] `madd` could be optimized when 2 inputs are constant | ✅ | | 5 | Inconsistent naming of PLONK gate terms | Made the comments consistent with the naming in gate-related structs | | 4 | `bool_t` has a constructor that takes context and value | ✅ | | 3 | Use `get_value` to get a boolean value | ✅ | | 2 | No restriction on `num_bits` in `ranged_less_than` | ✅ | | 1 | \[doc] Assumption over `ranged_less_than` is overly restrictive, method comment has mistakes | ✅ |
field_tIssues and Resolutions TrackerFor more details, see the comments below.
test_slice_randomnot covering the entire input spacesliceis replaced with a simpler methodsplit_attest_slice_randomsliceis replaced with a simpler methodsplit_atinvertset_publicon constantfield_tpassesIS_CONSTANTas a wire indexfield_tnot canonicalized to constantdecompose_into_bitswithnum_bits = 253does unnecessary aliasing checkslicemethodsliceis replaced with a simpler methodsplit_ataccumulateassert_equaloptimizationnormalizemaddcould be optimized when 2 inputs are constantbool_thas a constructor that takes context and valueget_valueto get a boolean valuenum_bitsinranged_less_thanranged_less_thanis overly restrictive, method comment has mistakes