feat(starknet_os,starknet_patricia,starknet_committer): deserializable commitment info

[dorimedini-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[dorimedini-starkware]

• feat(starknet_os): add contract class component hashes in OS input #3994
• feat(starknet_os): add transaction related fields in OS input #3993
• feat(starknet_os): add more missing fields in OS input #3992
• feat(starknet_os): compiled classes in OS input #3991
• feat(starknet_os,starknet_patricia,starknet_committer): deserializable commitment info #3990 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

Artifacts upload workflows:

• Started at 2025-02-05T17:10:35Z
• Started at 2025-02-05T21:28:36Z
• Started at 2025-02-05T21:36:52Z
• Started at 2025-02-06T08:03:58Z
• Started at 2025-02-06T14:40:11Z
• Started at 2025-02-06T14:55:03Z
• Started at 2025-02-06T15:33:29Z

[dorimedini-starkware]

Reviewable status: 0 of 12 files reviewed, all discussions resolved (waiting on @aner-starkware and @Yoni-Starkware)

---

crates/starknet_patricia/Cargo.toml line 13 at r2 (raw file):

[features]
deserialize = ["ethnum/serde"]

required to deserialize EdgePath (which wraps U256 from ethnum crate)

Code quote:

deserialize = ["ethnum/serde"]

---

Cargo.toml line 244 at r2 (raw file):

starknet_mempool_types = { path = "crates/starknet_mempool_types", version = "0.0.0" }
starknet_monitoring_endpoint = { path = "crates/starknet_monitoring_endpoint", version = "0.0.0" }
starknet_os = { path = "crates/starknet_os", version = "0.0.0" }

not actually related to this PR, but it was missing

Code quote:

starknet_os = { path = "crates/starknet_os", version = "0.0.0" }

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [35.175 ms 35.641 ms 36.181 ms]
change: [+1.1232% +2.4691% +4.0685%] (p = 0.00 < 0.05)
Performance has regressed.
Found 18 outliers among 100 measurements (18.00%)
5 (5.00%) high mild
13 (13.00%) high severe

[dorimedini-starkware]

Reviewable status: 0 of 12 files reviewed, all discussions resolved (waiting on @aner-starkware and @Yoni-Starkware)

---

crates/starknet_os/src/io/os_input.rs line 17 at r3 (raw file):

    _updated_root: HashOutput,
    _tree_height: SubTreeHeight,
    _commitment_facts: HashMap<HashOutput, NodeData<L>>,

I'm not sure we really need this type of mapping...
We only really need

1. initial + final values of all accessed leaves
2. patricia witnesses for (1)

(1) can be structured without a hash->preimage map; leaf_index->value is better.
for (2), we don't actually need the node data of any node; only hashes along the path.
perhaps it would be better to create a specific Witnesses struct in the starknet_patricia crate, with only the required data?
(i.e, given a set of leaf indices, store all required node hashes to verify the leaves)
internally (in the struct) we can have a HashMap<NodeIndex, HashOutput> field; in the future we will be able to construct witnesses from a "real" state, for now we can simply deserialize in the CLI and verify (on construction of a Witnesses instance) that all required NodeIndexes are set.

@aner-starkware @Yoni-Starkware WDYT?

Code quote:

_commitment_facts: HashMap<HashOutput, NodeData<L>>,

[Yoni-Starkware]

Reviewed 10 of 12 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @dorimedini-starkware)

---

crates/starknet_os/src/io/os_input.rs line 17 at r3 (raw file):

Previously, dorimedini-starkware wrote…

I'm not sure we really need this type of mapping...
We only really need

1. initial + final values of all accessed leaves
2. patricia witnesses for (1)

(1) can be structured without a hash->preimage map; leaf_index->value is better.
for (2), we don't actually need the node data of any node; only hashes along the path.
perhaps it would be better to create a specific Witnesses struct in the starknet_patricia crate, with only the required data?
(i.e, given a set of leaf indices, store all required node hashes to verify the leaves)
internally (in the struct) we can have a HashMap<NodeIndex, HashOutput> field; in the future we will be able to construct witnesses from a "real" state, for now we can simply deserialize in the CLI and verify (on construction of a Witnesses instance) that all required NodeIndexes are set.

@aner-starkware @Yoni-Starkware WDYT?

Wait, the py type was BinaryFactDict = Dict[int, Tuple[int, ...]] and worked with the patricia.cairo impl.
I think we should first integrate with MS code before changing stuff there. I don't know the technical details

---

crates/starknet_os/src/io/os_input.rs line 27 at r3 (raw file):

    _contract_commitments: CommitmentInfo<ContractState>,
    _storage_commitments: HashMap<ContractAddress, CommitmentInfo<StarknetStorageValue>>,
    _class_commitments: CommitmentInfo<CompiledClassHash>,

Suggestion:

pub struct StarknetOsInput {
    _contract_state_commitment_info: CommitmentInfo<ContractState>,
    _address_to_storage_commitment_info: HashMap<ContractAddress, CommitmentInfo<StarknetStorageValue>>,
    _contract_class_commitment_info: CommitmentInfo<CompiledClassHash>,

[dorimedini-starkware]

Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @Yoni-Starkware)

---

crates/starknet_os/src/io/os_input.rs line 17 at r3 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Wait, the py type was BinaryFactDict = Dict[int, Tuple[int, ...]] and worked with the patricia.cairo impl.
I think we should first integrate with MS code before changing stuff there. I don't know the technical details

So I should keep the type as is?

---

crates/starknet_os/src/io/os_input.rs line 27 at r3 (raw file):

    _contract_commitments: CommitmentInfo<ContractState>,
    _storage_commitments: HashMap<ContractAddress, CommitmentInfo<StarknetStorageValue>>,
    _class_commitments: CommitmentInfo<CompiledClassHash>,

Done.

[Yoni-Starkware]

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

---

crates/starknet_os/src/io/os_input.rs line 17 at r3 (raw file):

Previously, dorimedini-starkware wrote…

So I should keep the type as is?

How are you going to convert BinaryFactDict into something the MS expect? what are they working with?
It would be best to use their impl for Patricia update at first

[dorimedini-starkware]

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

---

crates/starknet_os/src/io/os_input.rs line 17 at r3 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

How are you going to convert BinaryFactDict into something the MS expect? what are they working with?
It would be best to use their impl for Patricia update at first

We won't be using patricia update on the rust side: the py side patricia update is only used to collect the witnesses.
We can collect witnesses on the py side and load everything required by the OS into these commitment maps;
no patricia code required on the rust side.
Am I missing something?

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [36.158 ms 36.628 ms 37.197 ms]
change: [+1.1597% +2.6326% +4.3723%] (p = 0.00 < 0.05)
Performance has regressed.
Found 16 outliers among 100 measurements (16.00%)
4 (4.00%) low severe
1 (1.00%) low mild
4 (4.00%) high mild
7 (7.00%) high severe

[Yoni-Starkware]

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

[dorimedini-starkware]

Reviewed 4 of 11 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aner-starkware)

[dorimedini-starkware]

Reviewed 1 of 12 files at r1, 7 of 11 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aner-starkware)