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

Feature: support Sierra 1.6 #328

Merged
merged 13 commits into from
Aug 29, 2024
Merged

Feature: support Sierra 1.6 #328

merged 13 commits into from
Aug 29, 2024

Conversation

odesenfans
Copy link
Contributor

Problem: some contracts on Sepolia are built with Sierra 1.6, which is
not supported by the version of the CASM compiler we use. But bumping
the version of the compiler requires bumping cairo-vm and Blockifier as
well.

Solution: bump everything! We now use:

  • cairo-vm 1.0.1
  • cairo-lang 2.7.1
  • blockifier 0.8.0-rc.2

With this, we can now compile any contract on Sepolia.

Issue Number: N/A

Type

  • feature
  • bugfix
  • dev (no functional changes, no API changes)
  • fmt (formatting, renaming)
  • build
  • docs
  • testing

Description

Breaking changes?

  • yes
  • no

Problem: some contracts on Sepolia are built with Sierra 1.6, which is
not supported by the version of the CASM compiler we use. But bumping
the version of the compiler requires bumping cairo-vm and Blockifier as
well.

Solution: bump everything! We now use:
* cairo-vm 1.0.1
* cairo-lang 2.7.1
* blockifier 0.8.0-rc.2

With this, we can now compile any contract on Sepolia.
This function is no longer required as felt types now all rely on
starknet-types-core.
This function is no longer required as felt types now all rely on
starknet-types-core.
This function is no longer required as felt types now all rely on
starknet-types-core.
This function is no longer required as felt types now all rely on
starknet-types-core.
@odesenfans odesenfans requested a review from notlesh as a code owner August 28, 2024 21:06
@odesenfans odesenfans mentioned this pull request Aug 28, 2024
9 tasks
@@ -1,15 +1,19 @@
use blockifier::block::{BlockInfo, GasPrices};
use blockifier::blockifier::block::{BlockInfo, GasPrices};
use blockifier::bouncer::BouncerConfig;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is a Bouncer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand it right, it's a mechanism used to determine how many txs can go in a block depending on different factors like gas, builtin usage, number of events etc.

Comment on lines 270 to +271
pub async fn storage_write(&self, _syscall_ptr: Relocatable) -> Result<(), HintError> {
let sys_hand = self.deprecated_syscall_handler.write().await;

let _ = sys_hand.exec_wrapper.execution_helper.write().await.execute_code_read_iter.next().ok_or(
HintError::SyscallError("No more storage writes available to replay".to_string().into_boxed_str()),
)?;

// Nothing to do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why there is nothing to do here?
What happen with the logic that were before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good remark! Newer versions of Blockifier now split reads and writes into two separate fields, CallInfo.storage_read_values and CallInfo.accessed_storage_keys. Before the two were merged so we had to take a value from the iterator (writes were always set to 0, the value itself has no interest) so that the read syscall could take the correct values.

Now, we can just iterate over storage_read_values in the read syscall and do nothing in the write syscall. Technically we could iterate over the accessed_storage_keys in the write syscall but there is no interest in doing that besides checking that we have an equal amount of values. It sounds a bit useless to me so I just replaced this code by a no-op.

async fn get_storage_at_async(&self, contract_address: ContractAddress, key: StorageKey) -> StateResult<Felt252> {
let storage_key: TreeIndex = key.0.key().to_biguint();

let mut ffc = self.ffc.clone();
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the motivation of this clone instead the mut reference ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because the trait now forces this method to take &self instead of &mut self. The FFC is built around an Arc<Mutex<Storage>> so we can just deal with this problem by cloning the FFC and then modifying the clone.

@@ -68,7 +67,7 @@ pub async fn unpack_blockifier_state_async<S: Storage + Send + Sync, H: HashFunc
) -> Result<(SharedState<S, H>, SharedState<S, H>), TreeError> {
let final_state = {
let state = blockifier_state.state.clone();
state.apply_commitment_state_diff(blockifier_state.to_state_diff()).await?
state.apply_commitment_state_diff(blockifier_state.to_state_diff().expect("todo").into()).await?
Copy link
Collaborator

Choose a reason for hiding this comment

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

.expect("todo") seems vague :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops, good point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. This function should not be there btw, this is only used in one integration test. I modified the expect message to be clearer, I'll move the function in another PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! Thanks!

Copy link
Collaborator

@HermanObst HermanObst left a comment

Choose a reason for hiding this comment

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

Awesome job! LGTM!

@HermanObst HermanObst merged commit a051468 into main Aug 29, 2024
6 checks passed
@HermanObst HermanObst deleted the od/support-sierra-1.6 branch August 29, 2024 13:31
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.

2 participants