Conversation
|
Thanks for the PR! |
| -- TODO is this the right place? | ||
| /-- degree is the mathlib degree + 1 -/ |
There was a problem hiding this comment.
This seems fine to me. Eventually we could have a Lemmas.lean file inside UniPoly folder for stuff like this
@alexanderlhicks That sounds really helpful! Happy to use the review bot as soon as possible. Where can I find more information on that? |
|
/review External: Internal: Comments: |
🤖 AI Review (with external context)\n\nGenerating AI review...Excellent, I will now review the pull request based on the provided information and instructions. Code Review ReportHigh-level SummaryThis pull request refactors the KZG polynomial commitment scheme to use the The PR also introduces a formal correctness proof for the scheme and adds key theorems to However, the PR is currently incomplete, with Correctness with Respect to Reference PaperThe formalization in
The transition to Potential Issues and ImprovementsHere are specific, actionable points for improving the code. 1. Inconsistent Parameterization and Type ErrorsThere appears to be a critical type mismatch in the definition and usage of the File: Issue: def commit (srs : Vector G₁ n) (coeffs : Fin n → ZMod p) : G₁ := ...It takes a structured reference string ( The parameter Suggestion: I recommend changing the signature of --- a/ArkLib/CommitmentScheme/KZG.lean
+++ b/ArkLib/CommitmentScheme/KZG.lean
@@ -48,8 +48,8 @@ def generateSrs (n : ℕ) (a : ZMod p) : Vector G₁ (n + 1) × Vector G₂ 2 :=
/-- To commit to an `n`-tuple of coefficients `coeffs` (corresponding to a polynomial of degree less
than `n`), we compute: `∏ i : Fin n, srs[i] ^ (p.coeff i)` -/
-def commit (srs : Vector G₁ n) (coeffs : Fin n → ZMod p) : G₁ :=
- ∏ i : Fin n, srs[i] ^ (coeffs i).val
+def commit {d : ℕ} (srs : Vector G₁ (d + 1)) (coeffs : Fin (d + 1) → ZMod p) : G₁ :=
+ ∏ i : Fin (d + 1), srs[i] ^ (coeffs i).valThis change will make the parameterization consistent with the SRS generation and the degree of polynomials being committed to. 2. Incomplete ProofsThe PR is blocked by two incomplete proofs. Issue: The theorems Suggestion:
3. Minor Style and Convention IssuesFile: Issue: -- Current
letI poly : UniPoly (ZMod p) := UniPoly.mk (Array.ofFn coeffs)
letI q : UniPoly (ZMod p) := ...
-- Suggested
let poly : UniPoly (ZMod p) := UniPoly.mk (Array.ofFn coeffs)
let q : UniPoly (ZMod p) := ...This is a minor point but improves adherence to Lean conventions. 4.
|
See above for an example :) |
|
/review External: Internal: Comments: |
|
For some reason some new dependency was required ("packaging", presumably stopped being important by something else) which was messing with the summary/review scripts but it should be fixed now. |
|
/review External: Internal: Comments: |
🤖 AI Review (with external context)\n\nGenerating AI review...Excellent, this pull request introduces a formalization of the KZG polynomial commitment scheme. The approach of using the Here is my detailed review with a focus on correctness with respect to the reference paper and the ArkLib framework integration. High-Level SummaryThe pull request successfully implements the core components of the KZG polynomial commitment scheme as described in the reference paper ("PolyCommitDL"). It defines the commitment, witness generation ( The main areas for improvement are the completion of several key correctness proofs, which are currently marked with Formalization CorrectnessThe formalization of the KZG scheme aligns well with the "PolyCommitDL" construction in Section 3.2 of the reference paper.
Actionable SuggestionsThe implementation is structurally sound, but its correctness hinges on completing the proofs. Here are specific, actionable items to address. 1. Complete the
|
|
/review External: Internal: Comments: |
🤖 AI Review (with external context)\n\nGenerating AI review...Hello, I have reviewed the pull request. Here is my expert code review. High-level summaryThis pull request introduces a formalization of the KZG polynomial commitment scheme. The changes include:
The formalization correctly captures the essence of the KZG scheme and demonstrates a good use of the ArkLib library's abstractions for commitment schemes and interactive proofs. However, the PR is not ready to merge due to incomplete proofs. Review of the Formalization and ProofsThe submission correctly models the KZG scheme (referred to as Core Logic
Instantiating the Commitment Scheme
Actionable FeedbackMajor Issues: Incomplete ProofsThe PR is blocked by two incomplete proofs. The scheme cannot be considered correct without them.
Here is an example of how you might structure the theorem correctness (hpG1 : Nat.card G₁ = p) (n : ℕ) (a : ZMod p)
(coeffs : Fin (n + 1) → ZMod p) (z : ZMod p) :
let poly : UniPoly (ZMod p) := UniPoly.mk (Array.ofFn coeffs)
let v : ZMod p := poly.eval z
let (psrs,vsrs) : Vector G₁ (n + 1) × Vector G₂ 2 := generateSrs (g₁:=g₁) (g₂:=g₂) n a
let C : G₁ := commit psrs coeffs
let q : UniPoly (ZMod p) := UniPoly.div (poly - UniPoly.C v) (UniPoly.X - UniPoly.C z)
let opening: G₁ := commit psrs ((coeff q) ∘ Fin.val)
verifyOpening pairing (g₁:=g₁) (g₂:=g₂) vsrs C opening z v := by
-- 1. State polynomial identity
have h_poly_identity : poly - UniPoly.C v = q * (UniPoly.X - UniPoly.C z) := by
-- This will require a proof about UniPoly.div, e.g., using toPoly
sorry
-- 2. Use eval as a homomorphism on the identity
have h_eval_identity : (poly - UniPoly.C v).eval a = (q * (UniPoly.X - UniPoly.C z)).eval a := by
rw [h_poly_identity]
-- Assuming you have lemmas for eval_sub, eval_mul, eval_C, eval_X...
simp only [eval_sub, eval_C, eval_mul, eval_X] at h_eval_identity
-- h_eval_identity is now `poly.eval a - v = q.eval a * (a - z)`
-- 3. Unfold verifyOpening and substitute commitments
unfold verifyOpening
have hC : C = g₁ ^ (poly.eval a).val := by
-- Use commit_eq_UniPoly
sorry
have hO : opening = g₁ ^ (q.eval a).val := by
-- Use commit_eq_UniPoly
sorry
rw [hC, hO]
-- 4. Use bilinearity of pairing to prove the final goal
-- This step involves manipulating group operations and pairing properties
-- to show both sides are equal, using h_eval_identity.
sorryConclusionThis is a valuable contribution that formalizes an important cryptographic primitive. The modeling choices are excellent. The main blocker is the incompleteness of the correctness proofs. Once those are addressed (including any necessary lemmas about |
|
/review External: Internal: Please review the formalization of the Function Binding reduction from the paper’s Proof of Lemma 9.1, with particular attention to the definition
Additionally, please review the formalization of the |
🤖 AI Review (with external context)\n\nGenerating AI review...Excellent, this pull request aims to formalize the KZG polynomial commitment scheme and its function binding security property based on the ARSDH assumption, as detailed in the paper "On the Fiat–Shamir Security of Succinct Arguments from Functional Commitments." This is a significant contribution to the library. My review focuses on the correctness of the formalization against the paper, its integration with the existing High-Level SummaryThe pull request introduces the following key changes:
Overall, the structure is sound, and the formalization closely follows the provided reference material. However, the implementation is currently incomplete, with several key components marked as Conformance with Reference DocumentThe formalization of the function binding property and the structure of the security reduction align well with the definitions and proofs in the provided paper (
Specific, Actionable FeedbackWhile the overall direction is excellent, several areas require attention before this PR can be merged. The most critical issue is the incompleteness of the proofs. 1. Incomplete Proofs and
|
🤖 Gemini PR SummaryThis Pull Request advances the formalization of the KZG (Kate-Zaverucha-Goldberg) polynomial commitment scheme and its security properties. The primary focus is on standardizing the commitment scheme interface to include key generation, formalizing the Adaptive Rational Strong Diffie–Hellman (ARSDH) assumption, and proving the correctness and function binding properties of KZG. Features
Fixes
Refactoring
Documentation
Analysis of Changes
✅ **Removed:** 2 `sorry`(s)
❌ **Added:** 16 `sorry`(s)
✏️ **Affected:** 2 `sorry`(s) (line number changed)
🎨 **Style Guide Adherence**ArkLib/CommitmentScheme/Basic.lean
ArkLib/CommitmentScheme/HardnessAssumptions.lean
ArkLib/CommitmentScheme/KZG.lean
📄 **Per-File Summaries**
Last updated: 2026-02-04 08:09 UTC. |
CommitmentSchemeto use a KeyGen functionCommitmentSchemeCorrectness)CommitmentScheme