-
Notifications
You must be signed in to change notification settings - Fork 312
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
feat(avm): interaction testing #11947
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
@@ -0,0 +1,93 @@ | |||
#include "barretenberg/vm2/constraining/polynomials.hpp" |
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.
Moved from proving_helper.
ddbcf23
to
95d3190
Compare
@@ -27,13 +46,26 @@ void check_relation_internal(const Trace& trace, std::span<size_t> subrelations) | |||
} | |||
} | |||
} | |||
// For all subrelations, the result should be zero at the end of the trace. |
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.
I changed this to work both with linearly dependent or independent subrelations. The dependent ones are only checked at the end.
} | ||
} | ||
|
||
template <typename Relation, typename Trace, typename RowGetter> |
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.
I needed to template how to get the row, because it works differently on traces and polynomials.
@@ -10,4 +10,14 @@ class InteractionBuilderInterface { | |||
virtual void process(TraceContainer& trace) = 0; | |||
}; | |||
|
|||
// We set a dummy value in the inverse column so that the size of the column is right. | |||
// The correct value will be set by the prover. | |||
template <typename LookupSettings> void SetDummyInverses(TraceContainer& trace) |
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.
I factored this out. It's clearer and it shouldn't hurt the theoretical complexity.
@@ -55,7 +55,7 @@ class TraceContainer { | |||
// Observe that therefore concurrent write access to different columns is cheap. | |||
struct SparseColumn { | |||
std::shared_mutex mutex; | |||
uint32_t max_row_number = 0; | |||
int64_t max_row_number = -1; |
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.
This is a fix. I was always reporting at least 1 row.
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.
Same question/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.
-1 means that the columns is empty, therefore no max row number. Added comment.
e4dca6b
to
554844d
Compare
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.
Looks good to me! But I would have @IlyasRidhuan and/or @jeanmon review as well.
trace.visit_column(col, [&](size_t row, const AvmProver::FF& value) { | ||
// We use `at` because we are sure the row exists and the value is non-zero. | ||
poly.at(row) = value; | ||
}); | ||
// We free columns as we go. | ||
// TODO: If we merge the init with the setting, this would be even more memory efficient. | ||
trace.clear_column(col); |
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.
I know this file was just moved, but I'd make a comment with a warning that this function kills the trace by clearing the columns.
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.
Done. In the hpp.
const auto it = std::max_element(keys.begin(), keys.end()); | ||
column_data.max_row_number = it == keys.end() ? 0 : *it; | ||
column_data.max_row_number = it == keys.end() ? -1 : static_cast<int64_t>(*it); | ||
column_data.row_number_dirty = false; |
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.
is -1 meant to basically be "max uint" here? maybe add 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.
-1 means that the columns is empty, therefore no max row number. Added comment.
@@ -55,7 +55,7 @@ class TraceContainer { | |||
// Observe that therefore concurrent write access to different columns is cheap. | |||
struct SparseColumn { | |||
std::shared_mutex mutex; | |||
uint32_t max_row_number = 0; | |||
int64_t max_row_number = -1; |
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.
Same question/comment
static std::string get_subrelation_label(size_t index) { | ||
{{! from generic_lookup_relation.hpp }} | ||
if (index == 0) { | ||
return "INVERSES_ARE_CORRECT"; | ||
} else if (index == 1) { | ||
return "ACCUMULATION_IS_CORRECT"; | ||
} | ||
return std::to_string(index); | ||
} |
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.
So do lookups and permutations have 2 subrelations (inverses & accumulation)? If not, this confuses me...
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.
That's right, they have 2 subrelations, but they are not in PIL because the relation code is pure C++ (it's bb's). See generic_lookup_relation.hpp and its accumulate function.
#include "barretenberg/vm2/tracegen/test_trace_container.hpp" | ||
|
||
namespace bb::avm2::constraining { | ||
namespace detail { |
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.
why the detail namespace?
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.
It's common in C++ when you want to hide/indicate that this should not be used externally, but you still need to keep it in a header file (templates, etc). In header files you should not use anonymous namespaces, which is what you'd do in cpp files.
// Finally we check the interaction. | ||
[&]<size_t... Is>(std::index_sequence<Is...>) { | ||
constexpr std::array<size_t, sizeof...(Is)> subrels = { Is... }; | ||
detail::check_relation_internal<Lookup>( | ||
polys, subrels, [](const auto& polys, size_t r) { return polys.get_row(r); }); | ||
}(std::make_index_sequence<Lookup::SUBRELATION_PARTIAL_LENGTHS.size()>()); |
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.
Can you comment here a bit more about the the use of the elipses and what Is
is? Also why do you need to create a lambda and then call it?
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.
Unfortunately that would require a course in C++ metaprogramming. The person attempting to understand this code will either understand it, or should not modify it 😆 . I can try to explain over huddle some day but really there is no short explanation that would fit in 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.
554844d
to
24bd945
Compare
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.
LGTM!
|
||
namespace bb::avm2::constraining { | ||
|
||
// Computes the polynomials from the trace, and destroys it in the process. |
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.
@fcarreiro : If this function destroys the trace, cannot we use std::move mechanism to make it clear that ownership is passed to the function?
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.
I tried, but this function is used in a context where the caller cannot bind to an && reference. A bit complicated. I might make some changes at some point but it was not trivial.
}); | ||
// clang-format on | ||
|
||
check_relation<execution>(trace); | ||
} | ||
|
||
// This is just a PoC. | ||
TEST(ExecutionConstrainingTest, Permutation) |
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.
@fcarreiro Do you have any similar test for dynamic lookups?
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.
I didn't write one but it should be the same just changing the permutation for the lookupintodynamic.
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.
Cool work @fcarreiro ! Please look at my comments before merging.
24bd945
to
b1c51ff
Compare
Merge activity
|
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.76.4</summary> ## [0.76.4](aztec-package-v0.76.3...aztec-package-v0.76.4) (2025-02-13) ### Miscellaneous * **aztec-package:** Synchronize aztec-packages versions </details> <details><summary>barretenberg.js: 0.76.4</summary> ## [0.76.4](barretenberg.js-v0.76.3...barretenberg.js-v0.76.4) (2025-02-13) ### Miscellaneous * Unify webpack dev server versions ([#11965](#11965)) ([921d2cd](921d2cd)) </details> <details><summary>aztec-packages: 0.76.4</summary> ## [0.76.4](aztec-packages-v0.76.3...aztec-packages-v0.76.4) (2025-02-13) ### Features * `FunctionDefinition::as_typed_expr` (noir-lang/noir#7358) ([5efdd57](5efdd57)) * Aes decryption oracle ([#11907](#11907)) ([c4ce913](c4ce913)) * **avm:** Constrained ec_add ([#11525](#11525)) ([f8fe602](f8fe602)) * **avm:** Interaction testing ([#11947](#11947)) ([fc647eb](fc647eb)) * **avm:** Relation microbenchmarks ([#11974](#11974)) ([95b581d](95b581d)) * **cli:** Add `--target-dir` option (noir-lang/noir#7350) ([5efdd57](5efdd57)) * Indexed protocol contracts tree ([#11897](#11897)) ([96e84d4](96e84d4)) * **performance:** Check sub operations against induction variables (noir-lang/noir#7356) ([5efdd57](5efdd57)) * **performance:** Use unchecked ops based upon known induction variables (noir-lang/noir#7344) ([5efdd57](5efdd57)) * Small blob fixes/improvements ([#11686](#11686)) ([4eab9fc](4eab9fc)) * Update fee model ([#11953](#11953)) ([2798d58](2798d58)) * Use brillig optimized sha256 ([#11696](#11696)) ([438c905](438c905)) ### Bug Fixes * Ci fixes ([#11973](#11973)) ([6386f4e](6386f4e)) * **cli:** Only lock the packages selected in the workspace (noir-lang/noir#7345) ([5efdd57](5efdd57)) * Deterministic generation of vkeys in ts ([#11951](#11951)) ([7901cac](7901cac)) * Incorrect secondary file in LSP errors (noir-lang/noir#7347) ([5efdd57](5efdd57)) * Lock git dependencies folder when resolving workspace (noir-lang/noir#7327) ([5efdd57](5efdd57)) * Perform SSA constraints check on final SSA (noir-lang/noir#7334) ([5efdd57](5efdd57)) * Remove deprecated artifacts ([#11979](#11979)) ([4f0dce7](4f0dce7)) * Remove serial queue in broker facade ([#11956](#11956)) ([3485b52](3485b52)) * **ssa:** Make the lookback feature opt-in (noir-lang/noir#7190) ([5efdd57](5efdd57)) ### Miscellaneous * **avm:** Tracegen interactions assertion ([#11972](#11972)) ([b865ccc](b865ccc)) * Avoid doing all brillig integer arithmetic on u128s (noir-lang/noir#7357) ([5efdd57](5efdd57)) * Basic test for MSM in Noir to catch performance improvements and regressions (noir-lang/noir#7341) ([5efdd57](5efdd57)) * Bump devnet boot node resources ([#11958](#11958)) ([bbcdefc](bbcdefc)) * **ci:** Add Vecs and vecs to cspell (noir-lang/noir#7342) ([5efdd57](5efdd57)) * Deprecate keccak256 (noir-lang/noir#7361) ([5efdd57](5efdd57)) * Fix warnings (noir-lang/noir#7330) ([5efdd57](5efdd57)) * Mark sha256 as deprecated from the stdlib (noir-lang/noir#7351) ([5efdd57](5efdd57)) * Moving storage slot out of `NoteHeader` ([#11904](#11904)) ([8c4bb1c](8c4bb1c)) * Normalize path displayed by `nargo new` (noir-lang/noir#7328) ([5efdd57](5efdd57)) * Redo typo PR by osrm (noir-lang/noir#7238) ([5efdd57](5efdd57)) * Release Noir(1.0.0-beta.2) (noir-lang/noir#6914) ([5efdd57](5efdd57)) * Remove foreign calls array from Brillig VM constructor (noir-lang/noir#7337) ([5efdd57](5efdd57)) * Remove misleading output from `nargo check` (noir-lang/noir#7329) ([5efdd57](5efdd57)) * Remove some unused types and functions in the AST (noir-lang/noir#7339) ([5efdd57](5efdd57)) * Remove unnecessary constants (noir-lang/noir#7326) ([5efdd57](5efdd57)) * Revive browser test before killing it ([#11964](#11964)) ([cb47cc0](cb47cc0)) * Split acirgen into multiple modules (noir-lang/noir#7310) ([5efdd57](5efdd57)) * Unify webpack dev server versions ([#11965](#11965)) ([921d2cd](921d2cd)) </details> <details><summary>barretenberg: 0.76.4</summary> ## [0.76.4](barretenberg-v0.76.3...barretenberg-v0.76.4) (2025-02-13) ### Features * Aes decryption oracle ([#11907](#11907)) ([c4ce913](c4ce913)) * **avm:** Constrained ec_add ([#11525](#11525)) ([f8fe602](f8fe602)) * **avm:** Interaction testing ([#11947](#11947)) ([fc647eb](fc647eb)) * **avm:** Relation microbenchmarks ([#11974](#11974)) ([95b581d](95b581d)) ### Miscellaneous * **avm:** Tracegen interactions assertion ([#11972](#11972)) ([b865ccc](b865ccc)) * Unify webpack dev server versions ([#11965](#11965)) ([921d2cd](921d2cd)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.76.4</summary> ## [0.76.4](AztecProtocol/aztec-packages@aztec-package-v0.76.3...aztec-package-v0.76.4) (2025-02-13) ### Miscellaneous * **aztec-package:** Synchronize aztec-packages versions </details> <details><summary>barretenberg.js: 0.76.4</summary> ## [0.76.4](AztecProtocol/aztec-packages@barretenberg.js-v0.76.3...barretenberg.js-v0.76.4) (2025-02-13) ### Miscellaneous * Unify webpack dev server versions ([#11965](AztecProtocol/aztec-packages#11965)) ([921d2cd](AztecProtocol/aztec-packages@921d2cd)) </details> <details><summary>aztec-packages: 0.76.4</summary> ## [0.76.4](AztecProtocol/aztec-packages@aztec-packages-v0.76.3...aztec-packages-v0.76.4) (2025-02-13) ### Features * `FunctionDefinition::as_typed_expr` (noir-lang/noir#7358) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Aes decryption oracle ([#11907](AztecProtocol/aztec-packages#11907)) ([c4ce913](AztecProtocol/aztec-packages@c4ce913)) * **avm:** Constrained ec_add ([#11525](AztecProtocol/aztec-packages#11525)) ([f8fe602](AztecProtocol/aztec-packages@f8fe602)) * **avm:** Interaction testing ([#11947](AztecProtocol/aztec-packages#11947)) ([fc647eb](AztecProtocol/aztec-packages@fc647eb)) * **avm:** Relation microbenchmarks ([#11974](AztecProtocol/aztec-packages#11974)) ([95b581d](AztecProtocol/aztec-packages@95b581d)) * **cli:** Add `--target-dir` option (noir-lang/noir#7350) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Indexed protocol contracts tree ([#11897](AztecProtocol/aztec-packages#11897)) ([96e84d4](AztecProtocol/aztec-packages@96e84d4)) * **performance:** Check sub operations against induction variables (noir-lang/noir#7356) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * **performance:** Use unchecked ops based upon known induction variables (noir-lang/noir#7344) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Small blob fixes/improvements ([#11686](AztecProtocol/aztec-packages#11686)) ([4eab9fc](AztecProtocol/aztec-packages@4eab9fc)) * Update fee model ([#11953](AztecProtocol/aztec-packages#11953)) ([2798d58](AztecProtocol/aztec-packages@2798d58)) * Use brillig optimized sha256 ([#11696](AztecProtocol/aztec-packages#11696)) ([438c905](AztecProtocol/aztec-packages@438c905)) ### Bug Fixes * Ci fixes ([#11973](AztecProtocol/aztec-packages#11973)) ([6386f4e](AztecProtocol/aztec-packages@6386f4e)) * **cli:** Only lock the packages selected in the workspace (noir-lang/noir#7345) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Deterministic generation of vkeys in ts ([#11951](AztecProtocol/aztec-packages#11951)) ([7901cac](AztecProtocol/aztec-packages@7901cac)) * Incorrect secondary file in LSP errors (noir-lang/noir#7347) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Lock git dependencies folder when resolving workspace (noir-lang/noir#7327) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Perform SSA constraints check on final SSA (noir-lang/noir#7334) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Remove deprecated artifacts ([#11979](AztecProtocol/aztec-packages#11979)) ([4f0dce7](AztecProtocol/aztec-packages@4f0dce7)) * Remove serial queue in broker facade ([#11956](AztecProtocol/aztec-packages#11956)) ([3485b52](AztecProtocol/aztec-packages@3485b52)) * **ssa:** Make the lookback feature opt-in (noir-lang/noir#7190) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) ### Miscellaneous * **avm:** Tracegen interactions assertion ([#11972](AztecProtocol/aztec-packages#11972)) ([b865ccc](AztecProtocol/aztec-packages@b865ccc)) * Avoid doing all brillig integer arithmetic on u128s (noir-lang/noir#7357) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Basic test for MSM in Noir to catch performance improvements and regressions (noir-lang/noir#7341) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Bump devnet boot node resources ([#11958](AztecProtocol/aztec-packages#11958)) ([bbcdefc](AztecProtocol/aztec-packages@bbcdefc)) * **ci:** Add Vecs and vecs to cspell (noir-lang/noir#7342) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Deprecate keccak256 (noir-lang/noir#7361) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Fix warnings (noir-lang/noir#7330) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Mark sha256 as deprecated from the stdlib (noir-lang/noir#7351) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Moving storage slot out of `NoteHeader` ([#11904](AztecProtocol/aztec-packages#11904)) ([8c4bb1c](AztecProtocol/aztec-packages@8c4bb1c)) * Normalize path displayed by `nargo new` (noir-lang/noir#7328) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Redo typo PR by osrm (noir-lang/noir#7238) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Release Noir(1.0.0-beta.2) (noir-lang/noir#6914) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Remove foreign calls array from Brillig VM constructor (noir-lang/noir#7337) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Remove misleading output from `nargo check` (noir-lang/noir#7329) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Remove some unused types and functions in the AST (noir-lang/noir#7339) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Remove unnecessary constants (noir-lang/noir#7326) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Revive browser test before killing it ([#11964](AztecProtocol/aztec-packages#11964)) ([cb47cc0](AztecProtocol/aztec-packages@cb47cc0)) * Split acirgen into multiple modules (noir-lang/noir#7310) ([5efdd57](AztecProtocol/aztec-packages@5efdd57)) * Unify webpack dev server versions ([#11965](AztecProtocol/aztec-packages#11965)) ([921d2cd](AztecProtocol/aztec-packages@921d2cd)) </details> <details><summary>barretenberg: 0.76.4</summary> ## [0.76.4](AztecProtocol/aztec-packages@barretenberg-v0.76.3...barretenberg-v0.76.4) (2025-02-13) ### Features * Aes decryption oracle ([#11907](AztecProtocol/aztec-packages#11907)) ([c4ce913](AztecProtocol/aztec-packages@c4ce913)) * **avm:** Constrained ec_add ([#11525](AztecProtocol/aztec-packages#11525)) ([f8fe602](AztecProtocol/aztec-packages@f8fe602)) * **avm:** Interaction testing ([#11947](AztecProtocol/aztec-packages#11947)) ([fc647eb](AztecProtocol/aztec-packages@fc647eb)) * **avm:** Relation microbenchmarks ([#11974](AztecProtocol/aztec-packages#11974)) ([95b581d](AztecProtocol/aztec-packages@95b581d)) ### Miscellaneous * **avm:** Tracegen interactions assertion ([#11972](AztecProtocol/aztec-packages#11972)) ([b865ccc](AztecProtocol/aztec-packages@b865ccc)) * Unify webpack dev server versions ([#11965](AztecProtocol/aztec-packages#11965)) ([921d2cd](AztecProtocol/aztec-packages@921d2cd)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
See dummy example in
execution.test.cpp
(constraining).Interactions are tested by computing the polynomials first.
Caveats:
check_interaction
.