chore(contracts): Calculate public call stack item hash from Noir #1141
Conversation
| @@ -246,15 +260,42 @@ struct PublicCircuitPublicInputs { | |||
| new_commitments: [Field; MAX_NEW_COMMITMENTS_PER_CALL], | |||
| new_nullifiers: [Field; crate::abi::MAX_NEW_NULLIFIERS_PER_CALL], | |||
| new_l2_to_l1_msgs: [Field; crate::abi::MAX_NEW_L2_TO_L1_MSGS_PER_CALL], | |||
| commitment_trees_roots: CommitmentTreesRoots, | |||
| commitment_trees_roots: CommitmentTreesRoots, // TODO: This is not present in cpp or ts, do we need to include it? | |||
There was a problem hiding this comment.
@Maddiaa0 I noted this field was added in Noir but has no counterpart in cpp. Should we add it there as well, so the two structs don't diverge?
spalladino
force-pushed
the
palla/pub-call-stack-item-hash-from-noir
branch
from
1dded6d to
70be9bd
Compare
✅ Deploy Preview for preeminent-bienenstitch-606ad0 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| /** | ||
| * Alias for toBuffer32. | ||
| * @returns A 32-byte Buffer containing the padded Ethereum address. | ||
| */ | ||
| public toBuffer() { | ||
| return this.toBuffer32(); | ||
| } |
There was a problem hiding this comment.
Serializing EthAddress to 20 bytes by default was causing issues with serialization to/from cpp (yay). Msgpack autobindings were calling toBuffer on this field for serializing, which returned 20 bytes, but cpp was expecting 32. This resulted in the eth address being left or right aligned in cpp depending on how it got there. This was not uncovered before because eth address deserialization handled both cases, but when we started hashing this field in cpp, the difference started to show.
Please add 4hs to the counter of "time lost due to serialization issues"!
spalladino
force-pushed
the
palla/pub-call-stack-item-hash-from-noir
branch
3 times, most recently
from
c7955b3 to
cad0757
Compare
spalladino
force-pushed
the
palla/pub-call-stack-item-hash-from-noir
branch
from
cad0757 to
1fd762f
Compare
| constexpr size_t PUBLIC_CIRCUIT_PUBLIC_INPUTS_HASH_INPUT_LENGTH = | ||
| 2 + RETURN_VALUES_LENGTH + // + 1 for args_hash + 1 call_context.hash | ||
| MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_CALL + MAX_PUBLIC_DATA_READS_PER_CALL + MAX_PUBLIC_CALL_STACK_LENGTH_PER_CALL + | ||
| MAX_NEW_COMMITMENTS_PER_CALL + MAX_NEW_NULLIFIERS_PER_CALL + MAX_NEW_L2_TO_L1_MSGS_PER_CALL + 5; |
There was a problem hiding this comment.
I think it would help readability if you describe why is there the +5.
There was a problem hiding this comment.
Agree. I will add it on a subsequent PR, for now I want to merge this one before I keep getting conflicts!
Removes a longstanding TODO to calculate the call stack item hash (for enqueued public calls) correctly in Noir, instead of overwriting it in
private_execution. This required changing severalhashmethods in cpp fromNCT::compresstoNCT::hash, so they match the Pedersen flavor used in Noir (cc @suyash67).