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. | |||
| @@ -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.
| @@ -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.
| // and that all side effects emitted in the child call have counters within the range | ||
| // [start_side_effect_counter, end_side_effect_counter]. | ||
| // Therefore, we only need to ensure that the next side effect from the current call starts after the | ||
| // end side effect from the child call. |
|
Thanks! I left comments about where the different issues you highlighted are resolved, the remaining minor bits were done in #22251. We can now close this. |
2 participants
Please read contributing guidelines and remove this line.
For audit-related pull requests, please use the audit PR template.