DO NOT MERGE: init & inner q's and todos#19720
Conversation
| /// | ||
| /// It's technically possible for both to be set, since one function in the tx will span phases. | ||
| /// Of course, that function probably doesn't _need_ to set both, since it _knows_ the counter at which the phases switch. | ||
| /// | ||
| /// MIKE: It's a shame that this introduces another thing which must be known before a single function | ||
| /// can be processed by a kernel, but is only learned later in the tx. (I still cling on to the idea | ||
| /// of state channels or collaborative proving of a single tx being possible. Any time something like | ||
| /// this is introduced it makes that harder to achieve). Obviously, there are other examples of this | ||
| /// pattern (such as the first nullifier hint), but I'd like to avoid more examples if we can. | ||
| /// | ||
| /// Tentative idea: | ||
| /// Can we not more-simply track two items: | ||
| /// max_end_counter_of_all_functions_which_voiced_an_expectation_that_theyre_non_revertible | ||
| /// min_start_counter_of_all_functions_which_voiced_an_expectation_that_theyre_revertible | ||
| /// (not genuine naming suggestions, I'm just trying to explain the idea) | ||
| /// | ||
| /// (it doesn't technically _have_ to be the explicit start and end counters, but it helps me explain my point) | ||
| /// Each time we process a function, we take the max between the function & the kernel's `max_...` value and the min of the `min_...` values. | ||
| /// Then we can simply check these against the min_revertible_side_effect counter in the tail circuit once known. | ||
| /// This removes the ugliness that each function needs to propagate its expectation of the `min_revertible_side_effect_counter`. |
There was a problem hiding this comment.
Flagging this as a worthwhile talking point
There was a problem hiding this comment.
You are right both can be set! The comment is misleading (sorry!). It should be for checking a "counter" (not a function) is in a particular phase. For example, a function could be checking that it is in revertible phase by setting its start counter to expected_revertible_side_effect_counter. And in the same function it could be checking that a note hash it's reading is indeed non-revertible by setting the note hash's counter to expected_non_revertible_side_effect_counter.
Your idea works great if we only care about the phase a function is in, but it won't be able to allow checking the phase of a random counter. But I'm also not sure if that's useful at all. So also happy to change it to what you suggested!
There was a problem hiding this comment.
I dropped the ball on this one.
but it won't be able to allow checking the phase of a random counter. But I'm also not sure if that's useful at all
I hadn't considered the use case of checking a counter outside the current function's execution frame. Not sure if that's useful.
No time to do anything differently to what we have, now, unfortunately.
...s/private-kernel-lib/src/components/private_call_data_validator/validate_contract_address.nr
Show resolved
Hide resolved
...ivate-kernel-lib/src/components/private_call_data_validator/validate_side_effect_counters.nr
Show resolved
Hide resolved
| // Q: I wonder if (future enhancement) we should hash every item in every claimed_length_array at the time the item is created. | ||
| // Some of these structs actually contain a lot of elements, and we're ROM-accessing each element when copying the arrays | ||
| // in every inner kernel iteration. | ||
| // PrivateLogData, KeyValidationRequestAndGenerator, PrivateCallRequest, PublicCallRequest are large. | ||
| // Also, the Scoped<Counted<>> wrappers add overhead too, so we could consider hashing those (unless we need that data in every kernel iteration). | ||
| // Then we can unpack the hashes as and when we need them in future circuits. | ||
| // E.g. when we pop a (hashed) private call off the stack, we can unpack the hash at that time. | ||
| // E.g. when we process key validation requests in the reset kernel, we can unpack the hash at that time. | ||
| // E.g. private log preimage data and public call request data can be passed to the mempool, much like we do with contract class log preimage data. | ||
| // This could save several thousand gates per kernel iteration. | ||
|
|
There was a problem hiding this comment.
Worthy of discussion (post alpha)
There was a problem hiding this comment.
Looks like the databus doesn't add much value if we still pack and unpack eventually 🤣
|
|
||
| impl Empty for BatchingBlobCommitment { | ||
| fn empty() -> Self { | ||
| // TODO(potential for bugs): this "empty" is not an all-zero witness, because the point_at_infinity |
There was a problem hiding this comment.
I don't think this is used at all, not even in tests. This was probably copy paste from another struct, and sometimes trait impl like this is not really needed but still implemented. I only removed those unused when I saw one, didn't do a deep clean everywhere and missed this one which should definitely be removed :(
...ir-protocol-circuits/crates/private-kernel-lib/src/components/private_call_data_validator.nr
Show resolved
Hide resolved
...ir-protocol-circuits/crates/private-kernel-lib/src/components/private_call_data_validator.nr
Show resolved
Hide resolved
...ir-protocol-circuits/crates/private-kernel-lib/src/components/private_call_data_validator.nr
Show resolved
Hide resolved
| /// | ||
| /// It's technically possible for both to be set, since one function in the tx will span phases. | ||
| /// Of course, that function probably doesn't _need_ to set both, since it _knows_ the counter at which the phases switch. | ||
| /// | ||
| /// MIKE: It's a shame that this introduces another thing which must be known before a single function | ||
| /// can be processed by a kernel, but is only learned later in the tx. (I still cling on to the idea | ||
| /// of state channels or collaborative proving of a single tx being possible. Any time something like | ||
| /// this is introduced it makes that harder to achieve). Obviously, there are other examples of this | ||
| /// pattern (such as the first nullifier hint), but I'd like to avoid more examples if we can. | ||
| /// | ||
| /// Tentative idea: | ||
| /// Can we not more-simply track two items: | ||
| /// max_end_counter_of_all_functions_which_voiced_an_expectation_that_theyre_non_revertible | ||
| /// min_start_counter_of_all_functions_which_voiced_an_expectation_that_theyre_revertible | ||
| /// (not genuine naming suggestions, I'm just trying to explain the idea) | ||
| /// | ||
| /// (it doesn't technically _have_ to be the explicit start and end counters, but it helps me explain my point) | ||
| /// Each time we process a function, we take the max between the function & the kernel's `max_...` value and the min of the `min_...` values. | ||
| /// Then we can simply check these against the min_revertible_side_effect counter in the tail circuit once known. | ||
| /// This removes the ugliness that each function needs to propagate its expectation of the `min_revertible_side_effect_counter`. |
There was a problem hiding this comment.
You are right both can be set! The comment is misleading (sorry!). It should be for checking a "counter" (not a function) is in a particular phase. For example, a function could be checking that it is in revertible phase by setting its start counter to expected_revertible_side_effect_counter. And in the same function it could be checking that a note hash it's reading is indeed non-revertible by setting the note hash's counter to expected_non_revertible_side_effect_counter.
Your idea works great if we only care about the phase a function is in, but it won't be able to allow checking the phase of a random counter. But I'm also not sure if that's useful at all. So also happy to change it to what you suggested!
| /// | ||
| /// TODO: Move this to abis/public_data/storage/read.nr | ||
| /// OR... consider inlining it into the function which called it, because it's not being used anywhere else (unless there are plans to test it in isolation?) | ||
| /// TODO: possible duplicate of aztec-nr/aztec/src/history/public_storage.nr? |
There was a problem hiding this comment.
I think it is! That's why I thought we could move it into public_data so that this could also be used in aztec-nr or contracts that want to read public data.
There was a problem hiding this comment.
That sounds good to me!
| // A: We don't have to. The init and inner circuits add contract address to non-empty nullifiers. | ||
| // So we know we should silo it if the contract address is not empty. | ||
| // | ||
| // Note to self: possibly accidental sentinel value; giving the contract address `0` special meaning in the protocol? |
There was a problem hiding this comment.
This is tech debt! We used to allow running the reset circuit that silos nullifiers or private logs more than once. And the protocol nullifier used to be already-siloed when added in the Init. We can remove the if-else and always silo it now. Good find!
| } | ||
|
|
||
| // Claude says: | ||
| // Ambiguous points_to_infinity semantics: The function relies on points_to_infinity() to handle the max leaf case. In TestLeafPreimage, this returns true when next_value == 0. This creates ambiguity: is next_value: 0 an uninitialized leaf or the max leaf? The empty_low_leaf test conflates these cases. |
There was a problem hiding this comment.
Yeah points_to_infinity does consider empty leaves to be leaves that point to infinity. I think it's fine because even though is_valid_low_leaf returns true for an empty leaf, it can't be used to prove non-membership. The actual value inserted to the tree is a hash of the leaf preimage, and the hash of an empty leaf preimage is not 0. So the uninitialized leaves in the tree are not really the empty leaves. They are essentially the same as other made up leaves that satisfy the is_valid_low_leaf check but don't exist.
There was a problem hiding this comment.
Oooops I was wrong. There's an if-else in the hash function for nullifier and public data leaf preimages. If the preimage is empty then its hash will be 0. So an empty leaf is actually a valid low leaf!!
Luckily when we insert nullifiers in private tx base we check that the low leaves are not empty.
But when we read the public data tree for contract upgrade, or for anything via the util in aztec-nr, we didn't check it. So it's possible to use an empty leaf as hint and ignore the upgraded contract class id.
WE HAVE A BUG!!!!
There was a problem hiding this comment.
Good work, claude!
noir-projects/noir-protocol-circuits/crates/types/src/data/public_data_tree_leaf_preimage.nr
Show resolved
Hide resolved
2 participants
Comments I wrote as I went, for discussion, and possibly for changes before external audit.