feat(starknet_consensus_orchestrator): pass central objects from batcher to cende

[Yael-Starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[Yael-Starkware]

• test(starknet_batcher): verify execution info order matches the order of transactions in a block #3616
• feat(starknet_consensus_orchestrator): add compressed_state_diff to cende objects #3557
• feat(starknet_consensus_orchestrator): pass central objects from batcher to cende #3384 👈 (View in Graphite)
• test(starknet_consensus_orchestrator): central transaction execution info object test #3337
• feat(sequencing): add execution info central object #3238
• chore(sequencing): reorder central transactions fields #3230
• test(sequencing): add non-default values to central objects test #3198
• test(sequencing): add non-default values to central objects test #3197
• main

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

[Yael-Starkware]

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

---

crates/starknet_batcher_types/src/batcher_types.rs line 93 at r1 (raw file):

}

#[derive(Clone, Debug, Serialize, Deserialize, PartialEq)]

I spoke to @Itay-Tsabary-Starkware , and he approved that all derive(Clone) can be removed from this file.

Code quote:

Clone, 

[github-actions]

Artifacts upload workflows:

• Started at 2025-01-16T13:50:52Z
• Started at 2025-01-22T06:16:51Z
• Started at 2025-01-22T06:20:42Z
• Started at 2025-01-22T06:37:52Z
• Started at 2025-01-22T12:14:07Z
• Started at 2025-01-22T12:17:34Z
• Started at 2025-01-22T12:53:37Z

[DvirYo-starkware]

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

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 39 at r1 (raw file):

    transactions: Vec<CentralTransactionWritten>,
    execution_infos: Vec<CentralTransactionExecutionInfo>,
    bouncer_weights: BouncerWeights,

Move this to before the transactions fields

Code quote:

bouncer_weights: BouncerWeights,

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 196 at r1 (raw file):

    pub(crate) transactions: Vec<Transaction>,
    pub(crate) execution_infos: Vec<TransactionExecutionInfo>,
    pub(crate) bouncer_weights: BouncerWeights,

1. also here move before transaction fields.
2. add a comment or create a type alias type CentralBouncerWeights = BouncerWeight to be clear that "by mistake" the Rust struct is good for us.

Code quote:

pub(crate) bouncer_weights: BouncerWeights

---

crates/starknet_batcher_types/Cargo.toml line 16 at r1 (raw file):

[dependencies]
async-trait.workspace = true
blockifier = { workspace = true, features = ["transaction_serde"] }

Can you remind me again what this feature means?

Code quote:

features = ["transaction_serde"]

---

crates/starknet_batcher/src/batcher.rs line 430 at r1 (raw file):

        .await?;
        let execution_infos: Vec<_> =
            block_execution_artifacts.execution_infos.into_iter().map(|(_, info)| info).collect();

We need to get the execution info in the same order as the transactions in the block.
If we can sort them, the batcher would be the best, but I don't know if this information is available for the batcher. Another solution could be to pass them with the hashes to Cende, and it will sort them.

Code quote:

        let execution_infos: Vec<_> =
            block_execution_artifacts.execution_infos.into_iter().map(|(_, info)| info).collect();

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 389 at r1 (raw file):

                state_diff,
                transactions,
                execution_infos: *execution_infos,

I think you don't need the Box in the enum and thus do not need the * here.
Vector is already pointer to the heap so it should be of fixed small size on the stack.

Code quote:

execution_infos: *execution_infos

---

crates/starknet_batcher_types/src/batcher_types.rs line 2 at r1 (raw file):

use std::fmt::Debug;

Add a TODO to create the new crate with the blockifier api.

---

crates/starknet_batcher_types/src/batcher_types.rs line 96 at r1 (raw file):

#[derive(Debug, Serialize, Deserialize, PartialEq)]
pub struct CentralObjects {

As I said above, I don't think you need the Box here.

[DvirYo-starkware]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)

---

a discussion (no related file):
In addition, we also need to get the compressed state diff from the batcher. It can be done in a different PR if you prefer.

[Yael-Starkware]

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

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 389 at r1 (raw file):

Previously, DvirYo-starkware wrote…

I think you don't need the Box in the enum and thus do not need the * here.
Vector is already pointer to the heap so it should be of fixed small size on the stack.

I need the box since I get a warning, but you are right, it is not specific for the execution_info, I moved it to point on the entire CentralObjects.
Done.

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 39 at r1 (raw file):

Previously, DvirYo-starkware wrote…

Move this to before the transactions fields

not sure why, but ok done.

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 196 at r1 (raw file):

Previously, DvirYo-starkware wrote…

1. also here move before transaction fields.
2. add a comment or create a type alias type CentralBouncerWeights = BouncerWeight to be clear that "by mistake" the Rust struct is good for us.

Done.

---

crates/starknet_batcher/src/batcher.rs line 430 at r1 (raw file):

Previously, DvirYo-starkware wrote…

We need to get the execution info in the same order as the transactions in the block.
If we can sort them, the batcher would be the best, but I don't know if this information is available for the batcher. Another solution could be to pass them with the hashes to Cende, and it will sort them.

The order of the execution info is already in the same order as the transactions in the block as the block_builder built it, and is stored in an indexmap, which keeps this order:
please see

sequencer/crates/starknet_batcher/src/block_builder.rs

Line 226 in 89ce8a3

async fn collect_execution_results_and_stream_txs(

---

crates/starknet_batcher_types/Cargo.toml line 16 at r1 (raw file):

Previously, DvirYo-starkware wrote…

Can you remind me again what this feature means?

it is needed in order to derive deserialize,
for some unknown reason - that is the way this implement it in the blockifier, may worth clarifying with Dori why .

---

crates/starknet_batcher_types/src/batcher_types.rs line 2 at r1 (raw file):

Previously, DvirYo-starkware wrote…

Add a TODO to create the new crate with the blockifier api.

there is a monday task :
https://starkware.monday.com/boards/8251190975/pulses/8127522567

---

crates/starknet_batcher_types/src/batcher_types.rs line 96 at r1 (raw file):

Previously, DvirYo-starkware wrote…

As I said above, I don't think you need the Box here.

replied above

[Yael-Starkware]

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

---

a discussion (no related file):

Previously, DvirYo-starkware wrote…

In addition, we also need to get the compressed state diff from the batcher. It can be done in a different PR if you prefer.

will do in a separate PR

[DvirYo-starkware]

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

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 389 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

I need the box since I get a warning, but you are right, it is not specific for the execution_info, I moved it to point on the entire CentralObjects.
Done.

I checked that. The size of the variant DecisionReached(BatcherResult<DecisionReachedResponse>) is 544 bytes (the second largest variant is 40 bytes).
the 544 bytes is as follow:

• ThinStateDiff is 384
• BouncerWeights is 128
• Vec<TransactionExecutionInfo> is 24
• GasAmount is 8

the problem is not the CentralObject. We should make all the DecisionReachedResponse in a Box and not the CentralObject

---

crates/starknet_batcher/src/batcher.rs line 430 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

The order of the execution info is already in the same order as the transactions in the block as the block_builder built it, and is stored in an indexmap, which keeps this order:
please see

sequencer/crates/starknet_batcher/src/block_builder.rs

Line 226 in 89ce8a3

async fn collect_execution_results_and_stream_txs(

Because you use IndexMap? It's worth adding a test for that. If the transactions that we send to the recorder are not in the correct order, that is a big bug in starkness.

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 31 at r4 (raw file):

use url::Url;

type CentralBouncerWeights = BouncerWeights;

Move this to the central objects module.

Code quote:

type CentralBouncerWeights = BouncerWeights;

[Yael-Starkware]

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

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 389 at r1 (raw file):

Previously, DvirYo-starkware wrote…

I checked that. The size of the variant DecisionReached(BatcherResult<DecisionReachedResponse>) is 544 bytes (the second largest variant is 40 bytes).
the 544 bytes is as follow:

• ThinStateDiff is 384
• BouncerWeights is 128
• Vec<TransactionExecutionInfo> is 24
• GasAmount is 8

the problem is not the CentralObject. We should make all the DecisionReachedResponse in a Box and not the CentralObject

I agree that this is a better option.
Done.

---

crates/sequencing/papyrus_consensus_orchestrator/src/cende/mod.rs line 31 at r4 (raw file):

Previously, DvirYo-starkware wrote…

Move this to the central objects module.

Done.

---

crates/starknet_batcher/src/batcher.rs line 430 at r1 (raw file):

Previously, DvirYo-starkware wrote…

Because you use IndexMap? It's worth adding a test for that. If the transactions that we send to the recorder are not in the correct order, that is a big bug in starkness.

what exactly do you want me to test?
that indexmap keeps the order of insertion?

I can pass the tx_hashes if we think it's safer. now you made me worried about this.

[DvirYo-starkware]

Reviewed 8 of 8 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)

---

crates/starknet_batcher/src/batcher.rs line 430 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

what exactly do you want me to test?
that indexmap keeps the order of insertion?

I can pass the tx_hashes if we think it's safer. now you made me worried about this.

I am afraid that someone in the future will change something in the batcher that doesn't save the order of transactions (for example, use a hash map instead of an index map).

1. Document in the API of the batcher that the transaction returns in the order of the block
2. Add a test to ensure that the execution information returned is in the order of the block. Create a batcher, pass him transactions, call desiction_reached, and check that the order of the exec info is as expected.

---

crates/starknet_batcher_types/src/communication.rs line 107 at r5 (raw file):

    ValidateBlock(BatcherResult<()>),
    SendProposalContent(BatcherResult<SendProposalContentResponse>),
    StartHeight(BatcherResult<()>),

See how it is done in the link. there is some weird trick with Box on the types crates.

sequencer/crates/starknet_state_sync_types/src/communication.rs

Line 131 in 33987a8

async fn get_block(

[DvirYo-starkware]

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

---

crates/starknet_batcher_types/src/batcher_types.rs line 97 at r5 (raw file):

#[derive(Debug, Serialize, Deserialize, PartialEq)]
pub struct CentralObjects {
    // The box is required because the object is big and causes a large variance in enum size.

remove this

Code quote:

// The box is required because the object is big and causes a large variance in enum size.

[Yael-Starkware]

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

---

crates/starknet_batcher/src/batcher.rs line 430 at r1 (raw file):

Previously, DvirYo-starkware wrote…

I am afraid that someone in the future will change something in the batcher that doesn't save the order of transactions (for example, use a hash map instead of an index map).

1. Document in the API of the batcher that the transaction returns in the order of the block
2. Add a test to ensure that the execution information returned is in the order of the block. Create a batcher, pass him transactions, call desiction_reached, and check that the order of the exec info is as expected.

ok, will do it in a separate PR :
moday task : https://starkware.monday.com/boards/8251190975/pulses/8296565726

---

crates/starknet_batcher_types/src/batcher_types.rs line 97 at r5 (raw file):

Previously, DvirYo-starkware wrote…

remove this

Done.

---

crates/starknet_batcher_types/src/communication.rs line 107 at r5 (raw file):

Previously, DvirYo-starkware wrote…

See how it is done in the link. there is some weird trick with Box on the types crates.

sequencer/crates/starknet_state_sync_types/src/communication.rs

Line 131 in 33987a8

async fn get_block(

Done.

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.800 ms 34.831 ms 34.864 ms]
change: [-4.3749% -2.7802% -1.3546%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
6 (6.00%) high mild

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.782 ms 34.815 ms 34.850 ms]
change: [-4.1179% -2.5804% -1.2192%] (p = 0.00 < 0.05)
Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
10 (10.00%) high mild
3 (3.00%) high severe

[DvirYo-starkware]

Reviewed 7 of 13 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: 9 of 14 files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @Yael-Starkware)

[DvirYo-starkware]

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