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

Increase the number of private reads & writes #2003

Closed
iAmMichaelConnor opened this issue Sep 5, 2023 · 3 comments
Closed

Increase the number of private reads & writes #2003

iAmMichaelConnor opened this issue Sep 5, 2023 · 3 comments
Assignees
Labels
A-ux/devex Area: relates to external ux / devex. (Users typically are devs) (See also A-internal-devex)

Comments

@iAmMichaelConnor
Copy link
Contributor

The number of per-function reads/writes is 4. This isn't enough, especially since the library is currently being inefficient with how many read requests, nullifiers, and commitments it creates (see here for the issue that will fix this inefficiency) (a get_note followed by a replace or remove is causing two nullifiers and two new commitments to be created).

Someone testing the Sandbox just ran into MAX_READ_REQUESTS_PER_CALL as a limit, for a very modest function.

Let's assume the "Zac bus" will enable 4096 fields to be passed between circuits (this is a conservative estimate from chatting with Zac last week). We should modify the code to enable this much data to be passed from an app circuit to a kernel circuit (and from a kernel circuit to the next kernel circuit). We can also continue to make use of public inputs. I might suggest (at least for now) we continue to use public inputs for fixed-size data, and we only use the bus for arrays of data (which might vary in size).

Eventually, we'll have multiple sizes of kernel circuit (for various sizes of arrays). Until then, we don't want to simulate 4096 reads (for example) if an app only needs to do 8 reads. So we shouldn't hard-code the sizes of for loops to some upper-bound: ideally all for loops in the simulator would be flexible to only do as much compute as is necessary. (Of course, when we come to write actual circuits, the for loops will need to be rigidly-sized).

We could use std::vector instead of std::array for all circuit arrays (so a dynamic amount of data can be passed around), but that might be a step backwards, if we intend to eventually unify simulated ('native') circuits and proper circuits.

Clearly this might be a fiddly piece of work. As a quick, intermediate solution, could the constants be easily bumped to something like 32 per function and 128 per tx? (We should benchmark kernel & rollup simulation times before making this change and after making this change). If this is "quick" to do (i.e. before the public release), that would be fantastic.

Tagging @dbanks12 @suyash67 @jeanmon for thoughts.

@github-project-automation github-project-automation bot moved this to Todo in A3 Sep 5, 2023
@iAmMichaelConnor iAmMichaelConnor added the A-ux/devex Area: relates to external ux / devex. (Users typically are devs) (See also A-internal-devex) label Sep 5, 2023
@suyash67 suyash67 added this to the 📢 Initial Public Sandbox Release milestone Sep 5, 2023
@dbanks12
Copy link
Collaborator

dbanks12 commented Sep 5, 2023

Yes, I believe we should be able to bump them to those smaller values as an intermediate solution.

@dbanks12
Copy link
Collaborator

dbanks12 commented Sep 6, 2023

Bumping MAX_READ_REQUESTS_PER_CALL to 64 causes wasm out-of-bounds error while generating circuits.js bindings. I will bump only to 32 for now.

@suyash67
Copy link
Contributor

suyash67 commented Sep 8, 2023

Resolved by #2062

@suyash67 suyash67 closed this as completed Sep 8, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in A3 Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ux/devex Area: relates to external ux / devex. (Users typically are devs) (See also A-internal-devex)
Projects
Archived in project
Development

No branches or pull requests

3 participants