Merging the drand fork #509

Robingoumaz · 2024-04-01T13:45:25Z

This PR is about merging the drand fork back into the dedis dyber.

Many files only got minimal changes, (comments, typos, moved to another folder) and some had bigger changes applied to them.

added index of signature

returning err only if not enough valid partials

* Implementing IBE on G2 & adapting tests for it * Adding CPA on G1 * Removing exposed insecure hash function from mod Int * Use iterative hashing also avoid loosing a bit before rehashing

* Closes #43 that was actually already fixed in #45, but this also fixes the issue from #374 * Adding table tests for MinimumT

Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.7.0 to 0.17.0. - [Commits](golang/crypto@v0.7.0...v0.17.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* bounds check when comparing polynomials * added extra check of threshold * fixing comment on PubPoly Equal being constant time --------- Co-authored-by: Yolan Romailler <[email protected]>

* Adding support for BLS12381 using CIRCL. * Register suite for bls12381. * Renaming to circl_bls12381 --------- Co-authored-by: Yolan Romailler <[email protected]>

Bumps [github.com/cloudflare/circl](https://github.com/cloudflare/circl) from 1.3.2 to 1.3.7. - [Release notes](https://github.com/cloudflare/circl/releases) - [Commits](cloudflare/circl@v1.3.2...v1.3.7) --- updated-dependencies: - dependency-name: github.com/cloudflare/circl dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Add support for signatures on G2 to bdn * Adapt bdn_test.go to new BDN interface * Make API functions methods of the Scheme type * Generalize G1- and G2-related comments * Fixing bdn tests * Make the BDN test suite generic over suite and scheme * Keeping public APIs from v1 --------- Co-authored-by: Yolan Romailler <[email protected]> Co-authored-by: Jakub Sztandera <[email protected]>

* implement ethereum bn254 * pull in missing changes from geth * make G2 points affine in ValidatePairing * move bn254 to own dir * fix bn254 GT Base and Null * test G1 suite against gnark-crypto * adapt G2 test to gnark-crypto, fix bn254 marshal ids * fix G1 hashToPoint reference tests * fix pairing/bn254/gfp_amd64.s * use keccak256 instead of sha256 in bn254 * implement TestGT for bn254 * implement hashToField, expandMsgXmd * implement mapToPoint * implement hashToPoint * rename toPointG1 -> fromBigInt * add comments on mapToPoint, cleanup * fix TestPointG1_HashToPoint with new keccak256 impl * use ref implementation of expand_message_xmd * panic if DST length > 255 * remove unnecessary iteration in expandMsgXmd * update tests to use full length DSTs * set DST in group instead of suite * fix: actually write bytes in zeroPadBytes * fix: leftpad intermediate prb in expandMsgXmd * fix: leftpad intermediate xored hashes in expandMsgXmd * add instructions to create hash-to-point reference values, remove old TODO * remove irrelevant issue400 test for bn254 * use CX instead of R15 in amd64 replaying fix from geth, see: ethereum/go-ethereum@ec64358 When using -buildmode=shared, R15 is clobbered by a global variable access; use a different register instead. * remove gfp.h in bn254 * use geth's Unmarshal func: error out on invalid coordinates * make constants z0 and z1 private * check that random points are different in TestG1Ops * implement Stringer interface for bn254 groups * remove irrelevant benchmarks * implement SVDW from RFC9380 for bn254 G1 map-to-point * encode the right variable in hashToField * borrow expand_message_xmd implementation from kilic-bls12381 * update test vectors in TestPointG1_HashToPoint, TestHashToField * test expandMsgXmd against a copy of gnark's * fix default DST for hash-to-curve * remove unused fromBigInt func * add sources for gfp exp/sqrt functions * use curveB constant in g(x) * create new DST buffers instead of passing refs

Still has an error in the ibe_test.go file

Corrected some of the comments that still mentioned wrong paths

+ Small formatting changes

…ing the merge

sonarcloud · 2024-04-04T19:59:56Z

Quality Gate failed

Failed conditions
41.1% Duplication on New Code (required ≤ 10%)

See analysis details on SonarCloud

AnomalRoil · 2024-04-05T16:31:05Z

encrypt/ibe/ibe_test.go

@@ -1,5 +1,6 @@
 package ibe

+/*


Why comment out the entire IBE tests code?

I uncommented them and used the circl bls12381 instead but there is a difference between the scalar of circl bls and the one of the other pairing curve which makes it not pass the tests. I am trying to change the circl implementation so that it uses mod.Int

The conversion is not needed if rejection sampling is updated as in:
https://github.com/Robingoumaz/kyber-drand/pull/1

AnomalRoil · 2024-04-05T16:31:48Z

examples/dkg_test.go

Not sure why we would want to delete this file?

sign/test/bls_test.go

AnomalRoil · 2024-04-05T16:36:52Z

hash.go

+
+// HashablePoint is an interface implemented by n
+type HashablePoint interface {
+	Hash([]byte) Point


Since we're doing a V4, one thing worth discussing is probably this:
drand#50

AnomalRoil · 2024-04-05T16:47:16Z

pairing/bn254/gfp_decl.go

+
+var hasBMI2 = cpu.X86.HasBMI2
+
+// go:noescape


you could disable this linter for this file in the ./.golangci.yml config file since it seems to apply to the whole file

xof/blake2xs/blake.go

xof/keccak/keccak.go

…cl.go" files from the linter

ineiti · 2024-04-15T06:23:28Z

This is a huge PR. Unless you want to keep up with the drand branch of kyber, I propose to split it in different PRs:

one for changes in comments / typos
one PR per existing and new module

This would make it more manageable, but would also make it difficult for future exchanges between the drand fork and dedis/kyber.

AnomalRoil · 2024-04-15T09:41:25Z

@ineiti the goal is to get rid of the drand/kyber fork and only have one unique kyber repo :D

ineiti · 2024-04-15T11:02:40Z

@ineiti the goal is to get rid of the drand/kyber fork and only have one unique kyber repo :D

Then I would definitely pack this up in several easy-to-digest PRs.

AnomalRoil · 2024-04-15T14:11:43Z

share/dkg/dkg.go

Here I think you should stick with the file structure from dedis/kyber and keep both Pedersen and Rabin subfolders in share/dkg/ instead of deleting Rabin and replacing everything with drand implementation.

Okay, so @nikkolasg told me actually that the Pedersen dedis/kyber is not the same implementation as the drand/kyber, the latter being the secure one, while the former might be somewhat faulty.

So I think we should 100% replace the dedis/kyber/share/dkg/pedersen with the drand/kyber/share/dkg code.
I don't have strong opinion about keeping or not the Rabin one, but he told me that one is very slow, which means it's inferior to Pedersen.
We could either:

keep both and make sure to benchmark them to warn users cc @matteosz

remove Rabin and keep only the drand one, which is what this PR is currently doing.

Preferences @pierluca ?

I vote for having both, and adding a benchmark is a good idea. Except if keeping the kyber dkg is a lot of work.

Thinking of kyber as a research library, so it's nice to have various algorithms for comparisons, or in case somebody has a brilliant idea to update one of the algorithms.

AnomalRoil · 2024-04-15T14:12:18Z

share/dkg/rabin/dkg.go

See my comment about keeping both pedersen and rabin packages, but having the drand dkg move to the pedersen folder rather than deleting and replacing with drand version

AnomalRoil · 2024-04-15T14:13:07Z

sign/dss/dss.go

Why deleting it?

AnomalRoil · 2024-04-15T14:14:35Z

util/random/rand.go

 	buff := make([]byte, readerBytes)
 	for _, reader := range r.Readers {
 		n, err := io.ReadFull(reader, buff)
 		if err != nil {
 			nerr++
+			errors = append(errors, err.Error())
 		}
 		b.Write(buff[:n])
 	}

 	// we are ok with few sources being insecure (i.e., providing less than
 	// readerBytes bytes), but not all of them
 	if nerr == len(r.Readers) {


you could get rid of nerr and use len(errors) instead here.

AnomalRoil · 2024-04-15T14:16:30Z

sign/test/threshold.go

This doesn't seem to conform to the _test nomenclature for Go tests and functions in it also don't use the proper TestXYZ format.

Please replace to use the proper Go test framework.

To do in a next PR

AnomalRoil · 2024-04-15T14:17:19Z

sign/test/scheme.go

same remark about it not being a proper test package as per the Go test nomenclature. Need _test.go in file name and TestName functions

To do in a next PR

AnomalRoil · 2024-04-15T14:25:25Z

sign/bls/bls_test.go

Why commenting it all?

pierluca · 2024-04-29T07:43:36Z

I see that the tests failed, which means Sonar wasn't run.
It might be worth it to bypass the tests once (in the CI) to let Sonar run and gather some feedback.
Sonar might be able to make a first pass to spot problematic issues.
Alternatively, you could also setup Sonar to analyze your own repo and provide a pointer here :)

nikkolasg and others added 30 commits December 28, 2019 17:31

signed and go.mod

34a525d

full replacement as per indication golang/go#30354 (comment)

7fe8641

external scheme test

c01e8ef

tbls and bdn adapted with scheme

84ec45b

extracted threshold scheme

53f989c

extended test for threshold

558f79c

go.sum changed

240720a

removing bn256 from pairing/

594491c

moved bn256 bls to bn256 package

0770c5d

removing pairing from suites

8de27ed

dkg returns an error when reader fails

22dd74e

updated building code

9e5bf6a

fully commit

b2458fe

building

d227ada

correct badge

52819db

new dkg shape

ed3c998

intermediate commit

be362b2

added index of signature

d5ec052

Merge pull request #1 from drand/index_tbls

30e90cc

added index of signature

returning err only if not enough valid partials

5c9abe5

Merge pull request #2 from drand/tbls

965fd62

returning err only if not enough valid partials

intermediate commit

eaa304a

using correct circleci config

19de0c1

simple dkg and threshold tests passing

0365a65

test resharing done

dae2766

full protocol test passing

38c5f3d

removed pedersen

b548f49

removed dss since unused for now

fba05b4

fast sync tested

e8fc438

additional test

d0711a1

AnomalRoil and others added 19 commits April 4, 2024 13:45

Updating dependencies (#47)

c458de1

Implementing IBE on G2 (#48)

841acc7

* Implementing IBE on G2 & adapting tests for it * Adding CPA on G1 * Removing exposed insecure hash function from mod Int * Use iterative hashing also avoid loosing a bit before rehashing

Fix MinimumT everywhere (#51)

0ba2d88

* Closes #43 that was actually already fixed in #45, but this also fixes the issue from #374 * Adding table tests for MinimumT

fixed nil check on nodes in DKG (#54)

359bf0f

bounds check comparing polynomials (#56)

ac6eb87

* bounds check when comparing polynomials * added extra check of threshold * fixing comment on PubPoly Equal being constant time --------- Co-authored-by: Yolan Romailler <[email protected]>

pairing: Adds support for BLS12381 using CIRCL library (#49)

3b2e0fc

* Adding support for BLS12381 using CIRCL. * Register suite for bls12381. * Renaming to circl_bls12381 --------- Co-authored-by: Yolan Romailler <[email protected]>

updating dependencies

03aea73

Changes to imports of all files.

3c05b97

Still has an error in the ibe_test.go file

Commented the ibe_test.go file

9a99479

Corrected some of the comments that still mentioned wrong paths

Commented the bls_test.go file

4612b62

go mod tidy

cfb9b8d

Changes to the makefile and readme because of the conflicts.

3110fcf

+ Small formatting changes

Merge branch 'master' of github.com:Robingoumaz/kyber-drand

9629a04

Format + error fixing after the merge of the master into the fork

8f039bf

going back to dedis makefile + fixing a function that got chopped dur…

1f8da36

…ing the merge

AnomalRoil reviewed Apr 5, 2024

View reviewed changes

Uncommented tests and fixed the errors that arised. Excluded the "_de…

16a0920

…cl.go" files from the linter

AnomalRoil reviewed Apr 15, 2024

View reviewed changes

AnomalRoil changed the base branch from master to drandmerge April 29, 2024 12:17

AnomalRoil merged commit 16a0920 into dedis:drandmerge Apr 29, 2024
5 of 9 checks passed

AnomalRoil mentioned this pull request Apr 29, 2024

Final merge PR for drand/kyber #512

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merging the drand fork #509

Merging the drand fork #509

Robingoumaz commented Apr 1, 2024

sonarcloud bot commented Apr 4, 2024

AnomalRoil Apr 5, 2024

Robingoumaz Apr 14, 2024

armfazh Apr 25, 2024

AnomalRoil Apr 5, 2024

AnomalRoil Apr 5, 2024

AnomalRoil Apr 5, 2024

ineiti commented Apr 15, 2024

AnomalRoil commented Apr 15, 2024

ineiti commented Apr 15, 2024

AnomalRoil Apr 15, 2024

AnomalRoil Apr 15, 2024

ineiti Apr 16, 2024

AnomalRoil Apr 15, 2024

AnomalRoil Apr 15, 2024

AnomalRoil Apr 15, 2024

AnomalRoil Apr 15, 2024

AnomalRoil Apr 29, 2024

AnomalRoil Apr 15, 2024

AnomalRoil Apr 29, 2024

AnomalRoil Apr 15, 2024

pierluca commented Apr 29, 2024

Merging the drand fork #509

Merging the drand fork #509

Conversation

Robingoumaz commented Apr 1, 2024

sonarcloud bot commented Apr 4, 2024

Quality Gate failed

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

ineiti commented Apr 15, 2024

AnomalRoil commented Apr 15, 2024

ineiti commented Apr 15, 2024

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

pierluca commented Apr 29, 2024