Skip to content

feat: process designated teardown function call#6244

Merged
just-mitch merged 6 commits intomasterfrom
05-06-feat_process_designated_teardown_function_call
May 9, 2024
Merged

feat: process designated teardown function call#6244
just-mitch merged 6 commits intomasterfrom
05-06-feat_process_designated_teardown_function_call

Conversation

@just-mitch
Copy link
Collaborator

@just-mitch just-mitch commented May 7, 2024

Deviations from the spec:

I needed to create a new stack for processing the teardown calls, instead of storing a single call. I.e.

class PublicKernelCircuitPublicInputs {
    // ... other fields
--- +CallRequest public_teardown_call_request
+++ +CallRequest[MAX_PUBLIC_CALL_STACK_LENGTH_PER_TX] public_teardown_call_stack
}

This is because a teardown function can call a nested function, and, similar to the current design for public/private calls,
we need a way to keep track of our execution stack.

Further, in order to pass in the CallRequest to the private kernel circuits, I needed to add a new parameter to the PrivateCallData.

Overview

We designate a function to be run for teardown as:

context.set_public_teardown_function(
    context.this_address(),
    FunctionSelector::from_signature("pay_fee_with_shielded_rebate(Field,(Field),Field)"),
    [amount, asset.to_field(), secret_hash]
);

As I note in a comment, I created #6277 for getting back to something like:

FPC::at(context.this_address()).pay_fee_with_shielded_rebate(amount, asset, secret_hash).set_public_teardown_function(&mut context)

This sets publicTeardownFunctionCall: PublicCallRequest in the encapsulating ClientExecutionContext, which defaults to PublicCallRequest.empty().

When private simulation is finished, we collect an array of all the public teardown functions that were set during the simulation.

We assert that the length of that array is 0 or 1.

When proving, we convert the publicTeardownFunctionCall to a CallRequest if it is not empty, otherwise we use CallRequest.empty().

This is specified in the PrivateCallData which is passed to the private kernel circuit.

In the private kernel circuits, we assert that if the public_teardown_function_hash is not zero on the PrivateCircuitPublicInputs, then it matches the hash of the publicTeardownFunctionCall in the PrivateCallData.

Further, we assert that if the teardown call request in the PrivateCallData is not empty, then the teardown call request from the previous kernel is empty.

In the private kernel tail, we assert that the public teardown call request is empty.

In private kernel tail to public, we initialize the teardown call stack to have the single element corresponding to the call request if it is not empty, and initialize it to an empty array otherwise.

Since teardown now has its own stack, we update the logic for how to know when we are in the different phases to simply look at each of their stacks:

  • setup uses end_non_revertible.public_call_stack
  • app logic uses end.public_call_stack
  • teardown uses public_teardown_call_stack

Note:

This does not change the fact that teardown is still non-revertible. That is covered by #5924

Copy link
Collaborator Author

just-mitch commented May 7, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @just-mitch and the rest of your teammates on Graphite Graphite

@just-mitch just-mitch force-pushed the 05-06-feat_process_designated_teardown_function_call branch 4 times, most recently from 226f621 to 60bbd5d Compare May 7, 2024 22:47
@AztecBot
Copy link
Collaborator

AztecBot commented May 7, 2024

Docs Preview

Hey there! 👋 You can check your preview at https://663d2d3f143d0349f0976b3c--aztec-docs-dev.netlify.app

@just-mitch just-mitch force-pushed the 05-06-feat_process_designated_teardown_function_call branch 5 times, most recently from e2f0bdb to 2f517be Compare May 8, 2024 16:44
@just-mitch just-mitch requested review from LeilaWang and spalladino May 8, 2024 17:25
@just-mitch just-mitch marked this pull request as ready for review May 8, 2024 17:53
@just-mitch just-mitch requested a review from dbanks12 May 8, 2024 18:00
@just-mitch just-mitch force-pushed the 05-06-feat_process_designated_teardown_function_call branch from 2f517be to d47e180 Compare May 8, 2024 20:13
@@ -88,14 +88,15 @@ impl PublicKernelTeardownCircuitPrivateInputs {
self.validate_inputs();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have different stacks for different phases, we can re-enable this check in validate_inputs:
assert(needs_setup == false)

ARG debug=""
FROM +end-to-end
RUN DEBUG=aztec:* yarn test $test
RUN DEBUG=$DEBUG yarn test $test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$debug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

export function collectPublicTeardownFunctionCall(execResult: ExecutionResult): PublicCallRequest {
const teardownCalls = [
execResult.publicTeardownFunctionCall,
...[...execResult.nestedExecutions].flatMap(collectPublicTeardownFunctionCall),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...execResult.nestedExecutions.flatMap().
This is probably copied from the function above, which is also overdone 😂

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cleaned them up a little!


const teardownCallStackLength = arrayNonEmptyLength(data.publicTeardownCallStack, f => f.isEmpty());

const teardownCallStack = teardownCallStackLength === 0 ? [] : [tx.publicTeardownFunctionCall];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it be const teardownCallStack = !tx.publicTeardownFunctionCall.isEmpty() ? [tx.publicTeardownFunctionCall] : []?
Using the length to create the stack makes it look like something is wrong: the length might be larger than 1 (although it won't be) but the array will only contain at most 1 item.

@just-mitch just-mitch force-pushed the 05-06-feat_process_designated_teardown_function_call branch from d47e180 to 575caab Compare May 8, 2024 20:22
@just-mitch just-mitch mentioned this pull request May 8, 2024
Copy link
Contributor

@LeilaWang LeilaWang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

Copy link
Contributor

@spalladino spalladino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@spalladino spalladino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I accidentally hit Comment instead of Approve :-P

@just-mitch just-mitch force-pushed the 05-06-feat_process_designated_teardown_function_call branch 3 times, most recently from fc7a463 to 968ecf7 Compare May 9, 2024 17:29
@AztecProtocol AztecProtocol deleted a comment from github-actions bot May 9, 2024
@just-mitch just-mitch force-pushed the 05-06-feat_process_designated_teardown_function_call branch from 0aabf76 to 19aea2e Compare May 9, 2024 19:44
@just-mitch just-mitch merged commit 553078c into master May 9, 2024
@just-mitch just-mitch deleted the 05-06-feat_process_designated_teardown_function_call branch May 9, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants