Giga fixes

PR_2862_REVIEW.md

@@ -0,0 +1,165 @@
+# PR #2862 Review: 3 Giga Fixes
+
+## Overview
+This PR contains three critical fixes for the Giga executor to match V2 behavior and resolve apphash mismatches. The changes are being merged into `staging-feb13/giga-6.3.0` for testing.
+
+## Summary of Changes
+
+### 1. Fix: Perform Initial Fee Validation (Commit `779ecdbc9`)
+**File**: `app/app.go`
+**Lines**: +73 lines added
+
+**Purpose**: Ports fee validation from PR #2782, adding upfront fee validation before EVM execution in the Giga path to match V2's ante handler behavior.
+
+**Changes**:
+- Adds 4 validation checks before EVM execution:
+  1. Fee cap < base fee check
+  2. Tip > fee cap check  
+  3. Gas limit * gas price overflow check
+  4. TX gas limit > block gas limit check
+- When validation fails:
+  - Increments nonce via keeper (not stateDB)
+  - Sets intrinsic gas as gasUsed
+  - Returns early without creating stateDB or executing
+- Imports `github.com/sei-protocol/sei-chain/giga/deps/xevm/types/ethtx` for `IsValidInt256`
+
+**Review Findings**:
+
+✅ **Correct**: The validation logic properly mirrors V2's ante handler checks from `app/ante/evm_checktx.go` (lines 284-286) and `x/evm/ante/basic.go` (lines 63-68).
+
+✅ **Correct**: Nonce increment behavior matches V2's `DeliverTxCallback` in `x/evm/ante/basic.go` (lines 32-38) and `app/ante/evm_delivertx.go` (lines 81-95).
+
+✅ **Correct**: Early exit skips stateDB creation, matching V2 behavior where validation failures occur in ante handlers before state transitions.
+
+⚠️ **Minor Issue**: Intrinsic gas calculation error is silently ignored with `intrinsicGas, _ := core.IntrinsicGas(...)`. While this is likely safe (intrinsic gas calculation rarely fails for valid tx structures), consider logging the error.
+
+✅ **Correct**: The comment clarifies V2 reports intrinsic gas even on validation failure, which is important for metrics consistency.
+
+### 2. Fix: Return All Balances in Bank Keeper (Commit `15b249ad8`)
+**File**: `giga/deps/xbank/keeper/view.go`
+**Lines**: +35 additions, -5 deletions
+
+**Purpose**: Ports fix from PR #2832 to resolve apphash mismatch in block 193104639. Makes `SpendableCoins()` return all token balances instead of only `usei`.
+
+**Changes**:
+- Adds `IterateAccountBalances()` method to iterate over all account balances
+- Adds `GetAllBalances()` method to return all balances for an account
+- Updates `SpendableCoins()` to:
+  - Use `GetAllBalances()` instead of only `GetBalance(ctx, addr, "usei")`
+  - Use `SafeSub()` instead of `Sub()` for safer arithmetic
+  - Return all spendable coins instead of only `usei`
+
+**Review Findings**:
+
+✅ **Correct**: Implementation properly mirrors standard Cosmos SDK bank keeper methods.
+
+✅ **Correct**: The change from hardcoded "usei" to all balances is critical for multi-token support and matches expected V2 behavior.
+
+✅ **Correct**: Using `SafeSub()` is safer than `Sub()` as it returns a boolean indicating negative results instead of panicking.
+
+✅ **Correct**: Iterator is properly closed with `defer func() { _ = iterator.Close() }()`.
+
+✅ **Correct**: Results are sorted with `.Sort()` to ensure deterministic ordering.
+
+**Impact**: This fixes a critical bug where accounts with non-usei tokens would have incorrect spendable balance calculations, leading to state divergence.
+
+### 3. Fix: Increase Nonce Even in Early-Exit Case (Commit `a80e3d4a4`)
+**File**: `app/app.go` (part of commit 1)
+
+**Purpose**: Fixes apphash mismatch caused by [this transaction](https://seiscan.io/tx/0x9bb68ac3a7ce771cd5234bd962706fc1046f589262298a9eae74540a36173f09) where nonce wasn't incremented on early validation failure.
+
+**Review Findings**:
+
+✅ **Correct**: This is the core fix in the validation error path - ensures nonce is incremented even when transactions fail validation.
+
+✅ **Correct**: Matches V2's behavior where `DeliverTxCallback` always increments nonce for transactions that enter DeliverTx, regardless of ante handler failures.
+
+✅ **Critical Fix**: Without this, transactions with validation errors would fail to increment nonce, causing nonce desync between Giga and V2 paths, leading to apphash mismatches.
+
+## Code Quality
+
+### Documentation
+✅ Excellent inline comments explaining the V2 behavior being matched
+✅ References to specific V2 source files and line numbers
+✅ Clear explanation of why certain behaviors exist (e.g., LastResultsHash computation)
+
+### Testing
+⚠️ **Note**: PR description indicates "Testing performed to validate your change" section is empty. Given these are critical fixes for apphash mismatches, recommend:
+- Unit tests for fee validation edge cases
+- Integration tests for nonce increment behavior on validation failures
+- Regression test for the specific transaction that caused the apphash mismatch
+
+### Error Handling
+✅ Validation errors have appropriate ABCI codes
+✅ Error messages are descriptive
+⚠️ Intrinsic gas calculation error is silently ignored (see note above)
+
+## Potential Issues & Recommendations
+
+### 1. Gas Calculation Error Handling
+**Current**:
+```go
+intrinsicGas, _ := core.IntrinsicGas(ethTx.Data(), ethTx.AccessList(), ethTx.SetCodeAuthorizations(), ethTx.To() == nil, true, true, true)
+```
+
+**Recommendation**: Consider logging errors or handling them explicitly:
+```go
+intrinsicGas, err := core.IntrinsicGas(ethTx.Data(), ethTx.AccessList(), ethTx.SetCodeAuthorizations(), ethTx.To() == nil, true, true, true)
+if err != nil {
+    ctx.Logger().Error("failed to calculate intrinsic gas", "error", err, "txHash", ethTx.Hash())
+    intrinsicGas = 0 // or use a reasonable default
+}
+```
+
+### 2. Validation Order
+✅ The validation order matches V2 and is appropriate:
+1. Fee cap vs base fee (most critical for spam prevention)
+2. Tip vs fee cap (transaction validity)
+3. Fee overflow (arithmetic safety)
+4. Gas limit vs block limit (resource constraints)
+
+### 3. Consistency with V2
+✅ All three fixes properly mirror V2 behavior
+✅ The PR successfully addresses the root causes of apphash mismatches
+✅ Nonce handling is now consistent between Giga and V2 execution paths
+
+## Security Considerations
+
+✅ **No security issues identified**
+- Fee validation prevents underpriced transactions
+- Nonce increment prevents replay attacks and maintains proper sequence
+- Bank keeper changes prevent incorrect balance calculations
+
+## Performance Considerations
+
+⚠️ **Minor performance note**: `GetAllBalances()` iterates through all account balances, which is more expensive than the previous single-denom lookup. However, this is necessary for correctness and matches V2 behavior.
+
+✅ **No performance regression**: The fee validation happens early and only on the failure path, so it doesn't affect successful transaction performance.
+
+## Conclusion
+
+### Approval Status: ✅ **APPROVED WITH MINOR SUGGESTIONS**
+
+This PR successfully addresses three critical issues in the Giga executor:
+1. ✅ Adds proper fee validation matching V2 ante handlers
+2. ✅ Fixes bank keeper to return all balances (multi-token support)
+3. ✅ Ensures nonce increment on validation failures (critical for consensus)
+
+### Strengths:
+- Well-documented with clear references to V2 source code
+- Addresses root causes of apphash mismatches
+- Properly matches V2 behavior for consensus-critical operations
+- Good error handling and code structure
+
+### Suggestions for Improvement:
+1. Add explicit error handling for intrinsic gas calculation (or at minimum, add a comment explaining why it's safe to ignore)
+2. Add unit/integration tests for the validation path (especially nonce increment on failure)
+3. Consider adding regression test for the specific transaction that triggered the apphash mismatch
+
+### Risk Assessment: **LOW**
+- Changes are well-understood ports from existing V2 logic
+- Fixes are targeted and don't introduce new attack vectors
+- The staging branch approach allows for proper testing before mainnet deployment
+
+### Ready to Merge: ✅ YES (to staging branch)
+The PR is ready to merge into `staging-feb13/giga-6.3.0` for testing. The fixes are critical for Giga/V2 parity and should be thoroughly tested in the staging environment before promotion to mainnet.