feat(avm)!: Unify nullifier, written slots and retrieved bytecodes tree traces#20949
Conversation
sirasistant
requested review from
IlyasRidhuan,
Maddiaa0,
fcarreiro and
jeanmon
as code owners
sirasistant
removed request for
IlyasRidhuan,
Maddiaa0,
fcarreiro and
jeanmon
| @@ -0,0 +1,282 @@ | |||
| include "merkle_check.pil"; | |||
There was a problem hiding this comment.
It's the old nullifier check renamed, with internal renamings of nullifier => value, leaf_not_exists renamed to not_exists, parametrized tree height and siloing separator, and exposing tree_size_after_write.
| @@ -0,0 +1,398 @@ | |||
| #include <gmock/gmock.h> | |||
There was a problem hiding this comment.
It's the old nullifier tree check test
| @@ -0,0 +1,233 @@ | |||
| #include "barretenberg/vm2/simulation/gadgets/indexed_tree_check.hpp" | |||
There was a problem hiding this comment.
It's the nullifier tree check renamed and with updated comments and minor changes. Diff it manually.
| @@ -0,0 +1,404 @@ | |||
| #include "barretenberg/vm2/simulation/gadgets/indexed_tree_check.hpp" | |||
There was a problem hiding this comment.
Also the old nullifier gadget test renamed :/
| #include "barretenberg/vm2/tracegen/lib/discard_reconstruction.hpp" | ||
|
|
||
| namespace bb::avm2::tracegen { | ||
|
|
There was a problem hiding this comment.
Also the nullifier one renamed with minor changes.
sirasistant
force-pushed
the
arv/unify_generic_indexed_tree_checks
branch
from
47d6285 to
2b23284
Compare
sirasistant
force-pushed
the
arv/unify_generic_indexed_tree_checks
branch
from
af66ee4 to
5ee027f
Compare
MirandaWood
left a comment
MirandaWood
left a comment
There was a problem hiding this comment.
Amazing stuff 🤩 Great work consolidating all our indexed tree circuits without missing any crucial checks!
| * This gadget is generic and can be used for any indexed tree with just one value per leaf (nullifier tree, retrieved bytecodes tree, | ||
| * written public data slots tree, etc.). The caller provides the tree height, and optionally a siloing | ||
| * separator and contract address to silo the value before insertion. | ||
| * | ||
| * Values can be optionally siloed with a contract address to bind them to their originating contract. | ||
| * The siloing_separator and address are provided by the caller when sel_silo is set. |
| // ========= HANDLE REDUNDANT WRITES ========= | ||
| // sel_insert implies sel since write implies sel | ||
| pol commit sel_insert; // @boolean (by definition) | ||
| sel_insert = write * (1 - exists); |
There was a problem hiding this comment.
I think we can use not_exists rather than 1 - exists here now!
There was a problem hiding this comment.
Ahh thanks for updating these! Sorry if it was a pain - I smashed these helpers out pretty quickly 😅
| }); | ||
|
|
||
| indexed_tree_check.assert_read(class_id, | ||
| std::nullopt, |
There was a problem hiding this comment.
Small thing (can come later!) - could we comment the nullopts here and in the public data slots simulator? I found myself easily losing track of which optional property was which 😅
| bool WrittenPublicDataSlotsTreeCheck::contains(const AztecAddress& contract_address, const FF& slot) | ||
| { | ||
| FF leaf_slot = compute_leaf_slot(contract_address, slot); | ||
| FF leaf_slot = unconstrained_compute_leaf_slot(contract_address, slot); |
There was a problem hiding this comment.
Just to check my understanding - we now use unconstrained here because the new indexed tree check performs this (constrained) as part of the siloing?
| // Shared between nullifier_exists.pil and emit_nullifier.pil. | ||
| // Lookup constant support: Can be removed when we support constants in lookups. | ||
| pol commit nullifier_tree_height; |
There was a problem hiding this comment.
Sorry another nit - could we clarify here that the constant value is set conditionally in nullifier_exists.pil and emit_nullifier.pil? Even something like // ~Shared between~ Constant height set in nullifier_exists.pil and emit_nullifier.pil. to make it a bit clearer it's not unconstrained anywhere that matters
BEGIN_COMMIT_OVERRIDE fix(avm)!: memory pre-audit (#21058) fix(avm)!: memory trace changes (#21059) fix!: AVM was missing range check on remainder for div in ALU (#21074) feat: run AVM NAPI simulations on dedicated threads instead of libuv pool (#21138) feat(avm)!: Unify nullifier, written slots and retrieved bytecodes tree traces (#20949) fix(avm)!: public inputs pre-audit (#21162) END_COMMIT_OVERRIDE
2 participants
Nullifier tree check was a superset of written slots and retrieved bytecodes tree check. Unify all three into the nullifier tree check implementation generalized as "indexed tree check"