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: SECP related hints #1829

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

odesenfans
Copy link
Contributor

Context: port of the Starknet OS to Rust.

We need an implementation of the hints used by the Starknet OS in the SECP syscalls. These hints rely on private primitives in cairo-vm and must be implemented in this repository.

Checklist

  • Linked to Github Issue
  • Unit tests added
  • Integration tests added.
  • This change requires new documentation.
    • Documentation has been added/updated.
    • CHANGELOG has been updated.

Problem: we need an implementation of the hints used by the Starknet OS
in the secp syscalls. These hints rely on private primitives in
`cairo-vm` and need to be implemented here.

Solution: this PR adds an implementation of all the hints that require
`cairo-vm` primitives in the `cairo-vm` repository.
@pefontana
Copy link
Collaborator

Hi @odesenfans !!
Sorry the delay, we didn't saw this PR
Can you merge to main and solve the conflicts?

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 57.26950% with 241 lines in your changes missing coverage. Please review.

Project coverage is 94.31%. Comparing base (63b4a55) to head (5337a25).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...int_processor/builtin_hint_processor/secp/hints.rs 64.57% 169 Missing ⚠️
...int_processor/builtin_hint_processor_definition.rs 14.28% 72 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1829      +/-   ##
==========================================
- Coverage   94.83%   94.31%   -0.53%     
==========================================
  Files         101      102       +1     
  Lines       39579    40142     +563     
==========================================
+ Hits        37536    37858     +322     
- Misses       2043     2284     +241     
Flag Coverage Δ
?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pefontana
Copy link
Collaborator

@odesenfans some more points:

  • Since the VM is only allowed to run whitelisted hints in production we cant enable the VM to execute this hints in production. There are two ways to solve this, leaving the hints under a feature flag like cairo-0-secp-hints`, the other way is that you implement your own hint processor in your repo
  • There are some lints error in the CI
  • There are some unwraps where you can handle the error, like the pop().unwrap(); and some more cases
  • Can you add some integration tests for the new hints?

@whichqua
Copy link

whichqua commented Nov 5, 2024

@pefontana The requested changes and integration tests have been added. Please feel free to run the workflows and review the code.

@pefontana
Copy link
Collaborator

@whichqua sorry for the delay, you can ping me on telegram if I don't respond here https://t.me/Pedrofontana5
Some CI checks are failing

@whichqua
Copy link

@JulianGCalderon @pefontana Let me know if there is anything else needed. Would be nice to get the tests passing.

@JulianGCalderon
Copy link
Contributor

Hi @whichqua, two jobs seem to be failing:

The second job fails because the hint is not found. This is because this job on the CI runs all the standalone files in cairo_programs and compares it's execution. You could try to move these files to another directory cairo_programs/cairo-0-secp-hints-feature and see if it fixes it. Keep in mind that this will probably fix the CI, but it will not fix the underlying error that is breaking the first job.

@whichqua
Copy link

whichqua commented Nov 20, 2024

@JulianGCalderon Is there a reason we cant add the feature cairo-0-secp-hints flag to cairo-vm-cli?
Seems like this is intentional and cairo-1 hints themselves are tested via cairo-1-run.

@JulianGCalderon
Copy link
Contributor

Hi @whichqua, the compute memory and execution traces with cairo-lang is working OK now. On the other hand, compute memory and execution traces with cairo-vm is still failing. To fix this, could you move the added programs to another directory cairo_programs/cairo-0-secp-hints-feature?

The reason you should do this is because the failing workflow compares the execution of all the standalone programs in the directory with the default features (not including the new added feature). There is another job in the workflow that already tests with other features, so there is no need to test them in this job also.

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.

5 participants