Feat/evm enginetest

[kamilchodola]

Fixes Closes Resolves #

Please choose one of the keywords above to refer to the issue this PR solves followed by the issue number (e.g. Fixes #000). If no issue number, remove the line. Also, remove everything marked optional that is not applicable. Remove this note after reading.

Changes

• List the changes

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Notes on testing

Optional. Remove if not applicable.

Documentation

Requires documentation update

• Yes
• No

If yes, link the PR to the docs update or the issue with the details labeled docs. Remove if not applicable.

Requires explanation in Release Notes

• Yes
• No

If yes, fill in the details here. Remove if not applicable.

Remarks

Optional. Remove if not applicable.

[claude]

Claude finished @kamilchodola's task in 4m 57s —— View job

---

PR Review — feat/evm-enginetest

This PR adds EEST blockchain_tests_engine fixture support: a new GitHub Actions workflow, a nethtest launcher script, and C# infrastructure to route engine API calls through IJsonRpcService for realistic deserialization. The approach is sound and the engine-test plumbing (RunNewPayloads, AssertPayloadStatus, validation-error mapping) is well-structured.

---

Findings

High

1. Workflow push trigger is feature-branch-specific (line 8)
The push trigger fires only on feat/evm-enginetest. After merging to master, the workflow will never run automatically unless updated. Decide before merge whether it should run on master pushes, PRs, or be manual-only.

2. Verify GitHub Actions version tags (line 67)
actions/checkout@v6, actions/upload-artifact@v7 (line 264), actions/setup-dotnet@v5, and actions/cache@v5 are all pinned to versions that may not exist. As of my knowledge cutoff the latest stable versions were @v4 for all of these. If the tags don't resolve, every run will fail immediately with a 404. Please verify each action's latest available tag.

Medium

3. NewPayloadParamCounts static initializer can produce opaque TypeInitializationException (BlockchainTestBase.cs line 360)
The reflection-based dictionary is built at class-load time. A NotSupportedException thrown during static initialization wraps the real cause in a TypeInitializationException, making future API mismatches hard to diagnose. Use a named factory method or defer building until first use.

4. Two distinct EEST exceptions mapped to the same regex (BlockchainTestBase.cs ~line 493)
BlockException.INVALID_DEPOSIT_EVENT_LAYOUT and BlockException.SYSTEM_CONTRACT_CALL_FAILED both match "InvalidBlockLevelAccessList: Suggested block-level access list missing account changes". AssertValidationError will accept either expected exception for a single Nethermind error string, masking regressions where the wrong underlying failure fires. If Nethermind cannot distinguish these cases, add an explicit comment. Fix this →

5. nethtest script missing --no-launch-profile (nethtest line 3)
The workflow always adds --no-launch-profile but the convenience script doesn't, meaning local runs can behave differently from CI when launchSettings.json is present.

6. Block tests without --jsonout produce no stdout output (Program.cs line 143)
When isBlockTest && !jsonOutput, results are computed but neither written to stdout (JSON is gated on forceJson) nor to the console (suppressOutput: true is always passed). Users running nethtest --blockTest ... locally without --jsonout see per-test PASS/FAIL on stderr but no summary. Consider always serializing results or documenting the requirement.

Low

7. LINQ in CollectFiles (Program.cs line 169)
Where + OrderBy in CollectFiles. Repo style prefers for/foreach. Non-hot path so no functional impact.

8. IsEip7843Enabled changed to read-only delegation (OverridableReleaseSpec.cs line 128)
Changed from a settable initialized property to => spec.IsEip7843Enabled. Any existing test code that initializes this override will silently stop having effect (or fail to compile). If intentional, a comment would help.

9. --workers $WORKERS unquoted (workflow line 243)
Safe for numeric input but should be quoted as "$WORKERS" for robustness.

10. No timeout on curl download (workflow line 98)
A slow GitHub release server will hang the step indefinitely. Consider --max-time 300 or similar.

---

What's working well

• Engine test routing through IJsonRpcService is the right architectural choice — it exercises the full deserialization path, not a synthetic shortcut.
• ValidationErrorSubstringMappings + ValidationErrorRegexMappings approach is clean and extensible.
• The write_summary trap-on-EXIT pattern in the workflow is resilient to test runner crashes.
• EthereumTestResult.LastPayloadStatus and LastBlockHash additions are clean structured-output improvements.
• The genesisUsesTargetFork / EIP-7928 logic is well-commented.

[claude]

High — Workflow trigger is branch-specific and will stop running after merge

The push trigger is hard-coded to feat/evm-enginetest. After this PR merges to master, the workflow will never run automatically. Decide on the post-merge trigger strategy (e.g. push: branches: [master], a pull_request trigger, or workflow_dispatch-only) before merging.

Suggested change

	push:
	push:
	branches: [master]

or remove the push trigger entirely if the workflow is intentionally manual-only.

[claude]

High — Verify GitHub Actions version numbers exist

actions/checkout@v6 and actions/upload-artifact@v7 (line 264) are pinned to versions that may not exist. As of my knowledge cutoff the latest stable tags were @v4 for both. The same question applies to actions/setup-dotnet@v5 (line 72) and actions/cache@v5 (line 75). If these tags don't exist, the workflow will fail immediately on every run with a 404.

Run gh release list -R actions/checkout | head -5 etc. to verify, or pin to known-stable versions.

[claude]

Medium — Reflection-based static initializer can produce opaque TypeInitializationException

If engine_newPayloadV{v} is not found on IEngineRpcModule (e.g. after an API refactor), the throw new NotSupportedException(...) fires during the static initializer, which wraps it in a TypeInitializationException. The real cause is buried in the inner exception and does not surface as a clear test failure — it crashes any test that touches this class with a confusing error.

Consider making this lazy (built on first use) or at least wrapping the static init in a try-catch that rethrows with context:

private static readonly Dictionary<int, int> NewPayloadParamCounts = BuildNewPayloadParamCounts();

private static Dictionary<int, int> BuildNewPayloadParamCounts()
{
    var result = new Dictionary<int, int>();
    for (int v = 1; v <= EngineApiVersions.NewPayload.Latest; v++)
    {
        MethodInfo? method = typeof(IEngineRpcModule).GetMethod($"engine_newPayloadV{v}");
        if (method is null)
            throw new InvalidOperationException($"engine_newPayloadV{v} not found on IEngineRpcModule. Update EngineApiVersions.NewPayload.Latest.");
        result[v] = method.GetParameters().Length;
    }
    return result;
}

[claude]

Medium — Script is missing --no-launch-profile that the workflow uses

The workflow runs dotnet run --no-launch-profile -c Release ... but this script uses dotnet run --no-build -c Release ... without --no-launch-profile. Without that flag, dotnet run picks up the project's launchSettings.json if present, which can set environment variables (e.g. ASPNETCORE_ENVIRONMENT, ports) that differ from CI. Add --no-launch-profile for consistent behavior:

Suggested change

	exec dotnet run --no-build -c Release --project "$SCRIPT_DIR/src/Nethermind/Nethermind.Test.Runner/Nethermind.Test.Runner.csproj" -- "$@"
	exec dotnet run --no-build --no-launch-profile -c Release --project "$SCRIPT_DIR/src/Nethermind/Nethermind.Test.Runner/Nethermind.Test.Runner.csproj" -- "$@"

[claude]

Medium — Block tests without --jsonout silently produce no stdout output

forceJson = isEngineTest || jsonOutput. When isBlockTest && !jsonOutput, forceJson = false, so the results computed by RunBlockTestFiles are discarded — nothing is written to stdout (the JSON serializer at line 145 is not called, and suppressOutput: true is always passed so per-test console output is suppressed too). A user running nethtest --blockTest --input ./fixtures with no --jsonout flag gets zero output beyond the per-test PASS/FAIL on stderr, with no summary.

Consider either always serializing block test results or explicitly documenting that --jsonout is required for block tests. The workflow always passes --jsonout, but local users will be confused.

[claude]

Low — LINQ used in non-hot path (coding style preference)

Both Where and OrderBy are LINQ. Per the repo coding style, for/foreach is preferred over LINQ. Since this is called once at startup (not a hot path), the impact is negligible, but worth aligning with the project convention.

Suggested change

	.Where(f => !f.Contains("/.meta/") && !f.Contains("\\.meta\\"))
	if (Directory.Exists(path))
	{
	List<string> result = [];
	foreach (string f in Directory.GetFiles(path, "*.json", SearchOption.AllDirectories))
	{
	if (!f.Contains("/.meta/") && !f.Contains("\\.meta\\"))
	result.Add(f);
	}
	result.Sort(StringComparer.Ordinal);
	return result;
	}