Ft/os upgrade #453

ftheirs · 2024-12-26T12:57:38Z

Upgrade to OS version V0.13.3. This PR includes the minimal changes that are needed to execute SNOS for blocks with the latest OS version.

Changes on deserialization from OS output
Some new hints that are needed for deserialization
Disable test that compares SNOS with reference PIE ---> Need to update the reference PIE with a new one (changes in output structure)

Issue Number: N/A

Type

Description

Breaking changes?

yes
no

HermanObst · 2024-12-27T17:21:37Z

crates/bin/prove_block/src/rpc_utils.rs

@@ -215,6 +215,7 @@ pub(crate) fn get_starknet_version(block_with_txs: &BlockWithTxs) -> blockifier:
        "0.13.1.1" => blockifier::versioned_constants::StarknetVersion::V0_13_1_1,
        "0.13.2" => blockifier::versioned_constants::StarknetVersion::V0_13_2,
        "0.13.2.1" => blockifier::versioned_constants::StarknetVersion::Latest,


Change this for:
StarknetVersion::V0_13_2_1

It was not available in the Blockifier version that we're using. I can double check this but updating Blockifier to a commit where it's defined lead to some extra changes that are meant for os v0.13.4 and are not needed

HermanObst

Good job man, I left some changes requested

HermanObst · 2024-12-27T17:22:21Z

crates/starknet-os/src/config.rs

+pub const DEFAULT_FEE_TOKEN_ADDR: &str = "7ce4aa542d72a82662cda96b147da9b041ecf8c61f67ef657f3bbb852fc698f";
+pub const DEFAULT_DEPRECATED_FEE_TOKEN_ADDR: &str = "5195ba458d98a8d5a390afa87e199566e473d1124c07a3c57bf19813255ac41";
+pub const SEQUENCER_ADDR_0_13_2: &str = "0x31c641e041f8d25997985b0efe68d0c5ce89d418ca9a127ae043aebed6851c5";


Why this is changing? could you point to where you are getting the information?

Yeap! It's defined in general_config.yaml from cairo-lang:
https://github.com/starkware-libs/cairo-lang/blob/master/src/starkware/starknet/definitions/general_config.yml

HermanObst · 2024-12-27T17:25:10Z

crates/bin/prove_block/tests/prove_block.rs

@@ -49,7 +49,7 @@ const DEFAULT_COMPILED_OS: &[u8] = include_bytes!("../../../../build/os_latest.j
 #[case::inconsistent_cairo0_class_hash_1(204936)]
 #[case::no_possible_convertion_1(155007)]
 #[case::no_possible_convertion_2(155029)]
-#[case::reference_pie_with_full_output_enabled(173404)]
+// #[case::reference_pie_with_full_output_enabled(173404)]


Dont remove this test:
The failure is related to the fact that this test is also comparing output and in this version the output structure has changed.

Fix: Find a new v0.13.3 PIE (we need them form Starkware) and modify this test to check that Block and output.

HermanObst · 2024-12-27T17:27:02Z

crates/starknet-os/src/config.rs

@@ -114,8 +112,7 @@ impl StarknetGeneralConfig {

    pub fn empty_block_context(&self) -> BlockContext {
        let mut versioned_constants = VersionedConstants::default();
-        versioned_constants.invoke_tx_max_n_steps = self.invoke_tx_max_n_steps;
-        versioned_constants.validate_max_n_steps = self.validate_max_n_steps;


Dont you need to change it for:
pub validate_max_n_steps_override: u32 ?

HermanObst · 2024-12-27T17:32:50Z

crates/starknet-os/src/hints/output.rs

+pub fn set_compressed_start(
+    vm: &mut VirtualMachine,
+    exec_scopes: &mut ExecutionScopes,
+    ids_data: &HashMap<String, HintReference>,
+    ap_tracking: &ApTracking,
+    _constants: &HashMap<String, Felt252>,
+) -> Result<(), HintError> {
+    let use_kzg_da_felt = exec_scopes.get::<Felt252>(vars::scopes::USE_KZG_DA)?;
+
+    let use_kzg_da = if use_kzg_da_felt == Felt252::ONE {
+        Ok(true)
+    } else if use_kzg_da_felt == Felt252::ZERO {
+        Ok(false)
+    } else {
+        Err(HintError::CustomHint("ids.use_kzg_da is not a boolean".to_string().into_boxed_str()))
+    }?;


Add unit test for this hint

HermanObst · 2024-12-27T17:37:02Z

crates/starknet-os/src/hints/output.rs

+    let n_updates_small_packing_bound = get_constant(vars::ids::N_UPDATES_SMALL_PACKING_BOUND, constants)?;
+
+    let is_n_updates_small =
+        if n_actual_updates < *n_updates_small_packing_bound { Felt252::ZERO } else { Felt252::ONE };


This should be the other way around

Suggested change

if n_actual_updates < *n_updates_small_packing_bound { Felt252::ZERO } else { Felt252::ONE };

if n_actual_updates < *n_updates_small_packing_bound { Felt252::ONE } else { Felt252::ZERO };

Good catch! Fixed this one and the deserialize_output that was comparing the wrong value 💪

HermanObst · 2024-12-27T17:38:19Z

crates/starknet-os/src/hints/output.rs

+    #[case(10, 0)]
+    #[case(255, 0)]
+    // big updates
+    #[case(256, 1)]
+    #[case(1024, 1)]


This conditions are also the other way around, right?

GMKrieger

I didn't find anything to comment on the code, but the CI tests are failing on the tests test_kzg_da_cairo_0 and test_kzg_da_cairo_1

ftheirs · 2024-12-30T22:14:55Z

I didn't find anything to comment on the code, but the CI tests are failing on the tests test_kzg_da_cairo_0 and test_kzg_da_cairo_1

Those tests depends on some hints that will be merge in the following PRs and that's why they are not passing!

HermanObst

Awesome job!!!! LGTM

ftheirs added 11 commits December 26, 2024 10:44

update cairo-lang to v0.13.3

494b307

expect Starknet v0.13.3 version

e4a4665

set_state_updates_start & set_compressed_start

b817540

update deserialize OS output

085e43c

hint implementation: set_n_updates_small

3fc423f

disable test until we get a reference PIE with the new OS version

a9e55ec

set full_output for integration tests

fbe56b8

deserialize output fix tests

e9de05e

update sequencer address

ec3fdf0

fix CI: bump cairo-lang to v0.13.3

213adde

clippy

8184adb

ftheirs force-pushed the ft/os_upgrade branch from 7832163 to 8184adb Compare December 26, 2024 13:44

ftheirs added 4 commits December 26, 2024 11:08

bump cairo-lang

d7a8a7a

fix tests

a394e9e

fix use_kzg_da global variable

96d4c29

add hint test

ee1cbad

ftheirs marked this pull request as ready for review December 27, 2024 14:02

ftheirs requested review from HermanObst and notlesh as code owners December 27, 2024 14:02

HermanObst reviewed Dec 27, 2024

View reviewed changes

HermanObst requested changes Dec 27, 2024

View reviewed changes

ftheirs added 3 commits December 27, 2024 15:23

fix is_s_updates_small

feda12c

add tests for set_compressed_start and set_state_updates_start

bd1ee68

fix StarknetGeneralConfig

8b1aed8

GMKrieger reviewed Dec 30, 2024

View reviewed changes

notlesh approved these changes Dec 30, 2024

View reviewed changes

add test with reference PIEs

106b2a7

HermanObst approved these changes Dec 30, 2024

View reviewed changes

ftheirs merged commit 0028b5e into os_v0_13_3 Dec 31, 2024
4 of 6 checks passed

ftheirs deleted the ft/os_upgrade branch December 31, 2024 12:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ft/os upgrade #453

Ft/os upgrade #453

ftheirs commented Dec 26, 2024

HermanObst Dec 27, 2024

ftheirs Dec 27, 2024

HermanObst left a comment

HermanObst Dec 27, 2024

ftheirs Dec 27, 2024

HermanObst Dec 27, 2024

HermanObst Dec 27, 2024

HermanObst Dec 27, 2024

HermanObst Dec 27, 2024

ftheirs Dec 27, 2024

HermanObst Dec 27, 2024

GMKrieger left a comment

ftheirs commented Dec 30, 2024

HermanObst left a comment

	if n_actual_updates < *n_updates_small_packing_bound { Felt252::ZERO } else { Felt252::ONE };
	if n_actual_updates < *n_updates_small_packing_bound { Felt252::ONE } else { Felt252::ZERO };

Ft/os upgrade #453

Ft/os upgrade #453

Conversation

ftheirs commented Dec 26, 2024

Type

Description

Breaking changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HermanObst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GMKrieger left a comment

Choose a reason for hiding this comment

ftheirs commented Dec 30, 2024

HermanObst left a comment

Choose a reason for hiding this comment