Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds neff shuffling of sequences #457

Merged
merged 10 commits into from
Nov 30, 2022
Merged

Conversation

nkcr
Copy link
Contributor

@nkcr nkcr commented Nov 23, 2021

@nkcr nkcr self-assigned this Nov 23, 2021
@nkcr nkcr force-pushed the feature/neff-shuffle-sequences branch from 7706572 to d7e49d7 Compare November 23, 2021 14:39
Copy link
Member

@ineiti ineiti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly style issues, but one cryptographic issue with the choice of the random number for the shuffle.

examples/neff_shuffle_test.go Outdated Show resolved Hide resolved
examples/neff_shuffle_test.go Outdated Show resolved Hide resolved
xs := make([]kyber.Point, sequenceLen)
ys := make([]kyber.Point, sequenceLen)

for i := 0; i < sequenceLen; i++ {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using twice the same iterator i, please change one.

Here and in other places.

examples/neff_shuffle_test.go Show resolved Hide resolved
shuffle/sequences.go Outdated Show resolved Hide resolved
for i := 0; i < k; i++ {
c[i] = suite.Scalar().Pick(rand)
C[i] = suite.Point().Mul(c[i], nil)
// fmt.Println(" "+C[i].String())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove debugging strings.

shuffle/shuffle_test.go Show resolved Hide resolved
shuffle/shuffle_test.go Show resolved Hide resolved
shuffle/shuffle_test.go Show resolved Hide resolved
examples/neff_shuffle_test.go Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Jul 8, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

98.5% 98.5% Coverage
0.0% 0.0% Duplication

@CLAassistant
Copy link

CLAassistant commented Nov 14, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link

🔒 Could not start CI tests due to missing safe PR label. Please contact one of the repo maintainers.

Copy link

@si-co si-co left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any error in the crypto, mostly stylistic comments: feel free to integrate them or ignore them if they aren't coherent with the code base.


// This example illustrates how to use the Neff shuffle protocol with simple,
// single pairs.
func Test_Example_Neff_Shuffle_Simple(t *testing.T) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use MixedCaps instead of underscores, but others examples use underscores too, so I guess that's fine. Other examples uses /* ... */ for the example explanation, maybe we want to remain consistent.

cs := make([]kyber.Point, numPairs)

for i := 0; i < numPairs; i++ {
c := suite.Point().Mul(suite.Scalar().Pick(suite.RandomStream()), nil)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd assign the point directly to cs[i], same for ks[i].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad practice, in my opinion. It makes the code less readable.


"github.com/stretchr/testify/require"
"go.dedis.ch/kyber/v3"
kproof "go.dedis.ch/kyber/v3/proof"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you use kproof? I think proof is fine and later we can use p instead of proof.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as a non crypto guy, I highly appreciate when variables are not only single letters :D

//
// Variable names are as representative to the paper as possible. Instead of
// representing (variable name with a bar on top), such as (X with a bar on top)
// with Xbar, we represent it with a repeating letter, such as XX
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it would be easier to use Xbar instead of XX since the paper uses $\overline{X}$. Why going with the repeating letter?


// Fisher–Yates shuffle
for i := k - 1; i > 0; i-- {

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove empty line.

// "Verifiable Mixing (Shuffling) of ElGamal Pairs" by Andrew Neff (April 2004)
//
// The function expects X and Y to be the same dimension, with each row having
// the same length. It also expect X and Y to have at least one element.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we don't enforce this at the beginning of the function? Since the cost of the assertion is negligible, I'd add it.

@nkcr nkcr requested a review from si-co November 29, 2022 16:50
@sonarcloud
Copy link

sonarcloud bot commented Nov 29, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

97.6% 97.6% Coverage
0.0% 0.0% Duplication

@nkcr nkcr merged commit 199132f into master Nov 30, 2022
@nkcr nkcr deleted the feature/neff-shuffle-sequences branch November 30, 2022 07:39
@pierluca pierluca mentioned this pull request Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants