[FEAT] lazy-loading circuits #7
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughIntroduces lazy-loading for ZK circuit initialization with client-side algorithm constants and tracking, a CGO callback bridge for on-demand loading, demo updates to resolve circuit paths and load PK/R1CS files at runtime, and a guard in proof generation to ensure algorithm initialization before creating ZK proofs. Changes
Sequence DiagramsequenceDiagram
participant Caller as Proof Generation
participant Client as client.EnsureAlgorithmInitialized
participant State as Algorithm State (RWMutex map)
participant Callback as Registered Init Callback (Go/C)
participant FileIO as Disk (PK/R1CS)
participant Init as InitAlgorithmWithTracking
Caller->>Client: EnsureAlgorithmInitialized(cipher)
Client->>Client: Resolve cipher -> algorithmID
Client->>State: IsAlgorithmInitialized(ID)?
alt already initialized
State-->>Client: true
Client-->>Caller: proceed
else not initialized
State-->>Client: false
Client->>Callback: invoke init callback(ID)
Callback->>FileIO: load provingKey & r1cs
FileIO-->>Callback: return data
Callback->>Init: InitAlgorithmWithTracking(ID, provingKey, r1cs)
Init->>Init: call underlying InitAlgorithm
Init->>State: MarkAlgorithmInitialized(ID)
Init-->>Callback: success/failure
alt success
Callback-->>Client: true
Client-->>Caller: proceed
else failure
Callback-->>Client: false
Client-->>Caller: error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
demo_lib/main.go (1)
59-94: Code duplication:circuitConfigandcircuitsRegistryare duplicated across files.The same
circuitConfigstruct andcircuitsRegistrymap are defined identically indemo_lib/main.go,demo_standalone/main.go, and partially inclient/zkprover.go. Consider extracting this into a shared package to maintain a single source of truth for circuit configuration.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/oprf.go(1 hunks)client/zkprover.go(1 hunks)demo_lib/main.go(3 hunks)demo_standalone/main.go(3 hunks)lib/libreclaim.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
client/oprf.go (1)
client/zkprover.go (1)
EnsureAlgorithmInitialized(106-130)
demo_standalone/main.go (4)
client/zkprover.go (5)
CHACHA20_OPRF(15-15)AES_128_OPRF(16-16)AES_256_OPRF(17-17)InitAlgorithmWithTracking(95-101)SetZKInitCallback(57-63)demo_lib/main.go (3)
CHACHA20_OPRF(61-61)AES_128_OPRF(62-62)AES_256_OPRF(63-63)client/logger.go (1)
GetLogger(12-31)lib/libreclaim.go (1)
SetZKInitCallback(331-357)
client/zkprover.go (1)
lib/libreclaim.go (5)
SetZKInitCallback(331-357)IsAlgorithmInitialized(376-378)GetAlgorithmID(360-364)GetCipherName(367-373)InitAlgorithm(285-323)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (12)
demo_lib/main.go (1)
196-203: LGTM: Graceful fallback on lazy loading setup failure.The main function properly handles the case where lazy loading setup fails by logging a warning and continuing execution. This is appropriate for a demo application.
client/oprf.go (1)
314-319: LGTM: Clean integration of lazy initialization.The lazy initialization check is correctly placed before ZK proof generation. The error handling properly wraps the underlying error with context, and the change aligns well with the new
EnsureAlgorithmInitializedfunction inclient/zkprover.go.demo_standalone/main.go (2)
27-46: Good: Uses constants fromimplpackage instead of redeclaring.Unlike
demo_lib/main.go, this file correctly referencesimpl.CHACHA20_OPRF,impl.AES_128_OPRF, andimpl.AES_256_OPRFfrom the shared prover package, which helps maintain consistency.
170-174: Appropriate behavior: Fatal error on lazy loading setup failure.For a standalone demo application, failing fast with
log.Fatalfwhen ZK lazy loading setup fails is appropriate, as the application cannot function without this capability.lib/libreclaim.go (3)
30-44: Consider thread-safety of C global callback pointer.The
g_zk_init_callbackC static variable is read and written without synchronization on the C side. While the Go side uses proper locking (zkInitCallbackLockinclient/zkprover.go), concurrent calls toSetZKInitCallbackandinvoke_zk_init_callbackfrom multiple threads could race on the C side.If this library is used in a multi-threaded context where the callback might be set while proving is in progress, consider adding synchronization.
315-316: LGTM: Proper delegation to tracking wrapper and callback chain.The
InitAlgorithmfunction correctly delegates toclient.InitAlgorithmWithTracking, and thecZKInitCallbackwrapper properly bridges the Go-C boundary by invoking the stored C callback.Also applies to: 325-328
330-357: LGTM: SetZKInitCallback properly wires Go and C callbacks.The function correctly:
- Initializes the logger if needed
- Stores the C callback via the internal setter
- Wires up the Go-side callback to invoke the C callback
- Clears both callbacks when passed nil
client/zkprover.go (5)
46-51: LGTM: Thread-safe global state management.The use of separate
sync.RWMutexinstances for the callback (zkInitCallbackLock) and initialization state (initializedLock) is appropriate and provides correct synchronization for concurrent access.
20-38: LGTM: Bidirectional cipher name mapping.The
cipherToAlgorithmIDandalgorithmIDToCiphermaps provide clean bidirectional lookup between cipher names and algorithm IDs. The cipher names appear to follow a consistent naming convention.
94-101: LGTM: Clean wrapper for initialization with tracking.
InitAlgorithmWithTrackingcorrectly wraps the underlyingprover.InitAlgorithmcall and marks the algorithm as initialized only on success.
103-129: Confirm TOCTOU vulnerability between IsAlgorithmInitialized check and callback invocation.The race condition is real: concurrent callers can both see
IsAlgorithmInitializedreturn false and invoke the callback for the same algorithm simultaneously. Since no lock guards the gap between the check (line 112) and callback invocation (line 124), both goroutines will callcb(algorithmID)→InitAlgorithmWithTracking→prover.InitAlgorithmconcurrently.This is acceptable only if
prover.InitAlgorithmis idempotent and handles double-initialization safely. Otherwise, consider:
- Using
sync.Onceper algorithm to ensure single initialization- Holding a lock during both the check and callback invocation
- Double-checking the initialized state after acquiring a lock
10-18: Ensure algorithm ID constants are synchronized with upstreamgithub.com/reclaimprotocol/zk-symmetric-crypto/gnark/libraries/prover/implpackage.These constants are duplicated locally from the upstream impl package. Verify that CHACHA20, AES_128, AES_256, CHACHA20_OPRF, AES_128_OPRF, and AES_256_OPRF maintain the same values (0-5 respectively) in the external dependency to prevent subtle mismatches during proving operations.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/results_build.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (1)
client/results_build.go (1)
23-24: Improved error message specificity.The updated error handling provides clearer feedback with "Attestation validation failed" instead of separate checks for signatures and general validation. This aligns well with the consolidated validation approach.
Note
Implements on-demand ZK circuit initialization and tightens protocol success validation.
client/zkprover.go: algorithm ID/cipher mapping, init-state tracking,SetZKInitCallback,InitAlgorithmWithTracking, andEnsureAlgorithmInitialized; invoked fromclient/oprf.gobeforeProvelib/libreclaim.go: exposeSetZKInitCallback,GetAlgorithmID,GetCipherName,IsAlgorithmInitialized; route init via tracking wrapper; bridge C callback; add CGO shimsdemo_lib,demo_standalone): set up lazy-loading callbacks that load circuit files and initialize algorithms on demand; adjust LDFLAGS pathclient/results_build.gototranscripts.BothReceived && validation.AttestationValidation.OverallValid && c.responseProcessingSuccessful; update error messages accordinglygithub.com/reclaimprotocol/jsonpathplus-go/v2and bump deps ingo.mod/go.sumWritten by Cursor Bugbot for commit 7eecbcf. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.