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

Add VM hooks as rust conditional feature #761

Merged
merged 1 commit into from
Feb 13, 2023

Conversation

tdelabro
Copy link
Contributor

Introduce VM hooks

Description

Cairo-rs based testing tools such as cairo-foundry or those built by FuzzingLabs, need access to the state of the VM at specific points during the execution.
This PR adds the possibility for users of the cairo-rs lib to execute their custom additional code during the program execution.
I used the Rust "feature" mechanism in order to guarantee that this ability is only available when the lib user needs it, and is not compiled when it's not required.

In this PR I created tree hooks:

  • before the first step
  • before each step
  • after each step

More hooks could easily be added in the future, as new needs arise.

This PR does not contain any tests, nor documentation, as its immediate goal is not to be merged but to spark a discussion with the LambdaClass team and validate that:

  • they agree on the way the feature is designed
  • they agree on the principle of adding non "canon" behaviors in the VM thanks to Rust "feature" mechanism

@unbalancedparentheses
Copy link
Member

This looks nice. We will check it tomorrow!

@pventuzelo
Copy link

Clearly something we will use for https://github.com/FuzzingLabs/cairo-fuzzer ;)

@Oppen
Copy link
Member

Oppen commented Jan 23, 2023

I love it. This will also be useful to migrate the debugging/tracing functionality to Rust. Can't wait to see the final version :)

@Oppen
Copy link
Member

Oppen commented Jan 23, 2023

BTW, the description says this is to spark conversation, so it would be nice to convert it to draft and add something like rfc: at the beginning of the title to avoid confusion.

@tdelabro tdelabro marked this pull request as draft January 23, 2023 15:44
@tdelabro tdelabro changed the title feat: add VM hooks as rust conditional feature RFC: add VM hooks as rust conditional feature Jan 23, 2023
@tdelabro tdelabro force-pushed the feature-hooks branch 2 times, most recently from 88e2890 to 2417b96 Compare January 27, 2023 13:06
@tdelabro tdelabro marked this pull request as ready for review January 27, 2023 13:06
@tdelabro
Copy link
Contributor Author

Changes made in the way the doc is built (using nightly features) allow for this kind of informative warning in the doc about optional features.
I hope you like it!

Screenshot 2023-01-27 at 14 10 43

Screenshot 2023-01-27 at 14 10 58

Makefile Show resolved Hide resolved
/// Prevent the execution of the next instruction
///
/// This hint doesn't belong to the Cairo common library
/// It's only added for testing proposes
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// It's only added for testing proposes
/// It's only added for testing purposes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@Oppen Oppen left a comment

Choose a reason for hiding this comment

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

Overall this seems really good. There are some minor notes tho. I'm also unsure why the run_until_* functions' interfaces need to change. Can't the field in VirtualMachine be used directly inside those?

src/vm/hooks.rs Show resolved Hide resolved
hint_data_dictionary: &HashMap<usize, Vec<Box<dyn Any>>>,
constants: &HashMap<String, Felt>,
) -> Result<(), VirtualMachineError> {
if let Some(hook_func) = self.hooks.clone().pre_step_instruction {
Copy link
Member

Choose a reason for hiding this comment

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

Same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same

src/vm/hooks.rs Show resolved Hide resolved
src/vm/hooks.rs Show resolved Hide resolved
src/vm/hooks.rs Show resolved Hide resolved
@tdelabro
Copy link
Contributor Author

tdelabro commented Feb 4, 2023

There are some minor notes tho. I'm also unsure why the run_until_* functions' interfaces need to change. Can't the field in VirtualMachine be used directly inside those?

Yeah totally it was some flags from a previous version of the code that I forgot to remove!

doc: improve doc support for optional features
Copy link
Member

@Oppen Oppen left a comment

Choose a reason for hiding this comment

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

Excellent work!

@tdelabro
Copy link
Contributor Author

tdelabro commented Feb 8, 2023

Thanks @Oppen, can you find us a second reviewer so we can merge ;)

Copy link
Contributor

@Jrigada Jrigada left a comment

Choose a reason for hiding this comment

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

Hey @tdelabro! Amazing work, just some small typos suggestions

@@ -165,8 +165,9 @@ compare_trace_proof: $(CAIRO_RS_TRACE_PROOF) $(CAIRO_TRACE_PROOF)
compare_memory_proof: $(CAIRO_RS_MEM_PROOF) $(CAIRO_MEM_PROOF)
cd tests; ./compare_vm_state.sh memory proof_mode

# Run with nightly enable the `doc_cfg` feature wich let us provide clear explaination about which parts of the code are behind a feature flag
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Run with nightly enable the `doc_cfg` feature wich let us provide clear explaination about which parts of the code are behind a feature flag
# Run with nightly enable the `doc_cfg` feature which let us provide clear explanation about which parts of the code are behind a feature flag

@Oppen Oppen changed the title RFC: add VM hooks as rust conditional feature Add VM hooks as rust conditional feature Feb 13, 2023
@Oppen Oppen added this pull request to the merge queue Feb 13, 2023
Merged via the queue into lambdaclass:main with commit 9fad715 Feb 13, 2023
@tdelabro tdelabro deleted the feature-hooks branch February 13, 2023 13:55
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.

None yet

5 participants