Feat: add getHighLen hint

coxmars commented 4 months ago

Implement getHighLen Hint

Description

This PR implements the getHighLen hint for the CairoVM in Go. The hint calculates the highest bit length of two variables and assigns the result to ids.len_hi.

Changes Applied

Added getHighLenCode string to hintcode.go.
Updated zerohint.go to handle the new hint string.
Implemented the getHighLen hint logic in zerohint_ec.go.
Created createGetHighLenHinter function to return the new hint.

Pending Tasks

Unit tests for the getHighLen hint in zerohint_ec_test.go.
Integration tests by copying the highest_bitlen.cairo file into the cairo_zero_hint_tests folder.

Note

I plan to add the necessary unit and integration tests soon. I would greatly appreciate your feedback on the current implementation.

Resolves #558

MaksymMalicki commented 4 months ago

Also, once you resolve the comments, make sure to add unit tests and resolve the conflicts

MaksymMalicki commented 4 months ago

Hey @coxmars, are you still working on this PR? Haven't heard from you in a couple of days 😛

coxmars commented 4 months ago

Hey @coxmars, are you still working on this PR? Haven't heard from you in a couple of days 😛

Yes sir, sorry for the delay I will send de PR today probably 🫡

coxmars commented 4 months ago

Hi @MaksymMalicki @TAdev0 I applied the comments, if I need to change/improve something else just let me know and I will do it, thanks 🫡

MaksymMalicki commented 4 months ago

Looks very good overall, good job! 🚀 Make sure to copy this integration test to the integration tests folder and make sure it works. You run the integration tests using make integration command. If it works, then I will give an approve!

EDIT: I see that the CI has thrown some errors. Make sure to run unit tests using make unit command and fix all the problems!

coxmars commented 4 months ago

Looks very good overall, good job! 🚀 Make sure to copy this integration test to the integration tests folder and make sure it works. You run the integration tests using make integration command. If it works, then I will give an approve!

EDIT: I see that the CI has thrown some errors. Make sure to run unit tests using make unit command and fix all the problems!

Thank you I will do it, if I get an error or obstacle I will let you know

coxmars commented 4 months ago

Hi @MaksymMalicki @TAdev0 I needed to fix typo, syntaxis errors and added integration test, however, I'm facing this error so I'm trying to solve it but for the moment I sent this changes. If you have suggestions about this issue could be great 🫡

MaksymMalicki commented 4 months ago

Hi @MaksymMalicki @TAdev0 I needed to fix typo, syntaxis errors and added integration test, however, I'm facing this error so I'm trying to solve it but for the moment I sent this changes. If you have suggestions about this issue could be great 🫡

Yes, in this line in unit tests: newGetHighLenHint(ctx.operanders["len_hi"], ctx.operanders["scalar_u"], ctx.operanders["scalar_v"]) you need to pass ctx.operanders["scalar_u.d0"] and ctx.operanders["scalar_v.d0"] instead of ctx.operanders["scalar_u"] and ctx.operanders["scalar_v"], as you are creating the references with those names in the operanders field. scalar_u.d0 represents an address to the first of the three cells. Treat this as reference: https://github.com/NethermindEth/cairo-vm-go/blob/ff34f199a27e80ced54c97ad05777de4eb8a8b6e/pkg/hintrunner/zero/zerohint_ec_test.go#L24

coxmars commented 4 months ago

Hi @MaksymMalicki @TAdev0 I needed to fix typo, syntaxis errors and added integration test, however, I'm facing this error so I'm trying to solve it but for the moment I sent this changes. If you have suggestions about this issue could be great 🫡

Yes, in this line in unit tests: newGetHighLenHint(ctx.operanders["len_hi"], ctx.operanders["scalar_u"], ctx.operanders["scalar_v"]) you need to pass ctx.operanders["scalar_u.d0"] and ctx.operanders["scalar_v.d0"] instead of ctx.operanders["scalar_u"] and ctx.operanders["scalar_v"], as you are creating the references with those names in the operanders field. scalar_u.d0 represents an address to the first of the three cells. Treat this as reference:

https://github.com/NethermindEth/cairo-vm-go/blob/ff34f199a27e80ced54c97ad05777de4eb8a8b6e/pkg/hintrunner/zero/zerohint_ec_test.go#L24

Thanks sir, now I think unit test is working as you can see below

TAdev0 commented 4 months ago

hi @coxmars integration is failing for this cairo line

    assert get_highlen(BigInt3(0, 0, 8), BigInt3(0, 0, 0)) = 3;

get_highlen cairo function returns 250 instead of 3 . There is probably a logic error in your hint

TAdev0 commented 4 months ago

@coxmars just added fmt.Println(bitLenU) and ran integration tests, indeed bitLenU returns 251

i think calling BitLen on a field is not a good idea, because a small value will be represente as a huge number as a felt. This is why we get 251 bits to represent the value 8 as a felt

TAdev0 commented 4 months ago

@coxmars convert to big int first before calculating the bit length and it will work :

                    var scalarUD2 big.Int
            scalarUD2 = *scalarUValues[2].BigInt(&scalarUD2)

            var scalarVD2 big.Int
            scalarVD2 = *scalarVValues[2].BigInt(&scalarVD2)

            bitLenU := scalarUD2.BitLen()
            bitLenV := scalarVD2.BitLen()

            lenHi := utils.Max(bitLenU, bitLenV) - 1

=== RUN   TestCairoZeroFiles
    cairozero_test.go:57: testing: cairo_zero_hint_tests/highest_bitlen.small.cairo
--- PASS: TestCairoZeroFiles (1.76s)
PASS
ok      github.com/NethermindEth/cairo-vm-go/integration_tests  1.969s

coxmars commented 4 months ago

lgtm!

Thanks for the help and feedback guys, I really appreciate it 🫡🤝🏼 @MaksymMalicki @TAdev0

NethermindEth / cairo-vm-go