feat: add .contractaddr to contract class log#12197
feat: add .contractaddr to contract class log#12197MirandaWood merged 3 commits intomw/cc-logs-pos-hashfrom
Conversation
iAmMichaelConnor
left a comment
iAmMichaelConnor
left a comment
There was a problem hiding this comment.
Reflecting, I think I can't properly review PRs in this "backwards" order, because the code that it's building upon hasn't been reviewed to be a good "basis", and because once it's merged, this same code will need to be re-reviewed when reviewing the PR that this merges into. So I think if we're going to stack PRs, the review order has to match the order in which the PRs are stacked, with the one closest to master being reviewed first.
So with all that being said, I can either:
- Wait until the PR closest to master is ready to
review; or - We can just merge this into that big branch and I can review that big PR once it's ready. (And in future we can do it in the proper order).
Sorry for any confusion. I've added some comments, which can be addressed in either this PR or the lower-down PR.
| pub global CONTRACT_CLASS_LOG_SIZE_IN_FIELDS: u32 = MAX_PACKED_BYTECODE_SIZE_PER_PRIVATE_FUNCTION_IN_FIELDS | ||
| pub global CONTRACT_CLASS_LOG_DATA_SIZE_IN_FIELDS: u32 = MAX_PACKED_BYTECODE_SIZE_PER_PRIVATE_FUNCTION_IN_FIELDS | ||
| + REGISTERER_PRIVATE_FUNCTION_BROADCASTED_ADDITIONAL_FIELDS; | ||
| pub global CONTRACT_CLASS_LOG_SIZE_IN_FIELDS: u32 = CONTRACT_CLASS_LOG_DATA_SIZE_IN_FIELDS + 1; // This includes the contract address, since it is always emitted with a contract class log |
There was a problem hiding this comment.
I'm seeing the term SCOPED_ in other names in this file. Does "scoped" mean "includes the contract address of the app"? If so, perhaps we can match that naming here? (The reason I suggest this is because I first saw these two names in a different file, and was like "What's the difference between "log size" and "log data size"?
There was a problem hiding this comment.
I was following the existing usage of DATA as we do for public logs. Also (I was a little confused by this) to 'scope' a log is to hash the first element with the address and drop the address, so technically a scoped log would just be CONTRACT_CLASS_LOG_DATA_SIZE_IN_FIELDS.
E.g.
pub struct PublicLog {
pub log: Log<PUBLIC_LOG_DATA_SIZE_IN_FIELDS>,
pub contract_address: AztecAddress,
}
But I think I prefer something lke:
pub struct PublicLog {
pub log: Log<PUBLIC_LOG_SIZE_IN_FIELDS>,
pub contract_address: AztecAddress,
}
Where the struct is SCOPED_PUBLIC_LOG_SIZE_IN_FIELDS long.
| } else { | ||
| let mut log = contract_class_log; | ||
| log.fields[0] = compute_siloed_contract_class_log_field(contract_address, log.fields[0]); | ||
| log.log.fields[0] = compute_siloed_contract_class_log_field( |
There was a problem hiding this comment.
For a private log, we keep field 0 of the log as "special", and we put a tag there, so hashing field 0 doesn't corrupt the main, meaty part of a log.
I'm worried this will corrupt the 0th field of the contract class log. What goes into field 0?
What did we do with the old "unencrypted logs" that we used to emit from private?
There was a problem hiding this comment.
This is how we've always done it for the dpeloyer and is required to be able to detect a log came from the protocol contract:
- The first element of a contract class log is always a magic value corresponding to what we are broadcasting e.g.
REGISTERER_UNCONSTRAINED_FUNCTION_BROADCASTED_MAGIC_VALUEorREGISTERER_PRIVATE_FUNCTION_BROADCASTED_MAGIC_VALUE. - We then silo that first element here with the address, which for cc logs is just
3. - When reading and dealing with broadcated logs, we look for the hash of 3 with some magic value.
Though we do constrain that only the address 3 contract can emit cc logs, I imagine the siloing is an extra layer of security. I can't remember, but I may have added it to match how the deployer contract does it, just to maintain some continuity.
| log: ContractClassLog, | ||
| log_hash: ScopedLogHash, | ||
| ) -> ContractClassLog { | ||
| // Validate address |
There was a problem hiding this comment.
Is this function called solely by the BaseRollup circuit? If so, perhaps it should live there. (Not necessary this PR).
There was a problem hiding this comment.
Makes sense, will do! I originally had it as something implemented for the cc log (like log.validate(log_hash)), can't remember why I changed it now...
| let padded_log_fields: [Field; CONTRACT_CLASS_LOG_DATA_SIZE_IN_FIELDS] = | ||
| array_concat(log_fields, [0; CONTRACT_CLASS_LOG_DATA_SIZE_IN_FIELDS - N]); | ||
| let log = ContractClassLog { | ||
| contract_address: self.tube_data.contract_address, |
There was a problem hiding this comment.
Why does tube_data carry a contract_address which happens to match the address of the Contract Class Registerer?
There was a problem hiding this comment.
This is all in the tests - tube_data is just a FixtureBuilder that carries around everything you could possibly need at every step of the circuits. We use it for basically all nr tests here to avoid having like 20 different structs for each kernel and rollup circuit.
| offset += total_private_logs_len; | ||
| // cc logs | ||
| let total_contract_class_logs_len = NUM_CC_LOGS * (cc_log.len() + 1); | ||
| let total_contract_class_logs_len = NUM_CC_LOGS * (cc_log.len() + 2); |
There was a problem hiding this comment.
I've added the address - this is just a test and it should be documented everywhere else. I can add it here too though?
| offset += total_private_logs_len; | ||
| // cc logs | ||
| let total_contract_class_logs_len = NUM_CC_LOGS * (cc_log.len() + 1); | ||
| let total_contract_class_logs_len = NUM_CC_LOGS * (cc_log.len() + 2); |
There was a problem hiding this comment.
I just opened an issue for the pattern in this file #12217
There was a problem hiding this comment.
Just FYI I think this behaviour is mostly in tests. We need it for the blob because we don't want to concat arrays, we want to fill an array with certain elements conditionally.
2 participants
Tracking an address alongside the cc log to aid filters in the archiver. The address is always constrained to be that of the registerer, but many archiver tests use random addresses so they brick without ugly hacks