Skip to content

perf(rpc): cut eth_call overhead in memory, code stream, and tx parse #12716

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kamilchodola wants to merge 9 commits into
base: master
Choose a base branch
from
Open

perf(rpc): cut eth_call overhead in memory, code stream, and tx parse #12716

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8994d58
perf(rpc): cut eth_call overhead in memory, code stream, and tx parse
kamilchodola Aug 6, 2026
fdc7da2
chore: trim verbose comments on eth_call perf changes
kamilchodola Aug 6, 2026
f58ff48
fix(evm): clamp GetTrace view and chunk-zero dirty memory
kamilchodola Aug 6, 2026
54e9009
fix: drop unused Json.Nodes using after reader-based tx parse
kamilchodola Aug 6, 2026
a05ef7c
Merge branch 'master' into perf/eth-call-memory-and-rpc-parse
kamilchodola Aug 6, 2026
e1c5c1b
fix: address remaining Claude review findings
kamilchodola Aug 6, 2026
67712f4
Update src/Nethermind/Nethermind.Evm/CodeAnalysis/CodeInfo.cs
kamilchodola Aug 6, 2026
1f3fc9d
perf(evm): reuse static empty TraceMemory for zero-size traces
kamilchodola Aug 6, 2026
c711e6d
Merge branch 'master' into perf/eth-call-memory-and-rpc-parse
kamilchodola Aug 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/Nethermind/Nethermind.Evm.Test/CodeAnalysis/CodeInfoTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,20 @@ public void PushNJumpdest_Over10k(int n)
}
}

[Test]
public void GetOrBuildStream_without_code_hash_is_unavailable()
{
byte[] code = Enumerable.Repeat((byte)Instruction.JUMPDEST, StreamInterpreter.BuildThreshold + 8).ToArray();
CodeInfo codeInfo = new(code);

for (int i = 0; i < StreamInterpreter.BuildThreshold + 2; i++)
{
Assert.That(codeInfo.GetOrBuildStream(), Is.Null);
}

Assert.That(codeInfo.GetOrBuildStream(), Is.Null);
}
Comment on lines +211 to +236

@claude claude Bot Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low — this test passes unchanged on master, so it doesn't guard the optimisation it's named after.

On master, GetOrBuildStream() with a default CodeHash also returns null on every call: the cache lookup is skipped by the CodeHash != default guard, the first BuildThreshold - 1 calls return null on the hit counter, and the call that crosses the threshold schedules a background build that BuildStream then refuses to publish (CodeHash != default fails) — returning null too. So every assertion here holds both before and after the change.

To actually pin the behaviour, assert the observable difference: that no build was scheduled. E.g. expose/observe _streamBuildState transitioning to StreamBuildUnavailable on the very first call, or assert InstructionStreamCache gained no entry and no work item ran (the sibling InstructionStreamTests.cs:490 already uses a SpinUntil on GetOrBuildStream and could be inverted here).

@kamilchodola kamilchodola Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: test now uses BuildThreshold = 1, asserts no stream appears for default CodeHash within a short spin, and a hashed control does build.


[TestCaseSource(nameof(Codes))]
public void JumpDestinationAnalyzer_are_equivalent(byte[] codeInput)
{
Expand Down
36 changes: 35 additions & 1 deletion src/Nethermind/Nethermind.Evm.Test/EvmPooledMemoryTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,12 @@ public void CalculateMemoryCost_LengthExceedsULong_ShouldReturnOutOfGas()
Assert.That(result, Is.EqualTo(0UL));
}

[TestCase(1024)]
[TestCase(4096)]
[TestCase(32 * 1024)]
[TestCase(70 * 1024)]
[TestCase(2 * 1024 * 1024)]
public void Large_pooled_buffer_is_zeroed_on_reuse(int size)
public void Pooled_buffer_is_zeroed_on_reuse(int size)
{
EvmPooledMemory dirty = new();
UInt256 zero = UInt256.Zero;
Expand All @@ -121,6 +124,37 @@ public void Large_pooled_buffer_is_zeroed_on_reuse(int size)
clean.Dispose();
}

[Test]
public void Grow_after_dirty_reuse_preserves_written_prefix_and_zeroes_tail()
{
const int firstSize = 2048;
const int secondSize = 8192;

EvmPooledMemory dirty = new();
Span<byte> pattern = new byte[firstSize];
pattern.Fill(0xaa);
Assert.That(dirty.TrySave(UInt256.Zero, pattern), Is.True);
dirty.Dispose();

EvmPooledMemory next = new();
try
{
byte[] firstWord = TestItem.KeccakA.BytesToArray();
Assert.That(next.TrySaveWord(UInt256.Zero, firstWord), Is.True);

UInt256 growOffset = (UInt256)(secondSize - EvmPooledMemory.WordSize);
Assert.That(next.TryLoadSpan(in growOffset, (UInt256)EvmPooledMemory.WordSize, out Span<byte> tail), Is.True);
Assert.That(tail.ToArray(), Is.EqualTo(new byte[EvmPooledMemory.WordSize]));

Assert.That(next.TryLoadSpan(UInt256.Zero, (UInt256)EvmPooledMemory.WordSize, out Span<byte> head), Is.True);
Assert.That(head.ToArray(), Is.EqualTo(firstWord));
}
finally
{
next.Dispose();
}
}

[Test]
public void CalculateMemoryCost_LengthExceedsLongMax_ShouldReturnOutOfGas()
{
Expand Down
8 changes: 7 additions & 1 deletion src/Nethermind/Nethermind.Evm/CodeAnalysis/CodeInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,13 @@ public CodeInfo(IPrecompile? precompile)
{
if (Volatile.Read(ref _streamBuildState) == StreamBuildUnavailable)
return null;
if (CodeHash != default && InstructionStreamCache.TryGet(CodeHash, out InstructionStream? cached))
// No cache key (e.g. eth_call overrides) — building would only be discarded.
if (CodeHash == default)
{
Volatile.Write(ref _streamBuildState, StreamBuildUnavailable);
return null;
}

@claude claude Bot Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium — latching StreamBuildUnavailable here permanently poisons the instance, and CodeHash is a public setter assigned after construction.

CodeHash is public ValueHash256 CodeHash { get; set; } and is filled in by StaticCodeCache.Set (StaticCodeCache.cs:21), i.e. after new CodeInfo(code). Today every cacheable path happens to Set before the instance is ever executed (CacheCodeInfoRepository.GetOrCacheCodeInfo, InsertCode, SetDelegation), so this is latent rather than live. But the moment any path executes a CodeInfo once before publishing it to the code cache — or publishes it to a cache implementation that doesn't assign CodeHash — that bytecode loses the stream interpreter for the entire lifetime of the cached instance, silently and with no way to recover. That's a hard-to-diagnose performance cliff on a hot contract.

The Volatile.Write buys essentially nothing: it saves one ValueHash256 comparison on a path that has already taken a Volatile.Read and is about to take a cache lookup. Making the skip stateless removes the failure mode entirely:

// No cache key (e.g. eth_call overrides) — building would only be discarded.
if (CodeHash == default)
    return null;

If you do want to keep the latch, CodeHash should become set-once/immutable so the ordering is enforced rather than assumed.

Fix this →

@kamilchodola kamilchodola Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: skip is now stateless (if (CodeHash == default) return null;) — no latch of StreamBuildUnavailable, so a late CodeHash assignment can still enable stream builds.

if (InstructionStreamCache.TryGet(CodeHash, out InstructionStream? cached))
return cached;
if (Interlocked.Increment(ref _streamHits) < StreamInterpreter.BuildThreshold)
return null;
Expand Down
59 changes: 32 additions & 27 deletions src/Nethermind/Nethermind.Evm/EvmPooledMemory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ public void Dispose()
if (memory is not null)
{
_memory = null;
ReturnClean(memory, (int)Math.Min(Size, (ulong)memory.Length));
Return(memory);
}
}

Expand Down Expand Up @@ -453,50 +453,48 @@ private void EnsureRented()

private const int MinRentSize = 1_024;
private const int MaxCachedArrayLength = 1 << 16;
private const int CleanCacheSlots = 16;
private const int CacheSlots = 16;

[ThreadStatic] private static byte[]?[]? _cleanArrays;
[ThreadStatic] private static int _cleanArrayCount;
[ThreadStatic] private static byte[]?[]? _cachedArrays;
[ThreadStatic] private static int _cachedArrayCount;

private static byte[] RentClean(int minLength)
// Cached dirty; RentSlow zero-extends [_lastZeroedSize, Size) on growth.
private static byte[] Rent(int minLength)
{
byte[]?[]? cache = _cleanArrays;
int cleanArrayCount = _cleanArrayCount - 1;
for (int i = cleanArrayCount; i >= 0; i--)
byte[]?[]? cache = _cachedArrays;
int cachedArrayCount = _cachedArrayCount - 1;
for (int i = cachedArrayCount; i >= 0; i--)
{
byte[] candidate = cache![i]!;
if (candidate.Length >= minLength)

@claude claude Bot Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High — the "no caller can observe bytes beyond the zeroed prefix" invariant does not hold for TraceMemory.

GetTrace() (line 396) does ClearForTracing(Size) and then hands the whole array to TraceMemory:

public TraceMemory GetTrace()
{
    ulong size = Size;
    ClearForTracing(size);
    return new(size, _memory);   // ReadOnlyMemory over _memory.Length, not size
}

TraceMemory.Slice(start, length) (Nethermind.Evm/Tracing/TraceMemory.cs:57) bounds-checks against span.Length — the array length — not Size:

ReadOnlySpan<byte> span = _memory.Span;
if (start + length > span.Length) { /* pad with zeros */ }
return span.Slice(start, length);   // <-- raw tail

Before this PR every rented buffer was fully zeroed (RentClean cleared the whole array, and RentSlow set _lastZeroedSize = _memory.Length), so an out-of-Size slice read back as zeros. Now _lastZeroedSize == Size and everything past it is the previous frame's / previous transaction's memory.

Failure scenario: debug_traceTransaction / debug_traceCall with a custom JS tracer whose step does log.memory.slice(0, 512) while log.memory.length() is 32 (Custom/JavaScript/Log.cs:69 → MemoryTrace.Slice). Old output: 32 real bytes + 480 zero bytes. New output: 32 real bytes + 480 bytes of whatever the thread-local cached buffer (MinRentSize = 1024) held from a prior frame — so trace output becomes non-deterministic and leaks unrelated EVM memory into the RPC response. NativePrestateTracer:160 and GethLikeTxDirectStreamingTracer:195 are safe because they clamp to Size.

Cheapest fix is to clamp what TraceMemory can see, which also restores the old zero-padding path:

public TraceMemory GetTrace()
{
    ulong size = Size;
    ClearForTracing(size);
    return new(size, _memory is null ? default : _memory.AsMemory(0, (int)Math.Min(size, (ulong)_memory.Length)));
}

Please also add a regression test (dirty a large buffer, dispose, then GetTrace().Slice(0, n) past Size and assert zeros).

Fix this →%20is%20zeroed.%20TraceMemory.Slice%20bounds-checks%20against%20the%20array%20length%2C%20so%20a%20JS%20tracer%20calling%20log.memory.slice%20past%20Size%20now%20reads%20stale%20bytes%20instead%20of%20zeros.%20Clamp%20the%20ReadOnlyMemory%20passed%20to%20TraceMemory%20to%20min(Size%2C%20_memory.Length)%20and%20add%20a%20regression%20test%20in%20EvmPooledMemoryTests.&repo=NethermindEth/nethermind)

@kamilchodola kamilchodola Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f58ff48.

GetTrace() now clamps the ReadOnlyMemory passed to TraceMemory to min(Size, _memory.Length), so Slice past the EVM high-water takes the zero-pad path instead of reading dirty tail bytes.

Regression: GetTrace_slice_past_size_does_not_leak_dirty_bytes — dirties a 1 KiB buffer, reuses it with Size=32, asserts trace.Slice(0, 512) is all zeros.

{
_cleanArrayCount = cleanArrayCount;
cache[i] = cache[cleanArrayCount];
cache[cleanArrayCount] = null;
_cachedArrayCount = cachedArrayCount;
cache[i] = cache[cachedArrayCount];
cache[cachedArrayCount] = null;
return candidate;
}
}

if (minLength > MaxCachedArrayLength)
{
byte[] pooled = RentLarge(minLength);
Array.Clear(pooled);
return pooled;
return RentLarge(minLength);
}

return new byte[BitOperations.RoundUpToPowerOf2((uint)minLength)];
}

private static void ReturnClean(byte[] array, int dirtyLength)
private static void Return(byte[] array)
{
if (array.Length > MaxCachedArrayLength)
{
ReturnLarge(array);
return;
}

byte[]?[] cache = _cleanArrays ??= new byte[CleanCacheSlots][];
if (_cleanArrayCount < CleanCacheSlots)
byte[]?[] cache = _cachedArrays ??= new byte[CacheSlots][];
if (_cachedArrayCount < CacheSlots)
{
Array.Clear(array, 0, dirtyLength);
cache[_cleanArrayCount++] = array;
cache[_cachedArrayCount++] = array;
}
}

Expand Down Expand Up @@ -528,19 +526,26 @@ private static void ReturnLarge(byte[] array)
[MethodImpl(MethodImplOptions.NoInlining)]
private void RentSlow()
{
if (_memory is null)
byte[]? memory = _memory;
if (memory is null)
{
_memory = RentClean((int)Math.Max((uint)Size, MinRentSize));
_memory = memory = Rent((int)Math.Max((uint)Size, MinRentSize));
_lastZeroedSize = 0;
}
else if (Size > (ulong)_memory.LongLength)
else if (Size > (ulong)memory.LongLength)
{
byte[] beforeResize = _memory;
_memory = RentClean(TruncateToInt32(Size));
Array.Copy(beforeResize, 0, _memory, 0, beforeResize.Length);
ReturnClean(beforeResize, beforeResize.Length);
byte[] grown = Rent(TruncateToInt32(Size));
Array.Copy(memory, 0, grown, 0, (int)_lastZeroedSize);
Return(memory);
_memory = memory = grown;
}

_lastZeroedSize = (ulong)_memory.Length;
ulong size = Size;
if (size > _lastZeroedSize)
{
Array.Clear(memory, (int)_lastZeroedSize, (int)(size - _lastZeroedSize));
_lastZeroedSize = size;
}
Comment on lines +553 to +561

@claude claude Bot Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium — this moves RentSlow from "once per buffer reallocation" to "once per memory expansion". That is a plausible block-processing regression, and the reported benchmark does not cover it.

EnsureRented fires when memory is null || Size > memory.Length || Size > _lastZeroedSize. Before this PR _lastZeroedSize was always _memory.Length, so the third clause was subsumed by the second and RentSlow was reached only when the buffer actually had to grow. Now the third clause is the live one, so every memory-expanding opcode takes the NoInlining RentSlow call plus an Array.Clear call.

Failure scenario: a contract that ABI-encodes sequentially (MSTORE walking the high-water mark 32 bytes at a time across 32 KiB) goes from ~1000 inlined no-op checks + one 32 KiB Array.Clear at frame exit, to ~1000 non-inlined RentSlow calls + 1000 32-byte Array.Clear calls. "Total bytes cleared are the same" is true, but small Array.Clear is dominated by call/setup cost, not by the stores — so the per-op cost goes up while only one bulk clear is removed.

The PR reports only an eth_call RPC sweep (−2.7% avg). This is the hot path for all block processing, so please run the reproducible payload benchmark (.github/workflows/run-expb-reproducible-benchmarks.yml, or the gas-benchmark skill) before merging, and post the result.

A cheap way to keep the deferred-clear win while amortising the call overhead is to zero-extend to a chunk boundary rather than exactly to Size:

ulong size = Size;
if (size > _lastZeroedSize)
{
    // Over-zero to a chunk boundary so sequential growth doesn't take RentSlow on every word.
    const ulong ZeroChunk = 4 * 1024;
    ulong target = Math.Min((ulong)memory.Length, (size + (ZeroChunk - 1)) & ~(ZeroChunk - 1));
    Array.Clear(memory, (int)_lastZeroedSize, (int)(target - _lastZeroedSize));
    _lastZeroedSize = target;
}

_lastZeroedSize >= Size is preserved, so every other invariant in the file still holds.

@kamilchodola kamilchodola Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f58ff48 (amortization part).

RentSlow now zero-extends to a 4 KiB boundary (capped at memory.Length), so sequential word-by-word MSTORE growth stays inside an already-zeroed chunk and does not take the NoInlining path per op. Invariant _lastZeroedSize >= Size still holds.

On the payload-benchmark ask: agreed this is block-processing hot path; will follow up with expb numbers (or gate on CI) before merge. The chunking change is the cheap insurance in the meantime.

}

// (int)(uint)value rather than (int)value: RyuJIT emits noticeably worse codegen for a
Expand Down
96 changes: 81 additions & 15 deletions src/Nethermind/Nethermind.Facade/Eth/RpcTransaction/TransactionForRpc.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Numerics;
using System.Reflection;
using System.Text;
using System.Text.Json;
using System.Text.Json.Nodes;

Check warning on line 11 in src/Nethermind/Nethermind.Facade/Eth/RpcTransaction/TransactionForRpc.cs

View workflow job for this annotation

GitHub Actions / Check code lint 

Using directive is unnecessary. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0005) [/home/runner/work/nethermind/nethermind/src/Nethermind/Nethermind.Facade/Nethermind.Facade.csproj]

@claude claude Bot Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low — using System.Text.Json.Nodes; is now unused and will fail the lint job.

JsonObject / JsonNode no longer appear anywhere in this file after DeriveTxType was rewritten. .github/workflows/code-lint.yml flags unused using directives, so CI will go red on this. Please drop the line.

Suggested change
using System.Text.Json.Nodes;

using System.Text.Json.Serialization;
using Nethermind.Core;
using Nethermind.Core.Crypto;
Expand Down Expand Up @@ -158,7 +160,8 @@
TxType = T.TxType,
Type = txType,
FromTransactionFunc = T.FromTransaction,
DiscriminatorProperties = uniqueProperties
DiscriminatorProperties = uniqueProperties,
DiscriminatorPropertiesUtf8 = Array.ConvertAll(uniqueProperties, static p => Encoding.UTF8.GetBytes(p.ToLowerInvariant()))
};

int existingTypeInfo = _txTypes.FindIndex(t => t.TxType == typeInfo.TxType);
Expand All @@ -184,12 +187,10 @@

public override TransactionForRpc? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
// Copy the reader so we can do a double parse:
// The first parse is used to check for fields, while the second parses the entire Transaction
// Peek property names for the concrete type, then deserialize (no DOM).
Utf8JsonReader txTypeReader = reader;
JsonObject untyped = JsonSerializer.Deserialize<JsonObject>(ref txTypeReader, options);

Type concreteTxType = DeriveTxType(untyped, options, out bool isDefaulted);
Type concreteTxType = DeriveTxType(ref txTypeReader, options, out bool isDefaulted);

TransactionForRpc? result = (TransactionForRpc?)JsonSerializer.Deserialize(ref reader, concreteTxType, options);
if (result is not null)
Expand All @@ -199,29 +200,76 @@
return result;
}

private Type DeriveTxType(JsonObject untyped, JsonSerializerOptions options, out bool isDefaulted)
private static ReadOnlySpan<byte> TypeFieldUtf8 => "type"u8;
private static ReadOnlySpan<byte> GasPriceFieldUtf8 => "gasprice"u8;

private Type DeriveTxType(ref Utf8JsonReader reader, JsonSerializerOptions options, out bool isDefaulted)
{
const string gasPriceFieldKey = nameof(LegacyTransactionForRpc.GasPrice);
const string typeFieldKey = nameof(TransactionForRpc.Type);
TxType? setType = null;
bool hasGasPrice = false;
// Bit i set ⇒ discriminator for _txTypes[i] seen; lowest bit wins (registration order).
ulong discriminated = 0;

if (reader.TokenType == JsonTokenType.StartObject)
{
while (reader.Read() && reader.TokenType == JsonTokenType.PropertyName)
{
if (setType is null && NameEqualsIgnoreCase(ref reader, TypeFieldUtf8))
{
reader.Read();
setType = JsonSerializer.Deserialize<TxType?>(ref reader, options);
continue;

@claude claude Bot Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low (perf, free win) — once setType is non-null the answer is fully determined, so the scan can stop here.

setType is not null short-circuits ahead of both hasGasPrice and viaDiscriminator, so nothing read after this point can change the result. Continuing means the reader still has to scan to the closing quote of input — which on the simulation-style payloads this PR is optimising for is hundreds of kilobytes, and type is conventionally the first property.

Suggested change
continue;
if (setType is not null) break;
continue;

@kamilchodola kamilchodola Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: break the property scan once setType is non-null.

}

if (!hasGasPrice && NameEqualsIgnoreCase(ref reader, GasPriceFieldUtf8))
{
hasGasPrice = true;
}
else
{
int count = Math.Min(_txTypes.Count, 64);
for (int i = 0; i < count; i++)
{
foreach (byte[] discriminator in _txTypes[i].DiscriminatorPropertiesUtf8)
{
if (NameEqualsIgnoreCase(ref reader, discriminator))
{
discriminated |= 1UL << i;
break;
}
}
}
}

reader.Read();
reader.Skip();

@claude claude Bot Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low — Utf8JsonReader.Skip() throws InvalidOperationException on a non-final-block reader; TrySkip() doesn't.

Per the BCL contract, Skip() throws "The reader was given partial data with more data to follow (that is, IsFinalBlock is false)". The previous JsonSerializer.Deserialize<JsonObject>(ref txTypeReader, options) had no such constraint.

Today all reachable call sites are safe — the JSON-RPC pipeline builds its reader with isFinalBlock: true (JsonRpcProcessor.cs:449) and batch items go through JsonDocument.Parse — and I couldn't find any IJsonSerializer.Deserialize<T>(Stream) call whose T transitively contains a TransactionForRpc. So this is a latent trap rather than a live bug, but the converter is a public extension point and the failure would surface as a 500 on eth_call, not a JSON parse error.

TrySkip() costs nothing here and removes the constraint:

Suggested change
reader.Skip();
if (!reader.TrySkip()) break;

@kamilchodola kamilchodola Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: reader.TrySkip() instead of Skip().

}
}

if (untyped.TryGetPropertyValue(typeFieldKey, out JsonNode? node))
Type? viaDiscriminator = null;
if (discriminated != 0)
{
TxType? setType = node.Deserialize<TxType?>(options);
if (setType is not null)
viaDiscriminator = _txTypes[BitOperations.TrailingZeroCount(discriminated)].Type;
}

if (setType is not null)
{
isDefaulted = false;
foreach (TxTypeInfo candidate in _txTypes)
{
isDefaulted = false;
return _txTypes.FirstOrDefault(p => p.TxType == setType)?.Type ?? throw new JsonException("Unknown transaction type");
if (candidate.TxType == setType) return candidate.Type;
}

throw new JsonException("Unknown transaction type");
}

if (untyped.ContainsKey(gasPriceFieldKey))
if (hasGasPrice)
{
isDefaulted = true;
return typeof(LegacyTransactionForRpc);
}

// Discriminator field is a strong signal — not a default.
Type? viaDiscriminator = _txTypes.FirstOrDefault(p => p.DiscriminatorProperties.Any(untyped.ContainsKey))?.Type;
if (viaDiscriminator is not null)
{
isDefaulted = false;
Expand All @@ -232,6 +280,23 @@
return typeof(EIP1559TransactionForRpc);
}

private static bool NameEqualsIgnoreCase(ref Utf8JsonReader reader, ReadOnlySpan<byte> lowerCaseName)
{
if (reader.HasValueSequence || reader.ValueIsEscaped)
{
return string.Equals(reader.GetString(), Encoding.UTF8.GetString(lowerCaseName), StringComparison.OrdinalIgnoreCase);

@claude claude Bot Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low — the escaped-name fallback allocates two strings per comparison, and it is called once per (property × discriminator) pair.

Encoding.UTF8.GetString(lowerCaseName) re-materialises a compile-time-constant literal on every call, and reader.GetString() allocates the unescaped name again for each of the ~7 discriminators plus type/gasprice. An RPC caller can trigger this path deliberately by escaping every property name ("type", "input", …), turning one request into O(properties × discriminators) string allocations.

reader.ValueTextEquals(utf8) handles both escaping and HasValueSequence allocation-free, but is ordinal — so the cleanest shape is to try the ordinal UTF-8 compare first and only fall back for genuinely mixed-case escaped names, or to unescape once per property (into a stack buffer) before entering the discriminator loop rather than once per comparison.

Also worth noting for future maintainers: the | 0x20 fold is only sound because every current discriminator name is pure ASCII letters. Since RegisterTransactionType<T> is an extension point (Optimism registers its own tx type), the invariant is worth stating in the doc comment — b | 0x20 == target has a second pre-image for non-letter targets.

@kamilchodola kamilchodola Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: hot path stays allocation-free; escaped path tries ValueTextEquals first, then one GetString with ASCII fold (no per-call Encoding.UTF8.GetString of the literal). Noted ASCII-letters invariant on the fold.

}

ReadOnlySpan<byte> name = reader.ValueSpan;
if (name.Length != lowerCaseName.Length) return false;
for (int i = 0; i < name.Length; i++)
{
if ((name[i] | 0x20) != lowerCaseName[i]) return false;
}

return true;
}

public override void Write(Utf8JsonWriter writer, TransactionForRpc value, JsonSerializerOptions options) => JsonSerializer.Serialize(writer, value, value.GetType(), options);

public static TransactionForRpc FromTransaction(Transaction tx, in TransactionForRpcContext extraData) => _txTypes.FirstOrDefault(t => t.TxType == tx.Type)?.FromTransactionFunc(tx, extraData)
Expand All @@ -243,6 +308,7 @@
public Type Type { get; set; }
public FromTransactionFunc FromTransactionFunc { get; set; }
public string[] DiscriminatorProperties { get; set; } = [];
public byte[][] DiscriminatorPropertiesUtf8 { get; set; } = [];

@claude claude Bot Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low — DiscriminatorProperties is now dead; per AGENTS.md ("If a change makes existing code unused, remove it") it should go.

After the rewrite, DiscriminatorProperties is only ever assigned — nothing reads it (I checked the whole repo; the only other hits are the assignment at line 163 and the <see cref> in the doc comment that the follow-up commit removed). Drop the string[] property and build the UTF-8 array straight from uniqueProperties in RegisterTransactionType.

Separately: Math.Min(_txTypes.Count, 64) at line 230 silently stops matching discriminators for any type registered at index ≥ 64. With 5 built-in types plus plugin registrations that's not reachable today, but the truncation is invisible. A Debug.Assert(_txTypes.Count <= 64) in RegisterTransactionType would make the bitmask's capacity an enforced invariant rather than a silent cap.

@kamilchodola kamilchodola Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: removed dead DiscriminatorProperties; build UTF-8 discriminators directly. Added Debug.Assert(_txTypes.Count < 64) on registration.

}
}

Expand Down
5 changes: 5 additions & 0 deletions src/Nethermind/Nethermind.JsonRpc.Test/Data/TransactionForRpcDeserializationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ public static IEnumerable TxJsonTestCases
yield return Make(TxType.EIP1559, """{"type":"0x2"}""");
yield return Make(TxType.Blob, """{"type":"0x3"}""");
yield return Make(TxType.SetCode, """{"type":"0x4"}""");

string largeInput = "0x" + new string('a', 64 * 1024);
yield return Make(TxType.EIP1559, $$"""{"type":"0x2","input":"{{largeInput}}","maxFeePerGas":"0x1"}""");
yield return Make(TxType.Legacy, $$"""{"gasPrice":"0x1","input":"{{largeInput}}"}""");
yield return Make(TxType.AccessList, $$"""{"accessList":[],"input":"{{largeInput}}"}""");
}
}

Expand Down
11 changes: 9 additions & 2 deletions src/Nethermind/Nethermind.State.Flat/SnapshotBundle.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,8 +469,15 @@ private static int CountBufferedNodes(IEnumerable<List<(TreePath Path, TrieNode
public void SetAccount(Address address, Account? account) =>
_changedAccounts[address] = account;

internal void PromoteAccount(Address address, Account? account) =>
_changedAccounts.TryAdd(address, account);
internal void PromoteAccount(Address address, Account? account)
{
// ContainsKey is lock-free; TryAdd alone would take the bucket lock on every hot re-promote.
HashedKey<Address> key = new(address);
if (!_changedAccounts.ContainsKey(key))

@claude claude Bot Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low (perf) — on the FlatWorldStateScope.Get path this ContainsKey is a guaranteed miss, so it is pure added work there.

PromoteAccount has exactly two live callers, and they have opposite profiles:

  • FlatWorldStateScope.cs:170 — HintGet(address, account, promote: !isInCurrentSnapshot). isInCurrentSnapshot is set by DoGetAccount (line 91) from _changedAccounts.TryGetValue(key, …) on the same dictionary. So promote is true only when the caller has just proven the key absent, and the new ContainsKey re-lookup will (barring a concurrent insert) always return false — a second hash + probe, then TryAdd takes the bucket lock anyway.
  • PrewarmerScopeProvider.cs:152 → FlatWorldStateScope.cs:186 (promote: true, unconditional). This is where the win is: repeated hints for the same hot account skip the bucket lock.

So the optimisation is real, but it's targeted at the prewarmer path only, and it taxes the Get path. Since DoGetAccount already computed HashedKey<Address> key and knows the answer, the cheapest shape is to let the caller that knows skip the probe — e.g. an overload that takes the already-hashed key and a knownAbsent flag, or simply keep TryAdd for the Get path and use the pre-check only on the unconditional HintGet.

Not a correctness issue (TryAdd still arbitrates the race), and the miss cost is a lock-free probe on an already-hot line — hence Low. But if the −2.7% was attributed partly to this, it's worth confirming which caller dominates before assuming the win generalises.

Fix this →

{
_changedAccounts.TryAdd(key, account);
}
}

public void SetChangedSlot(Address address, in UInt256 index, byte[] value)
{
Expand Down
Loading