feat(starknet_gateway): create transaction generator towards benchmarking

[ArniStarkware]

No description provided.

[reviewable-StarkWare]

This change is 

[ArniStarkware]

• wip: add tracing to the benched function #2983
• feat(starknet_gateway): create transaction generator towards benchmarking #2911 👈 (View in Graphite)
• refactor(starknet_gateway): revert gateway business logic towards bench test #3057
• main

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

[github-actions]

Artifacts upload workflows:

• Started at 2024-12-24T08:51:30Z
• Started at 2024-12-24T08:58:10Z
• Started at 2024-12-24T10:02:39Z
• Started at 2024-12-24T11:29:02Z
• Started at 2024-12-25T08:26:15Z
• Started at 2024-12-25T12:01:36Z
• Started at 2024-12-25T12:40:52Z
• Started at 2024-12-25T12:52:49Z
• Started at 2024-12-26T11:49:36Z
• Started at 2024-12-26T14:43:25Z
• Started at 2024-12-26T15:11:41Z
• Started at 2024-12-26T15:14:43Z
• Started at 2024-12-26T15:46:55Z
• Started at 2024-12-29T07:44:50Z
• Started at 2024-12-29T11:40:23Z
• Started at 2024-12-29T13:41:11Z
• Started at 2024-12-29T14:57:06Z
• Started at 2025-01-01T16:11:13Z
• Started at 2025-01-05T08:55:44Z
• Started at 2025-01-05T12:30:46Z
• Started at 2025-01-05T12:56:08Z
• Started at 2025-01-05T15:54:00Z

[github-actions]

Benchmark movements:
full_committer_flow performance regressed!
full_committer_flow time: [30.068 ms 30.144 ms 30.225 ms]
change: [+1.1724% +1.4805% +1.8053%] (p = 0.00 < 0.05)
Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severe

[yair-starkware]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @alonh5, @ArniStarkware, and @Yael-Starkware)

---

crates/starknet_gateway/src/gateway.rs line 29 at r1 (raw file):

pub mod gateway_test;

pub struct GatewayBusinessLogic {

Why not pub(crate)?

Code quote:

pub

---

crates/starknet_gateway/src/gateway.rs line 79 at r1 (raw file):

pub struct Gateway {
    pub mempool_client: SharedMempoolClient,
    pub business_logic: GatewayBusinessLogic,

Why not pub(crate)?

Code quote:

pub

---

crates/starknet_gateway/bench/gateway_bench.rs line 15 at r1 (raw file):

use starknet_gateway::bench_test_utils::{BenchTestSetup, BenchTestSetupConfig};

pub fn declare_benchmark(c: &mut Criterion) {

Can these functions not be pub?

Code quote:

pub

---

crates/starknet_gateway/bench/gateway_bench.rs line 28 at r1 (raw file):

    let test_setup = BenchTestSetup::new(tx_generator_config);
    c.bench_with_input(BenchmarkId::new("invoke", n_txs), &test_setup, |b, s| {

Can you give meaningful names?

Code quote:

b, s

---

crates/starknet_gateway/src/bench_test_utils.rs line 49 at r1 (raw file):

    sender_address: ContractAddress,
    test_contract_address: ContractAddress,
}

Can we reuse AccountTransactionGenerator from sn_api_test_utils?

Code quote:

struct TransactionGenerator {
    nonce_manager: NonceManager,
    sender_address: ContractAddress,
    test_contract_address: ContractAddress,
}

---

crates/starknet_gateway/src/bench_test_utils.rs line 71 at r1 (raw file):

}

impl BenchTestSetup {

Move the impl closer to the struct

Code quote:

impl BenchTestSetup

---

crates/starknet_gateway/src/bench_test_utils.rs line 73 at r1 (raw file):

impl BenchTestSetup {
    pub fn new(config: BenchTestSetupConfig) -> Self {
        let cairo_version = CairoVersion::Cairo0;

Why Cairo0?

Code quote:

Cairo0

---

crates/starknet_gateway/src/lib.rs line 1 at r1 (raw file):

#[cfg(any(feature = "testing", test))]

Why testing?

Code quote:

#[cfg(any(feature = "testing", test))]

---

crates/starknet_gateway/src/lib.rs line 2 at r1 (raw file):

#[cfg(any(feature = "testing", test))]
pub mod bench_test_utils;

Why pub?

Code quote:

pub mod bench_test_utils;

---

crates/starknet_gateway/src/lib.rs line 14 at r1 (raw file):

mod rpc_state_reader_test;
pub mod state_reader;
#[cfg(any(feature = "testing", test))]

Why testing?

Code quote:

#[cfg(any(feature = "testing", test))]

---

crates/starknet_gateway/Cargo.toml line 58 at r1 (raw file):

name = "gateway_bench"
path = "bench/gateway_bench.rs"
required-features = ["testing"]

Why do we need this?

Code quote:

required-features = ["testing"]

[ArniStarkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware and @yair-starkware)

---

crates/starknet_gateway/src/lib.rs line 12 at r7 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Instead of removing this can you add a "testing" feature and use it in the bench?

@yair-starkware - This is at odds with our discussions on the topic.

IIUC, the feature "testing" is about sharing utilities that other crates can use for testing. For example, starknet_api has a test_utils module in the "testing" feature so that the test utils can be used in the gateway's tests.
This is not the case here. Here are the utils used to test this specific crate.
Also, see this that indicates that the benchmark is not considered a test.

We can also create another feature, for example, "bench-testing." Or we can decide it is not an issue to have all this code in the "testing" feature - as, indeed, the bench is an external code that uses code from the module for testing purposes.

[yair-starkware]

crates/starknet_gateway/src/lib.rs line 12 at r7 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

@yair-starkware - This is at odds with our discussions on the topic.

IIUC, the feature "testing" is about sharing utilities that other crates can use for testing. For example, starknet_api has a test_utils module in the "testing" feature so that the test utils can be used in the gateway's tests.
This is not the case here. Here are the utils used to test this specific crate.
Also, see this that indicates that the benchmark is not considered a test.

We can also create another feature, for example, "bench-testing." Or we can decide it is not an issue to have all this code in the "testing" feature - as, indeed, the bench is an external code that uses code from the module for testing purposes.

Is it possible to make it an inner module of the bench test?

[ArniStarkware]

Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @alonh5, @Yael-Starkware, and @yair-starkware)

---

crates/starknet_gateway/src/lib.rs line 12 at r7 (raw file):

Previously, yair-starkware (Yair) wrote…

Is it possible to make it an inner module of the bench test?

It is possible to move the bench_test_utils to be an inner module of the bench test. We still have to address the visibility of state_reader_test_utils. This state reader is used for this bench.

[alonh5]

Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware, @Yael-Starkware, and @yair-starkware)

---

crates/starknet_gateway/src/lib.rs line 12 at r7 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

It is possible to move the bench_test_utils to be an inner module of the bench test. We still have to address the visibility of state_reader_test_utils. This state reader is used for this bench.

This is testing code, even if other crates currently don't use it. If it's possible please add the "testing" feature and use it in the bench.

[ArniStarkware]

crates/starknet_gateway/src/lib.rs line 12 at r7 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

This is testing code, even if other crates currently don't use it. If it's possible please add the "testing" feature and use it in the bench.

Done.

[alonh5]

Reviewed 1 of 2 files at r8, 2 of 3 files at r9, 4 of 4 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware and @yair-starkware)

[yair-starkware]

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

---

crates/starknet_gateway/src/lib.rs line 12 at r7 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Done.

I don't want to be too pedantic, but benchmarking is not testing.
You can collect benchmark data and test for regressions.

[ArniStarkware]

Reviewable status: 3 of 9 files reviewed, 1 unresolved discussion (waiting on @alonh5, @ArniStarkware, and @Yael-Starkware)

---

crates/starknet_gateway/src/lib.rs line 13 at r12 (raw file):

pub mod state_reader;
#[cfg(any(feature = "testing", test))]
pub mod state_reader_test_utils;

TODO: Move to mempool test utils.
Per our latest discussion.
We agreed this is non-blocking.

@alonh5 @yair-starkware

Code quote:

#[cfg(any(feature = "testing", test))]
pub mod state_reader_test_utils;

[alonh5]

Reviewed 3 of 5 files at r11, 1 of 6 files at r12.
Reviewable status: 4 of 9 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @Yael-Starkware)

[alonh5]

Reviewed 1 of 6 files at r12, all commit messages.
Reviewable status: 5 of 9 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware and @Yael-Starkware)

---

crates/starknet_gateway/bench/bench_lib.rs line 1 at r12 (raw file):

//! Benchmark module for the starknet gateway crate. It provides functionalities to benchmark

Why do we need this module and not just have the utils in bench/?

[ArniStarkware]

crates/starknet_gateway/bench/bench_lib.rs line 1 at r12 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Why do we need this module and not just have the utils in bench/?

Reverted.

[ArniStarkware]

Dismissed @alonh5 from a discussion.
Reviewable status: 5 of 9 files reviewed, all discussions resolved (waiting on @alonh5 and @Yael-Starkware)

[ArniStarkware]

Reviewed 1 of 2 files at r8, 2 of 6 files at r12, 3 of 4 files at r13, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alonh5 and @Yael-Starkware)

Reverted.