Fix an overflow bug in the square root implementation on 32-bit platforms

[daira]

Fix a bug on 32-bit platforms that could cause the square root implementation to return an incorrect result.

[codecov-commenter]

Codecov Report

Base: 59.88% // Head: 71.07% // Increases project coverage by +11.19% :tada:

Coverage data is based on head (1bdc094) compared to base (db83057). Patch has no changes to coverable lines.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #63 +/- ## =========================================== + Coverage 59.88% 71.07% +11.19% =========================================== Files 10 10 Lines 1199 1179 -20 =========================================== + Hits 718 838 +120 + Misses 481 341 -140 ``` | [Impacted Files](https://codecov.io/gh/zcash/pasta_curves/pull/63?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash) | Coverage Δ | | |---|---|---| | [src/arithmetic/fields.rs](https://codecov.io/gh/zcash/pasta_curves/pull/63?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL2FyaXRobWV0aWMvZmllbGRzLnJz) | `93.90% <ø> (+29.01%)` | :arrow_up: | | [src/fields/fp.rs](https://codecov.io/gh/zcash/pasta_curves/pull/63?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL2ZpZWxkcy9mcC5ycw==) | `82.52% <ø> (+6.45%)` | :arrow_up: | | [src/fields/fq.rs](https://codecov.io/gh/zcash/pasta_curves/pull/63?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL2ZpZWxkcy9mcS5ycw==) | `81.78% <ø> (+6.42%)` | :arrow_up: | | [src/hashtocurve.rs](https://codecov.io/gh/zcash/pasta_curves/pull/63?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL2hhc2h0b2N1cnZlLnJz) | `70.14% <0.00%> (-1.50%)` | :arrow_down: | | [src/macros.rs](https://codecov.io/gh/zcash/pasta_curves/pull/63?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL21hY3Jvcy5ycw==) | `51.61% <0.00%> (ø)` | | | [src/arithmetic/curves.rs](https://codecov.io/gh/zcash/pasta_curves/pull/63?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL2FyaXRobWV0aWMvY3VydmVzLnJz) | `0.00% <0.00%> (ø)` | | | [src/lib.rs](https://codecov.io/gh/zcash/pasta_curves/pull/63?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL2xpYi5ycw==) | | | | [src/serde\_impl.rs](https://codecov.io/gh/zcash/pasta_curves/pull/63?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL3NlcmRlX2ltcGwucnM=) | `83.33% <0.00%> (ø)` | | | ... and [3 more](https://codecov.io/gh/zcash/pasta_curves/pull/63?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash) | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[daira]

error: package `csv v1.2.0` cannot be built because it requires rustc 1.60 or newer, while the currently active rustc version is 1.56.0

Do we need to increase this crate's MSRV?

[str4d]

Do we need to increase this crate's MSRV?

Not for this issue (we can fix it by restricting the dev-dependencies). I'm doing so in #65.

However, this other issue is more annoying:

error: package `constant_time_eq v0.2.4` cannot be built because it requires rustc 1.59 or newer, while the currently active rustc version is 1.56.0

This is because blake2b_simd 1.0.1 bumped this dependency, raising its MSRV to 1.59. Technically users can still use this crate with our current MSRV by pinning blake2b_simd 1.0.0, but doing that in our Cargo.toml would be inappropriate (as it would prevent people running newer Rust versions from using the newer dependencies which they would be perfectly able to do). The problem of course is that our CI system both doesn't pin dependencies in a Cargo.lock (per library crates), and also tests against MSRV.

IIRC there's something we can do that is akin to "test against minimal dependency versions", but I recall hearing issues with it last year. I'll have a look and see if things have changed. In any case, we will want to release a pasta_curves 0.6.0 with a bumped MSRV, but we can and probably should still release this bugfix as pasta_curves 0.5.1 even if we can't make CI happy.

[str4d]

IIRC there's something we can do that is akin to "test against minimal dependency versions", but I recall hearing issues with it last year. I'll have a look and see if things have changed.

Ah yes:

Note: It is not recommended to use this feature. Because it enforces minimal versions for all transitive dependencies, its usefulness is limited since not all external dependencies declare proper lower version bounds. It is intended that it will be changed in the future to only enforce minimal versions for direct dependencies.

I think what we should do is merge #65, rebase this PR on it to ensure that the checks that can pass (namely the --no-default-features checks) still pass, and then make a PR on top of this that bumps MSRV to ensure that all tests pass there before merging this PR.

[str4d]

I've verified that we can reproduce this issue in CI here: https://github.com/zcash/pasta_curves/actions/runs/4315844857/jobs/7530796304

I'll move the relevant commit to #65 (where it won't reproduce because the bug only occurs with the alloc feature flag, which brings in blake2b_simd), and then when this PR is rebased I can confirm that it fixes the problem with the MSRV PR on top.