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 #457 utils.MaxSlice and utils.MinSlice #473

Merged
merged 2 commits into from
Jun 7, 2024
Merged

Conversation

romainbou
Copy link
Contributor

Addressing #457

  • Fix utils.MaxSlice and utils.MinSlice returning 0
  • Fix EqualSlice if slices have difference lengths
  • Add tests for slices utils

…es utils

Co-authored-by: Jean-Philippe Bossuat <jean-philippe@tuneinsight.com>
@romainbou romainbou requested a review from qantik May 14, 2024 15:37
@romainbou romainbou self-assigned this May 14, 2024
@Pro7ech
Copy link
Collaborator

Pro7ech commented May 16, 2024

EqualSlice is not constant time anymore after these changes

@romainbou
Copy link
Contributor Author

EqualSlice is not constant time anymore after these changes

Ah yes, but before it would also panic early depending on the size of b.

It's essentially the implementation of the standard library. (available since Go 1.21, we could use it once we drop older Go version support) https://cs.opensource.google/go/go/+/refs/tags/go1.22.3:src/slices/slices.go;l=18

The only critical use is here:

if !utils.EqualSlice(shareOut.BaseTwoDecompositionVectorSize(), crp.BaseTwoDecompositionVectorSize()) {

Do you think it's really a concern?

@Pro7ech
Copy link
Collaborator

Pro7ech commented May 17, 2024

EqualSlice is not constant time anymore after these changes

Ah yes, but before it would also panic early depending on the size of b.

It's essentially the implementation of the standard library. (available since Go 1.21, we could use it once we drop older Go version support) https://cs.opensource.google/go/go/+/refs/tags/go1.22.3:src/slices/slices.go;l=18

The only critical use is here:

if !utils.EqualSlice(shareOut.BaseTwoDecompositionVectorSize(), crp.BaseTwoDecompositionVectorSize()) {

Do you think it's really a concern?

To me it is not a concern since it is not used in critical code and but its behavior should then be documented (this is a cryptographic library). Since Go 1.21+ provides native support for this kind of utility, shouldn't it then be removed/replaced by it?

@romainbou romainbou merged commit 2dde0ae into master Jun 7, 2024
6 checks passed
@romainbou romainbou deleted the 457-fix-slices-utils branch September 25, 2024 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants