cairo-vm icon indicating copy to clipboard operation
cairo-vm copied to clipboard

feat: SECP related hints

Open odesenfans opened this issue 1 year ago • 3 comments
trafficstars

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
  • [x] Unit tests added
  • [ ] Integration tests added.
  • [ ] This change requires new documentation.
    • [ ] Documentation has been added/updated.
    • [x] CHANGELOG has been updated.

odesenfans avatar Aug 29 '24 11:08 odesenfans

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

pefontana avatar Sep 25 '24 22:09 pefontana

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 :warning:
...int_processor/builtin_hint_processor_definition.rs 14.28% 72 Missing :warning:
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.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Oct 02 '24 18:10 codecov[bot]

@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?

pefontana avatar Oct 07 '24 20:10 pefontana

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

whichqua avatar Nov 05 '24 14:11 whichqua

@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

pefontana avatar Nov 13 '24 20:11 pefontana

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

whichqua avatar Nov 14 '24 12:11 whichqua

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.

JulianGCalderon avatar Nov 15 '24 15:11 JulianGCalderon

@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.

whichqua avatar Nov 20 '24 15:11 whichqua

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.

JulianGCalderon avatar Nov 20 '24 21:11 JulianGCalderon