Skip to content

Enable storage logger #237

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

Merged
FletcherMan merged 1 commit into from
Nov 10, 2025
Merged

Enable storage logger #237

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion rollup/tracing/tracing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ func CreateTraceEnv(chainConfig *params.ChainConfig, chainContext core.ChainCont
env := CreateTraceEnvHelper(
chainConfig,
&logger.Config{
DisableStorage: true,
DisableStorage: false,
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Nov 10, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify test coverage and performance impact of enabling storage tracing.

The change correctly enables storage tracing by setting DisableStorage: false, which aligns with the PR objective and is consistent with the extensive storage proof collection throughout the codebase (lines 359-483). However, enabling storage tracing will increase CPU and memory usage during transaction execution.

Please verify:

  1. Test coverage: Ensure tests validate that storage data is correctly captured when DisableStorage: false.
  2. Performance impact: Benchmark the performance difference, especially for blocks with many storage operations.
  3. Downstream compatibility: Confirm that systems consuming block traces can handle the additional storage data.

If this hotfix addresses a bug where storage data was missing for the prover, consider adding a comment explaining why storage must be enabled for proving purposes.

🏁 Script executed:

#!/bin/bash
# Search for existing tests related to CreateTraceEnv and storage tracing
rg -n -C3 --type=go 'CreateTraceEnv.*[Tt]est|DisableStorage.*false' -g '**/*test.go'

# Check for performance benchmarks
rg -n --type=go 'func Benchmark.*[Tt]race' -g '**/*test.go'

Length of output: 361

I need to search more specifically for tests in the rollup/tracing module and verify coverage for the storage configuration change.

🏁 Script executed:

#!/bin/bash
# Search for test files in rollup/tracing
fd -e _test.go . rollup/tracing/ 2>/dev/null || echo "No test files found in rollup/tracing"

# Search for all CreateTraceEnv usages to understand where it's called
rg -n --type=go 'CreateTraceEnv' --context=3

# Search for DisableStorage usage patterns
rg -n --type=go 'DisableStorage' --context=2

Length of output: 7637

I need to check for integration tests and understand the context of this change better.


---

🏁 Script executed:

```shell
#!/bin/bash
# Search for integration tests or callers of the trace function
rg -n --type=go 'CreateTraceEnvAndGetBlockTrace' --context=3

# Search for any test files that might exercise block tracing
fd -e _test.go . eth/tracers/ | head -20

# Check git history context for when DisableStorage was previously configured
git log --oneline -n 20 -- rollup/tracing/tracing.go 2>/dev/null || echo "Git history not available"

Length of output: 2764

Add unit tests to rollup/tracing and verify consumers handle storage data.

This change correctly re-enables storage tracing (reversing commit 054bbd2) to provide storage data required by the prover. The infrastructure is sound—storage capture is fully implemented in the logger module. However, the rollup/tracing module has zero unit tests, creating risk for regressions.

  1. Add tests to rollup/tracing: Unit tests should verify that DisableStorage: false correctly captures storage operations (SLOAD, SSTORE) in traces.
  2. Verify downstream consumers: Confirm all systems consuming BlockTrace (particularly the prover) can handle and process the additional storage data.
  3. Document the rationale: Add a comment explaining why storage must be enabled for proving, referencing the prover's data requirements.
🤖 Prompt for AI Agents 
In rollup/tracing/tracing.go around line 151, add unit tests in the
rollup/tracing package that assert DisableStorage: false actually causes storage
operations (SLOAD, SSTORE) to be emitted in generated traces: create synthetic
transactions or traces (via the existing logger capture hooks) that perform
storage reads/writes, call the trace generation path, and assert the returned
BlockTrace contains storage entries on the corresponding trace frames/ops;
update any existing tests that consume BlockTrace (notably the prover
test-suite) to accept and validate the presence of storage fields so consumers
don’t break; and add a short comment at or above line 151 stating that storage
tracing must be enabled because the prover requires SLOAD/SSTORE data for proof
construction and referencing the prover data requirement.

DisableStack: true,
EnableMemory: false,
EnableReturnData: true,
Expand Down