test: add hand-derived golden tests for eth/71 and snap serializers - #12699
test: add hand-derived golden tests for eth/71 and snap serializers#12699benaadams wants to merge 3 commits into
Conversation
Every serializer test file in Eth V71 and Snap/V1 now asserts an exact wire encoding. The goldens are hand-derived from the RLP rules and verified with an independent encoder (pyrlp + pycryptodome keccak). Random request ids are pinned only in the goldened tests; ByteCodes gains a sibling roundtrip that keeps the constructor-assigned id, and GetAccountRange pins long.MaxValue to keep the eight-byte id path.
The request-id, keccak-of-empty-string, and range-hash fragments moved into SnapSerializerGoldens. Each golden fragment and its test input come from one hex constant, so the expectation cannot drift from the input.
|
Claude finished @benaadams's task in 6m 24s —— View job Review: golden wire-encoding tests for eth/71 and snap
Verdict: correct as far as I can verify by hand. No Critical/High/Medium findings — three Low nits, all posted inline. Verification I actually didI re-derived every new expectation from the RLP rules rather than trusting the PR body, then confirmed each serializer writes the fields in the order the golden asserts. All 11 check out:
Specific things I chased down rather than assumed:
Findings (all Low, inline)
The deliberate omissions in the PR body (V68, the anchored V69/snap files, the Caveat on my verificationI could not compile or execute the fixtures — |
There was a problem hiding this comment.
Pull request overview
Adds hand-derived “golden” wire-encoding assertions for Eth/71 and Snap/V1 message serializers in Nethermind.Network.Test, and centralizes repeated Snap golden fragments/inputs to keep test vectors deterministic and reviewable.
Changes:
- Added
SnapSerializerGoldensto share Snap/V1 RLP fragments and matchingHash256inputs. - Pinned exact RLP encodings for Snap/V1 serializer roundtrips (GetAccountRange/GetByteCodes/GetStorageRanges/ByteCodes).
- Strengthened Eth/V71
BlockAccessLists/GetBlockAccessListsserializer tests by making expected encodings mandatory and adding concrete golden vectors.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Nethermind/Nethermind.Network.Test/P2P/Subprotocols/Snap/V1/Messages/SnapSerializerGoldens.cs | New shared constants for Snap/V1 golden fragments + matching hash inputs. |
| src/Nethermind/Nethermind.Network.Test/P2P/Subprotocols/Snap/V1/Messages/GetStorageRangesMessageSerializerTests.cs | Hoists common hashes to goldens and pins empty-case wire encoding. |
| src/Nethermind/Nethermind.Network.Test/P2P/Subprotocols/Snap/V1/Messages/GetByteCodesMessageSerializerTests.cs | Pins empty-case wire encoding and fixes request id determinism for that case. |
| src/Nethermind/Nethermind.Network.Test/P2P/Subprotocols/Snap/V1/Messages/GetAccountRangeMessageSerializerTests.cs | Pins wire encodings for both explicit and default-normalization roundtrips. |
| src/Nethermind/Nethermind.Network.Test/P2P/Subprotocols/Snap/V1/Messages/ByteCodesMessageSerializerTests.cs | Adds a pinned golden and an additional roundtrip to cover constructor-assigned request ids. |
| src/Nethermind/Nethermind.Network.Test/P2P/Subprotocols/Eth/V71/BlockAccessListsMessageSerializerTests.cs | Removes nullable-expected guard and adds concrete goldens for previously unpinned cases. |
Suppressed comments (1)
src/Nethermind/Nethermind.Network.Test/P2P/Subprotocols/Snap/V1/Messages/ByteCodesMessageSerializerTests.cs:32
- ByteCodesMessage implements Dispose() (and disposes the pooled Codes list); this test currently never disposes the message, which can leak ArrayPoolList rentals (and triggers DEBUG finalizer warnings).
ByteCodesMessage message = new(new ByteArrayListAdapter(data));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| ArrayPoolList<byte[]> data = new(2) { new byte[] { 0xde, 0xad, 0xc0, 0xde }, new byte[] { 0xfe, 0xed } }; | ||
|
|
||
| ByteCodesMessage message = new(new ByteArrayListAdapter(data)) { RequestId = 1 }; |
| @@ -32,15 +31,22 @@ public void Roundtrip() | |||
| Assert.That(deserializedMsg.AccountRange.LimitHash, Is.EqualTo(msg.AccountRange.LimitHash)); | |||
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// </summary> | ||
| /// <remarks> | ||
| /// Each golden fragment and its input come from one hex constant, so the | ||
| /// expectation cannot drift from the input. The values are verified with an | ||
| /// independent encoder (pyrlp + pycryptodome keccak). | ||
| /// </remarks> | ||
| internal static class SnapSerializerGoldens | ||
| { | ||
| private const string EmptyStringKeccakHex = "c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470"; | ||
| private const string RangeStartHex = "15d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470"; | ||
| private const string RangeLimitHex = "20d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470"; | ||
|
|
||
| /// <summary>Request id 1111 as an RLP item: 0x82 length prefix + 0x0457.</summary> |
There was a problem hiding this comment.
Low — two nits on the drift-proofing story in this file.
-
The
<remarks>says "Each golden fragment and its input come from one hex constant, so the expectation cannot drift from the input." That holds forRangeStart/RangeLimit, but not forEmptyStringKeccakRlp: its input in every consuming test isKeccak.OfAnEmptyString, not aHash256built fromEmptyStringKeccakHex. That's arguably the better arrangement (the literal independently pins keccak(""), which the shared-constant fragments deliberately don't), but the doc comment currently claims a property that fragment doesn't have. Worth narrowing the sentence to the range hashes and saying explicitly that the keccak("") literal is independent of the input on purpose. -
RequestId1111Rlpis the one fragment whose input isn't co-located: each of the three consuming tests writesRequestId = 1111as a bare literal while the encoding lives here. Exposing the value alongside the fragment would make it match the pattern the rest of the file establishes:
/// <summary>The request id the range/bytecode request tests use.</summary>
public const long RequestId1111 = 1111;
/// <summary>Request id 1111 as an RLP item: 0x82 length prefix + 0x0457.</summary>
public const string RequestId1111Rlp = "820457";Neither is a correctness problem — the values themselves all check out.
| } | ||
|
|
||
| [Test] | ||
| public void Roundtrip_random_request_id() |
There was a problem hiding this comment.
Low — this new fixture member is the one place the PR moves against its own thesis: it adds a fresh non-deterministic test (MessageConstants.Random-assigned id via the constructor) while the rest of the diff is removing random ids. Its marginal coverage over what's already here looks thin — DecodeEncodeDecodeEmpty further down already pins the 8-byte-request-id path with exact bytes (202, 136, ... = ca 88 176a156ae55348b0 c0), so the only thing this adds is "an arbitrary id survives a roundtrip".
It also duplicates the three setup lines of Roundtrip verbatim, which AGENTS.md calls out directly ("When tests differ only by inputs and expected outputs, parameterize a single test with [TestCase(...)]"). Both cases differ only by id and expectation:
[TestCase(1L, "ca01c884deadc0de82feed")]
[TestCase(long.MaxValue, "d2887fffffffffffffffc884deadc0de82feed")]
public void Roundtrip(long requestId, string expectedData)
{
ArrayPoolList<byte[]> data = new(2) { new byte[] { 0xde, 0xad, 0xc0, 0xde }, new byte[] { 0xfe, 0xed } };
ByteCodesMessage message = new(new ByteArrayListAdapter(data)) { RequestId = requestId };
ByteCodesMessageSerializer serializer = new();
// The message encodes as [requestId, codes].
SerializerTester.TestZero(serializer, message, expectedData);
}That keeps the multi-byte-id path covered and deterministic. (Second vector derived the same way: 9 + 9 = 18 = 0x12 payload → 0xd2; please re-verify before taking it.) Sibling fixtures also spell case names Roundtrip_Empty / Roundtrip_Many, so Roundtrip_random_request_id is off-convention if it stays.
| })), | ||
| "e364e1a00000000000000000000000000000000000000000000000000000000000000000") | ||
| .SetName("Roundtrip_single_hash"); | ||
| // The hashes are Keccak.Zero, keccak("A"), and keccak("B"). |
There was a problem hiding this comment.
Low — this comment restates the three lines directly below it (Keccak.Zero, TestItem.KeccakA, TestItem.KeccakB), which AGENTS.md asks you to skip ("Comments that merely restate the code are noise"). The other two comments added in this file earn their keep — // Each hash encodes as 0xa0 + 32 bytes. and // A negative request id encodes as its unsigned two's-complement value. both explain a derivation a reader can't get from the code. This one doesn't.
If the intent was to make the two 32-byte literals traceable to their inputs, the useful version says that instead — e.g. // keccak("A") and keccak("B"); the same values #12696 pins in EthSerializerGoldens. — otherwise dropping the line is cleaner.
- ByteCodes: one parameterized Roundtrip; long.MaxValue pins the
eight-byte request id deterministically instead of a random id
- GetAccountRange: independent field asserts in EnterMultipleScope
- SnapSerializerGoldens: expose RequestId1111 next to its RLP item;
the remarks state which fragments share hex with inputs and that
the keccak("") fragment is an independent literal on purpose
Changes
How to review this fast: three commits - the golden tests, a user-requested constants hoist, then review-feedback fixes; 6 files, all under
Nethermind.Network.Test(verified: no non-test file in the diff); +113/-39.Rule for this chunk: every serializer test file in Eth V68/V69/V71 and Snap/V1 asserts an exact wire encoding somewhere. These encodings are consensus-observable, so no expected value was captured from Nethermind's own output - each was hand-derived from the RLP rules and independently verified with pyrlp (the ethereum reference RLP implementation) plus pycryptodome keccak.
Most files in these directories already had golden tests (V68, V69 Status/BlockRangeUpdate/Receipts with Geth-sourced vectors, Snap AccountRange/StorageRanges/TrieNodes/GetTrieNodes). The gaps:
BlockAccessListsMessageSerializerTests: the two roundtrip cases that passed a null expectation now pinc22ac0(empty) andca88ff...c0(a negative request id encodes as its unsigned two's-complement value); the now-unreachable null guard is gone and the expectation parameter is non-nullable. TheGetBlockAccessListsfixture had no exact-encoding assert at all; its three roundtrip cases now carry hand-derived expected values.GetAccountRangeMessageSerializerTests: both tests pinned.Roundtrip(request id 1111) pins the flat[requestId, rootHash, startingHash, limitHash, responseBytes]layout.Roundtrip_DefaultspinsRequestId = long.MaxValue- keeping the eight-byte-id encode path deterministically covered - and wire-pins the serializer's normalizations: a null limit hash goes out asKeccak.MaxValueand response bytes 0 as 1,000,000.GetByteCodes/GetStorageRanges: the empty-variant roundtrips pin their layouts; expected values are split at field boundaries so a reviewer can map segment to field.ByteCodesMessageSerializerTests: the roundtrip is parameterized over two pinned ids -1(ca01c884deadc0de82feed) andlong.MaxValue(d288...), which covers the eight-byte-id path deterministically. The constructor assigns a random id by default - a first golden attempt caught that live.MessageConstants.Random.NextLong()) are pinned only in tests that assert exact bytes; the other roundtrips keep them.SnapSerializerGoldens(second commit): the repeated fragments - request id 1111, keccak("") item, range start/limit items - live once; each golden fragment and itsHash256test input are built from the same hex constant, so the expectation cannot drift from the input.Derivations (each re-derived byte-for-byte by two independent reviews): request ids per the RLP integer rules (including
-1-> eight0xffbytes andlong.MaxValue->887fffffffffffffff); hash items as0xa0+ 32 bytes with keccak("") =c5d246...(cross-checked against the existing V62 GetBlockHeaders golden test), keccak("A")/("B") matching the values pinned in #12696; framing lengths verified by frame arithmetic and pyrlp.Deliberately untouched: V68 and the anchored V69/Snap files above; the V71 keccak hex duplicates a constant #12696 adds (
EthSerializerGoldens) - folding it in is a post-merge follow-up since that branch is not an ancestor. V69ReceiptMessageDecoder69Testsdoes not pin the eth/69 bloom-absence on the wire; it is a decoder test outside this rule's wording and is tracked for a later chunk.Part of the test-hygiene series (#12689, #12690, #12693, #12694, #12696).
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
requestId ^ 1in the shared eth/66 serializer base fails 15 V71 tests including all five new expected values; swapping starting/limit hash in GetAccountRange fails both of its golden tests; per-serializer request-id mutations fail each snap golden test (ByteCodes fails all three tests including the pre-existing fixed vector). The constants hoist landed after the mutation pass; the reassembled values were machine-verified byte-identical, and all fixtures re-ran green.Documentation
Requires documentation update
Requires explanation in Release Notes