chore(avm)!: Bytecode Retrieval pre-audit docs #20718
chore(avm)!: Bytecode Retrieval pre-audit docs #20718MirandaWood wants to merge 6 commits intomerge-train/avmfrom
Conversation
| * (2) TOO_MANY_BYTECODES: We have reached the limit of the number of bytecodes to retrieve for this tx (limit_error in cpp). (TODO(MW) rename to one or other) | ||
| * |
There was a problem hiding this comment.
Not a huge deal, but this error is known as TOO_MANY_BYTECODES in some places, and limit_error in cpp generally. Any preferences to which version to use everywhere?
| /////////////////////////////// | ||
| // Contract Retrieval | ||
| /////////////////////////////// | ||
|
|
||
| pol commit instance_exists; // @boolean (by lookup into contract_instance_retrieval) | ||
| // instance_exists is only on for active rows. | ||
| // TODO(MW): Added this to avoid switching on #[IS_NEW_CLASS_CHECK] for inactive rows but may not be required | ||
| #[SEL_ON_INSTANCE_EXISTS] | ||
| (1 - sel) * instance_exists = 0; |
There was a problem hiding this comment.
Removed the bool constraint (since CONTRACT_INSTANCE_RETRIEVAL covers this, and added a sel gate, since one didn't exist before, I'm not 100% on whether this is needed so can remove!
| pol TOO_MANY_BYTECODES = no_remaining_bytecodes * is_new_class; | ||
|
|
||
| #[IS_NEW_CLASS_CHECK] | ||
| instance_exists { | ||
| current_class_id, | ||
| is_new_class, | ||
| prev_retrieved_bytecodes_tree_root | ||
| } in retrieved_bytecodes_tree_check.sel { | ||
| retrieved_bytecodes_tree_check.class_id, | ||
| retrieved_bytecodes_tree_check.leaf_not_exists, | ||
| retrieved_bytecodes_tree_check.root | ||
| }; | ||
| // TODO(MW): Add INSTANCE_NOT_FOUND var (= 1 - instance_exists) for clarity/consistency with TOO_MANY_BYTECODES? | ||
|
|
There was a problem hiding this comment.
We don't need it but might be nice for clarity/consistency to have a local var like INSTANCE_NOT_FOUND rather than looking for 1 - instance_exists
| /////////////////////////////// | ||
| // Class ID Derivation | ||
| /////////////////////////////// | ||
|
|
||
| // This constrains the correctness of class id against the class members (see above description for details). | ||
| // Note: only need to derive the class id if the instance exists. TODO: Probably some latch is also needed. | ||
| // TODO(MW): I think the below is ok - class_id_derivation works over a single row with two lookups to poseidon | ||
| // anchored by the start and end rows with a matching result. We may need to add input_len to second lookup, but not relevant here. |
There was a problem hiding this comment.
I think we don't need a latch here - a sanity check would be appreciated!
| FF nullifier_root; // TODO(MW): Rename either nullifier_root -> nullifier_tree_root or... | ||
| FF public_data_tree_root; // public_data_tree_root -> public_data_root |
There was a problem hiding this comment.
The circuit uses the naming convention ..._tree_root, but a lot of cpp uses just .._root. Avoided doing this now because it creates a massive annoying diff to look at, so will create a follow up PR (unless we agree it's not worth the effort!)
There was a problem hiding this comment.
Another event suggestion - instead of the two bools, would it make more sense to use an enum like std::optional<BytecodeRetrievalEventEventError> error? IIRC it's more consistent with other events
| { | ||
| using C = Column; | ||
|
|
||
| // We start from row 1 to avoid the required precomputed row (TODO(MW): Check?) |
There was a problem hiding this comment.
Noticed we start at row 1 here but don't use shifted columns - is this correct?
| // If class ID derivation is disabled (error), force other members to 0. | ||
| #[ARTIFACT_HASH_IS_ZERO_IF_ERROR] | ||
| error * (artifact_hash - 0) = 0; | ||
| #[PRIVATE_FUNCTION_ROOT_IS_ZERO_IF_ERROR] | ||
| error * (private_functions_root - 0) = 0; | ||
| #[BYTECODE_ID_IS_ZERO_IF_ERROR] | ||
| error * (bytecode_id - 0) = 0; // Off so we do not lookup into this trace against a valid id. | ||
|
|
There was a problem hiding this comment.
I don't think we actually use artifact_hash or private_functions_root in the case of an error, so it should be safe to remove this zero-ing. Though it is more 'hygenic' to clear them out (and future proof in case we look them up later?), so we could keep these relations?
… pass preaudit WIP
MirandaWood
force-pushed
the
mw/avm-bc-ret-pre-audit-docs
branch
from
c3592ae to
fd65309
Compare
MirandaWood
requested review from
IlyasRidhuan,
Maddiaa0,
fcarreiro and
jeanmon
as code owners
1 participant
Bytecode retrieval pre-audit PR including:
This trace does a lot of delegation, so the pre audit has been completed assuming the target circuits constrain as expected! The actual columns used in these lookups have been checked though.
Closes AVM-54
TODOs/Notes: See comments for those to discuss (otherwise, I need to add
preconditions, doc on why we zero some properties on error, and tracegen testsUpdate: complete!).