diff --git a/PR_2862_REVIEW.md b/PR_2862_REVIEW.md new file mode 100644 index 0000000000..57094e52a4 --- /dev/null +++ b/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.