wip(benchmark): create simpler version of benchmark runner

[meyer9]

The web interface can stay in base/benchmark and the benchmark runner will live here eventually. This removes a lot of the old dependencies on Geth and simplifies the tool significantly.

[cb-heimdall]

🟡 Heimdall Review Status

Requirement	Status	More Info
Reviews	🟡 0/1	Denominator calculation Show calculation 1 if user is bot 0 1 if user is external 0 2 if repo is sensitive 0 From .codeflow.yml 1 Additional review requirements Show calculation Max 0 0 From CODEOWNERS 0 Global minimum 0 Max 1 1 1 if commit is unverified 1 Sum 2

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
base	Ignored	Preview	May 5, 2026 2:37pm

[github-actions]

BuilderClient creates a BaseRethNodeClient internally, which uses self.options.reth_bin as the binary (via self.binary()). The builder_bin field in ClientOptions is never read — so the builder client runs the reth binary instead of the builder binary.

You likely need to swap reth_bin for builder_bin here (or override the binary in the inner client):

Suggested change

	let fb_block_time = options.flashblocks_block_time_ms.unwrap_or(block_time_ms / 4);
	let inner = BaseRethNodeClient::new(options, internal, port_manager, None);
	let mut options_for_inner = options.clone();
	options_for_inner.reth_bin = options.builder_bin.clone();
	let inner = BaseRethNodeClient::new(options_for_inner, internal, port_manager, None);

[github-actions]

This only handles single JSON-RPC requests. If the load-test binary sends batch requests (JSON arrays), from_slice::<JsonRpcRequest> will fail and the entire batch gets forwarded to the upstream via proxy_raw — bypassing the mempool interception.

Consider also trying to parse as Vec<JsonRpcRequest> in the error branch, extracting any eth_sendRawTransaction calls from the batch.

[github-actions]

This function looks incorrect for extracting histogram sum and count. In the prometheus_parse crate, HistogramCount represents bucket entries with less_than (the bucket boundary) and count (the cumulative count up to that boundary). The histogram _sum and _count are separate parsed entries — _sum is typically not accessible from the bucket list.

The current code:

• sum: filters for less_than == +Inf and sums their count — but +Inf bucket's count is the total sample count, not the sum of observed values
• count: takes the last bucket's count, which happens to be the same total count

So both sum and count end up returning the total count, and the delta computation becomes (count_delta) / (count_delta) == 1.0 always.

You likely need to extract _sum and _count from separate Sample entries rather than from bucket data.

[github-actions]

Per CLAUDE.md: "All use imports must be at the top of the file or the top of a mod block. Never place use statements inside function bodies." This use alloy_primitives::keccak256 should be moved to the top of the file alongside the other alloy imports.

[github-actions]

Same issue — use alloy_rlp::Encodable should be at the top of the file, not inside a function body.

[github-actions]

These keys don't match the values in PORTING_REFERENCE.md which specifies:

• Batcher key: 0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80
• Prefund key: 0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d

If these were intentionally changed, the porting reference should be updated accordingly. If not, these need to match the Go source so benchmarks produce comparable results.

[github-actions]

Missing // SAFETY: comment on the unsafe block. Document why this is sound — e.g. that pre_exec runs after fork() in the child process, sigemptyset/pthread_sigmask are async-signal-safe, and no Rust invariants are broken because the child hasn't yet executed any Rust code post-fork.

Suggested change

	unsafe {
	cmd.pre_exec(|| {
	let mut sigset = std::mem::zeroed::<libc::sigset_t>();
	libc::sigemptyset(&mut sigset);
	libc::pthread_sigmask(libc::SIG_SETMASK, &sigset, std::ptr::null_mut());
	Ok(())
	});
	}
	// SAFETY: `pre_exec` runs in the forked child process before exec.
	// `sigemptyset` and `pthread_sigmask` are async-signal-safe (POSIX),
	// so they are safe to call in this context. We clear the signal mask
	// so the child process does not inherit blocked signals from the
	// parent's tokio runtime.
	unsafe {
	cmd.pre_exec(|| {
	let mut sigset = std::mem::zeroed::<libc::sigset_t>();
	libc::sigemptyset(&mut sigset);
	libc::pthread_sigmask(libc::SIG_SETMASK, &sigset, std::ptr::null_mut());
	Ok(())
	});
	}

[github-actions]

Bug: The Go source (per PORTING_REFERENCE.md §5) increments the timestamp from the previous block's timestamp by the block time, i.e. self.base.head_block_timestamp + block_time_secs. Using wall-clock time (SystemTime::now()) here means:

1. Timestamps can drift if block production doesn't match real-time.
2. If two blocks are proposed quickly (e.g. during setup), the timestamp could be the same or very close, causing the node to reject the payload.
3. Results are non-reproducible across runs.

Suggested change

	let now_secs = std::time::SystemTime::now()
	.duration_since(std::time::UNIX_EPOCH)
	.expect("system time after epoch")
	.as_secs();
	let block_time_secs = block_time.as_secs().max(1);
	let next_timestamp = now_secs + block_time_secs;
	let next_timestamp = self.base.head_block_timestamp + block_time.as_secs().max(1);

[github-actions]

Three .expect() calls on user-supplied data (prefund_key). If the prefund key is invalid hex or not 32 bytes, this panics the benchmark runner instead of returning a BenchmarkError. Convert to ?-based error handling:

Suggested change

	let alloc = {
	use alloy_signer_local::PrivateKeySigner;
	let key = prefund_key.trim_start_matches("0x");
	let signer = PrivateKeySigner::from_bytes(
	&alloy_primitives::hex::decode(key)
	.expect("valid hex prefund key")
	.as_slice()
	.try_into()
	.expect("32-byte private key"),
	)
	.expect("valid private key");
	let addr = format!("{:?}", signer.address());
	serde_json::json!({
	addr: {
	"balance": "0x3635C9ADC5DEA00000000",
	}
	})
	let alloc = {
	use alloy_signer_local::PrivateKeySigner;
	let key = prefund_key.trim_start_matches("0x");
	let key_bytes = alloy_primitives::hex::decode(key)
	.map_err(|e| BenchmarkError::Config(format!("invalid prefund key hex: {e}")))?;
	let key_array: [u8; 32] = key_bytes.as_slice()
	.try_into()
	.map_err(|_| BenchmarkError::Config(format!("prefund key must be 32 bytes, got {}", key_bytes.len())))?;
	let signer = PrivateKeySigner::from_bytes(&key_array)
	.map_err(|e| BenchmarkError::Config(format!("invalid prefund private key: {e}")))?;
	let addr = format!("{:?}", signer.address());
	serde_json::json!({
	addr: {
	"balance": "0x3635C9ADC5DEA00000000",
	}
	})
	};

Also: use alloy_signer_local::PrivateKeySigner is inside a function body. Per CLAUDE.md, use imports must be at the top of the file.

[github-actions]

Two issues:

1. use inside function body — use std::io::Seek should be at the top of the file, per project conventions.

2. Potential overflow — -(max_bytes as i64) will produce a wrong value if max_bytes > i64::MAX as u64. While unlikely in practice (this is called with 1MB), a defensive check or using i64::try_from(max_bytes) would be safer.

[github-actions]

This file is a byte-for-byte copy of crates/infra/benchmark/PORTING_REFERENCE.md. Having it at the repo root seems unintentional — the benchmark-specific porting reference should live only in the benchmark crate directory. Consider removing this duplicate.

[github-actions]

new_payload already sets self.base.head_block_hash, head_block_number, and head_block_timestamp internally (see consensus/mod.rs:197). This manual reassignment on line 259 is redundant — you can remove it.

[github-actions]

batch_send_txs silently swallows all HTTP errors — it logs warnings but always returns Ok(()). If the RPC endpoint is down or every batch fails, propose will continue as if transactions were submitted, producing empty blocks without any indication of failure to the caller.

Consider propagating at least one error from the JoinSet, or returning the count of failed batches so the caller can decide whether to continue:

async fn batch_send_txs(&self, txs: &[Bytes]) -> Result<(), BenchmarkError> {
    const BATCH_SIZE: usize = 100;
    let client = reqwest::Client::new();
    let mut set = JoinSet::new();

    for batch in txs.chunks(BATCH_SIZE) {
        let batch: Vec<serde_json::Value> = batch
            .iter()
            .map(|tx| {
                serde_json::json!({
                    "jsonrpc": "2.0",
                    "method": "eth_sendRawTransaction",
                    "params": [format!("0x{}", hex::encode(tx))],
                    "id": 1,
                })
            })
            .collect();

        let url = self.rpc_url.clone();
        let c = client.clone();
        set.spawn(async move {
            c.post(&url).json(&batch).send().await.map(|_| ())
        });
    }

    let mut last_err = None;
    while let Some(result) = set.join_next().await {
        if let Ok(Err(e)) = result {
            tracing::warn!(error = %e, "batch_send_txs http error");
            last_err = Some(e);
        }
    }
    if let Some(e) = last_err {
        return Err(BenchmarkError::Client(format!("batch tx send failed: {e}")));
    }
    Ok(())
}

[github-actions]

This changes TransferPayload::default() from sending 1,000–100,000 wei transfers to sending zero-value transfers for all users of the load-test crate, not just the benchmark runner. If any other caller relies on the load-test binary sending non-zero transfers (e.g. to verify balance changes or trigger specific EVM behavior), this silently breaks them.

If zero-value is only needed for benchmarking, consider making this configurable via the YAML config instead of changing the default.

[github-actions]

Cancellation safety issue: sequencer.propose() is not cancel-safe. When ready completes, the in-flight propose future is dropped. If propose was mid-way through (e.g., new_payload completed but the subsequent update_fork_choice(None) on line 260 hasn't run yet), the sequencer's internal state (head_block_hash, etc.) will be stale or inconsistent for the next propose call in the measured loop.

Consider running the setup propose to completion before re-checking readiness:

loop {
    tokio::select! {
        biased;
        result = &mut ready => {
            result?;
            break;
        }
        result = sequencer.propose(&mempool, block_time, gas_limit) => {
            result?;
            // Check readiness after each full block rather than mid-propose
        }
    }
}

Actually, the current structure already does this since propose is a single arm — but biased means ready is polled first, and if ready resolves exactly when propose has partially completed an await point, the propose future is cancelled. A safer pattern would be to poll ready only between blocks:

loop {
    if worker.is_ready() { break; }
    sequencer.propose(&mempool, block_time, gas_limit).await?;
}

[github-actions]

Per project conventions (CLAUDE.md): use prometheus_parse::Value; appears inside function/method bodies at lines 52 and 104. These should be moved to the file-level imports at the top.

[github-actions]

Bare .unwrap() on a hardcoded URL parse. While this will never fail, the project convention is to avoid .unwrap() in non-test code. Use .expect() with a message:

Suggested change

	let dummy_l1: Url = "http://127.0.0.1:1".parse().unwrap();
	let dummy_l1: Url = "http://127.0.0.1:1".parse().expect("valid hardcoded dummy L1 URL");

[github-actions]

The stop_flag binding on line 142 is now unused between its creation and passing to run_test_phases. Previously, install_signal_handler(stop_flag) was called here — now that's moved inside run_test_phases. This leaves a blank line and the stop_flag binding dangling far from its use as a function argument. Minor, but worth cleaning up:

Suggested change

	let mut runner = LoadRunner::new(load_config.clone())?;
	runner.set_config_summary(config_summary.clone());
	// Install signal handler before any long-running work. First signal sets
	// the stop flag so `run()` exits its loop gracefully and the drain sequence
	// runs. A second signal force-exits.
	let stop_flag = runner.stop_flag();
	let mut runner = LoadRunner::new(load_config.clone())?;
	runner.set_config_summary(config_summary.clone());
	let stop_flag = runner.stop_flag();

[github-actions]

Review Summary

This PR adds a new base-benchmark crate (~5700 lines) that ports a Go benchmark runner to Rust, plus minor changes to the load-test binary. The overall structure is reasonable — clean module separation, proper error enum with thiserror, and async tokio-based orchestration. However, there are several correctness and safety issues that should be addressed before merge.

Critical Issues

#	File	Issue
1	metrics/mod.rs:155-158	histogram_sum_count is incorrect — extracts +Inf bucket count as "sum" and last bucket count as "count", making both the same value. Delta averages will always be ~1.0. (existing comment)
2	consensus/mod.rs:231-236	Wall-clock timestamp instead of incremental — uses SystemTime::now() instead of head_block_timestamp + block_time, causing non-reproducible results and potential node rejection. (existing comment)
3	client/mod.rs:309	Builder runs reth binary — BuilderClient uses reth_bin instead of builder_bin for the inner node client. (existing comment)
4	consensus/mod.rs:291-324	batch_send_txs swallows all errors — always returns Ok(()) even if every HTTP request fails, leading to silent empty blocks. (new)
5	runner/mod.rs:222-234	Cancel-unsafe tokio::select! in setup loop — dropping a partially-completed propose future can leave sequencer consensus state inconsistent. (new)

Moderate Issues

#	File	Issue
6	runner/mod.rs:388-395	.expect() on user-supplied prefund key — panics instead of returning BenchmarkError. (existing)
7	process.rs:63-70	Missing // SAFETY: comment on unsafe block. (existing)
8	proxy/mod.rs:63-68	Only intercepts single JSON-RPC requests; batch arrays bypass mempool. (existing)
9	params.rs:17-21	Batcher/prefund keys don't match PORTING_REFERENCE.md. (existing)
10	runner/mod.rs:259	Redundant head_block_hash reassignment after new_payload. (existing)
11	PORTING_REFERENCE.md	Duplicate file at repo root. (existing)
12	load-tests/transfer.rs:31	Changing TransferPayload::default() to zero-value affects all load-test users, not just benchmark. (new)

Convention Issues

#	File	Issue
13	consensus/mod.rs:26,414-416	use statements inside function bodies. (existing)
14	metrics/mod.rs:52,104	use prometheus_parse::Value inside method bodies. (new)
15	output.rs:195	use std::io::Seek inside function body. (existing)
16	runner/mod.rs:386	use alloy_signer_local::PrivateKeySigner inside function body. (existing)
17	consensus/mod.rs:99	Bare .unwrap() on hardcoded URL parse — should be .expect(). (new)

Notes

• The load-test change (moving install_signal_handler after funding) makes sense for benchmark integration — the runner reads "Accounts funded." from stdout to detect readiness.
• The params.rs module is missing a //! doc comment, but it's not a mod.rs so this doesn't violate the specific CLAUDE.md rule.
• The Cargo.toml dependency ordering doesn't follow the workspace's waterfall style, but this is a new crate so it can be cleaned up separately.