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

feat: starknet 0.13.1 (without RPC changes) #1766

Merged
merged 17 commits into from
Feb 20, 2024
Merged

Conversation

sistemd
Copy link
Contributor

@sistemd sistemd commented Feb 18, 2024

Support for Starknet v0.13.1 (without RPC changes or P2P changes since the protobufs are not ready yet, oy vey).

This updates blockifier to rc1, the issue to update to the newest blockifier rc is here.

@sistemd sistemd requested a review from a team as a code owner February 18, 2024 13:50
@sistemd sistemd marked this pull request as draft February 18, 2024 13:54
@sistemd sistemd removed the request for review from kkovaacs February 18, 2024 13:54
@sistemd sistemd changed the title feat: starknet 0.13.1 feat: starknet 0.13.1 (without RPC changes) Feb 18, 2024
@sistemd
Copy link
Contributor Author

sistemd commented Feb 18, 2024

After resolving merge conflicts one of the tests is now failing. The crux of the issue seems to be that P2P protobufs for Starknet v0.13.1 are not quite ready yet, hence I can't specify a valid value for data_availability here. I see two options:

  1. We disable or comment out the test and make sure to note this down in Starknet v0.13.1 P2P changes #1768.
  2. I'm missing something and you guys enlighten me.

@sistemd sistemd force-pushed the sistemd/starknet-0.13.1 branch 2 times, most recently from e72eb9d to 7785820 Compare February 18, 2024 23:10
)
.context("Validating Sierra class")?;

// TODO: determine `max_bytecode_size`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to do this and I'm not sure if we want to do this at all. I will check this, but I have a feeling this API change is just a consequence of the cairo compiler wanting to add support for a new CLI flag or something like that to let users limit bytecode size. I might be completely wrong though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this new limit is just used by the gateway/sequencer to enforce some recently added limits.

I'm not sure it's terribly important for us to implement the same limit.

Copy link
Contributor

@Mirko-von-Leipzig Mirko-von-Leipzig Feb 19, 2024

Choose a reason for hiding this comment

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

I think it would be dangerous to impose any limit at all here. Starkware literally just changed the value for this on starknet without a starknet update. So any check here might cause problems in the future.

Since only "starknet valid" classes should arrive here, we ought to be safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since only "starknet valid" classes should arrive here, we ought to be safe.

That's not entirely true though. DECLARE transactions submitted via starknet_estimateFee / starknet_simulateTransactions also contain a class that ends up being compiled here.

@sistemd sistemd marked this pull request as ready for review February 18, 2024 23:37
Copy link
Contributor

@kkovaacs kkovaacs left a comment

Choose a reason for hiding this comment

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

We'll also have to come up with some new test cases for estimate/trace where l1_da_mode is BLOB.

)
.context("Validating Sierra class")?;

// TODO: determine `max_bytecode_size`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this new limit is just used by the gateway/sequencer to enforce some recently added limits.

I'm not sure it's terribly important for us to implement the same limit.

crates/executor/src/estimate.rs Outdated Show resolved Hide resolved
self.header.strk_l1_data_gas_price.0.try_into().unwrap()
},
},
use_kzg_da: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this will have to get fixed otherwise blockifier won't correctly use data gas.

We'll have to use either true/false depending on the l1_da_mode field of the block header. I guess we'll need to add that to the pathfinder_common::BlockHeader type too.

crates/executor/src/simulate.rs Outdated Show resolved Hide resolved
crates/pathfinder/examples/feeder_gateway.rs Outdated Show resolved Hide resolved
Comment on lines 74 to 83
{
Ok(block) => match block {
MaybePendingBlock::Pending(block) => {
return Some(U256::from(block.eth_l1_gas_price.0));
return Some(U256::from(block.eth_l1_gas_price().0));
}
MaybePendingBlock::Block(block) => {
return block.eth_l1_gas_price.map(|gp| U256::from(gp.0));
return block.eth_l1_gas_price().map(|gp| U256::from(gp.0));
}
},
Err(reason) => {
Copy link
Contributor

@kkovaacs kkovaacs Feb 19, 2024

Choose a reason for hiding this comment

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

It looks like this is unused (the whole file). We should remove this (as a new PR).

crates/rpc/src/jsonrpc/websocket/data.rs Show resolved Hide resolved
crates/rpc/src/lib.rs Show resolved Hide resolved
crates/storage/src/connection/block.rs Show resolved Hide resolved
crates/storage/src/schema/revision_0049.rs Show resolved Hide resolved
Copy link
Contributor

@Mirko-von-Leipzig Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

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

This is looking good so far.

crates/executor/src/call.rs Show resolved Hide resolved
Comment on lines +90 to +97
let eth_fee_token_address = starknet_api::core::ContractAddress(
PatriciaKey::try_from(ETH_FEE_TOKEN_ADDRESS.0.into_starkfelt())
.expect("ETH fee token address overflow"),
);
let strk_fee_token_address = starknet_api::core::ContractAddress(
PatriciaKey::try_from(STRK_FEE_TOKEN_ADDRESS.0.into_starkfelt())
.expect("STRK fee token address overflow"),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

We could create constants here for these and use tests for equivalence

crates/gateway-types/src/reply.rs Show resolved Hide resolved
crates/gateway-types/src/reply.rs Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Not super happy that blockifier makes it way into RPC directly. Ideally it would stay in execution.

The original motivation for this was that we didn't know which execution engine we would be using - starknet-in-rust or blockifier, and constraining the dep to only a single crate might let us exchange them more trivially.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could add a wrapper type into executor so that we don't need blockifier::ClassInfo here. Adds some extra hoops but does not require adding blockifier to rpc.

@kkovaacs kkovaacs force-pushed the sistemd/starknet-0.13.1 branch 4 times, most recently from 73f61e1 to 6f1490b Compare February 20, 2024 07:11
@kkovaacs kkovaacs merged commit 896f75c into main Feb 20, 2024
7 checks passed
@kkovaacs kkovaacs deleted the sistemd/starknet-0.13.1 branch February 20, 2024 14:56
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.

3 participants