Skip to content

chore: stdlib field pre-audit pt.2#14604

Merged
iakovenkos merged 47 commits into
nextfrom
si/field-work-2
Jun 8, 2025
Merged

chore: stdlib field pre-audit pt.2#14604
iakovenkos merged 47 commits into
nextfrom
si/field-work-2

Conversation

@iakovenkos

@iakovenkos iakovenkos commented May 29, 2025

Copy link
Copy Markdown
Contributor
  • accumulate() docs + tests + fix a minor bug
  • fix_witness() docs + test
  • is_zero():
    • use big_mul gates instead of poly gates --> no need to normalize() the witness
    • slightly expanded the tests
    • docs improved
  • == operator:
    • found a bug in the implementation - missing bool constraint for the resulting bool_t
    • now == is simply re-using .is_zero()
    • note: this led to VK changes for various circuits where == is used
    • expanded test docs by explaining the expected gate counts


if (witness_index == IS_CONSTANT) {
ASSERT(additive_constant != bb::fr(0), msg);
if (!this->is_constant()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason not just !is_constant()?

Comment thread barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.cpp

if (input.size() == 1) {
return input[0]; //.normalize();
return input[0].normalize();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we normalize the output in the general case, so seems logical to do so here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that we're being consistent, i.e., accumulate would always return a normalized output (so you don't have to do it manually when using accumulate. Some instances where you can remove normalize():

uint32_t result_index = field_t<Builder>::accumulate(sublimbs).normalize().get_witness_index();

uint32_t result_index = field_t<Builder>::accumulate(sublimbs).normalize().get_witness_index();

Comment thread barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.cpp
Comment thread barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.cpp
Comment thread barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.cpp
Comment thread barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.cpp
Comment thread barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.cpp
Comment thread barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.hpp
}
}

#ifndef NDEBUG

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EXPECT_DEATH leads to failed to die in most of the presets, e.g. in ./barretenberg/bootstrap.sh test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(out of curiosity) then how did the CI pass for previous PR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In CI it actually captures the ASSERT's correctly, so it passes, but it was somewhat disruptive for me to hit the failed to die when running bootstrap.sh test

Comment thread barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.cpp
Comment thread barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.cpp Outdated
Comment thread barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field.cpp
@iakovenkos iakovenkos added this pull request to the merge queue Jun 8, 2025
Merged via the queue into next with commit a725813 Jun 8, 2025
4 checks passed
@iakovenkos iakovenkos deleted the si/field-work-2 branch June 8, 2025 09:56
danielntmd pushed a commit to danielntmd/aztec-packages that referenced this pull request Jul 16, 2025
* `accumulate()` docs + tests + fix a minor bug 
* `fix_witness()` docs + test 
* `is_zero()`:
* + use `big_mul` gates instead of `poly` gates --> no need to
`normalize()` the witness
*   + slightly expanded the tests 
*   + docs improved
* `==` operator:
* + found a bug in the implementation - missing `bool` constraint for
the resulting `bool_t`
*   + now `==` is simply re-using `.is_zero()` 
* + note: this led to VK changes for various circuits where `==` is used
*   + expanded test docs by explaining the expected gate counts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants