Update PoW benchmarks and mitigation #79
Conversation
WalkthroughDocumentation and benchmarks standardize on "Keccak256", introduce a PoW‑masked accept/reject design and delegated‑solver economics, replace a placeholder GPU benchmark with a real Keccak256 CUDA harness, add a plan and minor Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Solver as Off‑chain Solver (GPU)
participant Commit as Committer (On‑chain)
participant Verifier as On‑chain Verifier
Client->>Commit: submit commit (x, randao)
Commit-->>Client: return commit receipt (includes randao)
Client->>Solver: request solution attempts (x, optional mask M)
Solver->>Solver: perform Keccak256 PoW trials
Solver-->>Client: return solution + nonce (+ proof)
Client->>Commit: submit solution + nonce (optionally masked with M)
Commit->>Verifier: verify PoW-derived mask M and solution validity
Verifier-->>Commit: accept / reject
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @igor53627, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the accuracy and comprehensiveness of the project's security and performance documentation. It integrates real-world GPU benchmarks for the Hash-PoW mechanism, introduces a novel security mitigation to strengthen the accept/reject process, and provides a detailed economic analysis for delegated solvers. The changes aim to offer a clearer and more robust understanding of the system's security properties and operational costs. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request provides a comprehensive update to the project's Proof-of-Work benchmarks and documentation. The most significant change is the replacement of a placeholder hash in the GPU benchmark script with a full, realistic Keccak256 implementation in CUDA. This allows for accurate performance measurements, and the results have been propagated consistently across all documentation, including the README, implementation/security guides, and LaTeX papers. The PR also introduces and documents the "PoW-masked accept/reject" mitigation strategy and adds an economic analysis of delegated solvers. The overall quality of the changes is high, with a clear focus on correctness and consistency. I have one suggestion to improve the maintainability of the updated benchmark script.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/gpu_benchmark/modal_hashpow_gpu.py`:
- Around line 340-365: The loops that compute t using (1ULL << d) risk undefined
overflow when d >= 64; update the code that iterates over diffs/num_diffs to
validate each difficulty before shifting (check that d is within a safe range,
e.g., 0 <= d && d < 64) and handle out-of-range values by either
rejecting/printing an error or computing the value with a floating-point-safe
method (e.g., use pow(2.0, d) or ldexp) instead of (1ULL << d); apply this
change for all places using (1ULL << d) (the three locations that compute t for
single GPU, 50 GPUs, and the per-difficulty JSON) and ensure any negative d is
also handled.
🧹 Nitpick comments (2)
tests/gpu_benchmark/modal_hashpow_gpu.py (1)
160-180: Potential edge case inclz8when input is zero.When
v == 0, the__clzintrinsic returns 32 for a 32-bit input, so__clz(0) - 24 = 8, which is correct (8 leading zeros in a zero byte). However,__builtin_clz(0)has undefined behavior in C/C++ per the standard. While this works in practice on most compilers, consider adding a guard.🛡️ Optional: Guard against undefined behavior
__device__ __host__ static inline int clz8(uint8_t v) { + if (v == 0) return 8; `#ifdef` __CUDA_ARCH__ return __clz((unsigned int)v) - 24; `#else` return __builtin_clz((unsigned int)v) - 24; `#endif` }docs/SECURITY.md (1)
103-108: Minor table formatting inconsistency.The markdownlint tool flagged spacing issues in the table pipes. While this doesn't affect rendering in most Markdown viewers, consistent spacing improves readability in raw form.
✨ Optional: Consistent table spacing
-| Dictionary size N | eps = 1% | eps = 5% | eps = 10% | -|-------------------|--------|--------|---------| -| 1M | $7,839 | $39,193 | $78,386 | +| Dictionary size N | eps = 1% | eps = 5% | eps = 10% | +|-------------------|----------|-----------|-----------| +| 1M | $7,839 | $39,193 | $78,386 |
|
Addressed review comments: added difficulty range validation (0–63) before shifting and added clz8(0) guard in tests/gpu_benchmark/modal_hashpow_gpu.py; normalized delegated-solver table spacing in docs/SECURITY.md. Latest commit: 9641e1e. Please re-review and resolve the threads when ready. |
|
Clippy warnings fixed and formatting retained. Latest commit: 7463fdf. Test/format results: cargo fmt; cargo clippy (clean); cargo test (19 passed, 3 ignored). Please re-review and resolve remaining threads. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/security/lattice_estimator.rs (2)
411-416:⚠️ Potential issue | 🟡 MinorStale comment: σ=8 should be σ=25.
The comment on line 412 states "σ=8" but
estimate_main_lwe_security()usesstddev: 25.0. Update the comment to match the implementation.📝 Fix the comment
- // Main LWE: n=384, q=65521, σ=8 should give ~112 bits (exact mode) + // Main LWE: n=384, q=65521, σ=25 should give ~112 bits (exact mode)
418-426:⚠️ Potential issue | 🟡 MinorStale comment: parameters don't match implementation.
Lines 421-422 state "Puzzle LWE: n=48, q=2039, ternary secret" but
puzzle_lwe_params()returns n=128 with uniform error in [-2, 2] (not ternary secret). Update the comment to reflect actual parameters.📝 Fix the comment
- // Puzzle LWE: n=48, q=2039, ternary secret - // Lattice attacks give ~32 bits (expected), but security model is 3^48 search space + // Puzzle LWE: n=128, q=2039, uniform secret, uniform error in [-2, 2] + // Lattice attacks give low bits, but security model is brute-force search space
🤖 Fix all issues with AI agents
In `@src/security/lattice_estimator.rs`:
- Around line 74-91: The DiscreteGaussianAlpha arm in
Distribution::DiscreteGaussianAlpha builds JSON in separate branches so the n
branch overwrites mean when both Some; update the logic in the
Distribution::DiscreteGaussianAlpha match arm to construct a single string that
includes alpha and conditionally appends both mean and n when present (check the
mean and n Option values and compose one formatted string instead of reassigning
s twice), referencing the variables alpha, mean, and n so the resulting
serialized string contains all provided fields (alpha plus mean and/or n)
without dropping any.
🧹 Nitpick comments (1)
src/bin/generate_wycheproof_vectors.rs (1)
882-891: Consider consolidating duplicated helper functions.These helper functions are identical to the private implementations in
src/lwe.rs(lines 112-122). While the duplication is justified since those are module-private, consider making thempub(crate)inlwe.rsand importing here to maintain a single source of truth.This is low-risk given the trivial implementations, but consolidation would align with the project structure where
lwe.rsis the canonical location for LWE-related utilities.Based on learnings: "Use Rust implementation files: ... lwe.rs for Layer 2 LWE encryption."
|
Added DiscreteGaussianAlpha serialization fix (mean+n) in commit a54701b. Checks: cargo fmt; cargo clippy (clean); cargo test (19 passed, 3 ignored). |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/security/lattice_estimator.rs (1)
287-303:⚠️ Potential issue | 🟡 MinorUpdate main LWE doc comment to σ=25 to match code/spec.
The comment says σ=8 but the implementation uses 25.0; update the doc to avoid confusion.
📝 Suggested fix
-/// Parameters: n=384, q=65521, σ=8 (Gaussian noise), uniform secret. +/// Parameters: n=384, q=65521, σ=25 (Gaussian noise), uniform secret.Based on learnings: Production LWE layer must use parameters: n=384 dimension, σ=25 Gaussian noise, q=65521 modulus, achieving ~2^112 PQ security.
🧹 Nitpick comments (1)
src/security/lattice_estimator.rs (1)
47-71: SimplifyDiscreteGaussianJSON construction to avoid repeated reformatting.The current logic rebuilds the string multiple times; you can match the incremental builder style used below for
DiscreteGaussianAlphato reduce churn and keep behavior the same.♻️ Suggested refactor
- Distribution::DiscreteGaussian { stddev, mean, n } => { - let mut s = format!( - r#"{{"distribution":"discrete_gaussian","stddev":{}}}"#, - stddev - ); - if let Some(m) = mean { - s = format!( - r#"{{"distribution":"discrete_gaussian","stddev":{},"mean":{}}}"#, - stddev, m - ); - } - if let Some(dim) = n { - s = format!( - r#"{{"distribution":"discrete_gaussian","stddev":{},"n":{}}}"#, - stddev, dim - ); - } - if mean.is_some() && n.is_some() { - s = format!( - r#"{{"distribution":"discrete_gaussian","stddev":{},"mean":{},"n":{}}}"#, - stddev, - mean.unwrap(), - n.unwrap() - ); - } - s - } + Distribution::DiscreteGaussian { stddev, mean, n } => { + let mut s = format!( + r#"{{"distribution":"discrete_gaussian","stddev":{}"#, + stddev + ); + if let Some(m) = mean { + s.push_str(&format!(r#","mean":{}"#, m)); + } + if let Some(dim) = n { + s.push_str(&format!(r#","n":{}"#, dim)); + } + s.push('}'); + s + }
Summary
Test Plan
Note
Low Risk
Primarily documentation and benchmarking-script changes; no on-chain contract or core protocol logic is modified, so functional risk is minimal aside from potential benchmark-number correctness.
Overview
Documentation and paper updates: Refreshes Hash-PoW benchmark claims to measured H200 Keccak256 throughput (and recalculates derived solve times/dictionary attack costs), standardizes wording to “Keccak256,” and updates the stance on delegated solvers to optional with new D=40 economics tables.
Security notes added: Documents the new “PoW-masked accept/reject” mitigation (with explicit side-channel limitations) across
README.md,docs/IMPLEMENTATION.md,docs/SECURITY.md, and both LaTeX papers.Benchmark tooling: Reworks
tests/gpu_benchmark/modal_hashpow_gpu.pyto run a real Keccak256 CUDA benchmark on Modal H200 (self-test, configurable difficulties, JSON output), adds a docs execution plan (docs/plans/...), and tweaks.gitignoreto ignoretmp/.*.Written by Cursor Bugbot for commit 2b1b6b1. Configure here.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.