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

feat: Multi threading support for circom prover #1120

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

munna0908
Copy link
Contributor

This PR adds multi-threading support to the circompat backend to prevent blocking during proof generation and verification.

@munna0908 munna0908 requested review from dryajov and gmega February 20, 2025 12:56
@munna0908 munna0908 self-assigned this Feb 20, 2025
@munna0908 munna0908 marked this pull request as draft February 20, 2025 13:05
@munna0908 munna0908 marked this pull request as ready for review February 20, 2025 15:18
Copy link
Contributor

@dryajov dryajov left a comment

Choose a reason for hiding this comment

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

I'm still reviwing this, but so far it's looking good!

self: CircomCompat,
proof: CircomProof,
inputs: ProofInputs[H],
success: ptr Atomic[bool],
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why we're passing a pointer to a bool, there is no reason to do this, just use a straight up Atomic[bool]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to read VerifyResult from the calling procedure, so I'm passing it as a pointer to avoid pass-by-value.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no advantage of using a pointer here at all, copying a pointer is equivalent to copying a bool value? I don't see material difference between this and prove procedure?

failure("Failed to verify proof - err code: " & $res)
finally:
inputs.releaseCircomInputs()
if error =? (await self.asyncVerify(proof, inputs, res)).errorOption:
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just return a bool here (or even void) and set to false on failure, true otherwise?

Copy link
Contributor Author

@munna0908 munna0908 Feb 21, 2025

Choose a reason for hiding this comment

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

@dryajov To check task completion upon cancellation, I need the response for validation, so I'm passing it as a parameter to asyncVerify.

Copy link
Contributor

Choose a reason for hiding this comment

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

The response is either passed or failed, right?

@@ -52,3 +54,32 @@ func toG2*(g: CircomG2): G2Point =

func toGroth16Proof*(proof: CircomProof): Groth16Proof =
Groth16Proof(a: proof.a.toG1, b: proof.b.toG2, c: proof.c.toG1)

proc newProof*(): ptr Proof =
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better if we declare this as types and use the standard new conventino to construct them.

type
  ProofPtr = ptr Proof

proc new(_: ProofPtr): ProofPtr = 
 ...

Same for the verify types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commit-link

@munna0908 munna0908 force-pushed the feat/async-prover branch 2 times, most recently from 0b7c77c to c486e8c Compare February 25, 2025 17:10
@munna0908 munna0908 force-pushed the feat/async-prover branch from 1de746b to f907cbc Compare March 1, 2025 20:13
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