feat(starknet_consensus_orchestrator): add sierras to cende blob

[Yael-Starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[Yael-Starkware]

• feat(starknet_consensus_orchestrator): add sierras to cende blob #3955 👈 (View in Graphite)
• main

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

[Yael-Starkware]

Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)

---

crates/starknet_consensus_orchestrator/src/cende/central_objects_test.rs line 601 at r1 (raw file):

#[tokio::test]
async fn test_get_sierras_and_casms() {

note: the test is not operational yet, it is just the skeleton,
will be enabled in the next PRs, once I have the transactions testing instances done.

---

crates/starknet_consensus_orchestrator/src/cende/mod.rs line 254 at r1 (raw file):

    ) -> CendeAmbassadorResult<()> {
        // TODO(dvir): as optimization, call the `into` and other preperation when writing to AS. -
        // Dvir, what is this? is it still relevant?

Dvir, question for you. is this still relevant?

Code quote:

        // TODO(dvir): as optimization, call the `into` and other preperation when writing to AS. -
        // Dvir, what is this? is it still relevant?

---

crates/starknet_consensus_orchestrator/src/cende/mod.rs line 331 at r1 (raw file):

    // TODO(Yael): remove the executable transactions and convert the rpc_transactions directly to
    // the central format.
    pub(crate) transactions: Vec<Transaction>,

this vector is here temporarily, will be deleted in the following PRs.

Code quote:

pub(crate) transactions: Vec<Transaction>,

[Yael-Starkware]

Reviewable status: 0 of 7 files reviewed, 5 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)

---

crates/starknet_consensus_orchestrator/src/cende/mod.rs line 255 at r1 (raw file):

        // TODO(dvir): as optimization, call the `into` and other preperation when writing to AS. -
        // Dvir, what is this? is it still relevant?
        let block_number = blob_parameters.block_info.block_number;

This is the same logic we previously had in the From function,
The only logic that has changed is how I get the contract classes.

---

crates/starknet_consensus_orchestrator/src/cende/mod.rs line 243 at r1 (raw file):

}

impl From<BlobParameters> for AerospikeBlob {

Since the new from logic requires self to be present, I moved it all to "prepare_blob_for_next_height",
The only logic that has changed is how I get the contract classes.

Code quote:

impl From<BlobParameters> for AerospikeBlob {

[Yael-Starkware]

Reviewable status: 0 of 9 files reviewed, 3 unresolved discussions

---

crates/starknet_consensus_orchestrator/src/cende/mod.rs line 255 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

This is the same logic we previously had in the From function,
The only logic that has changed is how I get the contract classes.

the contract classes and the transactions

[github-actions]

Artifacts upload workflows:

• Started at 2025-02-09T11:12:08Z
• Started at 2025-02-10T07:09:17Z
• Started at 2025-02-10T11:22:23Z

[DvirYo-starkware]

Reviewed 1 of 7 files at r1, 8 of 9 files at r2, all commit messages.
Reviewable status: 9 of 10 files reviewed, 10 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)

---

crates/starknet_consensus_orchestrator/src/cende/mod.rs line 254 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

Dvir, question for you. is this still relevant?

Yes. We will not do it for now, but it is a possible optimization. Remove your comment // Dvir, what is this? is it still relevant?

---

crates/starknet_consensus_orchestrator/src/cende/central_objects.rs line 322 at r2 (raw file):

            sierra_version: into_string_tuple(SierraVersion::from_str(
                &sierra.contract_class_version,
            )?),

Please compare this with a real class.

Code quote:

            sierra_program_size: sierra.sierra_program.len(),
            abi_size: sierra.abi.len(),
            sierra_version: into_string_tuple(SierraVersion::from_str(
                &sierra.contract_class_version,
            )?),

---

crates/starknet_consensus_orchestrator/src/cende/central_objects.rs line 370 at r2 (raw file):

    L1Handler(CentralL1HandlerTransaction),
}

document that the Option<&SierraContractClass> is only for declare

---

crates/starknet_consensus_orchestrator/src/cende/central_objects.rs line 492 at r2 (raw file):

    }
}

I don't like the way it is done. Can we do something like function get_innder_declrae and only if it return Some does all the things of declare?

---

crates/starknet_consensus_orchestrator/resources/central_sierra_contract_class.json line 6 at r2 (raw file):

    "0x1",
    "0x2"
  ],

special reason for the change here?

---

crates/starknet_consensus_orchestrator/src/cende/mod.rs line 63 at r2 (raw file):

    execution_infos: Vec<CentralTransactionExecutionInfo>,
    contract_classes: Vec<CentralSierraContractClassEntry>,
    compiled_classes: Vec<CentralCasmContractClassEntry>,

Consider using raw tuple here

Code quote:

    contract_classes: Vec<CentralSierraContractClassEntry>,
    compiled_classes: Vec<CentralCasmContractClassEntry>,

---

crates/starknet_consensus_orchestrator/resources/central_declare_tx.json line 30 at r2 (raw file):

        "sierra_program_size": 3,
        "sierra_version": [
            "0x0",

What is the reason for the change? Did you use real tx json?

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 439 at r2 (raw file):

            })
            .await
            // TODO(shahak): Do not panic here.

remove this

Code quote:

// TODO(shahak): Do not panic here

[Yael-Starkware]

Reviewable status: 9 of 10 files reviewed, 10 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)

---

crates/starknet_consensus_orchestrator/resources/central_declare_tx.json line 30 at r2 (raw file):

Previously, DvirYo-starkware wrote…

What is the reason for the change? Did you use real tx json?

no reason

---

crates/starknet_consensus_orchestrator/resources/central_sierra_contract_class.json line 6 at r2 (raw file):

Previously, DvirYo-starkware wrote…

special reason for the change here?

yes , it needs to be formatted to a three numbers string ,
in the declare tx

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 439 at r2 (raw file):

Previously, DvirYo-starkware wrote…

remove this

as discussed with matan : need to print an error and proceed normally.

[Yael-Starkware]

Reviewable status: 9 of 10 files reviewed, 10 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)

---

crates/starknet_consensus_orchestrator/src/cende/central_objects.rs line 492 at r2 (raw file):

Previously, DvirYo-starkware wrote…

I don't like the way it is done. Can we do something like function get_innder_declrae and only if it return Some does all the things of declare?

as discussed f2f, from process_transactions, I will call a different function for declare and a different function for the rest , and by that eliminate returning none values.

[Yael-Starkware]

Reviewable status: 6 of 10 files reviewed, 9 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 439 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

as discussed with matan : need to print an error and proceed normally.

done.

---

crates/starknet_consensus_orchestrator/src/cende/central_objects.rs line 322 at r2 (raw file):

Previously, DvirYo-starkware wrote…

Please compare this with a real class.

code copied form here:

sequencer/crates/starknet_class_manager_types/src/transaction_converter.rs

Line 195 in 96b113d

let class_info = ClassInfo {

---

crates/starknet_consensus_orchestrator/src/cende/central_objects.rs line 370 at r2 (raw file):

Previously, DvirYo-starkware wrote…

document that the Option<&SierraContractClass> is only for declare

Done.

---

crates/starknet_consensus_orchestrator/src/cende/central_objects.rs line 492 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

as discussed f2f, from process_transactions, I will call a different function for declare and a different function for the rest , and by that eliminate returning none values.

done.

---

crates/starknet_consensus_orchestrator/src/cende/mod.rs line 254 at r1 (raw file):

Previously, DvirYo-starkware wrote…

Yes. We will not do it for now, but it is a possible optimization. Remove your comment // Dvir, what is this? is it still relevant?

Done.

---

crates/starknet_consensus_orchestrator/src/cende/mod.rs line 63 at r2 (raw file):

Previously, DvirYo-starkware wrote…

Consider using raw tuple here

not sure it will be pretty

[DvirYo-starkware]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: 9 of 10 files reviewed, 5 unresolved discussions (waiting on @dafnamatsry)

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 442 at r3 (raw file):

                error!("Failed to prepare blob for next height: {e:?}");
            })
            .ok();

for what the ok()?

Code quote:

.ok();

---

crates/starknet_consensus_orchestrator/src/cende/central_objects_test.rs line 135 at r3 (raw file):

    "central_transaction_execution_info.json";

fn resource_bounds() -> AllResourceBounds {

What is the reason for the change here?

---

crates/starknet_consensus_orchestrator/src/cende/central_objects_test.rs line 147 at r3 (raw file):

}

fn declare_class_hash() -> ClassHash {

Can we do it as a const? also in declare_compiled_class_hash

---

crates/starknet_consensus_orchestrator/src/cende/central_objects_test.rs line 649 at r3 (raw file):

    let expected_sierra_contract_classes = vec![(declare_class_hash(), sierra_contract_class()); 2];
    assert_eq!(aerospike_blob.contract_classes, expected_sierra_contract_classes);

Now that we have more complex logic to convert txs to central format I think we should test it.
something like, creating a vector with one tx from each type, convert and check the result. wdyt?

[DvirYo-starkware]

Reviewed 1 of 9 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)

[Yael-Starkware]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 442 at r3 (raw file):

Previously, DvirYo-starkware wrote…

for what the ok()?

otherwise the compiler is complaining for unused result.
changed it to let _ =

---

crates/starknet_consensus_orchestrator/src/cende/central_objects_test.rs line 135 at r3 (raw file):

Previously, DvirYo-starkware wrote…

What is the reason for the change here?

yes, this is the typed that the consensusTransaction uses.

---

crates/starknet_consensus_orchestrator/src/cende/central_objects_test.rs line 147 at r3 (raw file):

Previously, DvirYo-starkware wrote…

Can we do it as a const? also in declare_compiled_class_hash

no, because it calls a function, but is can be lazy static. but I don't think it makes it more readable.

---

crates/starknet_consensus_orchestrator/src/cende/central_objects_test.rs line 649 at r3 (raw file):

Previously, DvirYo-starkware wrote…

Now that we have more complex logic to convert txs to central format I think we should test it.
something like, creating a vector with one tx from each type, convert and check the result. wdyt?

we already check the conversion of each transaction separately. so I don't see any reason to check the conversion again.
I added a check for the keeping the order and length of the transaction list.

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.841 ms 34.877 ms 34.922 ms]
change: [-3.9227% -2.4170% -1.0810%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
3 (3.00%) high mild
4 (4.00%) high severe

[DvirYo-starkware]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry)