Enforce strict leading-zero validation for QUANTITY hex in BlockOverrides

[manusw7]

Fixes #12102

Changes

• Reject QUANTITY hex strings with leading zero digits ("0x0b" → -32602) in BlockOverrides fields (number, time, gasLimit, baseFeePerGas, blobBaseFee) per EIP-1474
• Add NullableQuantityUInt256Converter in Nethermind.Core.JsonConverters for strict QUANTITY validation on UInt256? fields
• Extend NumericConverterHelper to detect leading zeros for ulong/long QUANTITY fields when strict mode is enabled
• Wire JsonRpcQuantityFormat.StrictMode (new shared flag in Nethermind.Core) to EthereumJsonSerializer.StrictHexFormat setter so Nethermind.Evm converters don't need to reference Nethermind.Serialization.Json

Types of changes

• Bugfix (a non-breaking change that fixes an issue)

Testing

Requires testing

• Yes

If yes, did you write tests?

• Yes

Notes on testing

Unit tests for NullableQuantityUInt256Converter (strict/lenient, null, accept/reject cases) and NumericConverterHelper ulong strict-mode cases. Integration tests in EthRpcModuleTests.EthCall covering all five BlockOverrides QUANTITY fields — both rejection of leading-zero inputs and acceptance of valid quantities.

Documentation

Requires documentation update

• No

Requires explanation in Release Notes

• No

[claude]

Claude finished @manusw7's task in 6m 7s —— View job

---

PR Review

• Gather context and load changed files
• Review new converters and core logic
• Review test coverage
• Review integration points and wiring
• Post inline comments and final summary

---

Summary

The overall approach is sound: NullableQuantityUInt256Converter handles the UInt256? fields that can't use NumericConverterHelper (which lives in a higher-level assembly), and the JsonRpcQuantityFormat.StrictMode bridge correctly avoids a circular dependency between Nethermind.Core and Nethermind.Serialization.Json. The test harness wires strict mode properly via TestRpcBlockchain, and the integration tests cover all five QUANTITY fields.

One High and two Medium issues need to be addressed before merge.

---

Findings

High (1)

Buffer underflow in NullableQuantityUInt256Converter.ReadHex for hex inputs of 65–76 chars after "0x"
src/Nethermind/Nethermind.Core/JsonConverters/NullableQuantityUInt256Converter.cs line 39

The > 78 total-length guard is calibrated for the max decimal UInt256 representation (78 digits), but it also allows hex strings that are too wide for the 32-byte stack buffer. For a hex body of 65 chars, length = 33 and bytes[(32 - 33)..] = bytes[-1..] throws ArgumentOutOfRangeException. The server recovers (RPC layer catches it and returns -32602 Invalid params), but the error message is generic rather than descriptive, and log spam results from what may be a benign or malformed client. Fix: add if (hex.Length > 64) ThrowJsonException(); immediately after stripping "0x". (See inline comment.)

Medium (2)

Write depends on options containing UInt256Converter
src/Nethermind/Nethermind.Core/JsonConverters/NullableQuantityUInt256Converter.cs line 93

JsonSerializer.Serialize(writer, value.Value, options) re-enters the serializer to find a UInt256 converter. The test fixture _options does not register one, so serialising via those options would silently produce a JSON object. The production path is safe (EthereumJsonSerializer always registers it), but the converter is fragile outside that context. Prefer caching and calling UInt256Converter directly. (See inline comment.)

JsonRpcQuantityFormat.StrictMode defaults to false while JsonRpcConfig.StrictHexFormat defaults to true
src/Nethermind/Nethermind.Core/JsonConverters/JsonRpcQuantityFormat.cs line 16

The window between process start and EthereumJsonSerializer.StrictHexFormat being wired from config leaves NullableQuantityUInt256Converter silently lenient for any requests that arrive during early startup. Initialising the field to true would match the production default and eliminate the gap. (See inline comment.)

Low (2)

[NonParallelizable] applied at class level unnecessarily serialises tests that don't mutate global state — prefer per-method scoping as NullableULongConverterTests does. (See inline comment.)

Missing comment on the 78 magic number in the length guard — worth a brief note that it covers the max decimal UInt256 representation (78 digits) while the hex-specific 64-digit limit is enforced inside ReadHex.

[claude]

High — buffer underflow for oversized hex inputs

The > 78 guard is calibrated for the max decimal representation of UInt256 (78 chars), but it is too loose for the hex path. After stripping the "0x" prefix the remaining hex body can be up to 76 characters, while a UInt256 only fits 64. For hex.Length of 65–76:

length = (hex.Length >> 1) + hex.Length % 2   // e.g. 33 for 65 chars
bytes[(32 - length)..]                          // 32 - 33 = -1 → ArgumentOutOfRangeException

This throws an unhandled ArgumentOutOfRangeException (caught by the outer RPC try-catch and reported as -32602 Invalid params) rather than the descriptive SafePublicMessageFormatException. Any caller sending {"baseFeePerGas":"0x1000000000000000000000000000000000000000000000000000000000000001"} (65 hex chars after "0x") triggers this.

Fix: add a post-strip guard in ReadHex before the buffer operations:

if (hex.StartsWith("0x"u8))
{
    hex = hex[2..];
    if (hex.Length > 64)
        ThrowJsonException(); // UInt256 is 32 bytes = 64 hex digits max
    if (JsonRpcQuantityFormat.StrictMode && hex.Length > 1 && hex[0] == (byte)'0')
        ThrowLeadingZero();
}

Also worth adding a comment on line 39 to clarify: > 78 covers the 78-digit decimal max; the hex max (66 total) is enforced separately inside ReadHex.

[claude]

Medium — Write will produce incorrect output outside EthereumJsonSerializer

JsonSerializer.Serialize(writer, value.Value, options);

This relies on options having a UInt256Converter registered. The test fixture uses new JsonSerializerOptions { Converters = { _converter } } which does not include UInt256Converter, so serializing via those options would produce a JSON object (all UInt256 public properties) instead of a hex string. In production this works only because EthereumJsonSerializer.CreateOptions() always registers UInt256Converter.

Consider caching a UInt256Converter instance and calling it directly:

private static readonly UInt256Converter _uint256Converter = new();

public override void Write(Utf8JsonWriter writer, UInt256? value, JsonSerializerOptions options)
{
    if (value is null) { writer.WriteNullValue(); return; }
    _uint256Converter.Write(writer, value.Value, options);
}

This removes the dependency on options containing the right converter and avoids the overhead of re-entering the serializer metadata resolver.

[claude]

Medium — global mutable static creates invisible coupling

StrictMode is a process-wide flag that changes the behavior of every NullableQuantityUInt256Converter instance regardless of which JsonSerializerOptions it belongs to. This means:

1. The converter behaves differently depending on whether EthereumJsonSerializer.StrictHexFormat was set before the first deserialization — e.g. if another test or plugin component uses the converter before startup wires the flag.
2. The two flags (EthereumJsonSerializer._strictHexFormat and JsonRpcQuantityFormat.StrictMode) must always be in sync. Right now they are kept in sync by the setter, but the _strictHexFormat backing field is directly accessible via EthereumJsonSerializer.StrictHexFormat read — which reads _strictHexFormat, not StrictMode. They diverge if one is ever set without going through the setter.

A small improvement: consider initialising StrictMode to the IJsonRpcConfig.StrictHexFormat default (true) so that the converter is strict-by-default even before the startup hook fires:

public static volatile bool StrictMode = true; // mirrors JsonRpcConfig default

That makes the field's default safe without changing any existing behaviour (startup sets it explicitly anyway).

[claude]

Low — [NonParallelizable] scope is too broad

Marking the entire fixture [NonParallelizable] serialises all tests in the class, including those that don't touch global state (Accepts_null, Can_read_value_lenient_mode without strict-mode mutations, Lenient_mode_accepts_leading_zeros). NullableULongConverterTests correctly scopes [NonParallelizable] to the individual methods that mutate global state. Prefer the same pattern here:

[TestFixture]
public class NullableQuantityUInt256ConverterTests  // remove [NonParallelizable] from class
{
    [NonParallelizable]
    [TestCase("\"0x0b\"")]
    ...
    public void Strict_rejects_leading_zero(string json) { ... }

    [NonParallelizable]
    [TestCase("\"0x0\"", 0UL)]
    ...
    public void Strict_accepts_valid_quantity(string json, ulong expected) { ... }
}

This also removes the need for the SetUp/TearDown save-restore scaffold (which could silently break if the save is taken while another thread has already mutated the flag).

[manusw7]

@claude review again

[claude]

Claude finished @manusw7's task in 6m 25s —— View job

---

PR Review

• Read changed files
• Review new converters and core logic
• Review test coverage
• Review integration points and wiring
• Post final summary

---

Summary

All five findings from the previous review have been addressed. The PR is clean and the implementation is correct.

---

Previous Findings — Status

Severity	Finding	Status
High	Buffer underflow in ReadHex for 65–76-char hex body	✅ Fixed — if (hex.Length > 64) ThrowJsonException() is now correctly placed inside ReadHex after stripping 0x, before the buffer operations
Medium	Write re-enters serializer via JsonSerializer.Serialize(writer, value.Value, options) — fragile outside EthereumJsonSerializer	✅ Fixed — Write now directly computes the big-endian hex string using ToBigEndian + Convert.ToHexStringLower, with no dependency on options
Medium	JsonRpcQuantityFormat.StrictMode defaulted to false while JsonRpcConfig.StrictHexFormat defaults to true	✅ Fixed — StrictMode is now initialised to true, matching the production default
Low	[NonParallelizable] applied at class level, serialising tests that don't mutate global state	✅ Fixed — class-level attribute removed; [NonParallelizable] is now scoped to individual methods that mutate StrictMode
Low	Missing comment explaining the 78 magic number	✅ Fixed — comment // 78 covers the longest decimal UInt256 (78 digits); hex is further bounded to 64 digits inside ReadHex is in place

---

Fresh Review Findings

Low (3)

"0x" (bare prefix, no digits) silently deserialises to 0 instead of being rejected
src/Nethermind/Nethermind.Core/JsonConverters/NullableQuantityUInt256Converter.cs — ReadHex

The outer length is 0 or > 78 guard passes "0x" (length 2). Inside ReadHex, after stripping "0x" the body is empty (hex.Length = 0). The > 64 guard doesn't fire (0 ≤ 64), the strict-mode guard doesn't fire (hex.Length > 1 is false), and Bytes.FromUtf8HexString([], bytes[32..]) is a no-op, so the 32-byte zero buffer is returned as UInt256(0). A valid EIP-1474 QUANTITY must have at least one digit; "0x" should throw. Adding if (hex.IsEmpty) ThrowJsonException(); immediately after stripping the prefix would fix this. Note: this is consistent with the behavior of the existing UInt256Converter in EthereumJsonSerializer, so this isn't a regression introduced by the PR.

---

_strictHexFormat starts false while StrictMode starts true — intentional asymmetry worth a comment
src/Nethermind/Nethermind.Serialization.Json/EthereumJsonSerializer.cs:50

private static bool _strictHexFormat; defaults to false (ulong/long fields are lenient until ApiBuilder wires config), while JsonRpcQuantityFormat.StrictMode = true (UInt256? fields are strict immediately). This is intentional: changing _strictHexFormat to true would break existing tests like Can_read_value that pass "0x000" without setting strict mode. In production the window is zero (config is wired at ApiBuilder.cs:40 before the server accepts connections). Still, a short inline comment on _strictHexFormat explaining why it doesn't match the StrictMode default would prevent future confusion.

---

Write uses string allocations for every non-zero value
src/Nethermind/Nethermind.Core/JsonConverters/NullableQuantityUInt256Converter.cs:106–108

string hex = Convert.ToHexStringLower(bytes[start..]);
writer.WriteStringValue(hex[0] == '0' ? "0x" + hex[1..] : "0x" + hex);

Two string allocations per serialisation call. NullableQuantityUInt256Converter is only used for BaseFeePerGas and BlobBaseFee in BlockOverride — not a hot path — so this is acceptable. Noting it for completeness; the UInt256Converter in Nethermind.Serialization.Json uses the same pattern.

---

Positive Notes

• The ReadHex logic is correct for all cases: "0x0" early-exit, > 64 hex guard, strict leading-zero check, and decimal fallback.
• The Write method correctly strips exactly one leading zero nibble (e.g., byte 0x0b → hex "0b" → "0xb"), producing EIP-1474-compliant minimal hex for all UInt256 values including those beyond ulong range.
• The [JsonConverter(typeof(NullableQuantityUInt256Converter))] attribute on BlockOverride.BaseFeePerGas and BlockOverride.BlobBaseFee correctly overrides the global NullableUInt256Converter, and applies to both eth_call and eth_simulateV1 since they share the same BlockOverride type.
• The integration tests cover all five QUANTITY fields in both rejection and acceptance paths.
• SafePublicMessageFormatException inherits FormatException — the Throws.InstanceOf<FormatException>() assertion in unit tests is correct.
• The JsonRpcQuantityFormat bridge cleanly avoids a circular dependency between Nethermind.Core and Nethermind.Serialization.Json.

---

All previously blocking findings have been resolved. The three remaining findings are Low severity and do not block merge.

[github-actions]

EVM Opcode Benchmark Diff

Aggregated runs: base=3, pr=3
Noisy rerun opcodes: DIV, SDIV

Improvements (1)

Opcode	Base Median (ns)	PR Median (ns)	Delta	Abs Δ (ns)	Base CV	PR CV	Threshold	Uncertainty	Effective
MULMOD	1103.570	219.580	-80.10%	883.990	1.8%	2.2%	±5.0%	±1.5%	±5.0%

[benaadams]

Need to fix the failing tests