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

Cheatcode for getting hash of the block #2648

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

PedroRosalba
Copy link

@PedroRosalba PedroRosalba commented Nov 6, 2024

Closes #684

Introduced changes

Checklist

  • Linked relevant issue
  • Updated relevant documentation
  • Added relevant tests
  • Performed self-review of the code
  • Added changes to CHANGELOG.md

Copy link
Collaborator

@kkawula kkawula left a comment

Choose a reason for hiding this comment

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

Hi @PedroRosalba, unfortunately, your solution isn't proper.
Please study other pr's that introduce cheatcode e.g. #2634 or many other you can find in our repo history.
Your solution should contain:

  • cheatcode implementation
  • entry in snfoundry_std
  • e2e tests (on cairo side)
  • documentation

@kkawula
Copy link
Collaborator

kkawula commented Nov 7, 2024

Hints for development:

  • run cargo test e2e to test locally
  • run cargo lint to check stylistic errors or deviations from a coding standard
  • run cargo fmt to format your code

If your tests pass, please submit your PR as ready to review.
Good luck!

@kkawula kkawula marked this pull request as draft November 7, 2024 08:35
@PedroRosalba PedroRosalba marked this pull request as ready for review November 8, 2024 02:49
@PedroRosalba
Copy link
Author

hey, can you review for some feedback? unfortunately I could not test the code (my computer even with no other tasks running cannot run cargo test e2e). sorry for the inconvenience. looking forward to continue working on this. sorry again.

@PedroRosalba
Copy link
Author

Do I have to deploy the contracts manually or smth like that? in the tests of CheatBlockHashChecker, I am getting an error that
---- cheatcodes::cheat_block_hash::cheat_block_hash_simple stdout ----
thread 'cheatcodes::cheat_block_hash::cheat_block_hash_simple' panicked at crates/cheatnet/tests/common/mod.rs:95:10:
called Result::unwrap() on an Err value: Unrecoverable(Anyhow(Failed to get contract artifact for name = CheatBlockHashChecker.

it is unable to find the contract.

@PedroRosalba
Copy link
Author

I have another question, is it allowed to 'request' changes in core lib files?

//I thought of creating an attribute block_hash in get_block_info using the

// pub extern fn get_block_hash_syscall(
// block_number: u64
// ) -> SyscallResult implicits(GasBuiltin, System) nopanic;

// //Is this allowed? Modifiying the core lib

for example this.

@PedroRosalba
Copy link
Author

failures:
e2e::build_profile::simple_package_build_profile
e2e::build_profile::simple_package_build_profile_and_pass_args
e2e::running::simple_package_with_git_dependency
e2e::steps::should_allow_more_than_10m

Hey, the only tests that I am getting errors are these, which are files that I havent modified (and I guess I should not, also). Do you have any ideas of why?

@PedroRosalba
Copy link
Author

Hey, mind reviewing for the feedback? Sorry for the inconvenience again.

@PedroRosalba
Copy link
Author

Hey, I don't understand the log of the errors.

In scarb fmt, it says Scarb.lock can't be find. Do you know why?

---- cheatcodes::cheat_block_hash::cheat_block_hash_simple stdout ----
thread 'cheatcodes::cheat_block_hash::cheat_block_hash_simple' panicked at crates/cheatnet/tests/common/mod.rs:95:10:
called Result::unwrap() on an Err value: Unrecoverable(Anyhow(Failed to get contract artifact for name = CheatBlockHashChecker.

In this second error, it is unable to find the contract. As I have already asked, is it necessary to declare or deploy these contracts separately?

@kkawula
Copy link
Collaborator

kkawula commented Nov 12, 2024

Hi! I will take a look today!

@PedroRosalba
Copy link
Author

alright, now I ran the scarbfmt scripts, things should be working properly. looking forward to the feedback, thx.

Copy link
Collaborator

@kkawula kkawula left a comment

Choose a reason for hiding this comment

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

Pretty good job so far! We are almost done.

  1. Add an entry to CHANGELOG.md
  2. In this Unify Felt usage #2655 pr, we unified felts in the entire repo, that's why some of your tests are failing, please replace it

@PedroRosalba
Copy link
Author

basically I need to create a get_block_hash_syscall function inside this cheated_syscalls.rs file? and then add the correspondent logic to the match selector? but why do I need to do that if my functions get_block_hash are all defined to call get_block_hash_syscall ? that is what I am not understanding very well

@PedroRosalba
Copy link
Author

Don't know if I got it: the thing is, to get the block hash, I need to get the block number, and for that, I need to access the function get_block_info, which is implemented via the block execution info syscall. What I got from what u said is that I need to define my function to get the block hash differently, without utilizing the block info execution syscall; instead use the get block hash syscall. but to use the block hash syscall I need to have the block number, which I get from the block info execution syscall, so I need to use it anyways. Where am I misunderstanding?

@cptartur
Copy link
Member

Don't know if I got it: the thing is, to get the block hash, I need to get the block number, and for that, I need to access the function get_block_info, which is implemented via the block execution info syscall. What I got from what u said is that I need to define my function to get the block hash differently, without utilizing the block info execution syscall; instead use the get block hash syscall. but to use the block hash syscall I need to have the block number, which I get from the block info execution syscall, so I need to use it anyways. Where am I misunderstanding?

@PedroRosalba You can still use the block execution info syscall in Cairo tests to get the block number first.

The problem is that in your implementation, you are changing the block hash stored in execution info. However, to check the hash of the block, you need to call the get_block_hash_syscall which is a different syscall.

That's why, even though you change the block hash in execution info, the get_block_hash_syscall doesn't know anything about the change and is returning the original (not cheated) block hash.
This is what is causing the tests to fail.

For the result of the get_block_hash_syscall to change, you need to create the logic for cheating it as I've outlined in my previous message.

@cptartur
Copy link
Member

In the cheatable_starknet_runtime_extension.rs, take a look at code match selector.

The get_block_info calls get_execution_info syscall under the hood:
https://github.com/starkware-libs/cairo/blob/1d9c6645f0e953fc12f28e7cbfa6efa3a7f38492/corelib/src/starknet/info.cairo#L81C8-L81C22

We handle this syscall in SyscallSelector::GetExecutionInfo case we handle this syscall and insert our cheated data into it.
However, in case of GetBlockHash we do not handle it and instead default implementation is used. This one doesn't have our cheated data so if you call get_block_hash_syscall you will still get the original hash, not the cheated one.

@kkawula
Copy link
Collaborator

kkawula commented Nov 26, 2024

HI @PedroRosalba any updates here?

@PedroRosalba
Copy link
Author

Hey, sorry for the delay. Updated the code, but still it is not solved.

I have a question, in implementing the logic for the cheated block hash (see crates/cheatnet/src/runtime_extensions/call_to_blockifier_runtime_extension/execution/cheated_syscalls.rs), how do I get the cheated block number? The only thing I thought was to try to use the cheated_block_info_ptr, but I dont know how to get the block number using that. Can you clarify?

.tool-versions Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@kkawula
Copy link
Collaborator

kkawula commented Dec 4, 2024

The only thing I thought was to try to use the cheated_block_info_ptr, but I dont know how to get the block number using that. Can you clarify?

You should look for the cheated block number in CheatState, look a the similar syscall overrides https://github.com/foundry-rs/starknet-foundry/blob/master/crates/cheatnet/src/runtime_extensions/call_to_blockifier_runtime_extension/execution/cheated_syscalls.rs#L65. I hope the blockifier code will also help you https://github.com/starkware-libs/sequencer/blob/main/crates/blockifier/src/execution/syscalls/mod.rs#L348

@PedroRosalba
Copy link
Author

Alright, updated now. Just a question, should I get the contractAddress from the cheated block number? how do I do that?

Copy link
Contributor

@Draggu Draggu left a comment

Choose a reason for hiding this comment

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

There should be new cheatcode defined.

@PedroRosalba
Copy link
Author

Hey, sorry for the delay. Now I believe the code is ready for production environment. Mind doing a review?

@kkawula
Copy link
Collaborator

kkawula commented Jan 17, 2025

Hi @PedroRosalba, how's it going? We really would like to finalize your changes, but I can see that the tests are still failing. Could you please look at this again?

Remember to format your code before pushing using cargo fmt, lint is also crucial cargo lint, output will give you hints you should fix your code.

Comment on lines 80 to +89
_ => Ok(SyscallHandlingResult::Forwarded),
SyscallSelector::GetBlockHash => self
.execute_syscall(
syscall_handler,
vm,
cheated_syscalls::get_block_hash_syscall,
SyscallSelector::Deploy,
)
.map(|()| SyscallHandlingResult::Handled),
_ => Ok(SyscallHandlingResult::Forwarded),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
_ => Ok(SyscallHandlingResult::Forwarded),
SyscallSelector::GetBlockHash => self
.execute_syscall(
syscall_handler,
vm,
cheated_syscalls::get_block_hash_syscall,
SyscallSelector::Deploy,
)
.map(|()| SyscallHandlingResult::Handled),
_ => Ok(SyscallHandlingResult::Forwarded),
SyscallSelector::GetBlockHash => self
.execute_syscall(
syscall_handler,
vm,
cheated_syscalls::get_block_hash_syscall,
SyscallSelector::Deploy,
)
.map(|()| SyscallHandlingResult::Handled),
_ => Ok(SyscallHandlingResult::Forwarded),

@@ -1 +1 @@
scarb 2.7.0
scarb 2.7.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
scarb 2.7.0
scarb 2.7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cheatcode mocking get_block_hash_syscall
4 participants