fix: prevent gas type overflow in ABCI responses #25271
Conversation
📝 WalkthroughWalkthroughAdds overflow-safe conversion from uint64 to int64 for gas values and applies it in CheckTx and ABCI response construction. Introduces a new test covering overflow and normal cases. Imports math where needed. No public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant BA as BaseApp
participant GM as Gas Meter
participant Conv as safeInt64FromUint64
C->>BA: CheckTx(tx)
BA->>GM: Simulate/validate tx
GM-->>BA: gasInfo {GasWanted: uint64, GasUsed: uint64}
BA->>Conv: clamp(gasInfo.GasWanted)
Conv-->>BA: int64 (<= MaxInt64)
BA->>Conv: clamp(gasInfo.GasUsed)
Conv-->>BA: int64 (<= MaxInt64)
BA-->>C: CheckTxResponse{GasWanted int64, GasUsed int64}
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 (2)
types/errors/abci.go (1)
11-18: Saturating cast helper is correct; consider centralizing to avoid duplication.The logic is sound and prevents negative gas via overflow. However, an identical helper exists in baseapp/abci.go (Lines 339–347), which risks drift. Consider consolidating into a shared internal utility (e.g., internal/mathutil) and reusing it from both packages in a follow-up PR.
Happy to draft the refactor plan or a follow-up PR skeleton.
baseapp/abci.go (1)
339-347: Local saturating cast helper duplicates the one in types/errors; prefer single source.Function is correct. To reduce drift, reuse a single implementation across packages in a follow-up (e.g., internal helper). Optionally, add a brief note explaining the saturation semantics to future readers.
Suggested inline comment:
func safeInt64FromUint64(val uint64) int64 { + // Saturate to MaxInt64 to avoid overflow on ABCI int64 fields. if val > math.MaxInt64 { return math.MaxInt64 } return int64(val) }
📜 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 (3)
baseapp/abci.go(3 hunks)baseapp/abci_test.go(2 hunks)types/errors/abci.go(3 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: Summary
🔇 Additional comments (3)
types/errors/abci.go (2)
28-29: Use of safe conversion for GasWanted/GasUsed in CheckTx response — LGTM.
42-43: Use of safe conversion for GasWanted/GasUsed in ExecTxResult — LGTM.baseapp/abci.go (1)
23-24: Import of math is appropriate for saturating cast helper.
baseapp/abci_test.go
Outdated
|
|
||
| func TestABCI_CheckTx_WithGasOverflow(t *testing.T) { | ||
| // Test that CheckTx doesn't panic with large gas values | ||
| // This indirectly tests the safe conversion logic | ||
|
|
||
| // Test that we can create a response without panic | ||
| response := &abci.CheckTxResponse{ | ||
| GasWanted: int64(math.MaxInt64), // Should be capped at MaxInt64 | ||
| GasUsed: int64(math.MaxInt64), // Should be capped at MaxInt64 | ||
| } | ||
|
|
||
| require.Equal(t, int64(math.MaxInt64), response.GasWanted) | ||
| require.Equal(t, int64(math.MaxInt64), response.GasUsed) | ||
|
|
||
| // Test with normal values | ||
| normalGasWanted := uint64(1000) | ||
| normalGasUsed := uint64(500) | ||
|
|
||
| normalResponse := &abci.CheckTxResponse{ | ||
| GasWanted: int64(normalGasWanted), | ||
| GasUsed: int64(normalGasUsed), | ||
| } | ||
|
|
||
| require.Equal(t, int64(1000), normalResponse.GasWanted) | ||
| require.Equal(t, int64(500), normalResponse.GasUsed) | ||
| } |
There was a problem hiding this comment.
Test does not exercise the overflow path; it only sets int64 fields directly.
This never triggers uint64→int64 conversion, so it can’t catch regressions. Replace with tests that call the constructors performing the conversion.
Apply this diff:
func TestABCI_CheckTx_WithGasOverflow(t *testing.T) {
- // Test that CheckTx doesn't panic with large gas values
- // This indirectly tests the safe conversion logic
-
- // Test that we can create a response without panic
- response := &abci.CheckTxResponse{
- GasWanted: int64(math.MaxInt64), // Should be capped at MaxInt64
- GasUsed: int64(math.MaxInt64), // Should be capped at MaxInt64
- }
-
- require.Equal(t, int64(math.MaxInt64), response.GasWanted)
- require.Equal(t, int64(math.MaxInt64), response.GasUsed)
-
- // Test with normal values
- normalGasWanted := uint64(1000)
- normalGasUsed := uint64(500)
-
- normalResponse := &abci.CheckTxResponse{
- GasWanted: int64(normalGasWanted),
- GasUsed: int64(normalGasUsed),
- }
-
- require.Equal(t, int64(1000), normalResponse.GasWanted)
- require.Equal(t, int64(500), normalResponse.GasUsed)
+ // overflow: uint64 -> int64 should saturate at MaxInt64
+ res := sdkerrors.ResponseCheckTxWithEvents(nil, math.MaxUint64, math.MaxUint64, nil, false)
+ require.Equal(t, int64(math.MaxInt64), res.GasWanted)
+ require.Equal(t, int64(math.MaxInt64), res.GasUsed)
+
+ // normal path
+ res = sdkerrors.ResponseCheckTxWithEvents(nil, 1000, 500, nil, false)
+ require.Equal(t, int64(1000), res.GasWanted)
+ require.Equal(t, int64(500), res.GasUsed)
+
+ // also assert for ExecTxResult path
+ execRes := sdkerrors.ResponseExecTxResultWithEvents(nil, math.MaxUint64, math.MaxUint64, nil, false)
+ require.Equal(t, int64(math.MaxInt64), execRes.GasWanted)
+ require.Equal(t, int64(math.MaxInt64), execRes.GasUsed)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestABCI_CheckTx_WithGasOverflow(t *testing.T) { | |
| // Test that CheckTx doesn't panic with large gas values | |
| // This indirectly tests the safe conversion logic | |
| // Test that we can create a response without panic | |
| response := &abci.CheckTxResponse{ | |
| GasWanted: int64(math.MaxInt64), // Should be capped at MaxInt64 | |
| GasUsed: int64(math.MaxInt64), // Should be capped at MaxInt64 | |
| } | |
| require.Equal(t, int64(math.MaxInt64), response.GasWanted) | |
| require.Equal(t, int64(math.MaxInt64), response.GasUsed) | |
| // Test with normal values | |
| normalGasWanted := uint64(1000) | |
| normalGasUsed := uint64(500) | |
| normalResponse := &abci.CheckTxResponse{ | |
| GasWanted: int64(normalGasWanted), | |
| GasUsed: int64(normalGasUsed), | |
| } | |
| require.Equal(t, int64(1000), normalResponse.GasWanted) | |
| require.Equal(t, int64(500), normalResponse.GasUsed) | |
| } | |
| func TestABCI_CheckTx_WithGasOverflow(t *testing.T) { | |
| // overflow: uint64 -> int64 should saturate at MaxInt64 | |
| res := sdkerrors.ResponseCheckTxWithEvents(nil, math.MaxUint64, math.MaxUint64, nil, false) | |
| require.Equal(t, int64(math.MaxInt64), res.GasWanted) | |
| require.Equal(t, int64(math.MaxInt64), res.GasUsed) | |
| // normal path | |
| res = sdkerrors.ResponseCheckTxWithEvents(nil, 1000, 500, nil, false) | |
| require.Equal(t, int64(1000), res.GasWanted) | |
| require.Equal(t, int64(500), res.GasUsed) | |
| // also assert for ExecTxResult path | |
| execRes := sdkerrors.ResponseExecTxResultWithEvents(nil, math.MaxUint64, math.MaxUint64, nil, false) | |
| require.Equal(t, int64(math.MaxInt64), execRes.GasWanted) | |
| require.Equal(t, int64(math.MaxInt64), execRes.GasUsed) | |
| } |
🤖 Prompt for AI Agents
In baseapp/abci_test.go around lines 2596 to 2621, the test sets
CheckTxResponse.GasWanted/GasUsed directly as int64 so it never exercises the
uint64→int64 conversion path; update the test to use the functions/constructors
that perform the safe conversion (the same helpers the production code uses) by
passing large uint64 values (including values > MaxInt64) and normal uint64
values into those constructors and asserting the returned CheckTxResponse has
GasWanted/GasUsed capped to MaxInt64 for overflow cases and correctly converted
for normal cases; ensure you assert via the constructor results rather than by
directly assigning int64 fields.
| GasWanted: safeInt64FromUint64(gasInfo.GasWanted), | ||
| GasUsed: safeInt64FromUint64(gasInfo.GasUsed), | ||
| Log: result.Log, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Correctly uses saturating conversion when constructing CheckTxResponse.
Covers the success path; the error path already routes through errors.ResponseCheckTxWithEvents which now also saturates. Nice.
Run to spot any remaining direct casts of gas into int64:
🏁 Script executed:
#!/bin/bash
# Find risky int64 casts into GasWanted/GasUsed initializers.
rg -nP -C2 --type=go '(GasWanted|GasUsed)\s*:\s*int64\('
# Find any ABCI response construction bypassing the safe helpers.
rg -nP --type=go -C2 'abci\.(CheckTxResponse|ExecTxResult)\s*{'Length of output: 4134
Refactor ExecTxResult to use saturating conversions for Gas fields
The success path in deliverTx still uses direct int64 casts, which can overflow. Replace these with the existing safeInt64FromUint64 helper to ensure saturation:
• File baseapp/baseapp.go, around line 723
- GasWanted: int64(gInfo.GasWanted),
- GasUsed: int64(gInfo.GasUsed),
+ GasWanted: safeInt64FromUint64(gInfo.GasWanted),
+ GasUsed: safeInt64FromUint64(gInfo.GasUsed),This aligns the success path with the CheckTx responses and error paths that already saturate.
🤖 Prompt for AI Agents
In baseapp/baseapp.go around line 723, the ExecTxResult success path currently
casts gas fields directly to int64 which can overflow; replace those direct
int64 casts for GasWanted and GasUsed with the existing safeInt64FromUint64
helper (i.e., set GasWanted: safeInt64FromUint64(gasInfo.GasWanted) and GasUsed:
safeInt64FromUint64(gasInfo.GasUsed)) so the values saturate like the CheckTx
and error paths.
| abci "github.com/cometbft/cometbft/v2/abci/types" | ||
|
|
||
| errorsmod "cosmossdk.io/errors" | ||
| ) | ||
|
|
||
| // safeInt64FromUint64 converts uint64 to int64 with overflow checking. | ||
| // If the value is too large to fit in int64, it returns math.MaxInt64. | ||
| func safeInt64FromUint64(val uint64) int64 { |
There was a problem hiding this comment.
Seems like we should just have this as a common function so we don't duplicate across the code
Resolves TODO comments in baseapp/abci.go by implementing safe uint64 to int64 conversion for gas values.
Changes
Summary by CodeRabbit
Bug Fixes
Tests