Add blinding scalars #651

moCello · 2021-12-14T09:11:35Z

Add blinding scalars in round 1, 2 and 3 of the proof

codecov · 2021-12-14T09:21:17Z

Codecov Report

Merging #651 (576fdfd) into master (3125d13) will increase coverage by 0.14%.
The diff coverage is 97.12%.

❗ Current head 576fdfd differs from pull request most recent head 6b5e281. Consider uploading reports for the commit 6b5e281 to get more accurate results

@@            Coverage Diff             @@
##           master     #651      +/-   ##
==========================================
+ Coverage   80.62%   80.76%   +0.14%     
==========================================
  Files          54       54              
  Lines        4479     4482       +3     
==========================================
+ Hits         3611     3620       +9     
+ Misses        868      862       -6

Impacted Files	Coverage Δ
src/proof_system/widget.rs	`94.45% <71.42%> (ø)`
src/permutation.rs	`90.00% <77.77%> (-0.44%)`	⬇️
src/circuit.rs	`73.33% <100.00%> (+0.73%)`	⬆️
src/constraint_system/composer.rs	`63.23% <100.00%> (+0.34%)`	⬆️
src/constraint_system/helper.rs	`87.50% <100.00%> (ø)`
src/proof_system/preprocess.rs	`95.85% <100.00%> (ø)`
src/proof_system/prover.rs	`88.60% <100.00%> (+3.06%)`	⬆️
src/proof_system/quotient_poly.rs	`98.87% <100.00%> (-0.24%)`	⬇️
src/proof_system/widget/permutation/proverkey.rs	`97.05% <100.00%> (ø)`
tests/circuit.rs	`80.00% <100.00%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3125d13...6b5e281. Read the comment docs.

src/circuit.rs

moCello · 2022-03-07T07:56:09Z

src/permutation.rs

        &self,
        domain: &EvaluationDomain,
        wires: [&[BlsScalar]; 4],
        beta: &BlsScalar,
        gamma: &BlsScalar,
        sigma_polys: [&Polynomial; 4],
-    ) -> Polynomial {
+    ) -> Vec<dusk_bls12_381::BlsScalar> {


You can use Vec<BlsScalar> and don't have to mention the dusk_bls12_381 module path since it is imported on the top of the file already.

More generally though, why do you not use Polynomial here? We have to convert the evals to to their polynomial form at some point, so why do not do it here?

Agreed with @moCello

the function domain.ifft(w_vec) requires a vector, we could compute it into the function but then in the prover, we need to treat each vector to be blinded in a different way. I think is more clear and clean to put the IFFT into the blind_poly(), and treat all inputs in the same way.

moCello · 2022-03-07T07:56:45Z

src/permutation.rs

@@ -308,7 +308,7 @@ impl Permutation {
        h_2: &[BlsScalar],
        delta: &BlsScalar,
        epsilon: &BlsScalar,
-    ) -> Polynomial {
+    ) -> Vec<dusk_bls12_381::BlsScalar> {


Same as above

src/proof_system/preprocess.rs

moCello · 2022-03-07T08:06:30Z

src/proof_system/prover.rs

+    /// if hiding degree = 2: (b3*X^(n+2) + b2*X(n+1) + b1*X^n - b3*X^2 - b2*X
+    /// - b1) + w_vec
+    pub(crate) fn blind_poly<R: RngCore + CryptoRng>(
+        w_vec: &Vec<dusk_bls12_381::BlsScalar>,


Above dusk_bls12_381::BlsScalar is already imported, so you can simply use &Vec<BlsScalar> here.

moCello · 2022-03-07T08:14:52Z

src/proof_system/prover.rs

+        domain: &EvaluationDomain,
+        mut rng: &mut R,
+    ) -> Polynomial {
+        let mut w_vec_i = domain.ifft(w_vec);


I find the names w_vec and w_vec_i not so descriptive. Isn't w_vec_i just w_poly and w_vec the poly in evaluation form? What about calling them w_poly and w_poly_evals?

I just followed the same naming used in the whole code, we should rename the whole code if we want to change it here... And I used w_vec_i where i comes from inverse. I used this name to distinguish the value we get before calling Polynomial::from_coefficients_vec()

Oh i see, then please call it w_vec_inverse. At first sight I thought that you used the i because you want to loop through the coefficients later on

src/proof_system/prover.rs

src/proof_system/widget/permutation/proverkey.rs

moCello

Left a few comments

vlopes11

Few nits/questions

vlopes11 · 2022-03-07T10:04:14Z

src/circuit.rs

    ) -> Result<Proof, Error> {
-        let (ck, _) = pub_params.trim(self.padded_gates())?;
+        let (ck, _) = pub_params.trim(self.padded_gates() + 6)?;


As @moCello mentioned, we should avoid magic numbers. Since its used in different parts, consider creating a documented constant

vlopes11 · 2022-03-07T10:06:43Z

src/circuit.rs

@@ -306,7 +308,7 @@ where

        // Add ProverKey to Prover
        prover.prover_key = Some(prover_key.clone());
-        prover.prove(&ck)
+        prover.prove(&ck, &mut rng)


We take rng as mut rng: &mut R. Mutable references are copy, even if the referred value isn't copy (they are just pointers).

Suggested change

prover.prove(&ck, &mut rng)

prover.prove(&ck, rng)

The current code works because &mut R is virtually congruent to &mut &mut R, but I believe this is a nit. We shouldn't take the reference as mutable, since we don't change the reference itself, but the data it points to (RNG).

src/circuit.rs

vlopes11 · 2022-03-07T10:07:51Z

src/permutation.rs

        &self,
        domain: &EvaluationDomain,
        wires: [&[BlsScalar]; 4],
        beta: &BlsScalar,
        gamma: &BlsScalar,
        sigma_polys: [&Polynomial; 4],
-    ) -> Polynomial {
+    ) -> Vec<dusk_bls12_381::BlsScalar> {


Agreed with @moCello

src/proof_system/preprocess.rs

src/proof_system/prover.rs

vlopes11

Small non-blocking question; waiting for the comments of @moCello to be addressed in order to approve

src/proof_system/preprocess.rs

moCello

Find the last approach better than this one. I think the decision we need to make is, if we want to perform the ifft before calling blind and have blind in part of the Polynomial or if we perform the ifft in blind and have blind part of the Prover as you had before. Your call :)

moCello · 2022-03-07T12:26:01Z

src/fft/polynomial.rs

+    /// Adds the blinding scalars to a given vector. Always the same elements
+    /// of 'w_vec' are modified at the beginning of it, and appended at the end:
+    /// if hiding degree = 1: (b2*X(n+1) + b1*X^n - b2*X - b1) + w_vec
+    /// if hiding degree = 2: (b3*X^(n+2) + b2*X(n+1) + b1*X^n - b3*X^2 - b2*X


missing the ^ before the (n+1) in both lines

vlopes11

LGTM

This is an important security fix and we might consider including this in testnet. cc @autholykos

moCello assigned xevisalle Dec 14, 2021

moCello added the team:Core Low Level Core Development Team (Rust) label Dec 14, 2021

moCello added this to In progress in PlonkUp Paper via automation Dec 14, 2021

moCello added this to the Matching PlonkUp paper with implementation milestone Dec 14, 2021

moCello linked an issue Dec 14, 2021 that may be closed by this pull request

Add blinding scalars #650

Closed

Blind polynomials

341b8e0

xevisalle force-pushed the blinding branch from ca6b124 to 341b8e0 Compare December 21, 2021 00:04

xevisalle added 2 commits March 5, 2022 19:45

Fix benchmarks

8efda59

Add randomness

23ea8a3

xevisalle added the fix:vulnerability Issues related to fix vulnerabilities of the architecture or software label Mar 5, 2022

xevisalle and others added 2 commits March 5, 2022 21:04

Fix fmt

de1db62

Merge branch 'master' into blinding

7de6fbd

xevisalle marked this pull request as ready for review March 5, 2022 20:28

xevisalle requested review from vlopes11 and autholykos March 5, 2022 20:28

xevisalle moved this from In progress to Review in progress in PlonkUp Paper Mar 5, 2022