Use DBImpl for giga mock balance validation#3750
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3750 +/- ##
==========================================
- Coverage 59.88% 59.04% -0.84%
==========================================
Files 2288 2201 -87
Lines 190032 179768 -10264
==========================================
- Hits 113792 106146 -7646
+ Misses 66093 64292 -1801
+ Partials 10147 9330 -817
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview
Adds a Reviewed by Cursor Bugbot for commit 05cd4da. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
The PR correctly routes giga validation balance reads through DBImpl so mock_balances builds top off before the affordability check, matching V2; it is a no-op in production and the balance summation is functionally equivalent. Two mock_balances-only divergences (test/benchmark builds) are worth noting but are not production blockers.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) is empty — that pass produced no output. Codex's review was present and its two points are addressed inline below.
- Both mock_balances findings share one root cause:
ensureMinimumBalance→topOffAccountmints via the bank keeper directly onctxand is NOT recorded in the DBImpl journal, sostateDB.Cleanup()cannot undo it. Moving the top-off into the validation precheck means it now runs on code paths (validation failure, unassociated V2 fallback) where the mint is still committed. Consider either (a) deferring the balance top-off until after the tx is known to execute on giga, or (b) skipping giga validation entirely for unassociated senders (which unconditionally fall back to V2) so no top-off is minted for them. Scope is limited to test/benchmark builds (mock_balances panics on pacific-1), so no consensus impact. - The new regression test only covers an associated, sufficiently-topped-off sender that succeeds. Consider adding mock_balances cases for (a) a tx whose cost exceeds the 100-SEI top-off so validation still fails, asserting the sender's balance is not increased, and (b) an unassociated sender, to pin the intended top-off/fallback behavior.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| senderBalance := app.GigaEvmKeeper.GetBalance(ctx, seiAddr) | ||
| // Route validation balance reads through DBImpl so mock_balances follows | ||
| // the same ensureMinimumBalance behavior as V2's StateTransition.BuyGas. | ||
| senderBalance := stateDB.GetBalance(sender).ToBig() |
There was a problem hiding this comment.
[suggestion] mock_balances only: stateDB.GetBalance(sender) invokes ensureMinimumBalance, which mints a 100-SEI top-off directly via the bank keeper on ctx. That mint is not journaled in the DBImpl, so defer stateDB.Cleanup() does not revert it. If validation then fails (e.g. cost exceeds the top-off), executeEVMTxWithGigaExecutor returns (validation.err, nil) and the synchronous caller still runs ctx.GigaMultiStore().WriteGiga() (app.go:1636), committing the top-off. A rejected tx therefore increases the sender's balance — unlike V2, whose ante-handler top-off runs on a cache context discarded on failure. Test/benchmark-build only (panics on pacific-1), so no consensus impact, but it diverges from the V2 parity this PR is trying to achieve.
| senderBalance = new(big.Int).Add(senderBalance, castBalance) | ||
| seiEVMAddr := common.BytesToAddress(seiAddr) | ||
| if seiEVMAddr != sender { | ||
| seiBalance := stateDB.GetBalance(seiEVMAddr).ToBig() |
There was a problem hiding this comment.
[suggestion] mock_balances only: for an unassociated sender both GetBalance(sender) (above) and this GetBalance(seiEVMAddr) top off two distinct derived accounts to 100 SEI each. Validation runs before the !isAssociated V2-fallback at line 2047-2051, so after topping off 200 SEI total the tx returns ErrBalanceMigrationRequired and falls back to V2 via ms.Write(), committing both mints; V2 then migrates both balances. This grants up to 200 SEI vs V2's standalone single 100-SEI top-off. Note the balance sum itself is otherwise equivalent to the previous seiAddr + castAddr logic. Test/benchmark-build only. Simplest fix: skip giga validation for unassociated senders since they always fall back to V2.
There was a problem hiding this comment.
Looks good — this is a well-scoped fix that aligns giga's validation-time balance reads with V2's DBImpl semantics.
What was reviewed: traced ensureMinimumBalance/GetBalance in giga/deps/xevm/state and confirmed the top-off hook is a no-op outside the mock_balances build tag, so production balance-check behavior is unchanged. Checked the unassociated-sender path (sender vs derived seiEVMAddr) for double-counting/double-top-off — they resolve to distinct Sei accounts via GetSeiAddressOrDefault, guarded by the seiEVMAddr != sender check, so no double-add. Verified stateDB construction moved earlier has no side effects beyond an in-memory snapshot, so reordering is safe. New mock_balances-tagged regression test exercises the fixed path end to end.
Extended reasoning...
Overview
Two-file change: app/app.go (giga EVM tx validation/execution) and a new mock_balances-tagged regression test. The core change routes validateGigaEVMTx's balance reads through the giga DBImpl (via stateDB.GetBalance) instead of GigaEvmKeeper.GetBalance directly, so validation sees the same ensureMinimumBalance top-off hook that V2's StateTransition.BuyGas already applies. DBImpl construction was also moved earlier in executeEVMTxWithGigaExecutor so it's available before validation runs.
Security risks
None identified. The behavioral change (balance top-off) is gated behind the mock_balances build tag, which panics if the chain ID is pacific-1 and is explicitly documented as test/benchmark-only. Outside that build tag, ensureMinimumBalance is a no-op, so mainnet/production balance-check semantics are unchanged.
Level of scrutiny
Moderate. This touches the DeliverTx-path validation/execution flow for the giga executor, which is consensus-relevant, but the functional change is confined to dev/test tooling parity (mock balances) rather than production logic — the PR is labeled non-app-hash-breaking, consistent with the no-op-in-prod behavior. I verified the unassociated-sender balance summation doesn't double-count or double-top-off the same underlying account, since sender's cast address and the derived seiEVMAddr's default address resolve to different accounts unless they're literally equal (in which case the code explicitly skips the second addition).
Other factors
The PR includes a targeted regression test (TestGigaMockBalancesValidationUsesDBImpl) that exercises exactly the scenario the fix addresses — an unfunded giga transfer within the top-off amount — under the real mock_balances build tag and giga sequential execution mode. CI (build/lint/tests) is green, and the bug-hunting pass found no issues; I independently checked the one candidate concern raised (possible balance doubling for unassociated senders) and confirmed it doesn't occur.
pompon0
left a comment
There was a problem hiding this comment.
Are the seidroid findings not relevant?
not really since mock balance is only built with non-noop with load test |
There was a problem hiding this comment.
The PR correctly routes giga's validation balance reads through the giga DBImpl so mock_balances builds follow V2's ensureMinimumBalance/BuyGas behavior; production (non-mock) behavior is unchanged since the balance sum matches the prior logic. No blocking issues found, but there is one mock_balances-only divergence for unassociated senders that the new test does not cover.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor second-opinion review (cursor-review.md) was empty — that pass produced no output. REVIEW_GUIDELINES.md was also empty, so no repo-specific standards were applied.
- The new regression test (TestGigaMockBalancesValidationUsesDBImpl) only exercises an associated sender. Consider adding coverage for the unassociated fallback path to document/verify the mock_balances behavior described below.
- Minor: the infinite gas meter is now set before validation (previously after). This appears intentional and benign (validation state reads won't exhaust a finite meter), but worth confirming it doesn't change any gas-accounting parity with V2's validation.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| // Include cast address balance for unassociated addresses (matches V2 PreprocessDecorator) | ||
| // Include the derived Sei address balance for unassociated addresses (matches V2 PreprocessDecorator). | ||
| if !isAssociated { |
There was a problem hiding this comment.
[suggestion] Confirmed the Codex P2 concern, scoped to mock_balances builds only. For an unassociated sender, stateDB.GetBalance(sender) tops off the cast address (sender[:]) and stateDB.GetBalance(seiEVMAddr) tops off the derived Sei address, each to the 100 SEI TopOffAmount — and ensureMinimumBalance→topOffAccount persists both mints. The tx then always falls back to V2 (ErrBalanceMigrationRequired), which migrates the cast balance, leaving ~200 SEI instead of the ~100 SEI a direct V2 execution creates. This has no production impact (ensureMinimumBalance is a no-op without the build tag, and it panics on pacific-1), but it can cause an app-hash divergence between giga and V2 specifically in mock_balances benchmark/test runs — the scenario this PR aims to align. Since the unassociated path always falls back to V2 anyway, consider skipping the DBImpl-backed top-off for unassociated senders (or reading raw balances there) and adding a test covering it.
There was a problem hiding this comment.
No bugs found by the bug hunting system. Beyond the seidroil findings (mock_balances/load-test only, already addressed by the author as not consensus-relevant), I independently traced the production-build address math for the unassociated-sender balance sum: stateDB.GetBalance(seiEVMAddr) resolves through GetSeiAddressOrDefault's cast-address fallback back to the original seiAddr bytes, so the new sum (cast(sender) + seiAddr) is mathematically identical to the old sum (seiAddr + castAddr). No behavior change in production (non-mock_balances) builds.
Extended reasoning...
Verified that in non-mock_balances builds, ensureMinimumBalance is a no-op (build-tag gated via mock_balances_noop.go), so the only change in production is routing the same balance reads through DBImpl instead of the keeper directly. Traced the unassociated-sender branch specifically: common.BytesToAddress(seiAddr) followed by stateDB.GetBalance round-trips through GetSeiAddressOrDefault's unassociated fallback (sdk.AccAddress(evmAddress[:])), which reconstructs the original 20-byte seiAddr, and stateDB.GetBalance(sender) resolves to cast(sender)'s balance the same way — so the new sum equals the old GigaEvmKeeper.GetBalance(seiAddr) + GigaEvmKeeper.GetBalance(castAddr) sum exactly. This is new ground beyond what seidroil/author discussed (which focused only on the mock_balances double top-off), so I'm recording it as a ruled-out concern rather than staying fully silent.
There was a problem hiding this comment.
An organization admin can view or raise the cap at claude.ai/admin-settings/claude-code. The cap resets at the start of the next billing period.
Once the cap resets or is raised, push a new commit or reopen this pull request to trigger a review.
There was a problem hiding this comment.
A clean, well-scoped fix that routes giga executor balance validation through DBImpl.GetBalance so mock_balances builds apply the minimum top-off consistently with V2's BuyGas; for the associated path it is behavior-preserving in production and the unassociated path always falls back to v2, so there is no state-hash risk. No code-level concerns found; only process notes about empty second-opinion inputs.
Findings: 0 blocking | 3 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output. Codex reported no material issues.
- REVIEW_GUIDELINES.md was empty/missing, so no repo-specific guidelines were applied.
- Optional: the mock_balances regression test asserts the tx succeeds (Code 0) but does not assert that the account was unfunded beforehand (i.e. that success is specifically due to the DBImpl top-off). A negative assertion of pre-balance would make the regression intent more explicit.
Summary
Routes giga executor balance validation reads through the giga EVM
DBImplsomock_balancesbuilds follow the sameensureMinimumBalancebehavior as V2'sStateTransition.BuyGas.Root Cause
validateGigaEVMTxwas checking balances withGigaEvmKeeper.GetBalance, bypassingDBImpl. In V2,BuyGasreads throughDBImpl.GetBalance, so mock-balance builds can apply the 100 SEI minimum top-off before the affordability comparison. Giga's manual validation precheck missed that StateDB boundary.Changes
DBImplbefore validation and pass it intovalidateGigaEVMTx.GetBalanceduring validation, including the unassociated-address balance case.mock_balancesregression test covering an unfunded giga transfer within the minimum top-off amount.Validation
go test ./giga/deps/xevm/state -run TestSubBalance -count=1go test ./giga/tests -run TestGigaValidation_InsufficientBalance -count=1go test -tags mock_balances ./giga/tests -run TestGigaMockBalancesValidationUsesDBImpl -count=1