[FVM] Cleanup metering code by janezpodhostnik · Pull Request #8601 · onflow/flow-go

janezpodhostnik · 2026-07-08T13:24:13Z

While working on a different issue, I have decided to cleanup the metering code.

This aims to shrink the metering interface while preserving all the current behavior.

Summary by CodeRabbit

New Features
- Execution output now surfaces metering as a single MeteringResult (computation used, memory estimate, and computation intensities).
- Event collections now expose an event counter instead of total byte size.
Bug Fixes
- Computation remaining no longer underflows and correctly reports exhaustion after over-limit metering attempts.
- Transaction fee deduction now uses the latest metering summary.
- Gas/computation sufficiency checks now use remaining computation.
Tests
- Updated tests and mocks to use the new MeteringResult structure and adjusted metering expectations.

github-actions · 2026-07-08T13:24:26Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

coderabbitai · 2026-07-08T13:25:00Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1c347820-e537-44c5-a3e3-037edde0a348

📥 Commits

Reviewing files that changed from the base of the PR and between ed60df7 and d96d806.

📒 Files selected for processing (1)

fvm/meter/computation_meter.go

🚧 Files skipped from review as they are similar to previous changes (1)

fvm/meter/computation_meter.go

📝 Walkthrough

Walkthrough

This PR consolidates separate metering accessors into MeteringResult, updates meter implementations and callers, changes EVM gas checks to use remaining computation, and fixes exhausted-limit handling in ComputationRemaining.

Changes

Metering API consolidation

Layer / File(s)	Summary
MeteringResult contract and computation limits `fvm/meter/`, `fvm/environment/meter.go`, `fvm/storage/state/`, `fvm/environment/event_emitter.go`	Adds `MeteringResult`, removes obsolete metering methods, and prevents computation-remaining underflow.
Meter implementations and adapters `cmd/util/ledger/util/nop_meter.go`, `fvm/evm/backends/wrappedEnv.go`, `fvm/evm/testutils/backend.go`	Updates meters and wrappers to expose `MeteringResult`.
Procedure output and execution flow `fvm/fvm.go`, `fvm/transactionInvoker.go`, `fvm/evm/handler/*`	Moves execution totals into `ProcedureOutput.MeteringResult`, updates fee deduction, and uses `ComputationRemaining` for gas checks.
Mocks and test call sites `fvm/environment/mock/`, `engine/execution/...`, `module/chunks/`, `integration/internal/emulator/tests/vm_test.go`	Regenerates metering mocks and updates test outputs to use nested metering results.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

onflow/flow-go#8416: Both changes update EVM gas/computation-limit validation in fvm/evm/handler/handler.go.

Suggested labels: Breaking Change, Improvement

Suggested reviewers: turbolent, zhangchiqing, fxamacker

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title is related to the change, but it is broad and doesn’t convey the main metering-interface rewrite clearly.	Rename it to reflect the primary change, e.g. "[FVM] Refactor metering interface" or "[FVM] Consolidate metering results".

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch janez/metering-cleanup

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

codecov-commenter · 2026-07-08T13:28:25Z

Codecov Report

❌ Patch coverage is 17.44186% with 71 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
fvm/environment/mock/environment.go	0.00%	30 Missing ⚠️
fvm/environment/mock/meter.go	0.00%	30 Missing ⚠️
fvm/evm/testutils/backend.go	0.00%	6 Missing ⚠️
cmd/util/ledger/util/nop_meter.go	0.00%	2 Missing ⚠️
fvm/evm/backends/wrappedEnv.go	0.00%	2 Missing ⚠️
fvm/fvm.go	66.66%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

AlexHentschel

Looks good.

There are some suggestions for incrementally improving error documentation. That might be out of scope or not worth the time investment for this PR - not sure, but also with AI it seems that such comments could be quick to add in some cases (unless it's error propagation, wrapping of errors across many implementation levels)

AlexHentschel · 2026-07-09T21:05:48Z

brief documentation of errors would be great on MeteringResult and MeterComputation, specifically because it seems we are dealing with sentinel errors. Please consider this as very desired but still optional, just in case properly documenting expected error returns would require a large stack of code changes at the lower layers.

AlexHentschel · 2026-07-09T21:07:50Z

similarly here, documentation of expected error returns would be great (mindset of leaving the code in a better place than it was before in terms of error documentation, even if it's only an incremental incomplete improvement). Yet, not strictly required for this PR to be merged.

AlexHentschel · 2026-07-09T21:09:04Z

+	if h.backend.ComputationRemaining(environment.ComputationKindEVMGasUsage) < uint64(limit) {
 		return types.ErrInsufficientComputation
 	}


would it make sense to document this error as being a potential return value in the method description?

AlexHentschel · 2026-07-09T21:15:43Z

+	// `computationUsed` can exceed `computationLimit`, since `MeterComputation` increments
+	// the usage before checking the limit; guard against underflow
+	if m.computationUsed >= m.params.computationLimit {


thanks. Great change, avoids underflow problem.

If you have your AI accumulate memory incl. coding guidelines, I found it worth while to point your AI explicitly to this change, tell it to recognize the underflow problem in the old code and always watch out for similar edge cases. In short: subtraction operations on uint types are always potential risks for bugs. Guideline: when subtracting unit types, there should always be a comment inside the code explaining why underflow cannot occur. Absence of such a statement indicates a potential problem and should either be fixed or a sufficient correctness argument be provided by a comment.

AlexHentschel · 2026-07-09T21:20:37Z


-	computationUsed, err := env.ComputationUsed()
+	meteringResult, err := env.MeteringResult()
 	if err != nil {


any critical vs benign errors we need to differentiate between here? Brief comment would help (desired but optional)

AlexHentschel · 2026-07-09T21:20:55Z

 	computationLimit := executor.txnState.TotalComputationLimit()

-	computationUsed, err := executor.env.ComputationUsed()
+	meteringResult, err := executor.env.MeteringResult()


any critical vs benign errors we need to differentiate between here? Brief comment would help (desired but optional)

janezpodhostnik · 2026-07-10T13:07:28Z

Thanks for the review @AlexHentschel. I will address most of the comments you have in subsequent PRs. I'm not done with cleaning up metering, and I want to keep PRs small and focused on a single task so that they are easier to digest.

I have a PR removing the error on metering results: #8605,
I want to do some renaming to make it cleared whats going on and I am working on cleaning up the logic behind disabling metering on certain things.

All this so that I will be more certain that I am correct when I fix: https://github.com/onflow/flow-go-internal/issues/7126

Co-authored-by: Alexander Hentschel <alex.hentschel@flowfoundation.org>

Cleanup metering

fb8da14

janezpodhostnik self-assigned this Jul 8, 2026

janezpodhostnik requested a review from a team as a code owner July 8, 2026 13:24

janezpodhostnik changed the title ~~Cleanup metering~~ [FVM] Cleanup metering code Jul 8, 2026

This comment has been minimized.

Sign in to view

janezpodhostnik requested review from turbolent and zhangchiqing July 9, 2026 16:01

zhangchiqing approved these changes Jul 9, 2026

View reviewed changes

janezpodhostnik requested a review from a team July 9, 2026 17:04

AlexHentschel approved these changes Jul 9, 2026

View reviewed changes

janezpodhostnik and others added 3 commits July 10, 2026 18:20

Merge branch 'master' into janez/metering-cleanup

d55afa7

Update fvm/meter/computation_meter.go

ed60df7

Co-authored-by: Alexander Hentschel <alex.hentschel@flowfoundation.org>

Fix goimports formatting in computation_meter.go

d96d806

Uh oh!

Conversation

janezpodhostnik commented Jul 8, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

github-actions Bot commented Jul 8, 2026

Dependency Review

Scanned Files

Uh oh!

coderabbitai Bot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 inconclusive)

Uh oh!

codecov-commenter commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment has been minimized.

AlexHentschel left a comment

Choose a reason for hiding this comment

Uh oh!

AlexHentschel Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

AlexHentschel Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

AlexHentschel Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

AlexHentschel Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexHentschel Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

AlexHentschel Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

janezpodhostnik commented Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

janezpodhostnik commented Jul 8, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 8, 2026 •

edited

Loading

codecov-commenter commented Jul 8, 2026 •

edited

Loading