fix(subnet): add overflow guards for all uint32 fields in SubnetHostEpochStats#1014
Open
Mayveskii wants to merge 1 commit intogonka-ai:mainfrom
Open
fix(subnet): add overflow guards for all uint32 fields in SubnetHostEpochStats#1014Mayveskii wants to merge 1 commit intogonka-ai:mainfrom
Mayveskii wants to merge 1 commit intogonka-ai:mainfrom
Conversation
…Stats Add math.MaxUint32 guards for Missed, Invalid, RequiredValidations, CompletedValidations in AggregateSubnetHostStats and EscrowCount guard in IncrementSubnetHostEscrowCount, following the pattern from PR gonka-ai#544. Without guards, any of these uint32 fields can silently wrap to zero. RequiredValidations wrapping to zero while CompletedValidations is non-zero produces division-by-zero or infinite completion rate in downstream reward/punishment logic (critical before merging gonka-ai#976/gonka-ai#977). Closes gonka-ai#979
0xgonka
approved these changes
Apr 10, 2026
Doog-bot534
approved these changes
Apr 15, 2026
Doog-bot534
left a comment
There was a problem hiding this comment.
Review: fix(subnet): add overflow guards for all uint32 fields in SubnetHostEpochStats
Approve ✅
Correct and well-scoped. Adds pre-addition overflow checks for Missed, Invalid, RequiredValidations, CompletedValidations (uint32), and EscrowCount (uint32 increment). The pattern existing.X > math.MaxUint32 - slotStats.X is canonical safe overflow check.
Strengths
- Comprehensive: covers all five uint32 fields plus the existing uint64 Cost guard
- Error messages for new fields include diagnostic values (
existing=%d add=%d) - Tests cover each overflow boundary and happy path
Suggestions
- The original Cost overflow message doesn't include diagnostic values — consider updating for consistency.
- Add a test verifying two large-but-not-overflowing values succeed (e.g.,
MaxUint32/2 + MaxUint32/2). - Confirm the caller of
AggregateSubnetHostStatshandles error return gracefully (log and skip, not chain halt).
Payout address: gonka10zaal553duxp05nvfpqtsqrm2g0j6j34r8nan7
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
AggregateSubnetHostStatsguardsCost(uint64) against overflow (existing guard from PR #544) but leaves four uint32 fields unprotected:Missed— no guardInvalid— no guardRequiredValidations— no guardCompletedValidations— no guardIncrementSubnetHostEscrowCountincrementsEscrowCount(uint32) with no bound check.uint32 wraps at 4,294,967,295. If
RequiredValidationswraps to 0 whileCompletedValidationsremains non-zero, downstream completion-rate logic produces division-by-zero or inflated ratios. Severity escalates to HIGH when issues #976/#977 integrate these stats into the punishment/reward pipeline.Closes #979
Fix
Add
math.MaxUint32subtraction guards for all five uint32 accumulation sites, following the established pattern from PR #544:For
EscrowCountincrement:Impact
Before: uint32 counters silently wrap to zero on overflow — corrupted stats stored on-chain with no error signal.
After: any accumulation that would overflow returns an error, halting stats aggregation for that slot. No impact on current punishment pipeline (stats not yet wired to INACTIVE/INVALID logic). Must be merged before #976/#977.
If
AggregateSubnetHostStatsreturns an error from withinSettleSubnetEscrow, Cosmos SDK rolls back the full tx via CacheContext — no partial bank payments are committed.Tests added:
subnet_host_stats_test.go— 7 tests covering all 5 fields (Missed, Invalid, RequiredValidations, CompletedValidations, EscrowCount overflow) + happy path.Run with:
CGO_CFLAGS='-D__BLST_PORTABLE__' go test ./x/inference/keeper/... -run TestAggregateSubnetHostStats