fix(x/slashing): Avoid overslashing on redelegation + unbonding in specific situations #20688
Conversation
WalkthroughWalkthroughThe recent updates introduce significant changes to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- simapp/mint_fn.go (1 hunks)
- tests/integration/slashing/keeper/slash_redelegation_test.go (1 hunks)
- x/staking/CHANGELOG.md (1 hunks)
- x/staking/keeper/slash.go (1 hunks)
Additional context used
Path-based instructions (4)
simapp/mint_fn.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/staking/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"x/staking/keeper/slash.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/integration/slashing/keeper/slash_redelegation_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
LanguageTool
x/staking/CHANGELOG.md
[style] ~99-~99: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM)
Context: ...pi/cosmos/staking/v1beta1".Infraction_` in order to remove dependency between modules on st...
[misspelling] ~106-~106: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’. (EN_A_VS_AN)
Context: ...ub.com//pull/18841) In a undelegation or redelegation if the sha...
Markdownlint
x/staking/CHANGELOG.md
58-58: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
60-60: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
61-61: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
63-63: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
64-64: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
66-66: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
67-67: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
69-69: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
71-71: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
73-73: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
74-74: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
77-77: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
78-78: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
80-80: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
82-82: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
84-84: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
86-86: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
88-88: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
91-91: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
93-93: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
95-95: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
98-98: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
102-102: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
50-50: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
101-101: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
44-44: null (MD024, no-duplicate-heading)
Multiple headings with the same content
Additional comments not posted (4)
simapp/mint_fn.go (1)
20-20: Refactored method signature to enhance clarity by separating sender and recipient module parameters.Verification successful
The review comment regarding the refactored method signature
SendCoinsFromModuleToModuleinsimapp/mint_fn.gohas been verified. The search results indicate that the new method signature is used consistently across the codebase.
- The new method signature is found in various files, including
x/protocolpool/types/expected_keepers.go,x/mint/types/expected_keepers.go,x/gov/testutil/expected_keepers.go, and others.- The older signature with
senderPoolandrecipientPoolis still present in some mock files and tests (x/staking/testutil/expected_keepers_mocks.go), which might be intentional for backward compatibility in tests.Please ensure that all instances, especially in test files, are updated to match the new signature if backward compatibility is not required.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `SendCoinsFromModuleToModule` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go $'SendCoinsFromModuleToModule'Length of output: 17735
x/staking/CHANGELOG.md (1)
30-30: The changelog entry correctly documents the bug fix for overslashing.x/staking/keeper/slash.go (1)
Line range hint
368-392: UpdatedSlashRedelegationto handle moved delegations more accurately, preventing overslashing.tests/integration/slashing/keeper/slash_redelegation_test.go (1)
189-392: IntroducedTestOverSlashingto ensure that the new slashing logic does not result in overslashing. The test is well-structured and covers the necessary scenarios.
| if err != nil { | ||
| return math.ZeroInt(), err | ||
| } | ||
| sharesToUnbond, err := dstVal.SharesFromTokensTruncated(slashAmount) |
There was a problem hiding this comment.
If I understood this correct, then you calculate the shares for the remaining slash amount. The slash amount may be (partially) filled from slashing unbonding delegations first. This looks correct to me and fixes the issue. Good work 👍
|
|
||
| func TestOverSlashing(t *testing.T) { | ||
| // slash penalty percentage | ||
| slashFraction := "0.50" |
There was a problem hiding this comment.
nit: prefer an odd value so that you which half was actually used. Seeing some rounding can be interesting, too.
There was a problem hiding this comment.
changed it to 45 👌
|
|
||
| require.Equal(t, balance1AfterSlashing.Amount, balance2AfterSlashing.Amount) | ||
| require.Equal(t, balance2AfterSlashing.Amount, balance3AfterSlashing.Amount) | ||
| } |
There was a problem hiding this comment.
Good test case.
There some more scenarios with multiple re-/undelegations that are not covered, yet. Maybe you can add them here, too.
For bonus points:
- assert bondend and unbonded amounts separately to see how much of each was slashed
- cover validator status unbonded
- run into
sharesToUnbond.GT(delegation.Shares) - (personal preference) refactor to a table test
// setup account and validators
specs := map[string]struct {
slashFrac math.LegacyDec
del []math.Int
redel []math.Int
undel []math.Int
expBonded math.Int
expUnbonded math.Int
}{
"": {},
}
for name, spec := range specs {
t.Run(name, func(t *testing.T) {
// run with cacheContext ...
})
}
There was a problem hiding this comment.
I'll take a look but I think I won't be able to make it a table test as afaict we need more than one delegation for this to happen
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
tests/integration/slashing/keeper/slash_redelegation_test.go (1)
81-91: Suggest using more descriptive validator details for clarity in tests.The test validators are created with generic descriptions ("test"). Providing more descriptive or context-specific details might enhance the readability and maintainability of the test cases, especially when dealing with multiple validators.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- tests/integration/slashing/keeper/slash_redelegation_test.go (6 hunks)
Additional context used
Path-based instructions (1)
tests/integration/slashing/keeper/slash_redelegation_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (3)
tests/integration/slashing/keeper/slash_redelegation_test.go (3)
76-77: Validation of account balances is correct.The assertions correctly validate that the initial funding of accounts was successful by comparing the pre- and post-funding balances.
187-367: Comprehensive test for over-slashing scenarios is well-implemented.The test function
TestOverSlashingprovides a detailed scenario to check for potential over-slashing issues. It includes multiple accounts and validators, and complex interactions like redelegations and undelegations. The logic appears sound, and the assertions are well-placed to validate the expected outcomes.
102-107: Check delegation logic for potential improvements.The delegation logic is straightforward, but consider adding checks or logs to ensure that the delegation transactions are executed as expected, especially in complex test scenarios involving multiple actions.
| testCoin := sdk.NewCoin(bondDenom, stakingKeeper.TokensFromConsensusPower(ctx, 10)) | ||
| fundAccount(t, ctx, bankKeeper, authKeeper, testAcc1, testCoin) | ||
| fundAccount(t, ctx, bankKeeper, authKeeper, testAcc2, testCoin) |
There was a problem hiding this comment.
Ensure consistent funding logic across test cases.
The function fundAccount is used multiple times to set up initial conditions for the test accounts. Consider refactoring to a helper function that initializes multiple accounts to reduce redundancy and improve readability.
| redelMsg := stakingtypes.NewMsgBeginRedelegate(testAcc1.String(), evilValAddr.String(), goodValAddr.String(), testCoin) | ||
| _, err = stakingMsgServer.BeginRedelegate(ctx, redelMsg) | ||
| require.NoError(t, err) | ||
|
|
||
| // Acc 1 undelegate from good val | ||
| undelMsg := stakingtypes.NewMsgUndelegate(testAcc1.String(), goodValAddr.String(), testCoins[0]) | ||
| undelMsg := stakingtypes.NewMsgUndelegate(testAcc1.String(), goodValAddr.String(), testCoin) | ||
| _, err = stakingMsgServer.Undelegate(ctx, undelMsg) | ||
| require.NoError(t, err) | ||
|
|
||
| // Acc 2 undelegate from evil val | ||
| undelMsg = stakingtypes.NewMsgUndelegate(testAcc2.String(), evilValAddr.String(), testCoins[0]) | ||
| undelMsg = stakingtypes.NewMsgUndelegate(testAcc2.String(), evilValAddr.String(), testCoin) |
There was a problem hiding this comment.
Tip
Codebase Verification
Consider adding more detailed assertions post-redelegation and undelegation.
The current test file tests/integration/slashing/keeper/slash_redelegation_test.go contains assertions for errors but lacks specific checks on the state of delegations after redelegation and undelegation operations.
- Add assertions to verify the state of delegations to ensure they reflect the expected changes accurately.
Analysis chain
Consider adding more detailed assertions post-redelegation and undelegation.
After performing redelegation and undelegation operations, it would be beneficial to add assertions checking the state of the delegations to ensure they reflect the expected changes accurately.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the state of delegations after redelegation and undelegation operations.
rg --type go 'GetDelegation' tests/integration/slashing/keeper
Length of output: 62
Script:
#!/bin/bash
# Description: Verify the presence of assertions or checks on the state of delegations after redelegation and undelegation operations.
# Search for assertion methods or patterns in the test file.
rg --type go 'require\..*' tests/integration/slashing/keeper/slash_redelegation_test.go
Length of output: 2151
Description
In some cases a user that performed a redelegation and a subsequent partial unbond would get slashed twice, once over its redelegation and once over its unbonding tokens.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
SendCoinsFromModuleToModulemethod to separately acceptsenderModuleandrecipientModule.TestOverSlashingto simulate and verify slashing scenarios in the blockchain network.Bug Fixes
Documentation
CHANGELOG.mdwith details on bug fixes and new features.