Skip to content

Avoid blowing the stack with serde_stacker#4617

Open
MartinquaXD wants to merge 5 commits into
mainfrom
serde-stacker
Open

Avoid blowing the stack with serde_stacker#4617
MartinquaXD wants to merge 5 commits into
mainfrom
serde-stacker

Conversation

@MartinquaXD

Copy link
Copy Markdown
Contributor

Description

Follow up to #4615 (comment).
While the concerns about blowing the stack limit are very likely non-issues protecting against the panic is also not very hard so let's be on the safe side.

Changes

use serde_stacker::Deserializer which manages the serde's stack while recursing through the object in a growable Vec instead of using the stack directly.

How to test

Used the not-checked in test again to see that everything deserializes correctly. This surfaced that we need #[serde(default)] on the calls field.

#[cfg(test)]
mod tests {
    use {super::*, alloy::primitives::b256};

    #[tokio::test]
    async fn test_base_tx() {
        let provider = ethrpc::Web3::new_from_url("http://localhost:8545");
        let traces = fetch_debug_trace(
            &provider.provider,
            eth::TxId(b256!(
                "0x5200aab12fbe4e0aef019748bf0f79266155fbbea00557bf1071fa2859e7eb9b"
            )),
        )
        .await
        .unwrap();
        dbg!(traces);
    }
}

@MartinquaXD MartinquaXD requested a review from a team as a code owner July 10, 2026 11:52
@socket-security

socket-security Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedserde_stacker@​0.1.1410010093100100

View full report

@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MartinquaXD's task in 2m 25s —— View job


PR Review: Avoid blowing the stack with serde_stacker

  • Read PR diff and changed files
  • Analyze correctness of the switch to direct deserialization
  • Check CI status
  • Post inline findings & summary

The core change is sound. Pairing disable_recursion_limit() (guards serde_json's depth counter) with serde_stacker::Deserializer (grows the actual thread stack) is the correct combination for handling pathologically deep call traces. Deserializing straight into the CallFrame domain type is fine here — serde ignores the extra geth fields, the lowercase field names match the JSON, Option<Address> defaults to None when to is absent, and #[serde(default)] correctly handles leaf frames that omit calls.

One cleanup left over from the refactor (posted inline):

  • Dead From impl in domain/blockchain.rsimpl From<alloy::rpc::types::trace::geth::CallFrame> no longer has any caller now that the GethTrace::CallTracer(...).into() path is gone. It won't trigger a compiler warning (trait impl on public types), so it'll silently rot. Worth removing.

Non-blocking note: this PR makes it possible to build very deeply nested CallFrame trees that previously failed at deserialization. The BFS traversal in find_settlement_trace_and_callers is queue-based and safe, but the recursive Drop of Vec<CallFrame> still runs on the normal (~2 MB tokio worker) stack. For a trace deep enough to need serde_stacker, dropping it could in theory overflow. In practice the drop frames are far smaller than serde's, so the threshold is much higher — likely fine, just flagging it as a latent edge case if extreme traces ever show up.

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request integrates serde_stacker to deserialize transaction call frames directly into the domain CallFrame struct, preventing stack overflows on deeply nested call stacks. A critical compilation error was identified in crates/autopilot/src/infra/blockchain/mod.rs where the error type returned by serde_stacker does not match the expected serde_json::Error in Error::DeserializationFailed. A code suggestion has been provided to map the error types correctly.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread crates/autopilot/src/infra/blockchain/mod.rs
// deserialize into the domain type.
/// Call frames of a transaction.
#[derive(Clone, Debug, Default)]
#[derive(Clone, Debug, Default, Deserialize)]

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.

Now that CallFrame derives Deserialize and is deserialized directly, the From<alloy::rpc::types::trace::geth::CallFrame> impl below (lines 26–35) becomes dead code — its only caller was the GethTrace::CallTracer(call_frame) => call_frame.into() arm that this PR removes. Nothing else in the workspace uses it (the settlement tests build CallFrame { .. } struct literals directly). Since it's a trait impl on public types the compiler won't flag it, so it'll silently rot. Suggest removing it.

Fix this →

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants