feat: support executing sub-calls with a different gas limit #25303
Conversation
- It's a common pattern to execute a sub-call with a lower gas limit than the whole tx gas limit, this PR introduce a primitive method in `Context` for that use case. Apply suggestions from code review
yihuang
force-pushed
the
proxygasmeter
branch
from
aed7571 to
8d642dc
Compare
📝 WalkthroughWalkthroughAdds a ProxyGasMeter wrapper to enforce a hard gas limit while delegating accounting, introduces Context.WithGasRemaining to run sub-calls with constrained remaining gas, adds corresponding tests for finite/infinite meters, and updates changelogs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Ctx as Context
participant Store as storetypes
participant UGM as Underlying GasMeter
participant PGM as ProxyGasMeter
Caller->>Ctx: WithGasRemaining(remaining)
Ctx->>Store: NewProxyGasMeter(Ctx.GasMeter(), remaining)
Store->>UGM: Wrap existing meter
Store-->>Ctx: PGM
Ctx-->>Caller: New Context (GasMeter=PGM)
rect rgb(235, 245, 255)
note over Caller,PGM: Sub-call execution with constrained gas
Caller->>PGM: ConsumeGas(x, desc)
PGM->>PGM: Check overflow and limit
alt within limit
PGM->>UGM: ConsumeGas(x, desc)
PGM-->>Caller: ok
else exceeds limit
PGM-->>Caller: panic ErrorOutOfGas
end
end
note over UGM: Tracks real-time consumption for parent context
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
types/context.go (1)
227-233: Tighten the doc comment and note the no-op case when remaining ≥ parent remaining.Clarifies behavior and avoids ambiguity.
-// WithGasRemaining returns a Context with a lower remaining gas, -// it's used to execute sub-calls with a lower gas limit. -// the gas consumption is still tracked by the parent gas meter in realtime, -// there's no risk of losing gas accounting. +// WithGasRemaining returns a Context whose tx GasMeter enforces a lower remaining gas budget. +// Use this to execute sub-calls with a stricter gas limit while still accounting on the parent meter in real time. +// Note: if remaining ≥ the current meter's remaining gas, the original meter is kept (no wrapping). func (c Context) WithGasRemaining(remaining storetypes.Gas) Context { return c.WithGasMeter(storetypes.NewProxyGasMeter(c.GasMeter(), remaining)) }store/types/proxygasmeter_test.go (2)
48-84: Rename typo in test name: “Infinit” → “Infinite”.Improves readability and discoverability in test output.
-func TestProxyGasMeterInfinit(t *testing.T) { +func TestProxyGasMeterInfinite(t *testing.T) {
48-84: Add a test for the “no wrap” path and zero limit edge case.Covers constructor branch when remaining ≥ parent remaining and validates 0-limit behavior.
Example additions:
func TestProxyGasMeter_NoWrapWhenRemainingNotLower(t *testing.T) { bgm := NewGasMeter(1000) pgm := NewProxyGasMeter(bgm, bgm.GasRemaining()) // equal _, isProxy := pgm.(*ProxyGasMeter) require.False(t, isProxy) pgm2 := NewProxyGasMeter(bgm, bgm.GasRemaining()+1) // greater _, isProxy = pgm2.(*ProxyGasMeter) require.False(t, isProxy) } func TestProxyGasMeter_ZeroLimit(t *testing.T) { bgm := NewGasMeter(1000) pgm := NewProxyGasMeter(bgm, 0) require.Equal(t, Gas(0), pgm.GasRemaining()) require.True(t, pgm.IsOutOfGas()) require.Panics(t, func() { pgm.ConsumeGas(1, "zero") }) }store/types/proxygasmeter.go (1)
9-16: Doc nit: tighten wording.Small readability tweak; no behavior change.
-// ProxyGasMeter wraps another GasMeter, but enforces a lower gas limit. -// Gas consumption is delegated to the wrapped GasMeter, so it won't risk losing gas accounting compared to standalone -// gas meter. +// ProxyGasMeter wraps another GasMeter and enforces a stricter (lower) gas limit. +// Gas consumption is delegated to the wrapped GasMeter to preserve accounting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)store/CHANGELOG.md(1 hunks)store/types/proxygasmeter.go(1 hunks)store/types/proxygasmeter_test.go(1 hunks)types/context.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: golangci-lint
store/types/proxygasmeter.go
[failure] 55-55:
QF1008: could remove embedded field "GasMeter" from selector (staticcheck)
🪛 GitHub Actions: Lint
store/types/proxygasmeter.go
[error] 55-55: Staticcheck: QF1008 - could remove embedded field 'GasMeter' from selector.
⏰ 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). (13)
- GitHub Check: test-system-legacy
- GitHub Check: test-system
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: tests (03)
- GitHub Check: test-store
- GitHub Check: test-integration
- GitHub Check: test-sim-nondeterminism
- GitHub Check: build (arm64)
- GitHub Check: Analyze
- GitHub Check: Gosec
- GitHub Check: Summary
🔇 Additional comments (3)
CHANGELOG.md (1)
58-58: Changelog entry reads well; no further changes needed.Accurately documents the new Context API under Features.
store/CHANGELOG.md (1)
32-35: Store changelog entry looks good.Clear and consistent with repository conventions.
store/types/proxygasmeter_test.go (1)
10-46: Solid coverage of basic delegation/limit semantics.Assertions verify consumed/remaining sync with the underlying meter and limit enforcement.
store/types/proxygasmeter.go
Outdated
| func NewProxyGasMeter(gasMeter GasMeter, remaining Gas) GasMeter { | ||
| if remaining >= gasMeter.GasRemaining() { | ||
| return gasMeter | ||
| } | ||
|
|
||
| return &ProxyGasMeter{ | ||
| GasMeter: gasMeter, | ||
| limit: remaining + gasMeter.GasConsumed(), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against uint64 overflow when computing the absolute limit.
If the wrapped meter has very high consumption (e.g., infinite meter), consumed + remaining can overflow and silently wrap. Panic consistently with ConsumeGas on overflow.
func NewProxyGasMeter(gasMeter GasMeter, remaining Gas) GasMeter {
if remaining >= gasMeter.GasRemaining() {
return gasMeter
}
- return &ProxyGasMeter{
- GasMeter: gasMeter,
- limit: remaining + gasMeter.GasConsumed(),
- }
+ limit, overflow := addUint64Overflow(gasMeter.GasConsumed(), remaining)
+ if overflow {
+ panic(ErrorGasOverflow{Descriptor: "NewProxyGasMeter"})
+ }
+ return &ProxyGasMeter{
+ GasMeter: gasMeter,
+ limit: limit,
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In store/types/proxygasmeter.go around lines 24 to 33, the computation limit :=
remaining + gasMeter.GasConsumed() can overflow uint64; detect this case before
summing and panic in the same way ConsumeGas does on overflow. Specifically,
fetch consumed := gasMeter.GasConsumed(), check if consumed > math.MaxUint64 -
remaining (import math or use ^uint64(0)), and if so invoke panic("gas
overflow") (or the exact ConsumeGas overflow message used elsewhere); otherwise
set limit = remaining + consumed and return the ProxyGasMeter.
| func (pgm ProxyGasMeter) ConsumeGas(amount Gas, descriptor string) { | ||
| consumed, overflow := addUint64Overflow(pgm.GasMeter.GasConsumed(), amount) | ||
| if overflow { | ||
| panic(ErrorGasOverflow{Descriptor: descriptor}) | ||
| } | ||
|
|
||
| if consumed > pgm.limit { | ||
| panic(ErrorOutOfGas{Descriptor: descriptor}) | ||
| } | ||
|
|
||
| pgm.GasMeter.ConsumeGas(amount, descriptor) | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix CI lint failure (staticcheck QF1008) by removing the embedded selector.
This is currently failing the lint job; use the promoted method instead.
-func (pgm ProxyGasMeter) ConsumeGas(amount Gas, descriptor string) {
- consumed, overflow := addUint64Overflow(pgm.GasMeter.GasConsumed(), amount)
+func (pgm ProxyGasMeter) ConsumeGas(amount Gas, descriptor string) {
+ consumed, overflow := addUint64Overflow(pgm.GasConsumed(), amount)
if overflow {
panic(ErrorGasOverflow{Descriptor: descriptor})
}
if consumed > pgm.limit {
panic(ErrorOutOfGas{Descriptor: descriptor})
}
pgm.GasMeter.ConsumeGas(amount, descriptor)
}🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 55-55:
QF1008: could remove embedded field "GasMeter" from selector (staticcheck)
🪛 GitHub Actions: Lint
[error] 55-55: Staticcheck: QF1008 - could remove embedded field 'GasMeter' from selector.
🤖 Prompt for AI Agents
In store/types/proxygasmeter.go around lines 54 to 66, the code uses the
embedded selector (pgm.GasMeter.GasConsumed() and pgm.GasMeter.ConsumeGas(...))
which triggers staticcheck QF1008; replace those calls with the promoted methods
on the embedding type (use pgm.GasConsumed() and pgm.ConsumeGas(amount,
descriptor)) so you no longer reference the embedded field directly and the
linter error is resolved.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
yihuang
changed the title
Description
It's a recurring pattern to execute a sub-call with a lower gas limit than the current tx gas limit,
this PR introduce a mechanism to do it safely.
It introduces a
ProxyGasMeterwhich behaviors just like abasicGasMeterbut it'll writes the gas changes to parent gas meter in real time, so there's no need to write back at the end of the sub-call, in the latter case, it need to be careful to make sure gas is always written back when error or panic happens.Thanks @Hellobloc for identity the issue.
Summary by CodeRabbit
New Features
Documentation
Tests