Conversation
nventuro
left a comment
nventuro
left a comment
There was a problem hiding this comment.
Thanks a lot for the comments! Replied to most of them, for some I'll be opening PRs soon, for others only ~1 week from now as we want to not cause a lot of API breakage right now, but we'll definitely get to them before March is over.
I do have some follow-up questions in some of these.
Thanks again!
| // If it's used for hashing with sha256 (as suggested in DAppPayload.to_be_bytes()), it could save quite a bit | ||
| // of gates. | ||
| // But the code doesn't seem to be used anywhere. Hashing of the payload uses serialize() and poseidon instead. | ||
| // Do we want to keep this? |
There was a problem hiding this comment.
Will remove in the next pass, thanks.
| @@ -1,2 +1,3 @@ | |||
| // TODO: Can be deleted. Not used anywhere. | |||
| // incremented by the next_counter function every time a side effect is emitted. | ||
| // This was added because the code below used to be: | ||
| // `self.min_revertible_side_effect_counter = self.side_effect_counter;` | ||
| // which made it necessary to increment the side effect counter manually. |
There was a problem hiding this comment.
Thanks! Are there downsides to unnecessarily incrementing these counters?
| // We hash the entire padded log to ensure a user cannot pass a shorter length and so emit incorrect shorter | ||
| // bytecode. | ||
| let log_hash = poseidon2_hash(log_to_emit); | ||
| let log_hash = compute_contract_class_log_hash(log_to_emit); |
There was a problem hiding this comment.
We should actualyl get rid of this function, it doesn't make sense for PrivateFunction to hold a fn that is only used by a protocol contract.
But are you saying this is wrong? If so, why? The protocol contracts don't use this version of the library, the use the aztec-sub-lib copy of it.
| @@ -1108,6 +1116,9 @@ impl PrivateContext { | |||
| args_hash: Field, | |||
| is_static_call: bool, | |||
| ) -> ReturnsHash { | |||
| // Q: It can be misleading to make a static call when the caller specifically requests a non-static call | |||
| // (`is_static_call == false`). Although this is not allowed and will cause the kernel to fail. | |||
| // But rather than altering the user's intent, throw an error from the simulator might be better? | |||
There was a problem hiding this comment.
Agreed, there's a planned refactor to call interfaces. In practice this ends up not happening as users don't construct these calls manually, but we should still fix the plumbing here.
| @@ -83,6 +83,7 @@ impl<Context> SingleUseClaim<Context> { | |||
| /// [`SingleUseClaim::assert_claimed`] and [`SingleUseClaim::has_claimed`] to coherently write and read state. | |||
| fn compute_nullifier(self, owner_nhk_app: Field) -> Field { | |||
| // TODO(F-180): make sure we follow the nullifier convention | |||
| // Q: The separator should be DOM_SEP__NULLIFIER? | |||
There was a problem hiding this comment.
It should actually be its own new separator, since __NULLIFIER is for note nullifiers and takes a note hash, which this doesn't. I addressed this yesterday in #21189
| @@ -46,6 +46,8 @@ pub fn fields_from_bytes<let N: u32>(bytes: BoundedVec<u8, N>) -> BoundedVec<Fie | |||
| let p = std::field::modulus_be_bytes(); | |||
|
|
|||
| // Since input length is a multiple of 32, we can simply process all chunks fully | |||
| // TODO: Should be `for i in 0..N / 32 { if i < bytes.len() / 32 { ... } }` instead. | |||
| // This hasn't caused any problem because it's only used in an unconstrained fn at the moment. | |||
There was a problem hiding this comment.
Good catch. This function is due a small rework, as the name doesn't clearly explain what it is it does (it's not 'conversion'), we'll make it unconstrained as part of that.
| for i in 0..N { | ||
| if idx == 32 { | ||
| randomness = random().to_be_bytes(); | ||
| idx = 1; // Skip the first byte as it's biased (not uniformly distributed over 0-255). |
There was a problem hiding this comment.
We got rid of this entire file as this function was bad, replaced for the new poseidon masking mechanism
| @@ -50,6 +50,8 @@ impl NoteHash for UintNote { | |||
|
|
|||
| // Then compute the completion note hash. In a real partial note this step would be performed in public. | |||
| partial_note.compute_complete_note_hash(self.value) | |||
|
|
|||
| // Q: The partial note and the complete note probably shouldn't use the same domain separator? | |||
There was a problem hiding this comment.
Yes, this is the followup work to be done in #21189 - we need new domseps, and we need to change how the final note hash is computed (so that it guarantees separation by storage slot).
Partial note hash construction is about to get reworked, which is why this looks wrong now.
| @@ -225,6 +227,7 @@ impl PartialUintNote { | |||
| // contract that emitted the nullifier) as it checks the value directly against the nullifier tree and all the | |||
| // nullifiers in the tree are siloed by the protocol. | |||
| let siloed_validity_commitment = compute_siloed_nullifier(context.this_address(), validity_commitment); | |||
| // Q: Why not use context.assert_nullifier_exists()? | |||
There was a problem hiding this comment.
Unclear, that function may not have been nicely exposed at the time. Agreed that this is better, thanks. Possibly done this way because the author was porting the public case, and had settled nullifiers in mind.
2 participants
Please read contributing guidelines and remove this line.
For audit-related pull requests, please use the audit PR template.