Skip to content

test: load the frame transaction fixtures by mapping their fork name #12854

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Marchhill wants to merge 2 commits into
base: eip8141-frame-txs-devnet7
Choose a base branch
from
+39 −5
Draft

test: load the frame transaction fixtures by mapping their fork name #12854

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ed23eca
test: load the frame transaction fixtures by mapping their fork name
Marchhill Aug 17, 2026
35ce825
Merge remote-tracking branch 'upstream/eip8141-frame-txs-devnet7' int…
Marchhill Aug 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions src/Nethermind/Ethereum.Test.Base/JsonToEthereumTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,16 +417,22 @@ private static List<TransactionTest> ConvertTransactionTests(Dictionary<string,
return tests;
}

// The shape fallback deliberately wraps deserialization only: letting it span conversion
// made an unmapped fork name resurface as an unrelated error against the trimmed shape.
public static IEnumerable<BlockchainTest> ConvertToBlockchainTests(string json)
{
try { return ConvertToBlockchainTests(_serializer.Deserialize<Dictionary<string, BlockchainTestJson>>(json)); }
catch (Exception) { return ConvertToBlockchainTests(CoerceFromHalf(_serializer.Deserialize<Dictionary<string, HalfBlockchainTestJson>>(json))); }
Dictionary<string, BlockchainTestJson> tests;
try { tests = _serializer.Deserialize<Dictionary<string, BlockchainTestJson>>(json); }
catch (Exception) { tests = CoerceFromHalf(_serializer.Deserialize<Dictionary<string, HalfBlockchainTestJson>>(json)); }
Comment on lines +420 to +426

@claude claude Bot Aug 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low — the other half of the same masking problem is still there.

The narrowing is correct and is the right fix — but catch (Exception) still discards the standard-shape exception entirely. When a fixture is neither shape (malformed JSON, a genuinely new field, a type change on the standard side), the only error that reaches FileTestsSource's Failed to load: {e} is the trimmed-shape error, which is exactly the "reported as an unrelated error against the trimmed shape" confusion this PR set out to remove. It just moved from conversion errors to deserialization errors.

Chaining the original preserves both:

Dictionary<string, BlockchainTestJson> tests;
try { tests = _serializer.Deserialize<Dictionary<string, BlockchainTestJson>>(json); }
catch (Exception standardShape)
{
    try { tests = CoerceFromHalf(_serializer.Deserialize<Dictionary<string, HalfBlockchainTestJson>>(json)); }
    catch (Exception halfShape) { throw new AggregateException("Fixture matched neither the standard nor the trimmed shape", standardShape, halfShape); }
}

Also relevant: robustness.md — "Never swallow exceptions … at minimum log the exception." catch (Exception) here doesn't even bind the exception. Pre-existing, but these are the lines the diff touches.

Two secondary notes on the same hunk:

  • Per AGENTS.md member-doc rules, a rationale that "applies to the member as a whole" should be <remarks>, not a leading in-line comment. As written it also sits above the string overload only, while describing both.
  • The comment is phrased historically ("letting it span conversion made an unmapped fork name resurface"), which reads as a changelog entry rather than an invariant. Something like <remarks>Scoped to deserialization so that conversion errors — e.g. an unmapped fork name — are reported as themselves rather than as a shape mismatch against the trimmed type.</remarks> survives better on the base branch.

Fix this →

return ConvertToBlockchainTests(tests);
}

public static IEnumerable<BlockchainTest> ConvertToBlockchainTests(ReadOnlySpan<byte> json)
{
try { return ConvertToBlockchainTests(_serializer.Deserialize<Dictionary<string, BlockchainTestJson>>(json)); }
catch (Exception) { return ConvertToBlockchainTests(CoerceFromHalf(_serializer.Deserialize<Dictionary<string, HalfBlockchainTestJson>>(json))); }
Dictionary<string, BlockchainTestJson> tests;
try { tests = _serializer.Deserialize<Dictionary<string, BlockchainTestJson>>(json); }
catch (Exception) { tests = CoerceFromHalf(_serializer.Deserialize<Dictionary<string, HalfBlockchainTestJson>>(json)); }
return ConvertToBlockchainTests(tests);
}

// Some BAL fixtures use the trimmed HalfBlockchainTestJson shape; coerce on demand.
Expand Down
27 changes: 27 additions & 0 deletions src/Nethermind/Nethermind.Specs.Test/SpecNameParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// SPDX-FileCopyrightText: 2026 Demerzel Solutions Limited
// SPDX-License-Identifier: LGPL-3.0-only

using System;
using Nethermind.Core.Specs;
using NUnit.Framework;

namespace Nethermind.Specs.Test;

public class SpecNameParserTests
{
[Test]
public void Parse_maps_Bogota_to_the_frame_transactions_fork()
{
IReleaseSpec spec = SpecNameParser.Parse("Bogota");

Assert.That(spec.IsEip8141Enabled, Is.True);
Comment on lines +12 to +17

@claude claude Bot Aug 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium — this assertion doesn't pin the mapping the test name claims.

IsEip8141Enabled is not unique to Bogota. Nethermind.Specs/Forks/Eip8141Prototype.cs also exists in this tree, and any future 8141-bearing fork would satisfy this too. Map "Bogota" to the wrong 8141 spec and the test stays green, so the "maps Bogota to the frame transactions fork" contract is untested — only "maps Bogota to something with 8141 on".

Assert identity instead; Parse returns the singleton, so reference equality is the exact contract:

Suggested change
[Test]
public void Parse_maps_Bogota_to_the_frame_transactions_fork()
{
IReleaseSpec spec = SpecNameParser.Parse("Bogota");
Assert.That(spec.IsEip8141Enabled, Is.True);
[Test]
public void Parse_maps_Bogota_to_the_frame_transactions_fork()
{
IReleaseSpec spec = SpecNameParser.Parse("Bogota");
Assert.That(spec, Is.SameAs(Bogota.Instance));
}

(needs using Nethermind.Specs.Forks;)

Low, same file, line 23: Assert.Throws<T> returns TActual?, and this project has <Nullable>enable</Nullable>, so NotSupportedException e = Assert.Throws<...>(...) emits CS8600. ChainSpecHardforkLabelTests.cs:172 in the same project uses the ! suffix — worth matching.

Low, structural: the recurring failure here isn't "Bogota specifically was missing", it's "SpecNameParser's switch drifts from Nethermind.Specs.Forks". This project already has the tool for that — ChainSpecHardforkLabelTests.ForkFor reflects over Nethermind.Specs.Forks.{name}.Instance precisely to avoid a hand-maintained mapping. A [TestCaseSource] over the NamedReleaseSpec types in that namespace asserting each is parseable would catch the next one at compile-of-the-fork time. It needs an explicit skip set — Olympic and MuirGlacier are fork classes that SpecNameParser deliberately doesn't map — and that set being explicit is itself the documentation. Optional for this PR, but it's the difference between fixing one instance and closing the class.

Fix this →

}

[Test]
public void Parse_names_the_offending_fork_when_unmapped()
{
NotSupportedException e = Assert.Throws<NotSupportedException>(() => SpecNameParser.Parse("NotAFork"));

Assert.That(e.Message, Does.Contain("NotAFork"));
}
}
3 changes: 2 additions & 1 deletion src/Nethermind/Nethermind.Specs/SpecNameParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ private static IReleaseSpec ParseUncached(string specName)
"BPO4" => BPO4.Instance,
"BPO5" => BPO5.Instance,
"Amsterdam" => Amsterdam.Instance,
_ => throw new NotSupportedException()
"Bogota" => Bogota.Instance,
_ => throw new NotSupportedException($"Unknown fork name '{specName}'")

@claude claude Bot Aug 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low — the message names the wrong string.

The switch matches on unambiguousSpecName, but the message interpolates specName. For any input that goes through the substitution chain above the two differ, and the name reported is not the one that failed to match:

  • "Merge+9999" → matched as "Paris+9999", reported as `Merge+9999`
  • "Shanghai+3541" → matched as "Shanghai+3541", reported the same (no substitution) — fine
  • "GrayGlacier+2929" → matched as "GrayGlacier+2929" — fine

So it only misleads for the Merge/Merged/EIP150/EIP158/DAO families, but those are the cases where a reader would most want to know what the parser actually looked up. Including both costs nothing:

Suggested change
_ => throw new NotSupportedException($"Unknown fork name '{specName}'")
_ => throw new NotSupportedException($"Unknown fork name '{specName}' (resolved to '{unambiguousSpecName}')")

Fix this →

};
}
}
Expand Down
Loading