[FVM] Unify metering-disabled behavior for ledger interaction - #8610
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughStorage read and write metering now enforce interaction limits without an enforcement flag. Execution state adds scoped metering suppression, with tests covering bypassed limits, resumed enforcement, cache behavior, and interaction thresholds. ChangesStorage metering behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ExecutionState
participant limitsController
participant InteractionMeter
ExecutionState->>limitsController: RunWithMeteringDisabled(callback)
limitsController->>ExecutionState: Disable metering
ExecutionState->>InteractionMeter: Skip storage read/write metering
limitsController->>ExecutionState: Restore previous metering state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fvm/meter/interaction_meter.go (1)
64-76: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate the remaining
MeterStorageReadcall.
fvm/meter/meter_test.goLine 968 still passestrue.MeterStorageReadnow accepts two arguments. Thefvm/metertest package will not compile until that argument is removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fvm/meter/interaction_meter.go` around lines 64 - 76, Update the remaining MeterStorageRead call in meter_test.go to pass only the storage key and value, removing the obsolete true argument. Keep the call aligned with the current InteractionMeter.MeterStorageRead signature so the fvm/meter tests compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fvm/fvm_blockcontext_test.go`:
- Around line 1303-1309: Update the test using
CreateMultiAccountCreationTransaction to assert that output.Err contains
errors.LimitKindLedgerInteraction, in addition to the existing Cadence runtime
error check, so it verifies the specific ledger-interaction limit cause.
---
Outside diff comments:
In `@fvm/meter/interaction_meter.go`:
- Around line 64-76: Update the remaining MeterStorageRead call in meter_test.go
to pass only the storage key and value, removing the obsolete true argument.
Keep the call aligned with the current InteractionMeter.MeterStorageRead
signature so the fvm/meter tests compile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b6d3f2a2-2281-47c5-8d04-e3fbf0b08228
📒 Files selected for processing (7)
fvm/fvm_blockcontext_test.gofvm/meter/interaction_meter.gofvm/meter/interaction_meter_test.gofvm/meter/meter_test.gofvm/storage/state/execution_state.gofvm/storage/state/execution_state_test.gofvm/storage/state/transaction_state.go
| // The account count is sized so that the metered interaction | ||
| // exceeds MaxStateInteractionSize, triggering the interaction | ||
| // limit from within Cadence execution. | ||
| _, txBodyBuilder := testutil.CreateMultiAccountCreationTransaction( | ||
| t, | ||
| chain, | ||
| 40) | ||
| 60) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the ledger-interaction failure cause.
The test only checks for a Cadence runtime error. A computation, memory, or event limit failure would also pass. Assert that output.Err contains errors.LimitKindLedgerInteraction to verify the new threshold behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fvm/fvm_blockcontext_test.go` around lines 1303 - 1309, Update the test using
CreateMultiAccountCreationTransaction to assert that output.Err contains
errors.LimitKindLedgerInteraction, in addition to the existing Cadence runtime
error check, so it verifies the specific ledger-interaction limit cause.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This comment has been minimized.
This comment has been minimized.
4 participants
RunWithMeteringDisablednow skips ledger-interaction metering the same way it already skips computation, memory, and events. Previously interaction bytes were still accumulated (only the limit was not enforced) while disabled. There was no specific reason to keep interaction separate, so this unifies the logic across all metered quantities.This changes execution behavior and requires an HCU to deploy: transactions close to the interaction limit could now fail where they previously succeeded (or succeed where they previously failed), so all execution nodes must switch at the same height.
Summary by CodeRabbit