Conversation
Lines of code reportTotal lines added: Detailed view |
|
Don't merge yet, still missing a row in the measures .md |
There was a problem hiding this comment.
Pull Request Overview
This PR adds tooling to measure L2 transactions per second (TPS) performance with a single SP1 prover on an RTX 4090. The changes introduce a new TPS load testing mode alongside the existing burst mode, and a genesis file generator for creating test accounts.
Key changes:
- Added continuous TPS load testing capability with configurable rate
- Implemented genesis file and private key generation for testing
- Refactored CLI structure to support subcommands (Load, GenerateGenesis)
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tooling/load_test/src/main.rs | Refactored CLI to subcommands; added TPS test mode, genesis generator, and improved error handling |
| tooling/load_test/Cargo.toml | Added dependencies for genesis generation (sha3, serde_json, ethrex-config) |
| docs/l2/bench/tps.md | Added TPS benchmark documentation with test results and reproduction steps |
| crates/l2/Makefile | Removed debug log level override for prover |
| crates/common/config/networks.rs | Added "local-devnet-l2" network support |
Comments suppressed due to low confidence (1)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
Don't merge this until reviewed by @mationorato |
ElFantasma
left a comment
There was a problem hiding this comment.
This branch is ~4 months old — please rebase on main and verify it compiles before requesting re-review or close it if it's no longer relevant
| address, | ||
| GenesisAccount { | ||
| code: Bytes::new(), | ||
| storage: HashMap::new(), |
There was a problem hiding this comment.
GenesisAccount.storage is BTreeMap<U256, U256>, but this passes HashMap::new(). This won't compile.
| storage: HashMap::new(), | |
| storage: Default::default(), |
Or explicitly BTreeMap::new() with the appropriate import.
| default_value_t = 1000, | ||
| help = "Number of transactions to send for each account." | ||
| help = "Name of the network or genesis file. Supported: mainnet, holesky, sepolia, hoodi, local-devnet-l2. Default: mainnet", | ||
| value_parser = clap::value_parser!(Network), |
There was a problem hiding this comment.
clap::value_parser!(Network) requires Network to implement FromStr (or ValueEnum), but Network only has From<&str>. This won't compile as-is.
Either add impl FromStr for Network in ethrex-config, or use a custom value parser here, e.g.:
value_parser = |s: &str| -> Result<Network, String> { Ok(Network::from(s)) },| let mut account_index = 0; | ||
| let mut tasks = JoinSet::new(); | ||
|
|
||
| loop { |
There was a problem hiding this comment.
tps_load_test runs an infinite loop and never returns Ok(()) — it can only exit via ? propagation or a panic. This means the caller's elapsed-time printout (line ~594) is dead code when LoadType::Tps is selected. Consider either:
- Adding a
--durationflag so the loop exits after a set time, or - Printing periodic stats inside the loop (e.g., every 10s), since the final summary will never be reached.
| chain_id: Some(chain_id), | ||
| value, | ||
| nonce: Some(current_nonce), | ||
| max_fee_per_gas: Some(i64::MAX as u64), |
There was a problem hiding this comment.
nit: i64::MAX as u64 is an unusual way to express "maximum willingness to pay". Consider u64::MAX directly, or better yet, fetch the current base fee and apply a multiplier — this avoids potentially overpaying if the load test is ever pointed at a real network.
| "holesky" => Network::PublicNetwork(PublicNetwork::Holesky), | ||
| "mainnet" => Network::PublicNetwork(PublicNetwork::Mainnet), | ||
| "sepolia" => Network::PublicNetwork(PublicNetwork::Sepolia), | ||
| "local-devnet-l2" => Network::LocalDevnetL2, |
There was a problem hiding this comment.
The "local-devnet-l2" arm is added before the GenesisPath fallback, which is correct. However, note the comment above says "we don't allow to manually specify the local devnet genesis" — this new arm intentionally allows it for the L2 variant. Might be worth updating that comment to clarify the distinction.
Motivation
To measure the maximum transactions per second that the current L2 can withstand, using a single prover with SP1 backend running on a RTX 4090.
Multiple L2 setups have been tested.
This also adds some tools used to easily test these scenarios