Skip to content

Feat/rework zk proving #1239

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

Draft
wants to merge 34 commits into
base: master
Choose a base branch
from
Draft

Feat/rework zk proving #1239

wants to merge 34 commits into from

Conversation

dryajov
Copy link
Contributor

@dryajov dryajov commented May 29, 2025

This reworks the prover to use threads and integrates the native nim-groth16 prover.

@dryajov dryajov requested a review from Copilot May 29, 2025 01:46
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request reworks the prover to use threads and integrates the native nim-groth16 prover. Key changes include refactoring generic types (renaming type parameters to SomeTree/SomeHash), updating asynchronous error handling with refined raises clauses, and adding new backend implementations along with removal of legacy modules.

Reviewed Changes

Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
codex/utils/arrayutils.nim Removed unused import.
codex/stores/treehelper.nim Updated async raises annotations for improved error context.
codex/slots/types.nim Renamed generic type parameter from H to SomeHash.
codex/slots/sampler/sampler.nim Refactored type parameters and adjusted method signatures.
codex/slots/proofs/proverfactory.nim Added integration for the NimGroth16 backend using taskpools.
codex/slots/proofs/prover.nim Updated proving procedure to select backend-specific implementation.
codex/slots/proofs/backends/nimgroth16.nim Implemented new NimGroth16 backend with asynchronous proof generation.
codex/slots/proofs/backends/converters.nim Updated conversion functions to support new backend types.
codex/slots/proofs/backends/circomcompat.nim Marked as deprecated in favor of NimGroth16 and updated error messages.
codex/slots/builder/builder.nim Refactored generic types and adjusted API calls to align with new types.
codex/node.nim Revised taskpool variable and improved error handling in node setup.
codex/erasure/erasure.nim Improved task spawning and pointer passing in async encode/decode.
codex/conf.nim Added new enum members for ProverBackendCmd and backend-specific fields.
codex/codex.nim Updated RNG import and taskpool initialization for consistency.
.gitmodules Added new submodules for nim-groth16, nim-goldilocks-hash, and circom-witnessgen.
Comments suppressed due to low confidence (1)

codex/slots/proofs/backendutils.nim:1

  • The entire module has been removed; please ensure that all dependencies on backendutils.nim have been refactored or removed from the codebase.
import ./backends

var
x: seq[byte]
y: seq[byte]

Copy link
Preview

Copilot AI May 29, 2025

Choose a reason for hiding this comment

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

Ensure that the variables 'x' and 'y' are properly initialized or pre-allocated before calling 'marshal' to avoid any potential issues with uninitialized data.

Suggested change
x.setLen(32) # Pre-allocate to expected size for marshaling
y.setLen(32) # Pre-allocate to expected size for marshaling

Copilot uses AI. Check for mistakes.

@dryajov dryajov force-pushed the feat/rework-zk-proving branch from 5fa0756 to 920d14b Compare May 30, 2025 05:45
@dryajov dryajov requested a review from Copilot May 30, 2025 06:04
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR reworks the proving system by introducing threaded operations and integrating the native nim-groth16 prover. Key changes include

  • updating asynchronous procedure signatures to propagate CancelledError (and related raw exceptions)
  • refactoring type parameters and naming (e.g. using SomeTree/SomeHash instead of generic T/H)
  • updating backend selection logic and configuration for the prover and node taskpool

Reviewed Changes

Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
codex/stores/treehelper.nim Updated async annotations on proof storing procs.
codex/slots/types.nim Renamed type parameter H to SomeHash for clarity.
codex/slots/sampler/sampler.nim Refactored generic parameters and updated error handling using the ? operator.
codex/slots/proofs/proverfactory.nim Added support for initializing different prover backends with proper error logging.
codex/slots/proofs/prover.nim Reworked Prover type to use backend-specific fields and updated the prove procedure accordingly.
codex/slots/proofs/backends/nimgroth16.nim Integrated nim-groth16 backend with threaded proof generation and improved error handling.
codex/slots/proofs/backends/converters.nim Adjusted converters and updated copyright information.
codex/slots/proofs/backends/circomcompat.nim Marked circomcompat as deprecated and improved its error signaling using failure returns.
codex/slots/builder/builder.nim Updated generic type names and refined several builder functions to leverage the new error handling.
codex/node.nim Tweaked async signatures and updated the prover and taskpool initialization.
codex/erasure/erasure.nim Added raises attributes to async encoding/decoding functions.
codex/conf.nim Refactored ThreadCount type and parsing; updated default values and configuration parameters.
codex/codex.nim Updated taskpool initialization and switched prover initialization to the new method.
codex.nim Minor import cleanup.
.gitmodules Added new submodules for nim-groth16, nim-goldilocks-hash, and circom-witnessgen.
Comments suppressed due to low confidence (1)

codex/slots/builder/builder.nim:129

  • Consider renaming 'slotIndiciesIter' to 'slotIndicesIter' to correct the spelling and improve clarity.
proc slotIndiciesIter*[SomeTree, SomeHash](self: SlotsBuilder[SomeTree, SomeHash], slot: Natural): ?!Iter[int] =

quit QuitFailure
ThreadCount(count)
proc parseCmdArg*(T: type ThreadCount, val: string): T {.raises: [ValueError].} =
ThreadCount(val.parseUInt())
Copy link
Preview

Copilot AI May 30, 2025

Choose a reason for hiding this comment

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

The previous version enforced a minimum thread count when the value was non-zero. Consider reintroducing a validation check (e.g. ensuring the parsed thread count is either 0 or at least 2) if lower positive values are not desired.

Suggested change
ThreadCount(val.parseUInt())
let threadCount = val.parseUInt()
if threadCount != 0 and threadCount < 2:
raise newException(ValueError, "Thread count must be either 0 or at least 2")
ThreadCount(threadCount)

Copilot uses AI. Check for mistakes.

@dryajov dryajov force-pushed the feat/rework-zk-proving branch from b838d21 to 630b255 Compare May 30, 2025 19:31
@benbierens
Copy link
Contributor

I've built the test docker images for commit sha-d0b23f5. It passes all the data-layer tests. But it cannot run the marketplace tests.

It crashes with

TRC 2025-06-05 15:48:27.276+00:00 Initializing NimGroth16 backend            topics="codex slots proverfactory" tid=1 count=4
TRC 2025-06-05 15:48:27.276+00:00 Unable to initialize NimGroth16 backend:   topics="codex slots proverfactory" tid=1 err="Graph file not accessible or not found" count=5
WRN 2025-06-05 15:48:27.276+00:00 Proving circuit files are not found. Please run the following to download them: topics="codex slots proverfactory" tid=1 instructions="'./cirdl \"datadir3/circuits\" ws://ctnr1-3-int.cdx-11f9e21f-7085-46b2-8e41-b1cce30a5d7a.svc.cluster.local:8083 0x111f5aaa5dff76510b220d152426fe878b4a87ae'" count=6
ERR 2025-06-05 15:48:27.276+00:00 Failed to start Codex                      topics="codex" tid=1 msg="Unable to create prover." count=7

Is this branch both switching the prover to a thread AND replacing it with Groth16?
In this case I suspect that the circuit downloader is not updated and it's fetching the old circuit.
The downloader is automatically called from here: /docker/docker-entrypoint.sh line:110.

@dryajov
Copy link
Contributor Author

dryajov commented Jun 5, 2025

I've built the test docker images for commit sha-d0b23f5. It passes all the data-layer tests. But it cannot run the marketplace tests.

It crashes with

TRC 2025-06-05 15:48:27.276+00:00 Initializing NimGroth16 backend            topics="codex slots proverfactory" tid=1 count=4
TRC 2025-06-05 15:48:27.276+00:00 Unable to initialize NimGroth16 backend:   topics="codex slots proverfactory" tid=1 err="Graph file not accessible or not found" count=5
WRN 2025-06-05 15:48:27.276+00:00 Proving circuit files are not found. Please run the following to download them: topics="codex slots proverfactory" tid=1 instructions="'./cirdl \"datadir3/circuits\" ws://ctnr1-3-int.cdx-11f9e21f-7085-46b2-8e41-b1cce30a5d7a.svc.cluster.local:8083 0x111f5aaa5dff76510b220d152426fe878b4a87ae'" count=6
ERR 2025-06-05 15:48:27.276+00:00 Failed to start Codex                      topics="codex" tid=1 msg="Unable to create prover." count=7

Is this branch both switching the prover to a thread AND replacing it with Groth16? In this case I suspect that the circuit downloader is not updated and it's fetching the old circuit. The downloader is automatically called from here: /docker/docker-entrypoint.sh line:110.

the command line parameters changed, did you adjust them?

@markspanbroek markspanbroek force-pushed the feat/rework-zk-proving branch from 3d53222 to 127972b Compare June 26, 2025 09:46
@markspanbroek
Copy link
Contributor

I took the liberty of rebasing this on top of the current master branch, I hope you don't mind. I have the original commits lying around if you need them.

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