refactor(Barretenberg): Static Analysis of Goblin Recursive Verifier by DanielKotov · Pull Request #14610 · AztecProtocol/aztec-packages

DanielKotov · 2025-05-29T15:02:37Z

This time Recursive Verifier was tested by static analysis tool.

There was found bug with using of insecure function "pow" that was removed. Also there was the bug in function compute_barycentric_evaluations, that appeared because of vector wasn't initialized correctly by default constructor.

Finally, there were many cases with unused variables in the circuit and they can be described this way: variable is often a result of sequential multiplications on some challenge, but there's no need to do the last multiplication, because we add new variables in the gate that won't be used in the circuit.

And there was a try to improve static analyzer performance again, maybe it became faster by some seconds, result weren't stable to get visible data.

Rumata888 · 2025-05-30T13:33:30Z

        fixed_variables.insert(this->to_real(ultra_circuit_builder, 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
+        // gate_variables.reserve(8);


Remove this line please

Rumata888 · 2025-05-30T13:33:40Z

 */
 template <typename FF>
-inline void Graph_<FF>::process_gate_variables(UltraCircuitBuilder& ultra_circuit_builder,
+inline void Graph_<FF>::process_gate_variables([[maybe_unused]] UltraCircuitBuilder& ultra_circuit_builder,


Why do you need this then?

Rumata888 · 2025-05-30T13:34:42Z

 *          4) Special handling for sorted constraints in delta range blocks
 */
-template <typename FF> Graph_<FF>::Graph_(bb::UltraCircuitBuilder& ultra_circuit_constructor)
+template <typename FF> Graph_<FF>::Graph_(bb::UltraCircuitBuilder& ultra_circuit_constructor, bool graph)


What does the "graph" variable mean? I think there should be a better name

Rumata888 · 2025-05-30T13:37:27Z

        // Compute the evaluation of the vanishing polynomia Z_H(X) at X =
        // gemini_evaluation_challenge
-        const FF vanishing_poly_eval = gemini_evaluation_challenge.pow(SUBGROUP_SIZE) - FF(1);
+        auto compute_vanishing_poly_eval = [&]() {


This is inefficient, please use field's implementation of pow. If it's broken, then we need to fix it

Alternatively, at least use square and multiply

iakovenkos · 2025-06-16T11:34:11Z

        commitments.emplace_back(claim.commitment);
        batched_eval += alpha_pow * claim.opening_pair.evaluation;
-        alpha_pow *= alpha;
+        if (idx < opening_claims.size() - 1) {


iakovenkos · 2025-06-16T11:35:30Z

            }
        }
-        if (shifted_exponent != 0) {
+        if (shifted_exponent >= 2) {


@suyash67 here's an example of a bug that Daniel's tool finds automatically

iakovenkos · 2025-06-16T11:37:58Z

    CommitmentLabels commitment_labels;

    const BF accumulated_result = transcript->template receive_from_prover<BF>("accumulated_result");
+    if constexpr (IsUltraBuilder<Builder>) {


de-instantiate Translator with MegaCircuitBuilder pls, it's not needed, then you'd avoid this constexpr

add a comment explaining what's going on

Made sure that the current impl of `field_t` `pow()` matches the audited (and [deprecated](#14563)) implementation of `bigfield` `pow()`. The `pow` method taking const integers as an argument is taken from #14610

iakovenkos

lgtm!

…tion

…sult prime basis limb

Made sure that the current impl of `field_t` `pow()` matches the audited (and [deprecated](AztecProtocol#14563)) implementation of `bigfield` `pow()`. The `pow` method taking const integers as an argument is taken from AztecProtocol#14610

…ztecProtocol#14610) This time Recursive Verifier was tested by static analysis tool. There was found bug with using of insecure function "pow" that was removed. Also there was the bug in function compute_barycentric_evaluations, that appeared because of vector wasn't initialized correctly by default constructor. Finally, there were many cases with unused variables in the circuit and they can be described this way: variable is often a result of sequential multiplications on some challenge, but there's no need to do the last multiplication, because we add new variables in the gate that won't be used in the circuit. And there was a try to improve static analyzer performance again, maybe it became faster by some seconds, result weren't stable to get visible data. --------- Co-authored-by: iakovenkos <sergey.s.yakovenko@gmail.com>

Made sure that the current impl of `field_t` `pow()` matches the audited (and [deprecated](AztecProtocol/aztec-packages#14563)) implementation of `bigfield` `pow()`. The `pow` method taking const integers as an argument is taken from AztecProtocol/aztec-packages#14610

DanielKotov requested review from Rumata888 and iakovenkos May 29, 2025 15:02

Rumata888 requested changes May 30, 2025

View reviewed changes

DanielKotov force-pushed the dk/goblin_recursive_verifier_PR branch from f226dbb to b54e8be Compare June 2, 2025 15:37

iakovenkos mentioned this pull request Jun 2, 2025

chore: field_t pow changes #14680

Merged

DanielKotov force-pushed the dk/goblin_recursive_verifier_PR branch 3 times, most recently from e96f36c to d4dee54 Compare June 11, 2025 16:01

iakovenkos reviewed Jun 16, 2025

View reviewed changes

DanielKotov force-pushed the dk/goblin_recursive_verifier_PR branch 2 times, most recently from 142a763 to 8ab1127 Compare June 17, 2025 12:18

iakovenkos mentioned this pull request Jun 17, 2025

chore: remove RelationUtils::scale_and_batch_elements method triggered when IsFoldingFlavor == false AztecProtocol/barretenberg#1443

Closed

DanielKotov force-pushed the dk/goblin_recursive_verifier_PR branch from b6de4d3 to f21205c Compare June 18, 2025 10:16

iakovenkos approved these changes Jun 18, 2025

View reviewed changes

Rumata888 approved these changes Jun 18, 2025

View reviewed changes

DanielKotov added this pull request to the merge queue Jun 18, 2025

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 18, 2025

DanielKotov force-pushed the dk/goblin_recursive_verifier_PR branch 2 times, most recently from 69a8266 to 621c979 Compare June 19, 2025 10:34

iakovenkos mentioned this pull request Jun 19, 2025

Internal audit: field_t AztecProtocol/barretenberg#1403

Closed

DanielKotov force-pushed the dk/goblin_recursive_verifier_PR branch from 1e71761 to ae610e6 Compare June 19, 2025 14:40

DanielKotov and others added 6 commits June 24, 2025 12:10

stuff for tests

bd18592

fix merge conflicts with graph constructors

56807f0

another boring routine

7fb47b0

test redesign after rebase

87693de

small fixes to remove unnecessary gates

dd2ab3e

quick and dirty fixes

aeb4e54

DanielKotov added 18 commits June 24, 2025 12:10

only one constructor for Ultra with possibility to turn of graph crea…

db126fb

…tion

remove all things for timing

1d6d452

last commit before PR

cd0df43

remover Kesha's remarks

c6ececc

merge conflict fixes in ultra_recursive_verifier test

4cc1591

sync gobling and boomerang base of the tests

84cc838

merge conflicts fixes after rebase

67aa6f3

vks weren't changed finally

e16daa7

vk fix again

cd30a4b

better naming

c2609b8

remove mega builder for translator + add comment about accumulated_re…

c804499

…sult prime basis limb

test redesign for goblin + fix for translator tests

f04958c

add TODO for myself

f92c6d1

change VKs again

95e10b2

changed a number of gates ultra recursive verifier tests

6358b04

I didn't see HEAD in the recursive flavor, but ok, let's update it again

4bc77a4

now variables in one gate size = 0

71f5b89

new VKS

f72f11b

DanielKotov force-pushed the dk/goblin_recursive_verifier_PR branch from 5339cf9 to f72f11b Compare June 24, 2025 12:11

DanielKotov added this pull request to the merge queue Jun 24, 2025

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 24, 2025

DanielKotov added this pull request to the merge queue Jun 24, 2025

DanielKotov removed this pull request from the merge queue due to a manual request Jun 24, 2025

DanielKotov added this pull request to the merge queue Jun 24, 2025

Merged via the queue into next with commit 282da3b Jun 24, 2025
4 checks passed

DanielKotov deleted the dk/goblin_recursive_verifier_PR branch June 24, 2025 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(Barretenberg): Static Analysis of Goblin Recursive Verifier#14610

refactor(Barretenberg): Static Analysis of Goblin Recursive Verifier#14610
DanielKotov merged 25 commits into
nextfrom
dk/goblin_recursive_verifier_PR

DanielKotov commented May 29, 2025

Uh oh!

Rumata888 May 30, 2025

Uh oh!

Rumata888 May 30, 2025

Uh oh!

Rumata888 May 30, 2025

Uh oh!

Rumata888 May 30, 2025

Uh oh!

Rumata888 May 30, 2025

Uh oh!

iakovenkos Jun 16, 2025

Uh oh!

iakovenkos Jun 16, 2025

Uh oh!

iakovenkos Jun 16, 2025

Uh oh!

iakovenkos left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

DanielKotov commented May 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iakovenkos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants