-
Notifications
You must be signed in to change notification settings - Fork 324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: contract class log refactor follow up #12402
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After removing this check, we no longer check the blobHashes
(part of EthBlobEvaluationInputs
) which are an input to getBlockFromRollupTx
. These originally come from L1 events as far I can see, so I think are safe, but can just be extracted in this fn rather than passed around the place. Maybe this is preferable?
Part of #12402 Refactor to make length methods used on right padded arrays clearer.
unsafe { | ||
emit_contract_class_unencrypted_log_private(contract_address, log_to_emit, counter); | ||
} | ||
// Safety: the below broadcasts the raw log and returns the hash, so we can provide it to the base rollup later to be constrained. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I just remember why we decided to hash in the circuit...
If I tweak the oracle to return a log_hash
that's for a completely different contract, and submit that along with the tx, all the checks will still pass. But because the log_hash
is not used in computing the class id, the class id will be associated with a wrong artifact. This will prevent people from calling that contract unless the correct artifact is provided using some non-standard codepath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaah that was it. So even though in the base rollup we must provide the 'wrong' contract class log preimage to match this tweaked log_hash
, we can still broadcast a mismatched class id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checked the code again, we don't have an api for fetching contract data by class id, the class id is separated from the log. But users will only be able to fetch the wrong contract data from the log store. And because it's not registered they will have to register it again to start using it.
As for the correct contract, its class id was registered and the nullifier being emitted, so it can't be registered and broadcasted with the correct log again. The contract data will have to be stored in the log store or provided to the pxe manually if wanting to use that contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! I will revert the part that hashes in the oracle
Closes #12325
Move the hashing of the contract class log to the oracle (as it was previously when we used sha hashing), since we constrain its correctness in the base rollup.(We don't want this!)TODO: is this comment still correct? NB from before the cc log PR, the only thing that has changed is we get a poseidon hash from the oracle (rather than SHA) and we constrain it in the rollup (rather than L1).