fix: 0.50.14 patch #69
Conversation
WalkthroughDocumentation updated for v0.50.14. Integration tests expanded to cover overflow behavior in distribution rewards deposits. Distribution keeper’s MsgServer adds post-deposit overflow checks with panic recovery to prevent historical rewards ratio overflow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant MsgServer as Distribution MsgServer
participant Keeper as Distribution Keeper
participant Store as KV Store
Client->>MsgServer: DepositValidatorRewardsPool(msg)
MsgServer->>Keeper: depositValidatorRewards(msg)
Keeper->>Store: Add tokens to validator rewards pool
alt Post-deposit validation
Keeper->>Keeper: Compute current rewards
rect rgba(230,240,255,0.6)
note right of Keeper: Attempt to add to historical rewards ratio<br/>with panic recovery
Keeper->>Keeper: Try ratio += current
end
alt Overflow detected (panic)
Keeper-->>MsgServer: error "deposit too large"
MsgServer-->>Client: Error response
else No overflow
Keeper-->>MsgServer: ok
MsgServer-->>Client: Success
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
CHANGELOG.md (1)
41-45: Include the x/distribution overflow fix in v0.50.14 Bug Fixes.Release notes call out an x/distribution historical rewards overflow fix; add a bullet here for parity and discoverability.
Apply this diff:
### Bug Fixes * (baseapp) [#21979](https://github.com/cosmos/cosmos-sdk/pull/21979) Create CheckTxHandler to allow extending the logic of CheckTx. * (baseapp) [#24074](https://github.com/cosmos/cosmos-sdk/pull/24074) Use CometBFT's ComputeProtoSizeForTxs in defaultTxSelector.SelectTxForProposal for consistency. +* (x/distribution) Prevent historical rewards ratio overflow during deposits; return error instead of panic.x/distribution/keeper/msg_server.go (1)
5-5: Prefer typed SDK errors over fmt for consistency.Drop fmt and wrap with cosmossdk.io/errors + sdkerrors to preserve ABCI codes.
-import ( - "context" - "fmt" +import ( + "context"RELEASE_NOTES.md (1)
9-9: Grammar: “recommended” → “recommend”.Small copy edit.
-We recommended upgrading to this patch release as soon as possible. +We recommend upgrading to this patch release as soon as possible.tests/integration/distribution/keeper/msg_server_test.go (1)
4-4: Optional: consolidate assertion library usage.You’re mixing gotest.tools/assert and testify/require. Consider standardizing on one (e.g., testify) for consistency.
Also applies to: 12-18
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)RELEASE_NOTES.md(1 hunks)tests/integration/distribution/keeper/msg_server_test.go(5 hunks)x/distribution/keeper/msg_server.go(2 hunks)
🔇 Additional comments (3)
tests/integration/distribution/keeper/msg_server_test.go (2)
151-157: LGTM: staking params, hooks, and MsgServer wiring added.These are needed for rewards distribution and staking message flow in tests.
Also applies to: 162-163
1012-1116: Verify context consistency and mint module account existence.
- Several calls in TestCannotDepositIfRewardPoolFull use both ctx (WithIsCheckTx/WithBlockHeight) and f.sdkCtx. Prefer a single context to avoid subtle state mismatches.
- Minting from minttypes.ModuleName assumes the mint module account exists. If not initialized, MintCoins may fail on some setups.
To check for potential pitfalls:
x/distribution/keeper/msg_server.go (1)
215-244: Guard against period underflow and return typed errors; explicitly discard Add result.
- Avoid
rewards.Period-1whenPeriod == 0(underflow).- Use
cosmossdk.io/errors+sdkerrorsinstead of fmt to keep error codes.- Make
_ = rewardRatio.Add(current...)explicit.// make sure the reward pool isn't already full. if !validator.GetTokens().IsZero() { rewards, err := k.GetValidatorCurrentRewards(ctx, valAddr) if err != nil { return nil, err } current := rewards.Rewards - historical, err := k.GetValidatorHistoricalRewards(ctx, valAddr, rewards.Period-1) - if err != nil { - return nil, err - } - if !historical.CumulativeRewardRatio.IsZero() { - rewardRatio := historical.CumulativeRewardRatio - var panicErr error - func() { - defer func() { - if r := recover(); r != nil { - panicErr = fmt.Errorf("deposit is too large: %v", r) - } - }() - rewardRatio.Add(current...) - }() - - // Check if the deferred function caught a panic - if panicErr != nil { - return nil, fmt.Errorf("unable to deposit coins: %w", panicErr) - } - } + // no previous period to compare against; skip overflow check + if rewards.Period > 0 { + historical, err := k.GetValidatorHistoricalRewards(ctx, valAddr, rewards.Period-1) + if err != nil { + return nil, err + } + if !historical.CumulativeRewardRatio.IsZero() { + rewardRatio := historical.CumulativeRewardRatio + var recovered interface{} + func() { + defer func() { + if r := recover(); r != nil { + recovered = r + } + }() + // provoke any overflow by performing the addition + _ = rewardRatio.Add(current...) + }() + if recovered != nil { + return nil, errors.Wrapf(sdkerrors.ErrInvalidRequest, "unable to deposit coins: %v", recovered) + } + } + } }Run to confirm assumptions about Period and Add:
Take v0.50.14 patch from cosmos-sdk upstream.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests