Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat DivModNSafeDivPlusOne hint (ec hint) #551

Merged
merged 35 commits into from
Jul 24, 2024

Conversation

danielcdz
Copy link
Contributor

@danielcdz danielcdz commented Jul 18, 2024

Summary

Closes #527
Closes #532

Copy link
Member

@TAdev0 TAdev0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielcdz just went through your PR quickly. It looks good, indeed all variables are *big.Int in scope, assigned in DivModNPackedDivmodV1 hint .

can you also add in cairo_zero_hint_tests folder a copy of this file ?

https://github.com/lambdaclass/cairo-vm/blob/db7fff18c9c1312024ebf4119fcdfeba23657b12/cairo_programs/div_mod_n.cairo#L29

you can run it with make integration command, which will check whether runnning this program with our vm and python one produces the same memory and trace files or not.

@danielcdz
Copy link
Contributor Author

Hey @TAdev0, when I run make integration I get these errors:
image
Do you know how I can solve those issues? I already have installed the cairo-compile command

@TAdev0
Copy link
Member

TAdev0 commented Jul 18, 2024

Mmh don't know. Can you use Cairo compile and Cairo run without issue? Maybe Python version ? Did you follow the read me to install everything properly

@danielcdz
Copy link
Contributor Author

Mmh don't know. Can you use Cairo compile and Cairo run without issue? Maybe Python version ? Did you follow the read me to install everything properly

I found what the issue was, I forgot to activate the python venv where I have installed the cairo 0 compiler dependencies

@danielcdz
Copy link
Contributor Author

@TAdev0 Now I have this error:
image
Is related to the hint implementation in Go? or what does it means exactly 🤔

@TAdev0
Copy link
Member

TAdev0 commented Jul 19, 2024

Can you successfully run all other integration tests ? If no , it's a setup issue

@danielcdz
Copy link
Contributor Author

@TAdev0 Here is a better view to the results:
image
I think other integration test are running as expected

@danielcdz
Copy link
Contributor Author

I changed the name of the new file to .small.cairo and now It returns this:
image

@TAdev0
Copy link
Member

TAdev0 commented Jul 19, 2024

OOOh thats perfect!!

https://github.com/lambdaclass/cairo-vm/blob/db7fff18c9c1312024ebf4119fcdfeba23657b12/cairo_programs/div_mod_n.cairo#L29

this is because we also miss this hint :

    %{
        from starkware.cairo.common.cairo_secp.secp_utils import pack
        from starkware.python.math_utils import div_mod, safe_div

        a = pack(ids.a, PRIME)
        b = pack(ids.b, PRIME)
        value = res = div_mod(a, b, N)
    %}

but your hint works

@TAdev0
Copy link
Member

TAdev0 commented Jul 19, 2024

@danielcdz can you also work on this hint and add it to this PR?

#532

so that the integration test :
https://github.com/lambdaclass/cairo-vm/blob/db7fff18c9c1312024ebf4119fcdfeba23657b12/cairo_programs/div_mod_n.cairo

will work entirely

@danielcdz
Copy link
Contributor Author

@TAdev0 for sure!

@danielcdz
Copy link
Contributor Author

@TAdev0 All the integration tests are passing now!
image

@danielcdz danielcdz requested a review from TAdev0 July 20, 2024 22:12
Copy link
Member

@TAdev0 TAdev0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work overall @danielcdz !!!
Just left a few minor comments and then we merge !

pkg/hintrunner/zero/zerohint_ec.go Outdated Show resolved Hide resolved
pkg/hintrunner/zero/zerohint_ec.go Show resolved Hide resolved
pkg/hintrunner/zero/zerohint_ec_test.go Outdated Show resolved Hide resolved
@TAdev0
Copy link
Member

TAdev0 commented Jul 23, 2024

@danielcdz can you resolve conflicts before final review?

pkg/hintrunner/zero/zerohint_ec.go Outdated Show resolved Hide resolved
pkg/hintrunner/zero/zerohint_ec.go Outdated Show resolved Hide resolved
@danielcdz danielcdz requested a review from TAdev0 July 23, 2024 18:55
@TAdev0
Copy link
Member

TAdev0 commented Jul 23, 2024

You still need to add value variable in scope. Here it works because as we saw there are 2 hints in a row but the first one is only used to import N and actually adds value variable in scope, it's second one that is actually tested (the one you implemented). Yours should add value in scope as well (and overwrite the previous one in the test)

What I meant in dm is that you should create a copy of the variable you assign to res , and assign it to value

@TAdev0
Copy link
Member

TAdev0 commented Jul 24, 2024

@danielcdz you can simply use new(big.Int) and BigInt method to make a local copy instead of doing the computation 2 times

@TAdev0 TAdev0 merged commit 8947731 into NethermindEth:main Jul 24, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DivModNPackedDivmodExternalN hint (secp hint) DivModNSafeDivPlusOne hint (ec hint)
3 participants