feat(starknet_integration_tests): use monitoring instead of reading storage

[yair-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload workflows:

• Started at 2025-01-28T10:15:16Z
• Started at 2025-01-28T11:18:56Z
• Started at 2025-01-28T11:28:29Z
• Started at 2025-01-28T11:30:33Z
• Started at 2025-01-28T12:30:59Z
• Started at 2025-01-28T12:58:12Z
• Started at 2025-01-28T14:31:59Z
• Started at 2025-01-29T10:12:49Z
• Started at 2025-01-30T08:00:10Z
• Started at 2025-01-30T08:08:08Z
• Started at 2025-01-30T11:24:20Z

[yair-starkware]

• feat(starknet_integration_tests): use batched txs metric instead of account nonce #3761
• feat(starknet_integration_tests): use monitoring instead of reading storage #3755 👈 (View in Graphite)
• fix(starknet_batcher,starknet_sequencer_infra): move metrics registration #3754
• main

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

[graphite-app]

Graphite Automations

"Yair - Auto-assign" took an action on this PR • (01/28/25)

1 assignee was added to this PR based on Yair's automation.

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @alonh5, @nadin-Starkware, and @yair-starkware)

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 338 at r1 (raw file):

/// Gets the latest block number from the batcher's metrics.
async fn get_latest_block_number(batcher_monitoring_client: &MonitoringClient) -> BlockNumber {

This is not the batcher's monitoring client, but the node's. Please rephrase accordingly (throughout).

Code quote:

batcher_monitoring_client: &MonitoringClient

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 352 at r1 (raw file):

async fn get_account_nonce(
    batcher_monitoring_client: &MonitoringClient,
    storage_reader: &StorageReader,

Do you plan on addressing this as well? If so, please add a todo.

Code quote:

storage_reader: &StorageReader,

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 355 at r1 (raw file):

    contract_address: ContractAddress,
) -> Nonce {
    let block_number = get_latest_block_number(batcher_monitoring_client).await;

WDYT about renaming this fn? The current context was with the batcher storage reader, hence this implied we are referring to the batcher block height. However, and following the previous comment, the fn name no longer indicates the block we are referring to.

Code quote:

get_latest_block_number

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 363 at r1 (raw file):

}

/// Sample a storage until sufficiently many blocks have been stored. Returns an error if after

Please update.

Code quote:

Sample a storage until sufficiently many blocks have been stored.

[yair-starkware]

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @alonh5, @Itay-Tsabary-Starkware, and @nadin-Starkware)

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 338 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

This is not the batcher's monitoring client, but the node's. Please rephrase accordingly (throughout).

It's the executable that contains the batcher, do we call each executable a node?

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 352 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Do you plan on addressing this as well? If so, please add a todo.

I'm not sure, this is not something that we can get from monitoring, we need RPC for this - @alonh5

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 355 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

WDYT about renaming this fn? The current context was with the batcher storage reader, hence this implied we are referring to the batcher block height. However, and following the previous comment, the fn name no longer indicates the block we are referring to.

The function is still returning the same block number it returned before thanks to the call to prev.
IIUC, your suggestion is to change that and fix all the callers?
If so, I will do it in a separate PR.

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 363 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please update.

Done.

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @alonh5, @nadin-Starkware, and @yair-starkware)

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 338 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

It's the executable that contains the batcher, do we call each executable a node?

Node: the set of all components (gateway, batcher, etc.)
Executable: an instance running the binary with a config, setting up one or more components

A node is run using one or more executables.

I do see your point though, it's the (executable that runs the batcher)'s monitoring client.
Can this be better emphasized though?

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 355 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

The function is still returning the same block number it returned before thanks to the call to prev.
IIUC, your suggestion is to change that and fix all the callers?
If so, I will do it in a separate PR.

Not at all, I suggested about renaming the fn from get_latest_block_number to get_batcher_latest_block_number, or something of that sort.
Motivation: to indicate it is about the batcher and not the consensus for example.

[alonh5]

Reviewed 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Itay-Tsabary-Starkware, @nadin-Starkware, and @yair-starkware)

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 352 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

I'm not sure, this is not something that we can get from monitoring, we need RPC for this - @alonh5

We don't actually need the nonce in this case, we just wanted to check all txs in general went in blocks. Can you use the n_txs metrics here?

[yair-starkware]

Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @alonh5, @Itay-Tsabary-Starkware, and @nadin-Starkware)

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 338 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Node: the set of all components (gateway, batcher, etc.)
Executable: an instance running the binary with a config, setting up one or more components

A node is run using one or more executables.

I do see your point though, it's the (executable that runs the batcher)'s monitoring client.
Can this be better emphasized though?

discussed f2f, leaving the current name

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 355 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Not at all, I suggested about renaming the fn from get_latest_block_number to get_batcher_latest_block_number, or something of that sort.
Motivation: to indicate it is about the batcher and not the consensus for example.

Done.

[yair-starkware]

crates/starknet_integration_tests/src/sequencer_manager.rs line 352 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

We don't actually need the nonce in this case, we just wanted to check all txs in general went in blocks. Can you use the n_txs metrics here?

Will do in a following PR

[alonh5]

Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @Itay-Tsabary-Starkware, @nadin-Starkware, and @yair-starkware)

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 285 at r2 (raw file):

            })
            .expect("Node 0 should be either idle or running.")
    }

Let's take one of the running nodes, it doesn't have to be the first node.

Suggestion:

    fn running_batcher_monitoring_client(&self) -> &MonitoringClient {
        self.idle_nodes
            .get(&0)
            .map(|node| node.batcher_monitoring_client())
            .or_else(|| {
                self.running_nodes
                    .get(&0)
                    .map(|running_node| running_node.node_setup.batcher_monitoring_client())
            })
            .expect("Node 0 should be either idle or running.")
    }

[alonh5]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Itay-Tsabary-Starkware, @nadin-Starkware, and @yair-starkware)

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 352 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

Will do in a following PR

np

[yair-starkware]

crates/starknet_integration_tests/src/sequencer_manager.rs line 352 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

np

https://reviewable.io/reviews/starkware-libs/sequencer/3761

[yair-starkware]

Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @alonh5, @Itay-Tsabary-Starkware, and @nadin-Starkware)

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 285 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Let's take one of the running nodes, it doesn't have to be the first node.

Done.

[alonh5]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Itay-Tsabary-Starkware, @nadin-Starkware, and @yair-starkware)

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 285 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

Done.

Rename as suggested?

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 346 at r4 (raw file):

    )
    .prev() // The metric is the height marker so we need to subtract 1 to get the latest.
    .unwrap()

expect in both

Code quote:

            .unwrap(),
    )
    .prev() // The metric is the height marker so we need to subtract 1 to get the latest.
    .unwrap()

[yair-starkware]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alonh5, @Itay-Tsabary-Starkware, and @nadin-Starkware)

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 285 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Rename as suggested?

Done.

---

crates/starknet_integration_tests/src/sequencer_manager.rs line 346 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

expect in both

Done.

[Itay-Tsabary-Starkware]

Reviewed 3 of 4 files at r1, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alonh5 and @nadin-Starkware)

[alonh5]

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

[graphite-app]

Merge activity

• Jan 30, 6:25 AM EST: Graphite disabled "merge when ready" on this PR due to: We hit a GitHub rate limit while trying to merge. If you are attempting to merge a large stack, breaking it up into smaller chunks may help..

[yair-starkware]

Reviewed 1 of 4 files at r1, 3 of 3 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nadin-Starkware)