feat(blockifier): add OS constants to VC

[nimrod-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload workflows:

• Started at 2024-11-26T08:59:30Z
• Started at 2024-11-26T09:08:09Z
• Started at 2024-11-26T13:52:13Z
• Started at 2024-11-26T14:29:50Z
• Started at 2024-11-27T11:07:55Z
• Started at 2024-11-28T09:57:56Z
• Started at 2024-11-28T14:04:57Z
• Started at 2024-11-28T14:23:59Z
• Started at 2024-12-01T12:06:19Z
• Started at 2024-12-02T06:48:57Z
• Started at 2024-12-02T12:37:38Z

[nimrod-starkware]

python side PR

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

[codecov]

Codecov Report

Attention: Patch coverage is 81.08108% with 7 lines in your changes missing coverage. Please review.

Project coverage is 63.16%. Comparing base (e3165c4) to head (9d0bc72).
Report is 659 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/blockifier/src/versioned_constants.rs	62.50%	6 Missing ⚠️
crates/papyrus_execution/src/lib.rs	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2281       +/-   ##
===========================================
+ Coverage   40.10%   63.16%   +23.06%     
===========================================
  Files          26      143      +117     
  Lines        1895    18387    +16492     
  Branches     1895    18387    +16492     
===========================================
+ Hits          760    11615    +10855     
- Misses       1100     5973     +4873     
- Partials       35      799      +764     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dorimedini-starkware]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nimrod-starkware and @Yoni-Starkware)

---

crates/blockifier/src/versioned_constants.rs line 651 at r1 (raw file):

impl OsConstants {
    // List of additinal os constants, beside the gas cost and validate rounding constants, that are
    // not used by the blockifier but included for transparency. These constanst will be ignored

won't you be using these constants? we don't verify deployed addresses are not reserved?

Code quote:

not used by the blockifier

---

crates/blockifier/resources/versioned_constants_0_13_4.json line 0 at r1 (raw file):
don't these changes also need to appear on the older VCs...?

[Yoni-Starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @nimrod-starkware)

---

crates/blockifier/src/versioned_constants.rs line 651 at r1 (raw file):

Previously, dorimedini-starkware wrote…

won't you be using these constants? we don't verify deployed addresses are not reserved?

No need to check it in the blockifier

[dorimedini-starkware]

Reviewed 15 of 15 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nimrod-starkware)

---

crates/blockifier/src/versioned_constants.rs line 709 at r2 (raw file):

    }
}
#[derive(Debug, Deserialize)]

Suggestion:

}

#[derive(Debug, Deserialize)]

---

crates/blockifier/src/versioned_constants.rs line 713 at r2 (raw file):

    pub block_hash: u8,
    pub alias: u8,
    pub reserved: u8,

change types from u8 to ContractAddress, as these are contract addresses... if you want to enforce these are less than 256 via typing, define struct OsContractAddress(u8) and impl From<OsContractAddress> for ContractAddress, but that seems overkill to me

Code quote:

    pub block_hash: u8,
    pub alias: u8,
    pub reserved: u8,

---

crates/blockifier/src/versioned_constants.rs line 719 at r2 (raw file):

    fn default() -> Self {
        Self { block_hash: 1, alias: 2, reserved: 3 }
    }

I don't like how these magic numbers appear here and in the VC jsons; how about like this?
(latest_constants returns a &'static VersionedConstants so there should very small overhead to loading the entire object)

Suggestion:

    fn default() -> Self {
        VersionedConstants::latest_constants().os_contract_addresses
    }

[dorimedini-starkware]

Reviewed 12 of 12 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nimrod-starkware)

[nimrod-starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/blockifier/resources/versioned_constants_0_13_4.json line at r1 (raw file):

Previously, dorimedini-starkware wrote…

don't these changes also need to appear on the older VCs...?

Done

---

crates/blockifier/src/versioned_constants.rs line 713 at r2 (raw file):

Previously, dorimedini-starkware wrote…

change types from u8 to ContractAddress, as these are contract addresses... if you want to enforce these are less than 256 via typing, define struct OsContractAddress(u8) and impl From<OsContractAddress> for ContractAddress, but that seems overkill to me

I did another trick, changing to

pub struct OsContractAddresses {
    block_hash_contract_address: ContractAddress,
    alias_contract_address: ContractAddress,
    reserved_contract_address: ContractAddress,
}

Doesn't work because of how ContractAddress deserializes.

I changed the fields to be private and added methods return it as a ContractAddress

---

crates/blockifier/src/versioned_constants.rs line 719 at r2 (raw file):

Previously, dorimedini-starkware wrote…

I don't like how these magic numbers appear here and in the VC jsons; how about like this?
(latest_constants returns a &'static VersionedConstants so there should very small overhead to loading the entire object)

Done.

---

crates/blockifier/src/versioned_constants.rs line 709 at r2 (raw file):

    }
}
#[derive(Debug, Deserialize)]

Done.

[dorimedini-starkware]

Reviewed 12 of 12 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)

---

crates/blockifier/src/versioned_constants.rs line 713 at r2 (raw file):

Previously, nimrod-starkware wrote…

I did another trick, changing to

pub struct OsContractAddresses {
    block_hash_contract_address: ContractAddress,
    alias_contract_address: ContractAddress,
    reserved_contract_address: ContractAddress,
}

Doesn't work because of how ContractAddress deserializes.

I changed the fields to be private and added methods return it as a ContractAddress

did you try #[serde(deserialize_with = "???")]? it may be a cleaner solution

[nimrod-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

---

crates/blockifier/src/versioned_constants.rs line 713 at r2 (raw file):

Previously, dorimedini-starkware wrote…

did you try #[serde(deserialize_with = "???")]? it may be a cleaner solution

It's possible with an intermediate struct that deserializes easily.
However, implementing a Default for that becomes more difficult if we want to use VersionedConstants::latest() as the fields there of ContractAddress type.

[dorimedini-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)

[dorimedini-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)

---

a discussion (no related file):
waiting for py side PR to pass

[Yoni-Starkware]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)

[nimrod-starkware]

Reviewable status: 3 of 15 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

---

a discussion (no related file):

Previously, dorimedini-starkware wrote…

waiting for py side PR to pass

passed

[dorimedini-starkware]

Reviewed 10 of 12 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)

[Yoni-Starkware]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)

[dorimedini-starkware]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)

[Yoni-Starkware]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)

---

crates/blockifier/src/versioned_constants.rs line 792 at r7 (raw file):

    #[serde(default)]
    validate_rounding_consts: ValidateRoundingConsts,
    #[serde(default)]

Why default?

Code quote:

#[serde(default)]

[nimrod-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)

---

crates/blockifier/src/versioned_constants.rs line 792 at r7 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Why default?

There were some tests for the logic of deserialization of OsConstantsRawJson that didn't explicitly add the struct to the json.
Removed it now.

[dorimedini-starkware]

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)

[Yoni-Starkware]

Reviewed 1 of 2 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)