feat: enable the skip_instruction_exectution for hints #645

Eikix · 2022-12-08T15:42:44Z

feat: enable the skip_instruction_execution for hints

the feature making hints able to disable next instruction executions was disabled

Description

New implementation for #518
#518

Adds an integration test for skip_instruction

Description of the pull request changes and motivation.

Checklist

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

Eikix · 2022-12-12T13:57:29Z

Hey:), what is the UnknownHint error?

Oppen · 2022-12-12T18:21:13Z

Hey:), what is the UnknownHint error?

The builtin hint processor has a few hardcoded supported hints. Whenever you pass a hint that isn't natively implemented, it will return that error. This allows catching this special case if you want to extend the supported hints with an extra processor while still using the optimized version for the supported hints.

Oppen

LGTM. Just remove the print and it's good to go.

src/vm/vm_core.rs

Oppen · 2022-12-12T18:55:46Z

There's one more thing I missed (which is the reason the tests fail): you'll need to implement that setting as a new hint in the builtin runner. You can avoid making the new function public as well when you do that.

Eikix · 2022-12-13T09:17:14Z

There's one more thing I missed (which is the reason the tests fail): you'll need to implement that setting as a new hint in the builtin runner. You can avoid making the new function public as well when you do that.

Thanks a lot for your reply! I'm on it

Eikix · 2022-12-13T17:05:11Z

I proposed an updated version with what I understood from your review:)!
Hope this is more what you had in mind. Thanks for your time

Oppen

Two very minor changes and it's good to go!

src/hint_processor/builtin_hint_processor/skip_next_instruction.rs

tests/skip_instruction_test.rs

Eikix · 2022-12-14T14:59:52Z

Thanks a lot for your time!

Oppen

Excellent!

Oppen · 2022-12-14T17:09:44Z

Just for the record. Our common practice is squash and merge, so force pushing isn't strictly necessary. If you can push fixes individually it helps GitHub separate them to see what changed since the last review (I often do partial reviews with that feature to check requested changes before making a final one with everything together).

Eikix · 2022-12-14T18:29:38Z

Just for the record. Our common practice is squash and merge, so force pushing isn't strictly necessary. If you can push fixes individually it helps GitHub separate them to see what changed since the last review (I often do partial reviews with that feature to check requested changes before making a final one with everything together).

Thanks for the explanation! Will push partial chances from now on!

Could you retrigger workflows?

Juan-M-V · 2022-12-15T15:10:13Z

Hi, thanks for the contribution!
I was looking at the rust/build check and saw that the cairo program test_skip_next_instruction.cairo does compile but it fails to run when using cairo-run with this error:

cairo_programs/test_skip_next_instruction.cairo:5:3: Error at pc=0:2:
Got an exception while executing a hint.
  %{ skip_next_instruction() %}
  ^***************************^
Traceback (most recent call last):
  File "cairo_programs/test_skip_next_instruction.cairo", line 5, in <module>
    %{ skip_next_instruction() %}
NameError: name 'skip_next_instruction' is not defined

I looked into it a bit and while I haven't found a solution, I did find some things that might be useful for fixing this.
It seems that while the function skip_next_instruction() doesn't exist, however, there is a field: skip_instruction_execution inside the VM that does what you want to do in the test. I tried doing something similar to what is done here https://github.com/starkware-libs/cairo-lang/blob/d61255f32a7011e9014e1204471103c719cfd5cb/src/starkware/cairo/lang/vm/vm_test.py#L461 but unfortunately it didn't work either, the vm object doesn't seem to be in scope, at least not like its seen in the test.

Would you mind taking a look and correcting this?

Eikix · 2022-12-15T16:03:11Z

Hi, thanks for the contribution! I was looking at the rust/build check and saw that the cairo program test_skip_next_instruction.cairo does compile but it fails to run when using cairo-run with this error:
cairo_programs/test_skip_next_instruction.cairo:5:3: Error at pc=0:2:
Got an exception while executing a hint.
  %{ skip_next_instruction() %}
  ^***************************^
Traceback (most recent call last):
  File "cairo_programs/test_skip_next_instruction.cairo", line 5, in <module>
    %{ skip_next_instruction() %}
NameError: name 'skip_next_instruction' is not defined
I looked into it a bit and while I haven't found a solution, I did find some things that might be useful for fixing this. It seems that while the function skip_next_instruction() doesn't exist, however, there is a field: skip_instruction_execution inside the VM that does what you want to do in the test. I tried doing something similar to what is done here https://github.com/starkware-libs/cairo-lang/blob/d61255f32a7011e9014e1204471103c719cfd5cb/src/starkware/cairo/lang/vm/vm_test.py#L461 but unfortunately it didn't work either, the vm object doesn't seem to be in scope, at least not like its seen in the test.

Would you mind taking a look and correcting this?

Hey, thanks a lot for your time:)!

What do you mean to say It seems that while the function skip_next_instruction() doesn't exist?

My understanding of how hints work rn in the VM is:

declare a hint code in hint_code file:
pub(crate) const SKIP_NEXT_INSTRUCTION: &str = "skip_next_instruction()";

this means that the hint processor will understand the text skip_next_instruction() inside %{ %} hint braces.

then, in the builtin_hint_processor file, you match the hint_code to the specific implementation of the hint

hint_code::SKIP_NEXT_INSTRUCTION => skip_next_instruction(vm),

the mapped function to the hint code is skip_next_instruction(vm)
it is implemented in skip_next_instruction
as:

pub fn skip_next_instruction(vm: &mut VirtualMachine) -> Result<(), VirtualMachineError> {
    vm.skip_next_instruction_execution();
    Ok(())
}

Meaning that when the runner encounters my hint in test_skip_next_instruction.cairo line 1:
%{ skip_next_instruction() %}
It will go up to point 3.

Juan-M-V · 2022-12-15T17:41:54Z

Hi, thanks for the contribution! I was looking at the rust/build check and saw that the cairo program test_skip_next_instruction.cairo does compile but it fails to run when using cairo-run with this error:
cairo_programs/test_skip_next_instruction.cairo:5:3: Error at pc=0:2:
Got an exception while executing a hint.
  %{ skip_next_instruction() %}
  ^***************************^
Traceback (most recent call last):
  File "cairo_programs/test_skip_next_instruction.cairo", line 5, in <module>
    %{ skip_next_instruction() %}
NameError: name 'skip_next_instruction' is not defined
I looked into it a bit and while I haven't found a solution, I did find some things that might be useful for fixing this. It seems that while the function skip_next_instruction() doesn't exist, however, there is a field: skip_instruction_execution inside the VM that does what you want to do in the test. I tried doing something similar to what is done here https://github.com/starkware-libs/cairo-lang/blob/d61255f32a7011e9014e1204471103c719cfd5cb/src/starkware/cairo/lang/vm/vm_test.py#L461 but unfortunately it didn't work either, the vm object doesn't seem to be in scope, at least not like its seen in the test.
Would you mind taking a look and correcting this?
Hey, thanks a lot for your time:)!

What do you mean to say It seems that while the function skip_next_instruction() doesn't exist?

My understanding of how hints work rn in the VM is:
1. declare a hint code in `hint_code` file:
   `pub(crate) const SKIP_NEXT_INSTRUCTION: &str = "skip_next_instruction()";`
this means that the hint processor will understand the text skip_next_instruction() inside %{ %} hint braces.
2. then, in the `builtin_hint_processor` file, you match the hint_code to the specific implementation of the hint
hint_code::SKIP_NEXT_INSTRUCTION => skip_next_instruction(vm),
3. the mapped function to the hint code is `skip_next_instruction(vm)`
   it is implemented in `skip_next_instruction`
   as:
pub fn skip_next_instruction(vm: &mut VirtualMachine) -> Result<(), VirtualMachineError> {
    vm.skip_next_instruction_execution();
    Ok(())
}
Meaning that when the runner encounters my hint in test_skip_next_instruction.cairo line 1: %{ skip_next_instruction() %} It will go up to point 3.

Oh, sorry, I didn't explain myself properly. I meant that the function doesn't exist in the original python implementation. The part that is failing to run is not cairo_rs, but cairo-lang from strakware

Eikix · 2022-12-15T20:01:38Z

Ohh thanks for the explanation, what are some steps I could take to fix this?

Oppen · 2022-12-19T16:28:14Z

src/hint_processor/builtin_hint_processor/skip_next_instruction.rs

+It's only added for testing proposes
+*/
+
+pub fn skip_next_instruction(vm: &mut VirtualMachine) -> Result<(), VirtualMachineError> {


This could be pub(crate).

i'm thinking this could be exposed for maximum composability with other test-oriented hints

Eikix · 2022-12-27T16:15:08Z

I'm not able to reproduce locally why tests fail.

In the CI:

Locally:

Both using make test -j

fmoletta · 2022-12-27T18:29:18Z

I'm not able to reproduce locally why tests fail.

In the CI:

Locally:

Both using make test -j

Hello! we merged a PR recently that changed how the hint_processor is used, we now receive a mutable reference to it instead of a normal reference. The CI uses main as a base to run the tests, so while your local branch may be working fine, once the changes are applied to the main branch it starts failing. You should pull from the main branch, update your branch, and then you can fix the errors (should be adding a couple muts here and there). Hope this is useful!

Eikix · 2022-12-27T21:20:23Z

I'm not able to reproduce locally why tests fail.
In the CI:
Locally:
Both using make test -j

Hello! we merged a PR recently that changed how the hint_processor is used, we now receive a mutable reference to it instead of a normal reference. The CI uses main as a base to run the tests, so while your local branch may be working fine, once the changes are applied to the main branch it starts failing. You should pull from the main branch, update your branch, and then you can fix the errors (should be adding a couple muts here and there). Hope this is useful!

Great! Thanks:)! Done

Eikix · 2023-01-10T11:58:23Z

Hi:)! I'll resolve the conflicts,
Also, I posted a message in the starknet-rs telegram chat that I reproduce hereafter:

hi all:)! 
i think this PR (approved) is closed to the finish line https://github.com/lambdaclass/cairo-rs/pull/645

TL;DR: some features (i.e. test runner features) i’d like to add into cairo-rs are intrinsically not retrocompatible with the python vm so this PR finds a way to devise a workaround way to add these features 

there is one thing i’d like to check with you, it is about VM features that do not have retrocompatibility with the Python VM.
An example of that is the skip_next_instruction() hint, and other hints aimed at making Cairo an easily testable language (with lifecycle hooks available and so on to be able to build a test runner).

I’d like to check the following flow with you:
- whenever someone PRs for a functionality that has no retrocompat with the python VM, they put it in a feature (in the case of language test utils, it’ll be in the feature test_utils.
Then, the repository test suites tests has the test_utils feature enabled but the cross VM benchmark has it disabled.

- Then, for .cairo files that test the test_utils features, their name is of format <my_test_filename>.noretrocompat.cairo so that the .sh script does not include them in cross VM tests.

…ow for new features

Eikix · 2023-01-16T14:24:51Z

What is the status on this PR:)?

Oppen · 2023-01-16T15:15:36Z

Hi @Eikix, sorry for the delays. I think the procedure in your last comment is sensible and the way to go.
I'll do a last round of review but I think it should be mergeable now.

Oppen · 2023-01-16T15:17:34Z

Hmmm, it doesn't build. It must have been broken by the toolchain updates.
There's another issue we need to handle internally where the benchmark failed to build but the pipeline didn't (it did technically fail but later than it should have). I'll raise this one in an issue.

Eikix · 2023-01-16T17:07:27Z

Hmmm, it doesn't build. It must have been broken by the toolchain updates. There's another issue we need to handle internally where the benchmark failed to build but the pipeline didn't (it did technically fail but later than it should have). I'll raise this one in an issue.

Thanks! Tried to rebase and fix issues due to rebase

Makefile

* DO NOT PUSH this commit * Add and AddAssign tests * Proptest for felt bitwise operations (#716) * DO NOT PUSH this commit * this works but a lot of the variables are 0 * add tests for the or and xor operations * fixed another test, where the prop_assume was in the wrong place to avoid dividing by zero * Update felt/src/lib.rs Co-authored-by: Mario Rugiero <mario.rugiero@lambdaclass.com> Co-authored-by: Martina <martina@martina> Co-authored-by: Mario Rugiero <mario.rugiero@lambdaclass.com> * Add proptest for felt subtraction (#722) * add test for subtraction * minor fix Co-authored-by: Martina <martina@martina> * Parse from string radix (#692) Co-authored-by: Juanma <juanma@Juanmas-MacBook-Air.local> * Neg prop test for felts (#721) * found a bug, when x is 0 the code breaks when applying the Neg operation * minor change * fixed bug for Neg operator Co-authored-by: Martina <martina@martina> * derive default for execution resources (#727) * Add Serialize trait to Felt (#724) Co-authored-by: Pedro Fontana <pedro.fontana@lamdaclass.com> * feat: enable the skip_instruction_exectution for hints (#645) * feat: enable the skip_instruction_exectution for hints * feat: add skip_next_instruction() as whitelisted hint * feat: update hint string to match python vm skip_instruction hint * feat: update hint text * feat: refactor skip_next_instruction into crate feature * fix: fix make compare command to ignore .noretrocompat.* files to allow for new features * feat: add mut to hintprocessor after main rebase * fix: fix clippy error -> reference instead of variable * fix: fix erros due to rebase * Whitespace Co-authored-by: Timothée Delabrouille <timothee.delabrouille@gmail.com> Co-authored-by: Mario Rugiero <mario.rugiero@nextroll.com> * Fix builtin security check. (#714) * verify signature hint (#702) * Add common signature test * Add hint to hint code * Add signature hint * Add signature test * Add test to signature-hint * Happy path test Co-authored-by: Juanma <juanma@Juanmas-MacBook-Air.local> * Mul assign felt proptest (#717) * DO NOT PUSH this commit * test for multiplication with assignment * minor fix Co-authored-by: Martina <martina@martina> Co-authored-by: Juan-M-V <102986292+Juan-M-V@users.noreply.github.com> * Fix: typos (#733) * Fix: typo Fix: typo * Fix: typos Fix: typos * Fix: typos Fix: typos * Fix: typos Fix: typos * Use CairoArg enum instead of Any in CairoRunner::run_from_entrypoint (#686) * Start get_traceback_entries + add convenience methos * Add fn is_call_instruction * add code * Refactor code * Clippy * Add get_traceback method * Fix get_error_attr_value * Add traceback to VmException * Make traceback non-optional * Add tests for is_call_instruction * Add traceback to error display * Add test + fix logic for get_traceback_entries * Code refactor * Add one more test for get_traceback_entries * Fix string format + add test for get_traceback * Improve fn * Add reference to is_call_instruction signature * Add reference to immediate in decode_instruction + remove clone * Fix hint_processor mutability in tests * Add Location::get_location_marks * Fix method to_string_with_contents * Fix string format * Fix string format * Update traceback tests * Add tests for Location::to_string_with_contents() * Fix intermediate string format * Fix test * Add tests for Location::get_location_marks() * Update VmException display * Fix string format * Fix string format * Remove debug print * Fix Display * Implement Display for MaybeRelocatable * Add real-case test for VmException Display * Remove debug format from erros containing MaybeRelocatable and Relocatable * Add tests for display implementation * Update Changelog * Clippy * Remove unnecessary & * Add hint location to InstructionLocation * Use InstructionLocation instead of Location in insruction_locations field of Program * Add hint location logic to get_location * Add rought version of VirtualMachineError::Hint * Add test for error display on HintError * Add test for get_location with hint_index * Start refactor * Update changelog * Finnish changing hint fns to HintError * Update custom hint example * Fix changelog format * Add changelog entry for this PR * Use CairoArg enum instead of Any in CairoRunner::run_from_entrypoint * Fix tests * Add tests + remove unused func * use slice instead of vec * Update changelog * Add non-typed helpers for ids variables * Swap BigInt for MaybeRelocatable in Dictionary * Update dict_hint_utils + dict macros in test_utils * Fix tests * Clippy * Add test for dict_write with relocatable * Add tests on hint_utils.rs * Add tetss on hint_processor_utils.rs * Add integration test for bug case * Update changelog * Fix eof * Remove debug print * Update readme (#729) * DO NOT PUSH this commit * Improve the About section * more changes * improve step by step instructions to use the repo * more changes * made requested changes in the hints section * removed (optional) as it was unnecessary Co-authored-by: Martina <martina@martina> * subtraction tests * delete unnecessary print Co-authored-by: Martina <martina@martina> Co-authored-by: Mario Rugiero <mario.rugiero@lambdaclass.com> Co-authored-by: Juan-M-V <102986292+Juan-M-V@users.noreply.github.com> Co-authored-by: Juanma <juanma@Juanmas-MacBook-Air.local> Co-authored-by: Juan Bono <juanbono94@gmail.com> Co-authored-by: Pedro Fontana <pedro.fontana@lambdaclass.com> Co-authored-by: Pedro Fontana <pedro.fontana@lamdaclass.com> Co-authored-by: Elias Tazartes <66871571+Eikix@users.noreply.github.com> Co-authored-by: Timothée Delabrouille <timothee.delabrouille@gmail.com> Co-authored-by: Mario Rugiero <mario.rugiero@nextroll.com> Co-authored-by: Alon Haramati <91828241+alonh5@users.noreply.github.com> Co-authored-by: omahs <73983677+omahs@users.noreply.github.com> Co-authored-by: fmoletta <99273364+fmoletta@users.noreply.github.com>

Eikix requested review from igaray, Oppen, fmoletta, entropidelic, HermanObst, juanbono, Jrigada and pefontana as code owners December 8, 2022 15:42

Eikix force-pushed the feat/skip-next-instruction-test branch from 55b95c8 to 044f871 Compare December 8, 2022 15:45

Eikix mentioned this pull request Dec 8, 2022

feat: enable the skip_instruction_exectution for hints #518

Closed

Oppen requested changes Dec 12, 2022

View reviewed changes

src/vm/vm_core.rs Outdated Show resolved Hide resolved

Eikix requested a review from Oppen December 13, 2022 16:38

Oppen requested changes Dec 14, 2022

View reviewed changes

src/hint_processor/builtin_hint_processor/skip_next_instruction.rs Outdated Show resolved Hide resolved

tests/skip_instruction_test.rs Outdated Show resolved Hide resolved

Eikix force-pushed the feat/skip-next-instruction-test branch from 2bbda52 to 2e0ff56 Compare December 14, 2022 15:26

Oppen approved these changes Dec 14, 2022

View reviewed changes

Eikix force-pushed the feat/skip-next-instruction-test branch from 2e0ff56 to e2a44e9 Compare December 14, 2022 16:51

Oppen approved these changes Dec 14, 2022

View reviewed changes

Juan-M-V approved these changes Dec 15, 2022

View reviewed changes

Oppen reviewed Dec 19, 2022

View reviewed changes

Juan-M-V approved these changes Dec 22, 2022

View reviewed changes

Eikix force-pushed the feat/skip-next-instruction-test branch from d688ceb to c256690 Compare December 27, 2022 21:19

Eikix force-pushed the feat/skip-next-instruction-test branch from c256690 to 31fc4e7 Compare December 28, 2022 19:23

Eikix requested review from Oppen and Juan-M-V January 10, 2023 11:56

Eikix force-pushed the feat/skip-next-instruction-test branch from a058880 to afa08f3 Compare January 10, 2023 12:01

tdelabro and others added 8 commits January 16, 2023 15:23

feat: enable the skip_instruction_exectution for hints

b22b02e

feat: add skip_next_instruction() as whitelisted hint

747330b

feat: update hint string to match python vm skip_instruction hint

417eabf

feat: update hint text

3e3b365

feat: refactor skip_next_instruction into crate feature

191d100

fix: fix make compare command to ignore .noretrocompat.* files to all…

679a2c4

…ow for new features

feat: add mut to hintprocessor after main rebase

6aa34e1

fix: fix clippy error -> reference instead of variable

b66b80b

Eikix force-pushed the feat/skip-next-instruction-test branch from afa08f3 to b66b80b Compare January 16, 2023 14:24

fix: fix erros due to rebase

480ba26

Oppen reviewed Jan 16, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Whitespace

b4f6fab

Oppen approved these changes Jan 16, 2023

View reviewed changes

Oppen merged commit e0b584b into lambdaclass:main Jan 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable the skip_instruction_exectution for hints #645

feat: enable the skip_instruction_exectution for hints #645

Eikix commented Dec 8, 2022 •

edited

Loading

Eikix commented Dec 12, 2022

Oppen commented Dec 12, 2022

Oppen left a comment

Oppen commented Dec 12, 2022

Eikix commented Dec 13, 2022

Eikix commented Dec 13, 2022

Oppen left a comment

Eikix commented Dec 14, 2022

Oppen left a comment

Oppen commented Dec 14, 2022

Eikix commented Dec 14, 2022

Juan-M-V commented Dec 15, 2022

Eikix commented Dec 15, 2022 •

edited

Loading

Juan-M-V commented Dec 15, 2022

Eikix commented Dec 15, 2022

Oppen Dec 19, 2022

Eikix Dec 29, 2022

Eikix commented Dec 27, 2022 •

edited

Loading

fmoletta commented Dec 27, 2022

Eikix commented Dec 27, 2022 •

edited

Loading

Eikix commented Jan 10, 2023

Eikix commented Jan 16, 2023

Oppen commented Jan 16, 2023

Oppen commented Jan 16, 2023

Eikix commented Jan 16, 2023

feat: enable the skip_instruction_exectution for hints #645

feat: enable the skip_instruction_exectution for hints #645

Conversation

Eikix commented Dec 8, 2022 • edited Loading

feat: enable the skip_instruction_execution for hints

Description

Checklist

Eikix commented Dec 12, 2022

Oppen commented Dec 12, 2022

Oppen left a comment

Choose a reason for hiding this comment

Oppen commented Dec 12, 2022

Eikix commented Dec 13, 2022

Eikix commented Dec 13, 2022

Oppen left a comment

Choose a reason for hiding this comment

Eikix commented Dec 14, 2022

Oppen left a comment

Choose a reason for hiding this comment

Oppen commented Dec 14, 2022

Eikix commented Dec 14, 2022

Juan-M-V commented Dec 15, 2022

Eikix commented Dec 15, 2022 • edited Loading

Juan-M-V commented Dec 15, 2022

Eikix commented Dec 15, 2022

Oppen Dec 19, 2022

Choose a reason for hiding this comment

Eikix Dec 29, 2022

Choose a reason for hiding this comment

Eikix commented Dec 27, 2022 • edited Loading

fmoletta commented Dec 27, 2022

Eikix commented Dec 27, 2022 • edited Loading

Eikix commented Jan 10, 2023

Eikix commented Jan 16, 2023

Oppen commented Jan 16, 2023

Oppen commented Jan 16, 2023

Eikix commented Jan 16, 2023

Eikix commented Dec 8, 2022 •

edited

Loading

Eikix commented Dec 15, 2022 •

edited

Loading

Eikix commented Dec 27, 2022 •

edited

Loading

Eikix commented Dec 27, 2022 •

edited

Loading