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

Fix IBE in the drand merge #514

Merged
merged 15 commits into from
May 22, 2024
Merged

Fix IBE in the drand merge #514

merged 15 commits into from
May 22, 2024

Conversation

K1li4nL
Copy link
Contributor

@K1li4nL K1li4nL commented May 7, 2024

This pull request aims to make the IBE implementation support different pairing suite. To that end it is necessary to add a couple functions to the Scalar interface.

Edit: cherry-picked the dkg bug-fix commit to make the CI happy.

@K1li4nL K1li4nL added the safe PR label May 7, 2024
@K1li4nL K1li4nL changed the base branch from drandmerge to bls12381 May 8, 2024 16:49
* Fix a typo
* BugFix: use array index instead of node index
@K1li4nL K1li4nL marked this pull request as ready for review May 11, 2024 14:25
encrypt/ibe/ibe_test.go Show resolved Hide resolved
group/mod/int.go Outdated

// GroupOrder returns the order of the underlying group
func (i *Int) GroupOrder() *big.Int {
return big.NewInt(0).SetBytes(i.M.Bytes())
Copy link
Contributor

Choose a reason for hiding this comment

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

why not returning a clone of i.M ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if you mean:

Suggested change
return big.NewInt(0).SetBytes(i.M.Bytes())
return i.Clone().(*Int).M

but Clone() doesn't do a deep copy of M, this may be a problem if one were to accidently modify it, for instance the Add method of big.Int modify the underlying big number.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, Clone should probably be doing a deep copy, that might be a bug.
but then

func (z *Int) Set(x *Int) *Int

is probably what to use instead of SetBytes, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

much more concise, sounds good to me, I can look into fixing Clone() as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Please do in another PR

pairing/bls12381/circl/scalar.go Outdated Show resolved Hide resolved
group/mod/int.go Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented May 16, 2024

@K1li4nL K1li4nL requested a review from AnomalRoil May 16, 2024 09:30
@AnomalRoil AnomalRoil merged commit 31e421b into bls12381 May 22, 2024
8 checks passed
@AnomalRoil AnomalRoil deleted the kilian-fix-ibe branch May 22, 2024 09:02
AnomalRoil added a commit that referenced this pull request Jun 13, 2024
* Added circl and kilic implementation of bls12381 + benchmarks
* Added deserialization tests compressed vectors +more comments
* Fix IBE in the drand merge (#514)
* Add ByteOrder() function + constant from int.go
* Update circl_bls12381 and edwards25519 scalars
* Add Order() to the scalar interface
* Adapt IBE to make it general
* Fix dkg bug (#515)
* BugFix: use array index instead of node index
* Fix endianess bool
* Add tests for endianess
* Fixed deserialization tests + circl issue
* Removed groupchecker iface
* Added back circl deserialization test (cloudflare/circl#499)
* Since Go 1.21 go mod tidy require the go directive to match the highest of our dependencies

---------

Co-authored-by: Kilian <79536516+K1li4nL@users.noreply.github.com>
Co-authored-by: Yolan Romailler <anomalroil@users.noreply.github.com>
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.

2 participants