fix: Eisenstein Half-GCD convergence

[feltroidprime]

No description provided.

[ivokub]

Looks good to me - maybe we could get rid of the conditional "best-norm remainder" loop and do it always?

Otherwise - if there is good reason not to do it, then I'm good to merge it. I also added unit test for quorem. I wasn't able to come up with a regression test quickly though for inputs where the remainder norm would be bigger than the divisor - do you have a test-case by any chance? Its fine if not.

[feltroidprime]

Looks good to me - maybe we could get rid of the conditional "best-norm remainder" loop and do it always?

Otherwise - if there is good reason not to do it, then I'm good to merge it. I also added unit test for quorem. I wasn't able to come up with a regression test quickly though for inputs where the remainder norm would be bigger than the divisor - do you have a test-case by any chance? Its fine if not.

Hey one example of (slow/infinite) convergence input for half gcd was (from halfgcd hint with (order - 2) as scalar for secpk1) :
200k iterations in the half gcd is not enough for it (i dont know if it converges at all)

a = 64502973549206556628585045361533709077 - 303414439467246543595250775667605759171*ω
b = -432420386565659656852420866390673177323 + 238911465918039986966665730306072050094*ω

Regarding the while loop, I think I agree, it does seem one pass through all neighbours is enough the make the norm smaller and we could replace it with a if r.Norm().Cmp(norm) >= 0 instead of for r.Norm().Cmp(norm) >= 0

Regarding passing through all neighbors each time, i think the initial guess will be correct most of the time and therefore it's a small optimization to not do it - but i didn't benchmarked it.

[ivokub]

I benchmarked locally quickly - when we always find the smallest neighbour in the QuoRem.

goos: linux
goarch: amd64
pkg: github.com/consensys/gnark-crypto/field/eisenstein
cpu: AMD Ryzen 9 7940HS w/ Radeon 780M Graphics     
           │   old.txt   │                new.txt                │
           │   sec/op    │    sec/op     vs base                 │
HalfGCD-16   73.64µ ± 6%   209.48µ ± 4%  +184.48% (p=0.000 n=20)
QuoRem-16    3.095µ ± 1%    6.668µ ± 1%  +115.48% (p=0.000 n=20)
geomean      15.10µ         37.37µ       +147.59%

           │   old.txt    │                new.txt                 │
           │     B/op     │     B/op       vs base                 │
HalfGCD-16   87.02Ki ± 5%   253.11Ki ± 5%  +190.85% (p=0.000 n=20)
QuoRem-16    2.157Ki ± 0%    6.415Ki ± 0%  +197.37% (p=0.000 n=20)
geomean      13.70Ki         40.29Ki       +194.10%

           │   old.txt   │               new.txt                │
           │  allocs/op  │  allocs/op   vs base                 │
HalfGCD-16   1.792k ± 5%   5.694k ± 5%  +217.81% (p=0.000 n=20)
QuoRem-16     47.00 ± 0%   145.00 ± 0%  +208.51% (p=0.000 n=20)
geomean       290.2         908.6       +213.12%

I guess lets stay with only finding smallest neighbour in case the norm is big (i.e. what you have right now). I only changed from for loop to if.

[ivokub]

Thanks for the fix. Looks good!