refactor: Improve codebase #490
Conversation
|
|
|
🔒 Could not start CI tests due to missing safe PR label. Please contact a DEDIS maintainer. |
1 similar comment
|
🔒 Could not start CI tests due to missing safe PR label. Please contact a DEDIS maintainer. |
share/dkg/pedersen/dkg.go
Outdated
| ver, _ := d.verifiers[dd.Index] | ||
| ver, ok := d.verifiers[dd.Index] | ||
| if !ok { | ||
| return nil, fmt.Errorf("verifiers not exists") |
There was a problem hiding this comment.
| return nil, fmt.Errorf("verifiers not exists") | |
| return nil, fmt.Errorf("missing verifiers") |
|
Kudos, SonarCloud Quality Gate passed! |
|
Please add a short description to the PR, and then I think it's good to go. |
|
Added! |
share/poly.go
Outdated
| @@ -35,7 +35,7 @@ type PriShare struct { | |||
| func (p *PriShare) Hash(s kyber.HashFactory) []byte { | |||
| h := s.Hash() | |||
| _, _ = p.V.MarshalTo(h) | |||
| _ = binary.Write(h, binary.LittleEndian, p.I) | |||
| _ = binary.Write(h, binary.LittleEndian, int32(p.I)) | |||
There was a problem hiding this comment.
Looking at it once more: could this be a breaking change? If the current int is 64 bit, now it will hash it as 32 bits.
It is definitely a good idea to cast the int here, I'm just not sure whether it will break protocols having used this with 64 bits. Any idea?
There was a problem hiding this comment.
I think int is architecture-dependent, which means both 32 and 64 bits should work. But it is definitely better to use a type with clear width
There was a problem hiding this comment.
Yes, that's what https://go.dev/tour/basics/11 says, too. So if a binary for a 64-bit platform used this library before the change, it will have used a 64 bit representation of p.I for the hashing. With this change it will use a 32 bit representation, and the comparison will fail, e.g., in a signature.
Same thing if you use int64 - for a 32 bit platform, this might make it not working anymore. So I'm trying to figure out if somebody out there actually relies on this, and cannot come up with a good way of deciding whether yes, or no :)
@pierluca or @jbsv, what is your take on this? Creating a new major version of kyber would be the safest, but it seems a bit too much, except if we do it for all hashes of ints.
There was a problem hiding this comment.
I am curious how about changing the datatype of p.I here? I think that should be the easier and safest too.
There was a problem hiding this comment.
Yes, hindsight is 20/20, that is what I would do now. But only after seeing problems like this and working for a couple of years with Rust :)
Now this change would be a breaking change, because then a lot of code would not even compile anymore. So let's wait for @pierluca 's return and see what he thinks. I'm currently thinking of keeping the p.I there without a cast. Which is wrong, but at least not in a breaking way!
If ever there is a version 4 of kyber, this is something that would need to be addressed at a moment...
There was a problem hiding this comment.
Apologies for the delay. First of all, thanks @howjmay for the good suggestions in this PR. Regarding this int32, I can only agree with @ineiti: it is a breaking change.
Furthermore, we're most commonly using this library on x64 systems on our end, so this would be very likely to break a few things.
This would indeed make sense for a version 4 of Kyber, for which right now we don't really have the resources. On the other hand, I think we're piling up quite a bit of changes that could justify a version 4, if the resources allow it on our side into 2024 (or there's interest on your side, @ineiti ), we should definitely plan for it.
There was a problem hiding this comment.
@ineiti So I think if I remove this change, the rest of the change is ok for merging?
There was a problem hiding this comment.
Yes, here and on line 307. Both need to be removed.
|
Kudos, SonarCloud Quality Gate passed! |
Improve error check and slice operations