Skip to content
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

feat(Event_Logs): Remove unencrypted emit in private #7161

Closed
Tracked by #2783
LHerskind opened this issue Jun 24, 2024 · 4 comments
Closed
Tracked by #2783

feat(Event_Logs): Remove unencrypted emit in private #7161

LHerskind opened this issue Jun 24, 2024 · 4 comments
Assignees

Comments

@LHerskind
Copy link
Contributor

LHerskind commented Jun 24, 2024

In PrivateContext we got emit_unencrypted_log. To reduce the option for error, I think it might be the best idea (as it is also written in the comment above the code) to kill it. It lures people in, and can wreck them. Also, if you "REALLY" want it, you can get very similar functionality by doing a emit_raw_event_log_with_masked_address where you are masking using randomness = 0. The main diff really "what" list you expect it to be in 🤷‍♂.

@sklppy88
Copy link
Contributor

After discussing with @LHerskind, we will not be removing this yet as it's used in the ContractInstanceDeployer and there is currently no good / clean way to replace this usage.

@LHerskind
Copy link
Contributor Author

LHerskind commented Jun 27, 2024

So it is here for prosperity. It is possible to use the raw masked data emission with a mask of 0, but this would end up broadcasting additional data, and since we are mainly interested in using it to broadcast code to be used in public, it makes sense to emit it using something that we would usually do only in public.

While slightly painful, just got thinking, could be actually have the function implemented inside the deployer itself? e.g., have emit_contract_class_unencrypted_log in there, but still purge it and emit_unencrypted_log from the context? While we need the logic, we might not need it be as easily available from there.

Separately, are we even able to constrain these without making an absolute explosion in the circuit size? Since it needs to know sizes at compile time don't we end up having absolutely crazy expensive deployments? 👀

From the code.

    // This fn exists separately from emit_unencrypted_log because sha hashing the preimage
    // is too large to compile (16,200 fields, 518,400 bytes) => the oracle hashes it
    // It is ONLY used with contract_class_registerer_contract since we already assert correctness:
    // - Contract class -> we will commit to the packed bytecode (currently a TODO)
    // - Private function -> we provide a membership proof
    // - Unconstrained function -> we provide a membership proof
    // Ordinary logs are not protected by the above so this fn shouldn't be called by anything else
    pub fn emit_contract_class_unencrypted_log<N>(&mut self, log: [Field; N]) {
        let event_selector = 5; // TODO: compute actual event selector.
        let contract_address = self.this_address();
        let counter = self.next_counter();
        let log_hash = emit_contract_class_unencrypted_log_private_internal(contract_address, event_selector, log, counter);
        // 44 = addr (32) + selector (4) + raw log len (4) + processed log len (4)
        let len = 44 + N * 32;
        let side_effect = LogHash { value: log_hash, counter, length: len };
        self.unencrypted_logs_hashes.push(side_effect);
    }

While the bytecode commits would allow you to "later" verify it, nothing here really enforces it. I guess if the hashing is not done correctly it would not be possible to include, so there might be a "kinda nasty" optimisation/assumption here that they will match, but if we have that assumption here, then we should also be able to hold similar assumptions for all the other logs (in which case we can save ~40K for the note encryption because of the hashing etc). The L1 contracts are enforcing that the data published will match the hash, so that might actually be a fair assumption. And since the kernels don't even know about those logs, it don't seem completely unfair to do, since the sequencer already need to be given the data directly, so it seems like it don't actually alter the assumptions?

Idea: Use oracle for the log_hash, the tx will only be included if the data is included AND matches the hash due to the data availability oracle, so it is essentially constrained on L1.

@LHerskind LHerskind reopened this Jun 27, 2024
@LHerskind
Copy link
Contributor Author

While #7232 helped make the user flows more aligned, there are now a bit of duplication of data, e.g., the event_selector in emit_unencrypted_log is for example populated and used, even though it is already inside the log argument, so we can cut off a few things here.

LHerskind added a commit that referenced this issue Jul 1, 2024
This pr took a stab at something we had discussed related to #7161: 
- remove the unencrypted emit functions from the private context
- have the logic reside in the contracts that need it
  - `contract_instance_deployer`
  - `contract_class_registerer`
 
Since having unencrypted emits easily available in the private context
is a huge footgun privacy-wise, we decided that it would be better to
get rid of it there. However, as we still needed it for the deployer (as
we need something to deploy the "first" public code), we left the public
inputs and just insert directly into those instead of using a neat
function.

The setup is still slightly different from what we are doing in private,
because it is dealing with `event_type_id` slightly odd, and doing a lot
of inefficient things. So it needs to be revisited at some point for
optimisations.
When the event macros are refined to also handle structs with non-field
elements we should be able to use a `to_be_bytes` value from in there to
more cleanly emit the event, and also update the "listener" such that we
could get rid of the current
`DEPLOYER_CONTRACT_INSTANCE_DEPLOYED_MAGIC_VALUE` and instead be looking
at just the `event_type_id` and the contract address to match it.
@nventuro
Copy link
Contributor

nventuro commented Oct 2, 2024

Closing in favor of #8978. We've already removed the unencrypted logs from the private context (though the oracles remain as the instance deployer uses them) - logs will likely be refactored in the kernel soon anyway.

@nventuro nventuro closed this as completed Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

3 participants