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 3 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
62 changes: 61 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,63 @@ 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 GetTrace_slice_past_size_does_not_leak_dirty_bytes()
{
const int dirtySize = 1024;
EvmPooledMemory dirty = new();
Span<byte> pattern = new byte[dirtySize];
pattern.Fill(0xff);
Assert.That(dirty.TrySave(UInt256.Zero, pattern), Is.True);
dirty.Dispose();

EvmPooledMemory memory = new();
try
{
memory.CalculateMemoryCost(0, 32, out bool outOfGas);
Assert.That(outOfGas, Is.False);

TraceMemory trace = memory.GetTrace();
Assert.That(trace.Size, Is.EqualTo(32UL));
Assert.That(trace.Slice(0, 512).ToArray(), Is.EqualTo(new byte[512]));

@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 regression test passes on the unfixed code, so it does not guard the GetTrace clamp. Two independent reasons, either one alone is enough to make it vacuous:

1. The dirty buffer is never rented. CalculateMemoryCost(0, 32, …) → ComputeMemoryExpansionCost → UpdateSize(newSize, rentIfNeeded: false) (EvmPooledMemory.cs:391). So Size becomes 32 but _memory is still null when GetTrace() runs. The assertion is satisfied by the brand-new _memory is null early return — and it was equally satisfied before the fix, because new(size, _memory) with a null array produced an empty ReadOnlyMemory<byte> and Slice took the zero-pad path anyway.

2. Even with a real rent, 1 KiB can't hold a dirty byte. dirtySize == MinRentSize == 1024, and the new 4 KiB chunk rounding in RentSlow zeroes min(memory.Length, roundUp(32, 4096)) = min(1024, 4096) = 1024 — the entire buffer. So no offset in that array is dirty, clamp or not.

To make it discriminate, force an actual rent and use a buffer bigger than the 4 KiB zero chunk:

Suggested change
const int dirtySize = 1024;
EvmPooledMemory dirty = new();
Span<byte> pattern = new byte[dirtySize];
pattern.Fill(0xff);
Assert.That(dirty.TrySave(UInt256.Zero, pattern), Is.True);
dirty.Dispose();
EvmPooledMemory memory = new();
try
{
memory.CalculateMemoryCost(0, 32, out bool outOfGas);
Assert.That(outOfGas, Is.False);
TraceMemory trace = memory.GetTrace();
Assert.That(trace.Size, Is.EqualTo(32UL));
Assert.That(trace.Slice(0, 512).ToArray(), Is.EqualTo(new byte[512]));
// Must exceed the 4 KiB RentSlow zero chunk, otherwise the whole buffer is zeroed anyway.
const int dirtySize = 32 * 1024;
EvmPooledMemory dirty = new();
Span<byte> pattern = new byte[dirtySize];
pattern.Fill(0xff);
Assert.That(dirty.TrySave(UInt256.Zero, pattern), Is.True);
dirty.Dispose();
EvmPooledMemory memory = new();
try
{
// TrySaveWord rents (unlike CalculateMemoryCost), so the dirty buffer is reused with Size = 32.
Assert.That(memory.TrySaveWord(UInt256.Zero, new byte[EvmPooledMemory.WordSize]), Is.True);
TraceMemory trace = memory.GetTrace();
Assert.That(trace.Size, Is.EqualTo((ulong)EvmPooledMemory.WordSize));
Assert.That(trace.Slice(0, 8 * 1024).ToArray(), Is.EqualTo(new byte[8 * 1024]), "trace leaked dirty tail bytes past Size");
}

Rent scans the thread-local cache from the top down and takes the first entry with Length >= minLength, and the dirtied 32 KiB array is the most recently returned — so it is deterministically the one picked, same assumption the sibling Pooled_buffer_is_zeroed_on_reuse already relies on. I verified this by reading; I couldn't run dotnet test in this job (command not in the allowlist), so please confirm the amended test fails with the GetTrace clamp reverted before merging.

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: test now dirties 32 KiB (past the 4 KiB zero chunk), forces a real rent via TrySaveWord, and asserts Slice(0, 8 KiB) is zeros. Confirmed locally with EvmPooledMemoryTests green.

}
finally
{
memory.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
68 changes: 40 additions & 28 deletions src/Nethermind/Nethermind.Evm/EvmPooledMemory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,11 @@ public TraceMemory GetTrace()
{
ulong size = Size;
ClearForTracing(size);
return new(size, _memory);
// Clamp to Size so TraceMemory.Slice past the EVM high-water cannot see dirty tail bytes.
if (_memory is null || size == 0)
return new(size, default);
Comment thread
LukaszRozmej marked this conversation as resolved.
int visible = (int)Math.Min(size, (ulong)_memory.Length);
return new(size, _memory.AsMemory(0, visible));
}

public void Dispose()
Expand All @@ -407,7 +411,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 +457,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 past Size in chunks 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)
{
_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 +530,29 @@ 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)
{
// Over-zero to a chunk boundary so sequential MSTORE growth does not take RentSlow per 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;
}
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
Loading
Loading