Skip to content

fix: double gas accounting context#1083

Open
technicallyty wants to merge 1 commit intomainfrom
technicallyty/context-usage
Open

fix: double gas accounting context#1083
technicallyty wants to merge 1 commit intomainfrom
technicallyty/context-usage

Conversation

@technicallyty
Copy link
Copy Markdown
Member

Description

somehow this change got reverted, needs to be added back.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • tackled an existing issue or discussed with a team member
  • left instructions on how to review the changes
  • targeted the main branch

@technicallyty technicallyty requested a review from vladjdk March 20, 2026 19:03
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 20, 2026

Greptile Summary

This PR fixes a regression in IBCOnTimeoutPacketCallback where the original ctx (with a basic gas meter) was mistakenly passed to CallEVM instead of the cachedCtx (with an infinite gas meter). The same pattern is correctly applied in both IBCReceivePacketCallback and IBCOnAcknowledgementPacketCallback: EVM calls run against a cached context with an infinite gas meter to prevent the EVM gas estimation from depleting all Cosmos gas, and only the actual res.GasUsed is charged back to the original context afterward. Passing ctx instead of cachedCtx caused double gas accounting — gas was consumed once directly through the original context's basic gas meter during the EVM call, and then again when ctx.GasMeter().ConsumeGas(res.GasUsed, ...) was called.

  • Root cause: CallEVM in IBCOnTimeoutPacketCallback was called with ctx instead of cachedCtx, causing gas to be consumed twice.
  • Fix: Replaced ctx with cachedCtx in the CallEVM call, aligning IBCOnTimeoutPacketCallback with the pattern in the other callback handlers.
  • Impact: No logic changes beyond correcting the gas meter context; the fix is minimal and targeted.

Confidence Score: 5/5

  • This PR is safe to merge — it is a one-line, clearly correct bug fix with no side effects.
  • The change is minimal (one token changed: ctxcachedCtx), the fix exactly mirrors the established pattern in the other two callback handlers in the same file, and the root cause (double gas accounting when using the basic gas meter directly in CallEVM) is well-understood. No new logic is introduced and no unrelated code is touched.
  • No files require special attention.

Important Files Changed

Filename Overview
x/ibc/callbacks/keeper/keeper.go Fixes double gas accounting in IBCOnTimeoutPacketCallback by passing cachedCtx (with an infinite gas meter) instead of the original ctx to CallEVM, consistent with the pattern used in IBCReceivePacketCallback and IBCOnAcknowledgementPacketCallback.

Last reviewed commit: "fix context usage"

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 20, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 63.39%. Comparing base (50b4817) to head (35ff4f6).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
x/ibc/callbacks/keeper/keeper.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1083   +/-   ##
=======================================
  Coverage   63.38%   63.39%           
=======================================
  Files         332      332           
  Lines       23958    23958           
=======================================
+ Hits        15186    15187    +1     
+ Misses       7174     7173    -1     
  Partials     1598     1598           
Files with missing lines Coverage Δ
x/ibc/callbacks/keeper/keeper.go 59.15% <0.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@technicallyty technicallyty enabled auto-merge March 20, 2026 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants