fix(jsonrpc): serialize receipt root as full-width DATA - #12706
fix(jsonrpc): serialize receipt root as full-width DATA#12706benaadams wants to merge 3 commits into
Conversation
|
Claude finished @benaadams's task in 3m 41s —— View job Review:
|
| Claim | Result |
|---|---|
ByteArrayConverter.Convert trims by default at nibble granularity |
✅ ByteArrayConverter.cs:328 — skipLeadingZeros = true, CountLeadingNibbleZeros() |
Hash256Converter.Write is fixed-width |
✅ Hash256Converter.cs:36 → HexWriter.WriteFixed32HexRawValue always emits 0x + 64 digits (HexWriter.cs:169-187) |
The converter is resolved from options |
✅ registered in EthereumJsonSerializer.cs:134; receipt.Root ?? Keccak.Zero is statically Hash256 (ReceiptForRpc.cs:63, Keccak.Zero is a Hash256), so the generic Serialize<Hash256> binds Hash256Converter — same path already used for blockHash on line 67 |
| Write surfaces limited to runner + t8n | ✅ only NethermindRunnerModule.cs:92 and tools/Evm/T8n/T8nCommand.cs:25 register it; standard receipt RPCs go through ReceiptForRpc, unaffected |
| No fixture pins a trimmed root | ✅ swept src/Nethermind and tools/ for "root" in .cs/.json — nothing pins 0x0 or a 63/62-digit root; there is no t8n golden-output test project, so no fixture churn |
| Read side unchanged / round-trip repaired | ✅ Hash256Converter.Read is untouched; 0x0 and odd-length hex fail in ByteArrayConverter, and a 62-digit root fails the Hash256 length check, so the old output really was unreadable by our own reader |
| No lint fallout | ✅ ByteArrayConverter and ForcedNumberConversion share namespace Nethermind.Serialization.Json, so the using is still needed — no dangling import |
Repo-sweep cross-check on "last trimming DATA writer": the remaining skipLeadingZeros: true writers are PublicKeyConverter.cs:38 (that's #12705, not this branch) and EvmWordConverter.cs:45 / TxTypeConverter.cs:33, both of which trim deliberately — EvmWordConverter documents itself as quantity format. So the claim holds.
Test design follows the repo rules: parameterized rather than duplicated, reuses CreateDiagnosticReceipt()/SerializeReceipt(), no var, is null used.
Findings
Low — geth parity for the empty-root case is partial. The PR body says "t8n output now matches the reference (geth) full-width form". That's exactly right for a real post-state root, but geth marshals PostState as hexutil.Bytes, so an absent root serializes as "0x" — not 64 zero digits. So for EIP-658 receipts t8n still differs from geth textually (0x0 before, 64 zeros now). It's strictly better than before (the new form at least round-trips through our own reader, 0x0 did not), and you scoped the field's shape out explicitly, so this is only a note on the wording of the release-notes/PR claim — worth narrowing it to the non-null case so nobody later reads it as a geth-parity guarantee for empty roots.
Low — one uncovered trim path in the test. The cases cover nibble trimming (63 digits) and the all-zero root, but not a root whose first byte is zero, which used to lose two digits. See the inline suggestion — one extra [TestCase]. Fix this →
Low — nits, take or leave.
ReceiptsForRpcTests.cs:80— theLeadingZeroRootHexconst sits between two test methods; every other private member in this fixture lives at the bottom.TxReceiptConverter.cs:34usesKeccak.Zerowhile line 67 usesHash256.Zerofor the same value. Pre-existing, and since the line is being touched anyway you could align it — but leaving it keeps the diff to one token, which is the better trade under the minimal-diff rule.
Not done
I could not execute the test suite — dotnet test isn't in my allowed tools in this run (permission denied), so the "fails on master / passes with the fix" behaviour is verified by reading ByteArrayConverter/Hash256Converter/HexWriter, not by running it. If you want that confirmed here, add dotnet test to --allowedTools.
· branch fix/txreceipt-root-json-full-width
There was a problem hiding this comment.
Pull request overview
This PR fixes JSON-RPC serialization of the transaction receipt root field to always emit fixed-width 32-byte DATA (0x + 64 hex digits), aligning with EIP-1474 and matching Nethermind’s existing Hash256Converter behavior.
Changes:
- Serialize receipt
rootviaJsonSerializer.Serialize(..., Hash256Converter)instead ofByteArrayConverter.Convert(...)(which trimmed leading zeros at nibble granularity). - Add a regression test covering both a leading-zero root (previously 63 hex digits) and a null root (previously
0x0, now full-width zero hash).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Nethermind/Nethermind.JsonRpc/Converters/TxReceiptConverter.cs | Switches root serialization to the Hash256 JSON converter to guarantee fixed-width output. |
| src/Nethermind/Nethermind.JsonRpc.Test/Data/ReceiptsForRpcTests.cs | Adds a parameterized regression test asserting full-width root serialization for leading-zero and null roots. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@claude re-review |
|
Claude finished @benaadams's task in 1m 53s —— View job Re-review:
|
| Check | Result |
|---|---|
receipt.Root ?? Keccak.Zero binds Hash256Converter |
✅ ReceiptForRpc.Root is Hash256? (ReceiptForRpc.cs:63) and Keccak.Zero is a Hash256, so Serialize<Hash256> resolves the registered converter — same as blockHash at TxReceiptConverter.cs:67 |
| Converter is present on the options the test uses | ✅ Hash256Converter(strictHexFormat) is in EthereumJsonSerializer's base set (EthereumJsonSerializer.cs:134); the test's EthereumJsonSerializer(new JsonConverter[]{ new TxReceiptConverter() }) ctor appends to that set rather than replacing it |
| Writer is unconditionally fixed-width | ✅ Hash256Converter.Write → HexWriter.WriteFixed32HexRawValue, which always emits " + 0x + 64 digits (HexWriter.cs:169-187); strictHexFormat only gates the read side |
ForcedNumberConversion.Value = Hex can't reintroduce trimming |
✅ Hash256Converter never consults it |
| Test wires the field the converter reads | ✅ PostTransactionState → ReceiptForRpc.Root (ReceiptForRpc.cs:34); setting it also flips Status to null, which is pre-existing shape and untouched by the assertion |
using Nethermind.Serialization.Json; still needed after dropping ByteArrayConverter |
✅ ForcedNumberConversion / NumberConversion live in the same namespace (TxReceiptConverter.cs:22-23,76) — no dangling import |
| No downstream consumer pins the old form | ✅ only "root" hit outside the changed files is tools/EngineApiProxy/Services/RequestOrchestrator.cs:105, an unrelated Beacon-API data.root read |
Test style matches the repo rules: parameterized rather than duplicated, reuses CreateDiagnosticReceipt() / SerializeReceipt(), no var, is null, consts instead of repeated literals.
Remaining Low notes (unchanged, both optional)
ReceiptsForRpcTests.cs:80-81— the two consts sit between test methods; every other private member in this fixture is at the bottom. Cosmetic. Fix this →- PR body / release-note wording — "t8n output now matches the reference (geth) full-width form" holds for a real post-state root, but geth marshals
PostStateashexutil.Bytes, so an absent root is"0x"there, not 64 zero digits. The new form is strictly better than0x0(it round-trips through our own reader), and you scoped the field's shape out explicitly — just worth narrowing the claim to the non-null case so it isn't later read as a geth-parity guarantee for empty roots.
Keccak.Zero at line 34 vs Hash256.Zero at line 67 is pre-existing inconsistency; leaving it keeps the diff at one line, which is the right trade here.
Not done
Still could not execute anything — both dotnet build and dotnet test are outside my allowed tools in this run (permission denied). The "fails on master / passes with the fix" behaviour and compilation of the new [TestCase] are verified by reading ByteArrayConverter / Hash256Converter / HexWriter and the NUnit attribute usage, not by running. Add dotnet build / dotnet test to --allowedTools if you want that confirmed here; CI covers it otherwise.
· branch fix/txreceipt-root-json-full-width
Changes
TxReceiptConverter.Writewrote the receiptrootviaByteArrayConverter.Convert's defaultskipLeadingZeros: true, which trims at nibble granularity. A receipt root is 32-byte DATA per EIP-1474 ("two hex digits per byte"), so a state root whose first hex digit is 0 lost digits (63 emitted), and a null root emitted0x0. The fix serializesrootthe same way the method already serializesblockHash- throughHash256Converter, whose writer is fixed-width (0x+ 64 digits) - removing the one line in the codebase that put aHash256on the wire trimmed.PostState.Receiptsis a rawTxReceipt[]- t8n output now matches the reference (geth) full-width form. Standard receipt RPCs (eth_getTransactionReceiptetc.) go throughReceiptForRpc, whoserootalready usedHash256Converter- unaffected (their pinned fixtures already show 64-digit roots).0x0and 63-digit roots throw on odd-length hex; a 62-digit root throws on length in theHash256constructor), so the fix also repairs write-read round-tripping.rooton every receipt, substituting the zero hash when there is no post-state root (EIP-658 receipts). Omitting the field instead would be a separate shape change.PublicKeyConverterfix; found by review on that PR. This was the last trimming DATA writer found in a repo sweep.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Serializes_root_as_full_width_data(parameterized: leading-zero-nibble root, leading-zero-byte root, null root - the 63-digit, 62-digit and0x0legacy outputs) fails every case on master and passes with the fix; all cases fail again under a fix revert and under expectation flips.0x0or 63-digit).Documentation
Requires documentation update
Requires explanation in Release Notes
t8n (and any surface serializing a raw
TxReceipt) now emitsrootas full-width DATA (0x+ 64 hex digits). A null root was previously0x0and is now 64 zero digits; roots with a leading zero digit were previously trimmed.