fix: stack overflow in debug_trace

[deffrian]

Fixes Closes Resolves #

#8380

Changes

• Allow more nested calls
• Iterative converter

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

[claude]

Claude finished @deffrian's task in 6m 45s —— View job

---

PR Review: fix/stack-overflow-trace

• Gather context and read changed files
• Read relevant rule files
• Analyze changes for correctness, edge cases, security, and performance
• Post inline comments for specific issues
• Final summary

---

Summary

The core fix — converting NativeCallTracerCallFrameConverter from recursive JSON serialization to an explicit iterative DFS work stack — is correct and well-implemented. It eliminates the ~9-frame-per-level call stack growth that caused stack overflows on chains approaching MaxCallDepth = 1024. The algorithm correctly handles leaf frames, frames with multiple children, and the open/close bracketing of the calls array. Tests are thoughtful and cover the regression, the streaming path, and the boundary condition.

---

Findings

Medium

DefaultMaxDepth = 4096 raises the deserialization depth limit — JsonSerializerOptions.MaxDepth governs both serialization and deserialization. The iterative converter no longer relies on serializer recursion depth for the calls tree; only the writer's MaxDepth (set via JsonWriterOptions) needs to be large. The old deserialization limit of 128 was a reasonable DoS guard for JSON-RPC input: a malicious client sending 4096-deep JSON now gets parsed rather than rejected. Consider splitting the concern with separate serialization/deserialization depth constants, or add an explicit rationale in code for why the unified 4096 limit is acceptable. (Inline comment on EthereumJsonSerializer.cs:25)

Low

1. MaxDepth on a SkipValidation = true writer is a no-op — Utf8JsonWriter enforces MaxDepth only inside ValidateStart(), which is skipped when SkipValidation = true. The MaxDepth = EthereumJsonSerializer.DefaultMaxDepth added to Startup.cs:429 has no runtime effect. Either remove it or add a comment clarifying it is intentional documentation only. (Inline comment on Startup.cs:429)

2. Test_CallTrace_DeepNesting_FailsBeyondMaxDepth hardcodes an internal .NET exception message — The assertion pins exact System.Text.Json wording that can change between .NET releases, making the test brittle. Consider asserting on a stable substring ("depth") instead of the full string. (Inline comment on GethLikeCallTracerTests.cs:705)

---

What's correct

• The DFS work stack correctly maintains writer state: each frame with calls does exactly one WriteStartArray/WriteEndArray pair, bracketed with WriteStartObject/WriteEndObject, and the frame index is correctly incremented when re-pushed. No double-close or missing-close edge cases.
• ForcedNumberConversion.ForcedConversion.Value is set once before the loop and restored in finally — this is equivalent to the old recursive approach and correct for all frames serialized by WriteFrameHeader.
• NativeCallTracerCallFrame.Dispose() calls Calls.DisposeRecursive(), so using on the root in tests correctly cleans up the entire chain.
• Test_CallTrace_DeepNesting_StreamedJsonIsComplete correctly counts 1024 traversed frames for a chain of MaxCallDepth = 1024 frames (the while loop runs 1023 times starting from observed = 1).
• The depth math in the boundary test is correct: a chain of N frames produces max JSON writer depth 2N − 1; at N = 2048 = DefaultMaxDepth / 2, depth is 4095 < 4096, and at N = 2049, depth 4097 triggers the exception.

[claude]

Medium — DefaultMaxDepth = 4096 widens the deserialization attack surface

JsonSerializerOptions.MaxDepth applies to both serialization and deserialization. The iterative converter removes the call-stack overflow by eliminating recursion in the converter itself; it writes depth via direct WriteStartObject/WriteStartArray calls, which the writer tracks — not the serializer's recursion depth. Only the writer's MaxDepth (set via JsonWriterOptions) needs to be 4096.

For deserialization of incoming JSON-RPC requests, the old limit of 128 was a reasonable DoS guard. Raising it 32× means a malicious client can send a 4096-level nested JSON body and the deserializer will happily recurse through it (custom converters chain recursively via JsonSerializer.Deserialize), consuming more CPU and stack.

Consider splitting the concern:

// Output (trace serialization) needs deep writer
public const int DefaultMaxDepth = 4096;

// Input (RPC deserialization) can stay conservative
public const int DefaultDeserializationMaxDepth = 128;

Or document explicitly why the unified limit increase is acceptable here.

[deffrian]

@claude We have input size limit, can you verify?

[claude]

Low — MaxDepth on a SkipValidation = true writer is a no-op

System.Text.Json's Utf8JsonWriter enforces MaxDepth inside ValidateStart(), which is only called when _options.SkipValidation is false. With SkipValidation = true, depth tracking still advances (the _currentDepth field increments) but the guard check is never reached, so MaxDepth = EthereumJsonSerializer.DefaultMaxDepth has no effect on overflow protection here.

If the intent is defensive documentation (signal the intended limit), add a comment. If the intent was actual enforcement, SkipValidation must be removed — though the actual overflow for the trace case is already fixed by the iterative converter. Fix this →

[claude]

Low — hardcoded System.Text.Json internal error message is brittle

This assertion pins the exact error text produced by Utf8JsonWriter when the depth limit is reached. That message is an internal implementation detail — it can change wording between .NET releases and will silently break this test.

Consider asserting only on exception type and optionally a stable substring ("MaxDepth", "depth"), or simply drop the message assertion entirely:

Throws.TypeOf<JsonException>()
    .With.InnerException.TypeOf<InvalidOperationException>()
    .And.InnerException.Message.Contains("depth", StringComparison.OrdinalIgnoreCase)

[claude]

Claude finished @deffrian's task in 3m 5s —— View job

---

PR Review: fix/stack-overflow-trace

• Gather context and read changed files
• Read relevant rule files
• Analyze changes for correctness, edge cases, security, and performance
• Post inline comments for specific issues
• Final summary

---

Summary

The core fix — converting NativeCallTracerCallFrameConverter from recursive JSON serialization to an explicit iterative DFS work stack — is correct and well-implemented. The algorithm correctly handles leaf frames, frames with multiple children, and the open/close bracketing of the calls array. Tests are thoughtful and cover the regression, the streaming path, and the boundary condition.

The findings below carry over from the initial review and remain unresolved. The Medium issue should be acknowledged or addressed before merge.

---

Findings

Medium

DefaultMaxDepth = 4096 widens the deserialization attack surface — JsonSerializerOptions.MaxDepth governs both serialization and deserialization. The iterative converter eliminates stack recursion in the serialization path; but for deserialization, custom converters still chain via JsonSerializer.Deserialize, which recurses per JSON depth level. The old limit of 128 was a reasonable guard for incoming RPC payloads. Now that:

• EthereumJsonSerializer.DefaultMaxDepth = 4096 (used by the static JsonOptions and as the default for instance options)
• IJsonRpcConfig.JsonSerializationMaxDepth defaults to EthereumJsonSerializer.DefaultMaxDepth (configured in ApiBuilder.cs:41)

…a malicious client can send 4096-deep nested JSON and the deserializer will happily recurse through it, consuming proportionally more CPU and stack. Consider splitting the concern with separate serialization/deserialization depth constants so the output depth can be 4096 while the input guard stays conservative (e.g., 128–256).

Low

1. MaxDepth on a SkipValidation = true writer is a no-op — Utf8JsonWriter enforces MaxDepth inside ValidateStart(), which is bypassed when SkipValidation = true. The MaxDepth = EthereumJsonSerializer.DefaultMaxDepth in Startup.cs:429 has no runtime effect. Either remove it or add a comment clarifying the intent. (Inline comment posted)

2. Test_CallTrace_DeepNesting_FailsBeyondMaxDepth hardcodes an internal .NET exception message — the assertion pins System.Text.Json wording that can change between .NET releases. Consider asserting on a stable substring ("depth" or "MaxDepth") rather than the full string. (Inline comment posted)

---

What's correct

• The iterative DFS work stack correctly handles all frame configurations: leaf frames terminate with WriteEndObject; frames with children push themselves (with incremented nextChild) then the child, so children are serialized in order. No double-close or missing-close edge cases.
• When nextChild >= 0 on re-entry, frame.Calls is guaranteed non-null (it was verified non-null and Count > 0 before the frame was first pushed with nextChild = 0).
• ForcedNumberConversion.ForcedConversion.Value is set once before the loop and restored in finally — equivalent to the recursive approach and correct for all frames.
• Test_CallTrace_DeepNesting_StreamedJsonIsComplete correctly counts 1024 traversed frames for a MaxCallDepth = 1024 chain.
• The depth boundary math is correct: N frames → max JSON writer depth 2N − 1; at N = boundary = DefaultMaxDepth / 2 = 2048, depth is 4095 < 4096 ✓; at N = 2049, depth 4097 triggers the exception ✓.

[claude]

Low — hardcoded System.Text.Json internal exception message is brittle

This exact wording is an internal implementation detail that can change between .NET releases, silently breaking the test. Consider asserting only on a stable substring:

Suggested change

	$"CurrentDepth ({EthereumJsonSerializer.DefaultMaxDepth}) is equal to or larger than the maximum allowed depth of {EthereumJsonSerializer.DefaultMaxDepth}. Cannot write the next JSON object or array."),
	.And.InnerException.Message.Contains("depth", StringComparison.OrdinalIgnoreCase),

[deffrian]

Better to leave as this. It's not hard to fix

[LukaszRozmej]

Is it potential DOS vector though?

[LukaszRozmej]

Should we have such large default? What about DOS vectors?