Conversation
|
@microsoft-github-policy-service agree |
| Ok(DoryBlind) | ||
| } | ||
|
|
||
| fn prove( |
There was a problem hiding this comment.
We need a PCS evaluation argument where claimed values are committed.
There was a problem hiding this comment.
Current understanding:
- Spartan stores
eval_Win plaintext inSpartanSNARKstruct (line 107, spartan.rs) - Spartan recomputes
comm_eval_W = PCS::commit([eval_W])in verifier - My current
DoryEvaluationArgumentduplicates the value invalue_bytes
Question: Should I:
- Remove
value_bytesfromDoryEvaluationArgumententirely? - If so, how should
verify()obtain the value to callquarks-zk::verify_eval(value, ...)? - Should I deserialize from
comm_evalparameter, or is the value expected to come from Spartan's proof struct?
Looking at Hyrax-PC, the IPA.verify() implicitly validates the value through the inner product check. Dory's verify_eval() explicitly needs the value parameter.
Could you clarify the expected flow for getting the evaluation value to the PCS verifier?
Thanks
There was a problem hiding this comment.
Current implementation stores the claimed value in DoryEvaluationArgument.value (native ArkFr).
The value is implicitly committed through the comm_eval parameter:
- Prover computes comm_eval = PCS::commit([eval_W], blind) (spartan.rs:306)
- Verifier receives both comm_eval and arg.value
- Verifier can check commit([arg.value]) == comm_eval for binding
Is this the pattern you're expecting, or should we handle it differently?
There was a problem hiding this comment.
Spartan ZK variant does not store evaluation values in plaintext.
Addresses review comment microsoft#1 from @srinathsetty Changes: - Updated quarks-zk to v0.1.2 with rerandomization support - Implemented rerandomize_commitment using Vega paper approach - Added h_gt generator to DoryCommitmentKey for rerandomization - Added 3 rigorous tests validating unlinkability and correctness Reference: Vega paper (eprint 2025/2094) Section 2.1
…ument - Store DoryPCSEvaluationProof and ArkFr natively (not Vec<u8>) - Custom serde implementation using ark_serialize - Addresses review microsoft#3: no hacky manual serialization
Apologies for the noise. Removed: - examples/benchmarks/** (criterion-generated files) - PR_DESCRIPTION.md (local reference) These were included unintentionally in the previous commit.
(n as f64).log2().ceil() is imprecise at power-of-two boundaries (e.g. n=4096 could yield 11.999... -> 12 or 12.000... -> 12). Use (usize::BITS - (n-1).leading_zeros()) which is exact for all inputs and avoids any floating-point dependency in cryptographic code.
DoryPCSParams doesn't implement Serialize/Deserialize, so it can't be stored in the commitment key. Since setup uses a fixed seed (ChaCha20Rng::seed_from_u64(0)), params for a given num_vars are deterministic. We cache them in a static OnceLock<Mutex<HashMap>> keyed by num_vars, computing them once and cloning on reuse. This eliminates redundant pairing-heavy SRS generation that was happening on every commit(), prove(), and verify() call — the main contributor to the ~9.5s prove bottleneck reported in benchmarks.
Replace inline QuarksDoryPCS::setup() call with get_dory_params(). commit() was regenerating the full SRS on every call — now it hits the cache after the first invocation for a given num_vars.
Replace inline QuarksDoryPCS::setup() call with get_dory_params(). prove() was the worst offender — regenerating the full BLS12-381 SRS before every proof generation, accounting for most of the ~9.5s spent in PCS prove.
Replace inline QuarksDoryPCS::setup() call with get_dory_params(). The verifier no longer pays SRS generation cost on every verify call.
Byte concatenation produced invalid commitments that would fail deserialization in verify(). Now we deserialize each DoryPCSCommitment, multiply their tier2 elements in GT (additive notation in ark), and reserialize. For single-commitment inputs (most circuits), this is a zero-cost clone.
Old test asserted byte length grew (concat behavior). New tests: - Single commitment: verify pass-through identity - Multiple commitments: verify result is a valid GT element and differs from both inputs (non-trivial multiplication)
Was only checking bytes.is_empty(). Now attempts full CanonicalDeserialize into DoryPCSCommitment, catching truncated, corrupted, or malformed commitment bytes early.
Complements the empty-bytes test: verifies that arbitrary non-GT bytes (0xDEADBEEF) are caught by deserialization validation.
Serialization failure is not an input length problem — it's an internal error in the commitment pipeline. Use InternalError.
Deserialization failures are InvalidPCS (corrupt cryptographic data). Serialization failure after rerandomize is InternalError (should never happen with valid inputs).
Corrupt commitment bytes are a PCS-level error, not input length.
A failed verification is not an input length problem — it's a proof verification error. ProofVerifyError is the canonical variant for this in the SpartanError enum.
Machine-specific benchmark data doesn't belong in the repository. Results are not reproducible across hardware and go stale quickly. The PR description already contains the benchmark summary.
quarks-zk 0.1.5 depends on ark-ff/ark-serialize/ark-bls12-381 0.5. Having 0.4 in Cargo.toml caused two incompatible versions of ArkFr in the dependency graph, making all type conversions between Spartan2's ark types and quarks-zk's ark types fail at compile time.
When setup was extracted to get_dory_params(), the local rng variable was lost. prove_eval() still needs an rng for proof generation. Restore it with the same deterministic seed.
Use explicit reassignment (a = a + b) instead of +=. quarks-zk's Bls381GT implements Add (GT multiplication over Fq12) but not the compound assignment variant.
|
@srinathsetty Thanks for the thorough review. I've pushed a batch of fixes addressing the issues raised plus additional problems found during a deeper audit. Addressing your review commentsRe: comment #1 — Rerandomization (Vega §2.1)
This was addressed in Re: comment #3 — Native types instead of pre-serialized bytes
Addressed in Re: comment #2 — Committed evaluation values
This is the remaining architectural question. Currently For the ZK variant where
Happy to discuss the preferred approach. This is the one open item. Additional fixes in this pushPerformance — params cache (
Correctness — Was concatenating serialized bytes, which produces invalid commitments that fail deserialization. Now deserializes each Correctness — Was only checking Correctness —
Correctness — error types ( All error paths were using
Dependencies — ark- version bump (
Cleanup — removed Machine-specific data that goes stale. The PR description already has the benchmark summary. All 36 tests pass (20 BLS12-381 provider + 16 Dory-PC adapter). Compiles clean with and without |
Add BLS12-381 Curve Provider and Dory-PC Backend
Summary
This PR adds support for:
Motivation
BLS12-381 is widely used in production systems (Ethereum 2.0, Zcash) and offers strong security guarantees. Dory-PC provides O(1) commitment size and O(log n) verification.
Changes
New Files
src/provider/bls12_381.rs- BLS12-381 curve implementation viahalo2curvessrc/provider/pcs/dory_pc.rs- Dory-PC adapter wrappingquarks-zkcratebenches/pcs_comparison.rs- Benchmark comparing Hyrax vs Doryexamples/sha256_bls12_381_dory.rs- SHA-256 circuit example with Dory-PCexamples/BENCHMARK_RESULTS.md- Benchmark resultsModified Files
src/provider/mod.rs- RegisterBLS12381HyraxEngineandBLS12381DoryEnginesrc/provider/pcs/mod.rs- Exportdory_pcmoduleCargo.toml- Add dependencies (quarks-zk,ark-*crates)New Engines
Benchmark Results (Simple Circuit)
SHA-256 Circuit Results (Heavy Circuit, 131K+ constraints)
Performance Observations
What works well:
Current bottlenecks:
Potential optimizations (not in scope for this PR):
Assessment
This is a research-grade implementation. The numbers are expected for:
For production use, significant optimization work would be needed.
Testing
cargo test --features dory cargo bench --bench pcs_comparison --features dory cargo run --release --example sha256_bls12_381_dory --features doryDependencies
New optional