chore: Remove Params concept #1541

ledwards2225 · 2023-08-11T21:09:09Z

The Params concept, originally introduced by Adrian, was outdated and largely superseded by the Flavor concept. It previously specified the Curve, various Curve subtypes, and defined CommitmentKey and VerificationKey for different PCS suites. The information contained in Params was redundant and confusing and led to issues in my work on the recursive verifier. This work:

Defines CommitmentKey and VerificationKey in unique files as single classes templated by Curve with appropriate specializations
Removes the concept of Params altogether (along with the poorly named commitment_key.hpp). Things previously templated by Params are now templated by Curve. (The new commitment_key.hpp now just contains.. the CommitmentKey)
Specifies the Curve, CommitmentKey, and PCSVerificationKey directly in the Flavor classes

Checklist:

Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.

If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
Every change is related to the PR description.
I have linked this pull request to relevant issues (if any exist).

ludamad

LGTM. When you say concept do you just mean common type shape passed throughout? I was somewhat expecting e.g. template <CurveLike Curve> with a CurveLike C++ concept

ledwards2225 · 2023-08-12T00:22:38Z

LGTM. When you say concept do you just mean common type shape passed throughout? I was somewhat expecting e.g. template <CurveLike Curve> with a CurveLike C++ concept

Hah no I meant it in the colloquial sense. I was conscious of the potential confusion as I was typing the word. Probably time I drop it from my vernacular.

ludamad · 2023-08-12T00:29:12Z

Hah no I meant it in the colloquial sense. I was conscious of the potential confusion as I was typing the word.
Probably time I drop it from my vernacular.

At least when discussing template parameters maybe :D

maramihali

Really great work cleaning this up :) I just had some comments relating documentation and naming.

maramihali · 2023-08-11T21:45:02Z

circuits/cpp/barretenberg/cpp/src/barretenberg/honk/composer/ultra_composer.hpp

-    using PCSCommitmentKey = typename PCSParams::CommitmentKey;
-    using PCSVerificationKey = typename PCSParams::VerificationKey;
+    using PCSCommitmentKey = typename Flavor::CommitmentKey;
+    using PCSVerificationKey = typename Flavor::PCSVerificationKey;


same here about naming

maramihali · 2023-08-11T21:55:36Z

circuits/cpp/barretenberg/cpp/src/barretenberg/honk/flavor/goblin_ultra.hpp

    using GroupElement = Curve::Element;
    using Commitment = Curve::AffineElement;
    using CommitmentHandle = Curve::AffineElement;
    using FF = Curve::ScalarField;
    using Polynomial = barretenberg::Polynomial<FF>;
    using PolynomialHandle = std::span<FF>;
+    using CommitmentKey = pcs::CommitmentKey<Curve>;


Can we have either both of these name with or without the PCS prefix (I think we can do without without clashing with anything else).

Decided to change these to CommitmentKey and VerifierCommitmentKey. We already have a VerificationKey so that language is ambiguous even with the PCS prefix. The verifier is actually using this object to compute commitments (in the full sense in IPA but also in Shplonk) so I think the name is appropriate and more descriptive. What do you think?

sure, sounds reasonable!

maramihali · 2023-08-11T21:56:06Z

circuits/cpp/barretenberg/cpp/src/barretenberg/honk/flavor/standard.hpp

    using GroupElement = Curve::Element;
    using Commitment = Curve::AffineElement;
    using CommitmentHandle = Curve::AffineElement;
    using FF = Curve::ScalarField;
    using Polynomial = barretenberg::Polynomial<FF>;
    using PolynomialHandle = std::span<FF>;
+    using CommitmentKey = pcs::CommitmentKey<Curve>;
+    using PCSVerificationKey = pcs::VerificationKey<Curve>;


circuits/cpp/barretenberg/cpp/src/barretenberg/honk/pcs/claim.hpp

maramihali · 2023-08-11T21:58:55Z

circuits/cpp/barretenberg/cpp/src/barretenberg/honk/pcs/commitment_key.hpp

@@ -23,239 +23,55 @@

 namespace proof_system::honk::pcs {

-namespace kzg {
+/**
+ * @brief CommitmentKey object over a pairing group 𝔾₁, using a structured reference string (SRS).


This is only applicable for KZG, for IPA the srs is random points in G_1

maramihali · 2023-08-11T22:00:14Z

circuits/cpp/barretenberg/cpp/src/barretenberg/honk/pcs/commitment_key.hpp


    /**
-     * @brief Simulates a KZG CommitmentKey, but where we know the secret trapdoor
-     * which allows us to commit to polynomials using a single group multiplication.
+     * @brief Uses the ProverSRS to create a commitment to p(X)


Same here, there are comments about the SRS for IPA in the ipa namespace of the previous version.

Corrected. thanks

maramihali · 2023-08-12T11:34:59Z

circuits/cpp/barretenberg/cpp/src/barretenberg/honk/pcs/gemini/gemini.hpp

-    using Commitment = typename Params::Commitment;
+template <typename Curve> class GeminiVerifier_ {
+    using Fr = typename Curve::ScalarField;
+    using GroupElement = typename Curve::Element;


I don't have strong opinions between Element and GroupElement for the projective form. I found both of them rather unintuitive. I much prefer ProjectiveElement and AffineElement but this is open for discussion

yeah I was just going with what was there. I don't have strong feelings either so unless you do I;ll just leave it for now

ledwards2225 changed the title ~~Lde/params refactor~~ chore: Lde/params refactor Aug 11, 2023

ledwards2225 changed the title ~~chore: Lde/params refactor~~ chore: Remove Params concept Aug 11, 2023

ledwards2225 self-assigned this Aug 11, 2023

ledwards2225 requested a review from maramihali August 11, 2023 21:38

ludamad approved these changes Aug 11, 2023

View reviewed changes

maramihali requested changes Aug 12, 2023

View reviewed changes

ledwards2225 added 7 commits August 12, 2023 13:48

move comm and ver keys to separate classes

d565353

remove CK and VK from Params altogether

b2cea8f

everything templated by Curve instead of Params

0dc738e

commitment key.hpp erradicated

3a5d5f0

fix build error

675d6fd

some cleanup and comments

aa180a4

comments and naming in response to review

9e52249

ledwards2225 force-pushed the lde/params_refactor branch from 6045d4f to 9e52249 Compare August 13, 2023 16:39

iAmMichaelConnor added the crypto cryptography label Aug 14, 2023

maramihali approved these changes Aug 14, 2023

View reviewed changes

ledwards2225 merged commit f4bd85e into master Aug 14, 2023

ledwards2225 deleted the lde/params_refactor branch August 14, 2023 16:32

github-actions bot mentioned this pull request Sep 8, 2023

chore(master): Release 0.5.2 #2141

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Remove Params concept #1541

chore: Remove Params concept #1541

ledwards2225 commented Aug 11, 2023 •

edited

Loading

ludamad left a comment

ledwards2225 commented Aug 12, 2023

ludamad commented Aug 12, 2023 •

edited

Loading

maramihali left a comment

maramihali Aug 11, 2023

maramihali Aug 11, 2023

ledwards2225 Aug 13, 2023

maramihali Aug 14, 2023

maramihali Aug 11, 2023

maramihali Aug 11, 2023

maramihali Aug 11, 2023

ledwards2225 Aug 13, 2023

maramihali Aug 12, 2023

ledwards2225 Aug 13, 2023

chore: Remove Params concept #1541

chore: Remove Params concept #1541

Conversation

ledwards2225 commented Aug 11, 2023 • edited Loading

Checklist:

ludamad left a comment

Choose a reason for hiding this comment

ledwards2225 commented Aug 12, 2023

ludamad commented Aug 12, 2023 • edited Loading

maramihali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ledwards2225 commented Aug 11, 2023 •

edited

Loading

ludamad commented Aug 12, 2023 •

edited

Loading