feat: add create cluster tests by varex83 · Pull Request #325 · NethermindEth/pluto

varex83 · 2026-04-14T14:22:03Z

Summary

Ports the Go TestCreateCluster test suite to Rust, covering all major create_cluster flows end-to-end, and fixes several correctness issues uncovered in the process.

New tests (`crates/cli/src/commands/create_cluster.rs`)

A shared run_test_create_cluster helper drives all test cases, mirroring the Go test loop. Each case exercises run() against a temp directory and asserts:

the command succeeds (or returns a specific error)
the generated cluster-lock.json passes verify_hashes() and verify_signatures()
all distributed validators have unique public keys
deposit-data amounts match expectations
CLI output and produced file tree match golden snapshots

Test cases covered:

Test	Notes
`simnet`	4-node simnet cluster
`goerli`	Goerli network, 3-node
`custom_testnet_flags`	Custom chain ID / fork version / genesis timestamp
`custom_target_gas_limit`	Non-default `target_gas_limit`
`solo_flow_definition_from_disk`	Definition loaded from a local JSON file
`solo_flow_definition_from_network`	Definition served over HTTP (wiremock)
`splitkeys`	`--split-existing-keys`, keys generated in a temp dir
`splitkeys_with_cluster_definition`	Split keys combined with a pre-existing definition
`four_partial_deposits` / `two_partial_deposits`	Multiple deposit amounts per validator
`with_fee_recipient_and_withdrawal_addresses`	Custom fee-recipient + withdrawal addresses
`validate_def`	Invalid definition triggers expected error

Golden files live in crates/cli/src/commands/testdata/ and can be regenerated with UPDATE_GOLDEN=1 cargo test.

Bug fixes

create_cluster::run — call def.set_definition_hashes() before proceeding with key generation; the hashes were previously unset, causing verify_hashes() to fail.
definition.rs deserialization — validate that num_validators == validator_addresses.len() for schema versions v1.5–v1.10; add #[serde(default)] to fee_recipient_address, withdrawal_address, and compounding so older JSON examples without those fields still deserialize correctly.
lock.rs no_registration check — remove the zero-fee_recipient guard. The Go reference uses len == 0 (missing field), not the zero Ethereum address; a legitimately zero fee-recipient is valid. Only an all-zero BLS signature or pubkey indicates a missing registration.
cluster-definition-005.json — fix malformed timestamp (stray spaces around : separators).

Test plan

cargo test -p pluto-cli passes with all new test cases green
UPDATE_GOLDEN=1 cargo test regenerates golden files without diff
Existing tests unaffected

…ster

…ster-test

mskrzypkows

One comment left, LGTM in general. Will you ask claude to review it?

…ster-test

emlautarom1

The golden test matching logic is no particularly good, specially since it does not show where the mismatch is located in case there is one. We can revisit this latter for improvements but I don't intend to block this.

emlautarom1 · 2026-04-22T16:14:05Z

+        .map_err(CreateClusterError::DefinitionError)?;



No need for explicit map_err

emlautarom1 · 2026-04-22T17:36:14Z

+            String::from_utf8_lossy(actual),
+            String::from_utf8_lossy(&expected),


Do not parse as utf8, compare the bytes directly:

Suggested change

String::from_utf8_lossy(actual),

String::from_utf8_lossy(&expected),

actual,

&expected,

emlautarom1 · 2026-04-22T17:47:24Z

+    async fn test_eth1_client() -> EthClient {
+        EthClient::new("http://127.0.0.1:8545").await.unwrap()
+    }


If we need an actual Eth client running then we should use testcontainers to spin one up. If not, make this an empty string so we use the Noop client.

emlautarom1 · 2026-04-22T17:52:03Z

+            genesis_timestamp: config.testnet_chain_id.map(|_| {
+                u64::try_from(chrono::Utc::now().timestamp()).expect("timestamp fits u64")
+            }),


Is this intentional? If there is no testnet_chain_id then the timestamp is None, otherwise its the current timestamp (now)?

emlautarom1 · 2026-04-22T17:59:09Z

+        if let Some(expected) = expected_err {
+            let err = result.unwrap_err();
+            let err_str = format!("{err}");
+            assert!(
+                err_str.contains(expected),
+                "expected error containing '{expected}', got: {err_str}"
+            );
+            return;
        }


It would be better to match on the CliError instead of using substrings

emlautarom1 · 2026-04-22T20:47:16Z

@claude

claude · 2026-04-22T20:47:32Z

Claude finished @emlautarom1's task in 5m 12s —— View job

Code Review

Summary

This is a solid port of the Go TestCreateCluster suite. The structure is clear, the helper decomposition works well, and the bug fixes in definition.rs and lock.rs are correct. A few issues need attention before merge.

Bug: `cluster-definition-005.json` timestamp regression

The PR introduced a malformed timestamp, not fixed one. The git diff for commit 7d13197 shows:

- "timestamp": "2023-05-18T15:12:46+02:00",   ← correct
+ "timestamp": "2023-05-18T15: 12: 46+02: 00", ← spaces inserted by this PR

The PR description says "fix malformed timestamp" but the direction is reversed — the timestamp was correct before this PR. This should be reverted to "2023-05-18T15:12:46+02:00".

Fix this →

Tests require a live Ethereum node (`http://127.0.0.1:8545`)

Multiple tests fail in CI unless an eth1 node is running locally:

test_eth1_client() at line 1524 connects to http://127.0.0.1:8545
validate_cluster_lock() calls test_eth1_client().await (line 1616)
build_create_cluster_args() sets execution_engine_addr = Some("http://127.0.0.1:8545") whenever a definition_file is present (line 1581–1583)
run() at line 393–397 creates a real eth1 client from execution_engine_addr

Since verify_signatures only validates BLS signatures and the eth1 client is passed but unused for those validations (based on lock.rs), passing an empty string (triggering a Noop client) or restructuring to decouple eth1 from signature verification would make these tests hermetic. [This was also flagged by @emlautarom1.]

Non-deterministic `genesis_timestamp` in test config

build_create_cluster_args at lines 1575–1577:

genesis_timestamp: config.testnet_chain_id.map(|_| {
    u64::try_from(chrono::Utc::now().timestamp()).expect("timestamp fits u64")
}),

This sets genesis timestamp to "now" whenever a testnet_chain_id is present. This is non-deterministic (differs per run) and semantically wrong — genesis timestamp should be a fixed historical time, not the current clock. This can also make the golden file comparison for custom_testnet_flags brittle if the output includes the timestamp. [Flagged by @emlautarom1.]

Fragile substring-based error matching

Throughout the tests, errors are matched via err_str.contains(expected). This pattern:

Passes if a future error message grows to include the expected substring for a different reason
Gives misleading failures if messages are refactored
Cannot distinguish between two errors that both contain the same substring

Using structured matching on CliError variants as suggested by @emlautarom1 is the right fix. Where the error is CliError::CreateClusterError(CreateClusterError::SomeVariant), pattern matching gives compile-time safety.

`load_ref_def` manual JSON patching is redundant after `#[serde(default)]` fix

load_ref_def() (line 1535–1542) and validate_definition test (line 2175–2181) both manually patch compounding: false into the JSON value before deserializing:

if value.get("compounding").is_none() {
    value["compounding"] = serde_json::json!(false);
}

The PR adds #[serde(default)] to DefinitionV1x10::compounding (line 1517), which makes this manual patching unnecessary. Both call sites should be cleaned up.

`require_golden_bytes`: byte comparison preferred over lossy UTF-8

At lines 1470–1475:

assert_eq!(
    String::from_utf8_lossy(actual),
    String::from_utf8_lossy(&expected),
    ...
);

from_utf8_lossy silently replaces invalid UTF-8 with \u{FFFD}, which can mask encoding bugs. As suggested by @emlautarom1, comparing bytes directly (assert_eq!(actual, &expected)) is more precise.

Test ordering differs from Charon

As acknowledged by @varex83 in reply to @mskrzypkows, the test case order doesn't match Charon's (where the "valid lock" / basic passing case comes last). The last refactor commit (4cf6184) split the helper but didn't reorder the cases. This is a readability/parity issue but doesn't affect correctness.

Minor: `map_err` at line 499 is correct and necessary

The reviewer comment "No need for explicit map_err" at line 499 appears to be a mistake. Since run() returns CliResult<()> (i.e. Result<(), CliError>), and Rust's ? only performs one From conversion, the chain DefinitionError → CliError requires an explicit step via CreateClusterError::DefinitionError. The current .map_err(CreateClusterError::DefinitionError)? is correct.

`simnet` test name uses `goerli` network

The test labeled "simnet" (line 1840) uses network: Some("goerli"). This is a naming mismatch — if the intent is to mirror Go's simnet (a local simulated network), it should either use a simnet-specific network value or the test should be renamed to reflect that it's a goerli cluster test.

Overall Assessment

The core logic — the run_test_create_cluster helper, validate_cluster_lock, check_golden_files, the bug fixes in definition.rs and lock.rs — is well-structured and a good faithful port of the Go test suite. The main blockers are the timestamp regression in cluster-definition-005.json and the non-hermetic eth1 client dependency. The error matching and non-deterministic timestamp should also be addressed before merge.

varex83 added 13 commits March 30, 2026 17:02

wip: create cluster

9772c70

feat: create cluster finish some functions

5e02cb3

fix create cluster review feedback

096b863

fix: review comments

5aa1ef6

Merge remote-tracking branch 'origin/main' into bohdan/cmd-create-clu…

d1b9ff9

…ster

fix: review comments

427e870

fix: review comment

1733554

feat: add create cluster test

e7afc37

feat: add golden tests

077f2c3

feat: add test validate def

1a7912c

feat: add test split keys

8093d66

feat: add test target gas limit

7d13197

feat: add create cluster tests

d8abd91

Base automatically changed from bohdan/cmd-create-cluster to main April 15, 2026 08:37

Merge remote-tracking branch 'origin/main' into bohdan/cmd-create-clu…

2f55faa

…ster-test

mskrzypkows reviewed Apr 22, 2026

View reviewed changes

Comment thread crates/cli/src/commands/create_cluster.rs Outdated

mskrzypkows approved these changes Apr 22, 2026

View reviewed changes

varex83 added 2 commits April 22, 2026 16:05

Merge remote-tracking branch 'origin/main' into bohdan/cmd-create-clu…

f81728d

…ster-test

refactor: split run_test_create_cluster into several functions

4cf6184

emlautarom1 approved these changes Apr 22, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add create cluster tests#325

feat: add create cluster tests#325
varex83 wants to merge 16 commits intomainfrom
bohdan/cmd-create-cluster-test

varex83 commented Apr 14, 2026 •

edited by varex83agent

Loading

Uh oh!

Uh oh!

mskrzypkows left a comment

Uh oh!

emlautarom1 left a comment

Uh oh!

Uh oh!

emlautarom1 Apr 22, 2026

Uh oh!

emlautarom1 Apr 22, 2026

Uh oh!

emlautarom1 Apr 22, 2026

Uh oh!

emlautarom1 Apr 22, 2026

Uh oh!

emlautarom1 Apr 22, 2026

Uh oh!

emlautarom1 commented Apr 22, 2026

Uh oh!

claude Bot commented Apr 22, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		String::from_utf8_lossy(actual),
		String::from_utf8_lossy(&expected),

Conversation

varex83 commented Apr 14, 2026 • edited by varex83agent Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

New tests (crates/cli/src/commands/create_cluster.rs)

Bug fixes

Test plan

Uh oh!

Uh oh!

mskrzypkows left a comment

Choose a reason for hiding this comment

Uh oh!

emlautarom1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

emlautarom1 Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

emlautarom1 Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

emlautarom1 Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

emlautarom1 Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

emlautarom1 Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

emlautarom1 commented Apr 22, 2026

Uh oh!

claude Bot commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review

Summary

Bug: cluster-definition-005.json timestamp regression

Tests require a live Ethereum node (http://127.0.0.1:8545)

Non-deterministic genesis_timestamp in test config

Fragile substring-based error matching

load_ref_def manual JSON patching is redundant after #[serde(default)] fix

require_golden_bytes: byte comparison preferred over lossy UTF-8

Test ordering differs from Charon

Minor: map_err at line 499 is correct and necessary

simnet test name uses goerli network

Overall Assessment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

varex83 commented Apr 14, 2026 •

edited by varex83agent

Loading

New tests (`crates/cli/src/commands/create_cluster.rs`)

claude Bot commented Apr 22, 2026 •

edited

Loading

Bug: `cluster-definition-005.json` timestamp regression

Tests require a live Ethereum node (`http://127.0.0.1:8545`)

Non-deterministic `genesis_timestamp` in test config

`load_ref_def` manual JSON patching is redundant after `#[serde(default)]` fix

`require_golden_bytes`: byte comparison preferred over lossy UTF-8

Minor: `map_err` at line 499 is correct and necessary

`simnet` test name uses `goerli` network