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

Public Kernel - Update side effect counter after nested call #6052

Open
MirandaWood opened this issue Apr 26, 2024 · 0 comments
Open

Public Kernel - Update side effect counter after nested call #6052

MirandaWood opened this issue Apr 26, 2024 · 0 comments
Labels

Comments

@MirandaWood
Copy link
Contributor

In public -> public nested calls, the context currently increments the side effect counter by 1 rather than extracting the updated counter from the call itself (in public_context.nr):

    pub fn call_public_function_with_packed_args<RETURNS_COUNT>(
     <....>
    ) -> FunctionReturns<RETURNS_COUNT> {
        let side_effect_counter = self.side_effect_counter;
        // TODO get next value from output of `call_public_function_internal`
        self.side_effect_counter += 1;

        let raw_returns = call_public_function_internal(
            <...>
        );

        FunctionReturns::new(raw_returns)
    }

This causes issues as the nested call may increase the side effect counter by more than one. In private_context.nr we return a call stack item from the internal call oracle, and extract the updated counter, and increment to account for the call itself being a side effect:

self.side_effect_counter = item.public_inputs.end_side_effect_counter + 1;

The public call oracle only returns the return values of the nested call. To have it also return the side effect counter probably requires arithmetic on generics (to set the fn to return an array of RETURN_COUNT + 1).


Example of issue:

    #[aztec(public)]
    fn two_logs() {
        context.emit_unencrypted_log(1);
        context.emit_unencrypted_log(2);
    }

    #[aztec(public)]
    fn set_value_with_nested_calls() {
        ThisContract::at(context.this_address()).two_logs().call(&mut context);
        context.emit_unencrypted_log(20);
    }

Calling set_value_with_nested_calls results in the side effects representing the log of value 2 and 20 having the same counter. Logs in the incorrect order in the kernel circuits will give an incorrect logs hash.

MirandaWood added a commit that referenced this issue May 2, 2024
## A follow-up to #5718:

- [x] - Hash logs inside the circuit (closing #1165)

Complete, with some caveats. In most cases, whatever log is being
broadcast can be converted to bytes with one of the traits (in
`log_traits`) and sha hashed. Note we now need these traits because
`sha256_slice` has (understandably) been removed, so we must define a
fixed length input for generic types.

The one exception to this is broadcasting a contract class which ends up
emitting up to 518,400 bytes. This is still hashed in ts via a new
oracle method `emit_contract_class_unencrypted_log`. I believe it's fine
to hash this outside of the circuit, because correctness will be
guaranteed by the bytecode commitment (as opposed to a generic log,
where we need the hash to come from the circuit to trust it).
- [x] - Accumulate logs length inside the circuit

Complete, perhaps we should track lengths of each log to more easily
split them into revertible/non-revertible later on?

- [x] - Flat hash the logs in `tail` + update documentation

Now that `sha256_slice` is removed, we would have to flat hash all the
empty space up to max logs - unsure whether this would give us any
benefit?
EDIT: After some testing, it is more efficient for larger numbers of
logs (~9+) in both the circuit and L1, so I have implemented flat
hashing.

- [x] - Add a `logsCache`, like `noteCache`, to track logs + ordering in
ts in nested executions

Note that the `logsCache` is only implemented (and required) in the
private context for now.
Public calls don't require squashing and removal of logs representing
nullified notes, so an array (`allUnencryptedLogs`) will do. It
currently just keeps track of ordering when we have nested calls, but
will be useful for removing transient logs (#1641).

- [x] - Investigate + solve issue with `tx.ts` error not throwing

I'm not sure why this check exists - the comment:
```
      // This check is present because each private function invocation creates encrypted FunctionL2Logs object and
      // both public and private function invocations create unencrypted FunctionL2Logs object. Hence "num unencrypted"
      // >= "num encrypted".
```
implies that functions must emit both types of logs? A tx with one
private call, which emits one encrypted log, should fail here, and I
don't see why.
EDIT: Have removed the check as it seems redundant.

---

Note that in nested calls that have more than one side effect, logs will
have duplicate side effect counters and so cannot be sorted correctly
(#6052). Currently the logs collected in `allUnencryptedLogs` are the
only place that have correctly ordered logs in this case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Todo
Development

No branches or pull requests

1 participant