fix(invariant): remove unused cloned calldata #12893
Conversation
subwaycookiecrunch
requested review from
0xrusowsky,
DaniPopes,
grandizzy,
mattsse,
onbjerg and
zerosnacks
as code owners
There was a problem hiding this comment.
You're right , this file isn't required for the invariant memory fix.
Its presence here isn't intentional for this change, and I’ll remove it
from this PR so the scope stays minimal.
subwaycookiecrunch
force-pushed
the
fix/invariant-memory-usage
branch
from
278ae4d to
68b258a
Compare
| self.test_data.fuzz_cases.push(FuzzedCases::new(run.fuzz_runs)); | ||
|
|
||
| // Prune older cases if we have too many. | ||
| // We keep a rolling window of full traces for the last 4096 runs to aid debugging, | ||
| // but prune older ones to avoid unbounded memory usage. | ||
| const MAX_KEPT_CALLDATA: usize = 4096; | ||
| if self.test_data.fuzz_cases.len() > MAX_KEPT_CALLDATA { | ||
| let prune_index = self.test_data.fuzz_cases.len() - MAX_KEPT_CALLDATA - 1; | ||
| self.test_data.fuzz_cases[prune_index].prune_calldata(); | ||
| } | ||
|
|
There was a problem hiding this comment.
This doesn't actually shrink the fuzz_cases vec. Maybe we should use a ring buffer or altogether disable this for long running campaigns
cc @grandizzy
There was a problem hiding this comment.
Or vec deque
use std::collections::VecDeque;
if self.test_data.fuzz_cases.len() >= 4096 {
self.test_data.fuzz_cases.pop_front();
}
self.test_data.fuzz_cases.push_back(new_case);
There was a problem hiding this comment.
Good point. I realized that while pruning calldata removes the bulk of the payload, we're still effectively leaking the FuzzedCases structs and the inner Vec headers (which adds up to ~4KB per run at depth 100). I'll proceed with your suggestion to use a ring buffer (or simply drop the oldest cases) to strictly bound the vector to MAX_KEPT_CALLDATA entries. This means we'll lose the gas history for pruned runs, but that's a necessary trade-off for unbounded campaigns.
There was a problem hiding this comment.
@subwaycookiecrunch @0xalpharush I don't think the fuzz cases are shared between runs so we can just prune after each run? (We can have a bool config to preserve backwards compatibility)
There was a problem hiding this comment.
actually at a closer look that one is not even used, great catch @subwaycookiecrunch I updated pr to just get rid of the calldata field #12893 (comment)
Aggressively prunes calldata from completed invariant runs to bound memory usage during long-running fuzzing campaigns. Unlike foundry-rs/foundry#12893 which keeps a rolling window of 4096 runs, this implementation prunes immediately after each run completes. This is safe because: - Calldata is not shared between runs - Shrinking/replay uses last_run_inputs, not historical fuzz_cases - Gas/stipend metrics are preserved for reporting - Coverage is collected during execution, not reconstructed Ref: foundry-rs/foundry#12397 Amp-Thread-ID: https://ampcode.com/threads/T-019c1560-4151-77ce-ba30-f46f9fb80353 Co-authored-by: Amp <amp@ampcode.com>
|
Based on @grandizzy's observation that fuzz cases aren't shared between runs, I investigated further and found that The simplest fix is to remove the field entirely: pub struct FuzzCase {
pub gas: u64,
pub stipend: u64,
}This eliminates memory accumulation during long invariant runs without needing pruning logic, ring buffers, or config flags. I've pushed a clean implementation to
Net: -4 lines, simpler struct, no memory accumulation. |
The calldata field in FuzzCase was stored but never read after construction. Removing it entirely eliminates memory accumulation during long invariant runs. Changes: - Remove FuzzCase.calldata field (unused after construction) - Remove prune_calldata() methods (no longer needed) - Restore vyper test files that were incorrectly deleted Fixes foundry-rs#12397 Amp-Thread-ID: https://ampcode.com/threads/T-019c17c9-d969-7370-bf0d-495e473e8e30 Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019c17c9-d969-7370-bf0d-495e473e8e30 Co-authored-by: Amp <amp@ampcode.com>
gakonst
force-pushed
the
fix/invariant-memory-usage
branch
from
c70ed2e to
d581a6b
Compare
grandizzy
left a comment
grandizzy
left a comment
There was a problem hiding this comment.
Lgtm, thank you, great finding. Waiting on one more reviewer before merge cc @DaniPopes @0xalpharush
grandizzy
changed the title
5 participants
Description
Fixes #12397
This PR addresses the issue of unbounded memory usage during long-running invariant tests. Previously, the
InvariantExecutorretained the fullcalldatafor every successful fuzz case. In testing sessions with millions of runs (e.g., overnight fuzzing), this caused memory usage to grow indefinitely, eventually leading to OOM crashes.Changes
crates/evm/fuzz: Addedprune_calldata()methods toFuzzCaseandFuzzedCasesto allow clearing the stored calldata while preserving metrics (gas, stipend).crates/evm/evm: UpdatedInvariantExecutorto enforce a rolling window of stored calldata. It now keeps full traces only for the last 4,096 runs (for debugging purposes) and prunes the calldata from all older runs.Verification
calldatais pruned, the total count of runs and other metrics remain accurate for reporting.