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

Recursive public fns in ACIR public simulator #467

Merged
merged 7 commits into from
May 5, 2023

Conversation

spalladino
Copy link
Collaborator

  • Refactors the public executor to split up the executor and an execution instance
  • Makes it a responsibility of the executor to fetch the bytecode to run
  • Adds a callPublicFunction oracle function in Noir
  • Tests public function call via new public methods in the parent and child contracts used for testing private nested calls

Related to #430

@spalladino spalladino marked this pull request as ready for review May 4, 2023 19:48
@spalladino spalladino requested a review from sirasistant May 4, 2023 19:48
@@ -16,6 +16,7 @@ export interface ACIRCallback {
notifyCreatedNote(params: ACVMField[]): Promise<[ACVMField]>;
notifyNullifiedNote(params: ACVMField[]): Promise<[ACVMField]>;
callPrivateFunction(params: ACVMField[]): Promise<ACVMField[]>;
callPublicFunction(params: ACVMField[]): Promise<ACVMField[]>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a suggestion but I feel like at this point we could type the callback as a Record with key string and value functions with type (params: ACVMField[]) => Promise<ACVMField[]>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately not all functions have the same signature: we have return types ACVMField[] and [ACVMField]. We could use ACVMField[] for all, but I think it's nice to hace an extra check. And using record with key string means we don't get type checks for the function names. I think I'd stick with the verbose approach for now.

private log = createDebugLogger('aztec:simulator:public-executor'),
) {}

public async execute(request: TxRequest): Promise<PublicExecutionResult>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it be clearer if the Executor just handles TxRequests and creates Executions with them? and then executions can create nested executions (similar to the simulator=>execution relationship of the private calls). I'm not sure though :D

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 wanted the execution object to be as dumb as possible, so it just carries the information of the execution itself, while keeping the logic in the executor. Maybe the catch is keeping this execute method simpler, to just handle executions, and have a separate createExecutionFromTxRequest as you suggest? I'll think about it!

Copy link
Collaborator

Choose a reason for hiding this comment

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

The createExecutionFromTxRequest approach sounds good to me! however I was just being picky about readability, whatever you think is best :D

@spalladino spalladino force-pushed the palla/recursive-public-fns branch from 475dcdf to 327290c Compare May 5, 2023 12:49
@spalladino spalladino merged commit c6feff5 into master May 5, 2023
@spalladino spalladino deleted the palla/recursive-public-fns branch May 5, 2023 13:15
ludamad pushed a commit that referenced this pull request Jul 14, 2023
…5s wasm time). (#434)

* Remove foundation submodule, add build-system
* Initial bindgen work.
* GPT4 created rust bindgen.
* Debug build preset wasm-dbg for stack traces etc.
* Async call support. Refactor binds. Common is module. WASM stubs unused wasi.
* Move some c_bind to module root and make use new abi.
* Initial working pthreads.
* Polynomial cache
* Custom poly allocator. Prealloc.
* Cleanup. Several parallel_for solutions. Might be worth keeping them around until threading work stabilises.
* Improve mem usage by slab allocating large parts of pippenger runtime state, and allowing slab reuse by removing from pk.
* Don't need binaryen.
* Bump alpine to get clang 16
* Backwards compatible cbinds.
* Move slab allocator to common. Init slab allocator just before creating pk.
* Dont need bleeding edge fork of libc anymore
* bb.js is standalone prover that handles noir constraint/witness output. (#471)
* format msgpack serialization and excldue msgpack-c from clang-format (#467)
* Fix horrific nasty memory bug.

---------

Co-authored-by: Suyash Bagad <[email protected]>
Co-authored-by: Maxim Vezenov <[email protected]>
Co-authored-by: Maddiaa <[email protected]>
Co-authored-by: ledwards2225 <[email protected]>
Co-authored-by: Innokentii Sennovskii <[email protected]>
Co-authored-by: kevaundray <[email protected]>
Co-authored-by: zac-williamson <[email protected]>
Co-authored-by: codygunton <[email protected]>
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.

2 participants