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

Async circuits, pt. 1 - async circuit runners #1450

Merged
merged 34 commits into from
Mar 7, 2024

Conversation

mitschabaude
Copy link
Member

@mitschabaude mitschabaude commented Feb 20, 2024

Starts to address MinaFoundation/Core-Grants#4

This is the first step of supporting async circuits. It does not yet support async methods in contracts / zkprogram, but brings significant internal changes towards that support:

  • Switch to a version of Pickles/Snarky which support async circuits under the hood. Internally, make the circuits that are passed to Pickles async functions
  • Change low-level circuit runners -- constraintSystem() and runAndCheck() -- to be async, and support async callbacks in them. This is a necessary step because we run contract methods through these runners

bindings: o1-labs/o1js-bindings#248
mina: MinaProtocol/mina#15084
snarky: o1-labs/snarky#836

@mitschabaude mitschabaude changed the title Async circuits Async circuits, pt. 1 Feb 27, 2024
"methods": {
"addEntry": {
"rows": 1317,
"digest": "86c4cb3e10764e94741e23cce1342192"
"rows": 5530,
Copy link
Member Author

Choose a reason for hiding this comment

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

wtf happened here??

Copy link
Member

Choose a reason for hiding this comment

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

wow :D

Copy link
Member Author

Choose a reason for hiding this comment

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

it turns out that 298c4bd fixed a major bug - previously, reduce() thought there are no actions emitted in any method if the only method that emitted actions called reduce() itself

The new constraint count is the correct one 😅

@mitschabaude mitschabaude marked this pull request as ready for review February 27, 2024 13:54
@mitschabaude mitschabaude requested a review from a team as a code owner February 27, 2024 13:54
@mitschabaude mitschabaude changed the title Async circuits, pt. 1 Async circuits, pt. 1 - async circuit runners Feb 27, 2024
@mitschabaude
Copy link
Member Author

The last commit b47224f addressed the weird reduction by one constraint for many of our circuits

@@ -101,6 +101,8 @@ function get<C>(t: Context.t<C>): C {
return current.context;
}

// FIXME there are many common scenarios where this error occurs, which weren't expected when this was written
Copy link
Member

Choose a reason for hiding this comment

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

+1 also encountered them recently

@mitschabaude mitschabaude changed the base branch from main to release/v0.16.3 March 6, 2024 07:59
Base automatically changed from release/v0.16.3 to main March 6, 2024 08:43
@mitschabaude
Copy link
Member Author

Just wanted to share that this massively improves our "witness generation" benchmark. The reason is that this witness generation step no longer maintains and updates a constraint system (it really doesn't need to).

I'm also expecting big improvements in uncached compilation since we run the circuit fewer times.

ecdsa - build constraint system

time: 3016.122ms ± 3.6%
change: -2.820% (p = 0.53 > 0.05)
Change within noise threshold.


ecdsa - witness generation

time: 885.910ms ± 3.0%
change: -57.055% (p = 0.00 < 0.05)
Performance has improved


ecdsa - compile (cached)

time: 8019.811ms ± 2.3%
change: -3.375% (p = 0.02 < 0.05)
Performance has improved

@mitschabaude mitschabaude merged commit 84233e9 into main Mar 7, 2024
13 checks passed
@mitschabaude mitschabaude deleted the feature/async-circuits branch March 7, 2024 09:17
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.

None yet

2 participants