test: anchor crypto and RLP tests to independent expectations by benaadams · Pull Request #12712 · NethermindEth/nethermind

benaadams · 2026-08-06T02:24:03Z

Changes

Part of the test-hygiene series (#12689, #12690 - merged, #12693, #12694, #12696 - merged, #12699, #12705, #12710, #12711): tests that cannot meaningfully fail get real expectations. Rule for this chunk: an expectation must not come from the code under test. Three files in Nethermind.Core.Test; no product code changed (verified).

KeccakTests.Span: compared the Span overload against the array overload of the same function - both could be wrong identically. Both overloads now assert an independent vector: keccak-256 of the 1024-byte i % 256 pattern, derived with pycryptodome.
RlpTests: Length_of_ulong_same_as_uint256 compared two Rlp.LengthOf overloads against each other over a pow2-boundary sweep - a shared wrong boundary passes. Renamed Length_of_ulong_matches_spec: both overloads now assert a spec-derived expected (Yellow Paper appendix B: one byte below 0x80, else one prefix byte plus the minimal big-endian bytes), keeping the sweep and the cross-overload agreement transitively.
BlockDecoderTests:
- Can_do_roundtrip_scenarios (encode-decode-encode) never looked at the original block; it now also compares the decoded block to the original: block hash, fourteen header fields, the transaction-hash sequence, and uncle/withdrawal counts.
- Can_do_roundtrip_regression is the only test that decodes canonical fixed wire, which makes it the only place a self-canceling encode/decode error is observable. It now asserts field values derived from the fixed hex with an independent RLP decoder (pyrlp): number, gas limit, gas used, timestamp, state root, the single transaction's nonce and value, and the empty uncle list.
- The [Ignore]d Write_rlp_of_blocks_to_file is deleted: a permanently ignored, assert-free hive-debugging utility (resurrectable from git history).

Types of changes

What types of changes does your code introduce?

Refactoring

Testing

Requires testing

Yes

If yes, did you write tests?

Yes

Notes on testing

Every strengthened assertion was mutation-checked (deliberate product break, confirm red, revert). The two headline mutations target exactly the blindness this chunk removes:

A coordinated boundary shift in both Rlp.LengthOf overloads (< 128 to < 127 in the ulong and UInt256 paths) keeps the overloads agreeing - the old cross-overload test stays logically satisfied - but fails the spec anchor at exactly value 127.
A fully self-canceling GasLimit/GasUsed swap in HeaderDecoder (decode and encode) passes every byte-roundtrip, hash, and scenario-field assert in the file - the decoded object is correct because the two swaps cancel; only the wire layout is wrong - and fails exactly the two pyrlp-anchored asserts in the canonical-wire regression test.
A coordinated transaction-order reversal (decode and the live encode loop) is caught by the existing pre-encoded-transactions comparison tests (verified live, 7 failures), so the wire-order class is covered at fixture level.
Expectation flips (vector nibble, spec-helper threshold) confirmed red.

Full local suite green (windows-x64, release): Core.Test 5984 total, 0 failed (the skip count drops by one - the deleted [Ignore]d case).

Golden derivations: the keccak vector and the regression-block field values were derived with pycryptodome and pyrlp respectively; the spec-length helper transcribes Yellow Paper appendix B and is structurally different from the product implementation (loop vs LeadingZeroCount).

Documentation

Requires documentation update

No

Requires explanation in Release Notes

No

…ed file writer

…d indexed asserts

claude · 2026-08-06T02:24:19Z

Claude finished @benaadams's task in 7m 17s —— View job

Review: test hygiene for crypto and RLP tests

Gather context (diff, changed files, surrounding tests)
Verify the independently-derived expectations
Check repo rules adherence (test guidelines, coding style)
Post review (3 inline comments)

No product code is touched; test-only. The direction is right and the mutation-testing notes in the description are the kind of evidence that makes a "strengthen the asserts" PR reviewable. Three things below; one is worth acting on.

Verification I could actually do

dotnet and python3 are not runnable in this environment, so I hand-decoded the fixed regression hex (BlockDecoderTests.cs:115) item by item instead of trusting the pyrlp claim. Every anchored value checks out:

assert	wire item	value
`Number`	`82 160c`	5644 ✓
`Difficulty`	`01`	1 ✓
`Beneficiary`	`94 0000…00`	`Address.Zero` ✓
`GasLimit`	`83 7a1200`	8,000,000 ✓
`GasUsed`	`82 5208`	21,000 ✓
`Timestamp`	`84 5c54648e`	1,549,034,638 ✓
`StateRoot`	`a0 fe77dd4a…`	✓ (4th header item)
`tx.Nonce` / `GasPrice` / `GasLimit`	`80` / `84 3b9aca00` / `82 5208`	0 / 1e9 / 21,000 ✓
`tx.To`	`94 22ea9f6b…`	✓
`tx.Value`	`8a 152d02c7e14af6800000`	1e23 ✓
`Uncles` empty	trailing `c0`	✓

Field order also matches HeaderDecoder.DecodeInternal, so the anchors are pinned to the right items — not just to plausible numbers.

SpecLengthOf agrees with both production overloads at the interesting points: Rlp.cs:694 and Rlp.cs:627 both return 1 for value < 128, and ULongValues() yields 0 (via pow2 - 1 at i == 0), so the sub-0x80 branch is genuinely exercised rather than assumed. The loop-vs-LeadingZeroCount divergence is real, so the coordinated-boundary-shift mutation described in the PR body would indeed be caught at 127. Yellow Paper reference in the <remarks> satisfies the coding-style rule for spec-derived logic.

The keccak vector I could not re-derive, but a wrong constant fails both asserts in CI, so it can't land silently.

Findings

Medium — Can_do_roundtrip_scenarios covers 18 of 23 round-tripped header fields (inline). Bloom, ParentBeaconBlockRoot, RequestsHash, BlockAccessListHash and SlotNumber are missing. Symmetric errors in those fields stay invisible — exactly the gap this PR exists to close, on the newest fields. Bloom is one line (set in every scenario, implements IEquatable<Bloom>); the four trailing optional fields are never set by any scenario in BuildScenarios(), so one extra scenario using the existing WithParentBeaconBlockRoot/WithRequestsHash/WithBlockAccessListHash builders would close the tail. Fix this →

Low — decoded.Hash == block.Hash cannot fail (inline). HeaderDecoder.cs:54 sets Hash from the raw incoming span and the builder sets the original via CalculateHash() over the same encoding, so both sides hash identical bytes. Its only content is cross-checking the block-embedded header encoding against the standalone one. The comment gestures at this, but the assert leads the block where a reader will read it as the strongest check. Same caveat, with genuine value, on the transaction loop: BaseTxDecoder.cs:68 derives Hash from tx wire bytes, so it does catch ordering and asymmetric errors but not a symmetric field swap inside TxDecoder — worth noting for a later chunk of the series.

Low — uncles and withdrawals are compared by count only. The scenarios build two uncles that differ deliberately (alternating WithdrawalsRoot), and withdrawal contents are never inspected. Not a regression — just the same class of gap one level down from the header.

Low (nits). KeccakTests.Span no longer describes what it asserts (inline). SpecLengthOf sits mid-file while RlpTests keeps its other helpers near the bottom (ULongValues() at ~line 628).

Things that are right

Deleting the permanently-[Ignore]d, assert-free Write_rlp_of_blocks_to_file matches the repo's "remove code over adding code" ordering, and the two now-unused usings went with it — nothing else in the file references File./ILogger.
The multiple-scope blocks are entered after the length/null guards, so the indexed asserts can't throw inside the scope.
No product code, no new public surface, no package changes.

Marking not-mergeable only on the Medium field-coverage point — it's a completeness gap in a test-hardening PR, not a product-correctness risk. If you'd rather scope the header tail to a later chunk in the series, say so and it's settled.
· branch test-hygiene/self-consistency-crypto-rlp

Copilot

Pull request overview

Strengthens Nethermind.Core.Test coverage by replacing self-referential roundtrip/cross-overload assertions with independently derived expectations, so these tests can fail when the product code is wrong in a coordinated way.

Changes:

Anchor KeccakTests.Span to an independent keccak-256 vector for a deterministic 1024-byte pattern.
Replace Rlp.LengthOf cross-overload equality with a spec-derived expected length helper (Yellow Paper appendix B rule shape).
Strengthen BlockDecoderTests by adding decoded-vs-original field assertions for scenario roundtrips and spec-anchored assertions for a fixed canonical-wire regression case; remove a permanently ignored debug utility test.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/Nethermind/Nethermind.Core.Test/RlpTests.cs	Replaces overload-vs-overload equality with a spec-anchored expected-length helper and asserts both overloads against it.
src/Nethermind/Nethermind.Core.Test/KeccakTests.cs	Anchors Span and array overload outputs to an independent keccak-256 expected hash.
src/Nethermind/Nethermind.Core.Test/Encoding/BlockDecoderTests.cs	Adds decoded-field assertions to scenario roundtrips, adds independent-value assertions for canonical-wire regression decoding, and deletes an ignored debug-only test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+        // A re-encode comparison alone cannot find a symmetric encode/decode error. The hash values also derive
+        // from the encoded bytes. Only direct field comparisons can find such an error.


claude · 2026-08-06T02:30:43Z

+            Assert.That(actual.WithdrawalsRoot, Is.EqualTo(expected.WithdrawalsRoot));
+            Assert.That(actual.BlobGasUsed, Is.EqualTo(expected.BlobGasUsed));
+            Assert.That(actual.ExcessBlobGas, Is.EqualTo(expected.ExcessBlobGas));
+            Assert.That(actual.MixHash, Is.EqualTo(expected.MixHash));


Medium — the field list stops short of the newest header fields.

HeaderDecoder round-trips 23 header items; this list covers 18. Missing: Bloom, ParentBeaconBlockRoot, RequestsHash, BlockAccessListHash, SlotNumber (see HeaderDecoder.cs:52 and :60-63). Since a symmetric encode/decode error is invisible to the re-encode comparison, those five fields keep exactly the blindness this PR is removing — and they're the newest, i.e. the likeliest to grow a bug.

Bloom is set in every scenario (BlockHeaderBuilder.cs:34 → Bloom.Empty) and Bloom implements IEquatable<Bloom>, so it's a free one-liner. The four trailing optional fields are never set by any scenario in BuildScenarios(), so they're vacuous until a scenario sets them — BlockBuilder already has WithParentBeaconBlockRoot/WithRequestsHash/WithBlockAccessListHash, so one extra scenario would close the whole tail.

Suggested change

Assert.That(actual.MixHash, Is.EqualTo(expected.MixHash));

Assert.That(actual.MixHash, Is.EqualTo(expected.MixHash));

Assert.That(actual.Bloom, Is.EqualTo(expected.Bloom));

Assert.That(actual.ParentBeaconBlockRoot, Is.EqualTo(expected.ParentBeaconBlockRoot));

Assert.That(actual.RequestsHash, Is.EqualTo(expected.RequestsHash));

Assert.That(actual.BlockAccessListHash, Is.EqualTo(expected.BlockAccessListHash));

Assert.That(actual.SlotNumber, Is.EqualTo(expected.SlotNumber));

claude · 2026-08-06T02:30:54Z

+        using (Assert.EnterMultipleScope())
+        {
+            Assert.That(encoded2.Bytes.ToHexString(), Is.EqualTo(encoded.Bytes.ToHexString()));
+            Assert.That(decoded.Hash, Is.EqualTo(block.Hash));


Low — this assert can't fail, and it reads like it can.

HeaderDecoder.cs:54 sets Hash = Keccak.Compute(headerRlp) from the raw incoming span, and the builder sets the original's Hash via CalculateHash() over the same encoding (BlockHeaderBuilder.cs:19). So both sides hash the same bytes: no decode defect — symmetric or asymmetric — can move this assert. Its only real content is that the header bytes embedded by BlockDecoder match what the standalone header encoder produces.

The comment two lines up gestures at this ("The hash values also derive from the encoded bytes"), but the assert is then placed first, where a reader will take it as the strongest check in the block. Either drop it or say explicitly that it cross-checks the block-embedded header encoding against the standalone one — otherwise it's the same self-consistency pattern the PR is removing elsewhere.

The same caveat applies to the transaction loop below: BaseTxDecoder.cs:68 sets Hash from the tx wire bytes, so comparing hashes catches ordering and asymmetric errors (good — that's real coverage) but not a symmetric field swap inside TxDecoder.

claude · 2026-08-06T02:31:02Z


-            Assert.That(Keccak.Compute(byteArray.AsSpan()), Is.EqualTo(Keccak.Compute(byteArray)));
+            // An independent keccak-256 implementation (pycryptodome) produced the expected hash.
+            Hash256 expected = new("0x5902e53903be0d0f9656bdbd5b9f0d8c2d815f865645d629eef77f5185f6cd7f");


Low (nit) — the test name Span described the old assertion (span overload vs array overload); now that both overloads assert a fixed vector, something like Computes_known_hash_for_span_and_array would say what it checks. Optional, and it does grow the diff.

I couldn't re-derive 0x5902e5… in this environment (no Python/dotnet execution available here), but a wrong constant fails both asserts in CI rather than passing silently, so the risk of a bad vector landing is nil.

…mments

benaadams · 2026-08-06T11:26:13Z

@claude re-review

claude · 2026-08-06T11:26:29Z

Claude finished @benaadams's task in 4m 53s —— View job

Re-review: test hygiene for crypto and RLP tests

Diff the current branch against the base and read the three files in full
Check the prior round's findings (1 Medium, 3 Low) against the new commits
Verify the new scenario's tail-field asserts are not vacuous
Re-check the spec helper and the canonical-wire anchors
Post review (1 inline comment)

Two commits landed since my last pass (7204812, 9d41c5a). The Medium is resolved and I could verify it's not a paper fix. No Critical/High/Medium findings remain; all 14 CI runs are green, including Nethermind tests, Nethermind tests (Flat DB), Code formatting and Code Lint.

Prior findings, re-checked

Finding	Status
Medium — 18 of 23 header fields compared	Resolved — all 23 now asserted (`BlockDecoderTests.cs:189-211`), and a new scenario makes the tail non-vacuous
Low — `decoded.Hash == block.Hash` reads stronger than it is	Resolved — line 184 now states plainly that it pins the embedded header bytes against the standalone encoding and cannot find decode errors
Low — `KeccakTests.Span` name no longer describes the assert	Resolved — renamed `Computes_known_hash_for_span_and_array`
Low — uncles/withdrawals compared by count only	Unchanged — inline, fine to defer
Copilot's note on the scenario comment overclaiming	Resolved — the comment (lines 176-178) now says a re-encode comparison cannot find a decode error the matching encode hides, and points at `Can_do_roundtrip_regression` for that class

The new scenario is genuinely discriminating

The risk with "add a scenario that sets the tail fields" is that the values collide and a field swap still passes. They don't — every tail field gets a distinct value, so any pairwise swap between same-typed fields is caught:

field	value	distinct from siblings
`Bloom`	`new Bloom([Build.A.LogEntry.TestObject])`	✓ — `LogEntry(Address.Zero, [], [Keccak.Zero])` sets bits, so ≠ `Bloom.Empty` (every other scenario)
`BaseFeePerGas`	3	✓ (other scenarios use 1)
`WithdrawalsRoot`	`new WithdrawalTrie(withdrawals).RootHash` (`BlockBuilder.cs:281-289`)	✓ vs Keccak A/B/C
`BlobGasUsed` / `ExcessBlobGas`	1 / 2	✓ — the pair differs, so the EIP-4844 pair-swap is visible
`ParentBeaconBlockRoot` / `RequestsHash` / `BlockAccessListHash`	KeccakA / KeccakB / KeccakC	✓
`SlotNumber`	7	✓ vs both blob-gas values

Setting all eight tail fields also drives HeaderDecoder's requiredItems cascade (HeaderDecoder.cs:144-167) to write the full tail, so the eight decode branches at :56-63 are all exercised — previously nothing past ExcessBlobGas was reached by any scenario. All builder methods used exist (WithBloom, WithParentBeaconBlockRoot, WithRequestsHash, WithBlockAccessListHash, WithSlotNumber), and Bloom : IEquatable<Bloom>, so the Bloom assert compares by value.

Re-verified from the previous round

The 13 pyrlp-anchored values in Can_do_roundtrip_regression still match my hand decode of the fixed hex, item by item, and still line up with HeaderDecoder.DecodeInternal field order — so they're pinned to the right wire items, not just to plausible numbers.
SpecLengthOf is exercised at the boundary: ULongValues() (RlpTests.cs:628-641) yields pow2 - 1, pow2, pow2 + 1, so i == 7 supplies 127/128/129, and i == 0 supplies 0 (Rlp.LengthOf(0) == 1, correct per appendix B). The loop-vs-LeadingZeroCount divergence is real, so the coordinated boundary shift described in the PR body is caught.

python3 and dotnet exist here but neither is runnable under this sandbox, so I could not re-derive the keccak vector or run the suite locally — as before, a wrong constant fails both asserts in CI rather than passing silently, and CI is green.

Remaining Low notes

Body compared by count only (inline). Uncle hashes would be discriminating today; withdrawals need distinct values first.
The tail-field wire layout has no canonical anchor anywhere. The scenario asserts prove the decoded object is right, but a symmetric swap of, say, ParentBeaconBlockRoot ↔ RequestsHash in both Encode and DecodeInternal still yields a correct object — and regression5644 is a pre-1559 block with no tail items, so it can't see the modern layout either. I checked HeaderDecoderTests: Can_encode_decode_with_cancun_fields and friends (:128-182) are all self-roundtrips too, so nothing in the repo pins post-Cancun header bytes. That's the honest residual for this class, it's outside this PR's files, and it's a natural next chunk in the series.
Pre-existing duplication: the scenarios at :79-88 and :89-97 are identical inputs differing only in line wrapping, so one is dead weight in every [ValueSource] consumer. Pre-existing in master and the repo rule is to leave unrelated code alone — noting only.

Things that are right

The field list is complete and ordered to match HeaderDecoder, which makes it easy to audit against the decoder when a field is next added.
The indexed transaction asserts stay guarded by the length check outside the multiple-scope (line 180), so a count mismatch reports rather than throwing.
The EnterMultipleScope() usage means one run surfaces every mismatched field instead of stopping at the first — worth having with 23 asserts.
Still test-only: no product code, no new public surface, no package changes.

Mergeable. The one Medium from the prior round is closed with a fix I could verify is discriminating rather than nominal; everything left is Low and explicitly scoped to later chunks in the series.
· branch test-hygiene/self-consistency-crypto-rlp

claude · 2026-08-06T11:30:40Z

+            Assert.That(decoded.Uncles.Length, Is.EqualTo(block.Uncles.Length));
+            Assert.That(decoded.Withdrawals?.Length, Is.EqualTo(block.Withdrawals?.Length));


Low (carry-over, optional) — the body is still compared by count only, one level below the now-complete header comparison.

Uncles are the cheap half and they're already discriminating: BuildScenarios() builds uncles[0] with WithdrawalsRoot == null and uncles[1] with Keccak.Compute("1") (line 39), so decoded.Uncles[i].Hash vs block.Uncles[i].Hash would catch an order or content error the count assert can't see. If you add that loop, the length guard has to move outside EnterMultipleScope() — the way the transaction guard sits at line 180 — otherwise a count mismatch throws IndexOutOfRangeException inside the scope instead of reporting.

Withdrawals are weaker: WithWithdrawals(8) fills the array with eight default new Withdrawal() instances, so a content comparison would be vacuous until a scenario uses distinct ones (TestItem.WithdrawalA_1Eth…WithdrawalF_6Eth exist).

Fine to leave for a later chunk in the series — this PR's stated scope is the header and the crypto/RLP anchors.

The body compared uncles by count only. The scenarios build two uncles with distinct headers, so the hash comparison catches an order or content error the count cannot see. The count guard moves outside the multiple-assert scope like the transaction guard.

benaadams added 7 commits August 6, 2026 02:27

test(core): anchor the keccak span test to an independent vector

e2e6cf3

test(core): anchor RLP ulong lengths to the spec

dcf731a

test(core): compare decoded blocks to the original and drop the ignor…

fc4ee9f

…ed file writer

test(core): compare decoded header fields to the original block

6040988

test(core): anchor the regression block decode to pyrlp-derived fields

4a51eba

test(core): apply review round - drop dead using, widen anchors, guar…

b26b434

…d indexed asserts

test(core): apply round-2 review polish

8eb5104

Copilot AI review requested due to automatic review settings August 6, 2026 02:24

core-repository-dispatch-app Bot added refactoring test labels Aug 6, 2026

Copilot started reviewing on behalf of benaadams August 6, 2026 02:24 View session

Copilot AI reviewed Aug 6, 2026

View reviewed changes

claude Bot reviewed Aug 6, 2026

View reviewed changes

benaadams added 2 commits August 6, 2026 03:54

test(core): cover the header tail fields and sharpen the roundtrip co…

7204812

…mments

test(core): apply confirm-round nits

9d41c5a

claude Bot reviewed Aug 6, 2026

View reviewed changes

Navigation Menu

Uh oh!

test: anchor crypto and RLP tests to independent expectations - #12712

test: anchor crypto and RLP tests to independent expectations#12712
benaadams wants to merge 10 commits into
masterfrom
test-hygiene/self-consistency-crypto-rlp

benaadams commented Aug 6, 2026

Uh oh!

claude Bot commented Aug 6, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

claude Bot Aug 6, 2026

Uh oh!

claude Bot Aug 6, 2026

Uh oh!

claude Bot Aug 6, 2026

Uh oh!

benaadams commented Aug 6, 2026

Uh oh!

claude Bot commented Aug 6, 2026 •

edited

Loading

Uh oh!

claude Bot Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// A re-encode comparison alone cannot find a symmetric encode/decode error. The hash values also derive
		// from the encoded bytes. Only direct field comparisons can find such an error.

-            Assert.That(actual.MixHash, Is.EqualTo(expected.MixHash));
+            Assert.That(actual.MixHash, Is.EqualTo(expected.MixHash));
+            Assert.That(actual.Bloom, Is.EqualTo(expected.Bloom));
+            Assert.That(actual.ParentBeaconBlockRoot, Is.EqualTo(expected.ParentBeaconBlockRoot));
+            Assert.That(actual.RequestsHash, Is.EqualTo(expected.RequestsHash));
+            Assert.That(actual.BlockAccessListHash, Is.EqualTo(expected.BlockAccessListHash));
+            Assert.That(actual.SlotNumber, Is.EqualTo(expected.SlotNumber));

		Assert.That(decoded.Uncles.Length, Is.EqualTo(block.Uncles.Length));
		Assert.That(decoded.Withdrawals?.Length, Is.EqualTo(block.Withdrawals?.Length));

Uh oh!

Conversation

benaadams commented Aug 6, 2026

Changes

Types of changes

What types of changes does your code introduce?

Testing

Requires testing

If yes, did you write tests?

Notes on testing

Documentation

Requires documentation update

Requires explanation in Release Notes

Uh oh!

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

Uh oh!

Review: test hygiene for crypto and RLP tests

Verification I could actually do

Findings

Things that are right

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

claude Bot Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

benaadams commented Aug 6, 2026

Uh oh!

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

Uh oh!

Re-review: test hygiene for crypto and RLP tests

Prior findings, re-checked

The new scenario is genuinely discriminating

Re-verified from the previous round

Remaining Low notes

Things that are right

Uh oh!

claude Bot Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

claude Bot commented Aug 6, 2026 •

edited

Loading

claude Bot commented Aug 6, 2026 •

edited

Loading