Enable storage logger#237
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rollup/tracing/tracing.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
| chainConfig, | ||
| &logger.Config{ | ||
| DisableStorage: true, | ||
| DisableStorage: false, |
There was a problem hiding this comment.
🧩 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:
- Test coverage: Ensure tests validate that storage data is correctly captured when
DisableStorage: false. - Performance impact: Benchmark the performance difference, especially for blocks with many storage operations.
- 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=2Length 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.
- Add tests to rollup/tracing: Unit tests should verify that
DisableStorage: falsecorrectly captures storage operations (SLOAD, SSTORE) in traces. - Verify downstream consumers: Confirm all systems consuming
BlockTrace(particularly the prover) can handle and process the additional storage data. - 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.
2 participants
1. Purpose or design rationale of this PR
...
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit