test: load the frame transaction fixtures by mapping their fork name - #12854
test: load the frame transaction fixtures by mapping their fork name#12854Marchhill wants to merge 2 commits into
Conversation
The EIP-8141 fixture suites declare their network as Bogota, which SpecNameParser did not map, so every file failed to load. The failure surfaced as an unrelated Hash256 conversion error because ConvertToBlockchainTests wrapped both deserialization and conversion in the HalfBlockchainTestJson shape fallback: the unmapped fork name threw during conversion, and the retry against the trimmed shape then failed on its differently typed postState. Narrow the fallback to deserialization and name the fork in the exception.
|
Claude finished @Marchhill's task in 5m 19s —— View job Review: PR #12854
VerdictThe core change is right, and the diagnosis in the commit message checks out. I traced the masking path: I also checked the narrowing doesn't lose a load-bearing path. For the fallback to have been useful across conversion, the standard deserialize would have to succeed and the half-shape deserialize succeed on the same JSON — mutually exclusive, since a dict One Medium, on test strength rather than production behaviour. Nothing Critical or High. No security or performance surface — all three files are test-path or a cold parser behind a
Two things about scope, for the reviewerNeither is a defect — both follow from decisions the PR body already states — but they change what "loads" means here. No fixture class is registered. The description contradicts itself. "all 159 blockchain cases load and execute" sits against Remarks' "96 of the 159 cases are still rejected … before they execute." The Remarks version is the accurate one; the summary line reads as a stronger claim than the branch supports. What I could not verifyBuild and test execution were blocked — Blocking on the Medium only in the sense that the new test doesn't yet guard what its name promises — a one-line change to |
…o claude/eip8141-fixture-suite
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // 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)); } |
There was a problem hiding this comment.
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 thestringoverload 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.
| "Amsterdam" => Amsterdam.Instance, | ||
| _ => throw new NotSupportedException() | ||
| "Bogota" => Bogota.Instance, | ||
| _ => throw new NotSupportedException($"Unknown fork name '{specName}'") |
There was a problem hiding this comment.
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:
| _ => throw new NotSupportedException($"Unknown fork name '{specName}'") | |
| _ => throw new NotSupportedException($"Unknown fork name '{specName}' (resolved to '{unambiguousSpecName}')") |
| [Test] | ||
| public void Parse_maps_Bogota_to_the_frame_transactions_fork() | ||
| { | ||
| IReleaseSpec spec = SpecNameParser.Parse("Bogota"); | ||
|
|
||
| Assert.That(spec.IsEip8141Enabled, Is.True); |
There was a problem hiding this comment.
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:
| [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.
1 participant
Changes
BogotainSpecNameParser, so the EIP-8141 fixture suites — which declare that network — load instead of failing every file.HalfBlockchainTestJsonshape fallback inConvertToBlockchainTeststo deserialization only. It previously wrapped conversion too, so an unmapped fork name was reported as an unrelatedHash256conversion error against the trimmed shape rather than as itself.NotSupportedExceptioninstead of throwing it bare.Before this change the whole suite failed to load; after it, all 159 blockchain cases load and execute.
Fixtures come from the
tests-frames-devnet@v0.0.0release (fixtures_frames-devnet.tar.gz). Note that EIP-8141 merged to theeips/amsterdam/eip-8141branch rather than master, and the pyspecDEFAULT_ARCHIVE_VERSIONpin contains none of these tests, so wiring this into CI needs a second archive pin.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
SpecNameParserTestscovers both the mapping and the exception message. Both assertions were revert-checked: removing the mapping and restoring the bare throw fails them.Documentation
Requires documentation update
Requires explanation in Release Notes
Remarks
The state-test path does not gate anything — do not wire it into CI
TransactionJsonhas noframes/signaturesfields, so a frame transaction cannot be represented in a state-test fixture. The 40 EIP-8141 state-test files therefore load to zero cases, and the runner exits 0. Anything that runs that path today reports a pass while asserting nothing. Only the blockchain-test path, which decodes block RLP and so exercises the real frame decoder, is meaningful. Supporting state tests means adding the frame fields to the loader; until then that path should be treated as unwired, not as green.Loading is only half the story
This branch composes
Bogotaon Osaka, whereas the fixtures compose it on Amsterdam, so 96 of the 159 cases are still rejected atBlockLevelAccessListHashNotEnabledbefore they execute. That fork-composition question is deliberately left open here.Frame targets under EIP-7702 delegation
With
Bogotacomposed on Amsterdam for measurement, the delegated-target cases fail on every branch tried, including one carrying devnet-8 gas and the frame entry charge.ExecuteFrameresolves the target's code withwhich follows the delegation for code but discards the delegation address, so the delegation target's access cost is never charged. Affects
test_delegated_target_entry_charge(cold and warm),test_delegated_to_precompile_targetandtest_verify_frame_delegated_to_precompile_target.