feat: Replace secp_utils usage of big.Int with uint256 library.

[TAdev0]

No description provided.

[TAdev0]

@rodrigo-pino i have a few questions :

should i also modify SecPPacked and SecPSplit functions?

Also, GetSecPBig for example now returns a uint256.Int, i guess i should also modify the code where this utils is used (i.e., zerohint_ec.go and zerohint_signature(s).go ) ?

[rodrigo-pino]

Hey @TAdev0! Yes please add both functions since their values are never bigger than the 256 bits space. Regarding the code that uses them, it must be updated as well since we need the CI to pass in order to merge.

In case the consecuences of using uint64 library are very hard to integrate we can do smth good enough now and follow it up with another PR focused on that

[TAdev0]

@rodrigo-pino ready for a first review. Not sure everything is correct with my syntax.

[rodrigo-pino]

There are some errors. Especially compilations one. Could you please fix them.

Also you can test most of the CI by doing make unit.
This will compile & run all unit tests.

[TAdev0]

@rodrigo-pino yeah thank you! i'm installing Go right now, will do that.

[TAdev0]

@rodrigo-pino I have residual mismatch issues, for example:

   zerohint_utils_test.go:163: value scope value mismatch:
            have: 25711014748331347274616151948719391133326249308066029554892800
            want: 25711014748331348032841661176477624755444844903972403171819520

I double checked that i correctly defined constants in GetSecP , getBase and GetSecp256R1_P.

I suspect that the issue comes from SecPPacked as it is the common denominator for all failing tests. But i'm struggling debugging efficiently, i checked if it could come from DivMod which behaves differently with BigInt and uint256, but it seems ok.

[rodrigo-pino]

I suspect that the issue comes from SecPPacked as it is the common denominator for all failing tests. But i'm struggling debugging efficiently, i checked if it could come from DivMod which behaves differently with BigInt and uint256, but it seems ok.

I see that's weird, maybe something to do with references? My tip for debugging would be testing the failing functions one by one with some prints scattered around. You'll eventually find the wrong piece of code

[TAdev0]

@rodrigo-pino comments addressed. I still need to figure out this issue.

[rodrigo-pino]

Ok left some other small ones, regarding the issue, I'll try to look it later. Can you debug it manually with prints?

[TAdev0]

@rodrigo-pino ok so basically, i fail for all EcNegate tests (and others, but i think the root cause is the same). These tests call newEcNegateHint , which will use GetSecPUint256 and SecPPacked among other stuff i didnt change.

GetSecPUint256 returns a const which should be fine.
I tried printing different variables and indeed, the result value of newEcNegateHint, which is yBig , is always a little bit different, with generally 8 identical bytes and the rest different.

yBig is the NEG + MOD result of SecPPacked , and SecPPacked seems ok as well, using getBaseUint256 .

I even tried keeping BigInt for SecPPacked computation, without any modification, and i still have these errors.

[har777]

I did a super quick check. In SecPPacked before returning the packedBig variable I tried converting it to a uint256 and then back to a big.Int and tests failed.

Before:

return *packedBig, nil

After:

packedUint256 := new(uint256.Int)
packedUint256.SetFromBig(packedBig)

return *packedUint256.ToBig(), nil

The failing tests seem to be using quite large values though. Could it be the test values are just unrealistic? Though I would say the results should match the python vm regardless.

[rodrigo-pino]

By looking at the python VM code you immediately realize that input values are always less than 256 bits, and output values are always smaller as well. This means that assuming the current implementation is correct, the one with big.Ints was flawed. It is possible the tests are actually not returning a correct value by using wrong inputs.

The python and the go vm should always return the same value, but if the python vm expect always it's input to be in the 256 bits range, whatever we throw at it bigger than it is not valid for comparison. We are loooking to match the python vm in the 256 bit range not on bigger stuff, assuming of course, that to the python VM you could feed bigger stuff, to which there is not test to show it.

[rodrigo-pino]

If there is a test that inputs a number bigger than 256 bits or returns as an output a number bigger than 256 bits, that test is not correct.

[TAdev0]

The first failing seems to be related to this code:

	if num.Cmp(uint256.NewInt(0)) != 0 {
		return nil, fmt.Errorf("num != 0")
	}

error is not thrown while it should
The 2nd error is related to this code:

			if rUint256.Cmp(uint256.NewInt(0)) != 0 {
				return fmt.Errorf("verify_zero: Invalid input (%v, %v, %v)", valValues[0], valValues[1], valValues[2])
			}

Their might be an issue related to the Cmp version for uint256?

[har777]

I dont think so. I tried changing just that part:

num2 := new(uint256.Int)
num2.SetFromBig(num)
if num2.Cmp(uint256.NewInt(0)) != 0 {
  return nil, fmt.Errorf("num != 0")
}

And it passed.

[TAdev0]

I also tried for DivMod it passes as well

[har777]

Lets just remove the test. I was trying to make the results always be consistent with the python vm. If we are using uint256 then might as well assume the value will always be a uint256 written to scope. The test which fails makes no sense in that context.

[TAdev0]

@har777 you want to remove all failing tests? what is the reason for the 2 others failing? I cannot find any reason for now.

[har777]

@har777 you want to remove all failing tests? what is the reason for the 2 others failing? I cannot find any reason for now.

Just this one:

    --- FAIL: TestZeroHintEc/NondetBigint3V1_3 (0.00s)
        zerohint_utils_test.go:195: expected an error containing "num != 0", got nil err

[TAdev0]

mmh i dont understand. If i remove this test, i still have many tests failing.

[TAdev0]

@rodrigo-pino @har777 I annotated all failing tests so that tests pass for the CI.

For now, I use uint256 instead of bigint everywhere for utils

It includes :

• a bunch of EcNegate
• one FastEcAddAssignNewX test
• one GetPointFromX test
• one VerifyZero test

[TAdev0]

@rodrigo-pino comments addressed. I left the type where it makes sense to leave it, and remove in all other places

[TAdev0]

@rodrigo-pino had a few conflicts, i have other tests failing now. It is getting a bit difficult to understand whats going on, maybe should we close this and do it once all hints are implemented? Or maybe merging this soon, because there will be additionnal conflicts after PR are merged

[rodrigo-pino]

Hey @TAdev0 the solution is good, let's put it on hold until we can asses with clarity the uint256 vs BigInt situation

[TAdev0]

sure

[cicr99]

I'm adding the blocked label here, so we know we won't be working on this rn

[cicr99]

We want to wait until the hints are all merged, this means there are going to be a lot of conflicts to fix and a lot more of code to modify. I'm closing this PR for the moment, we can open a new one with fresh changes