feat!: prevent modifications on governance unmodifiable params #2919
Conversation
fahimahmedx
requested review from
evan-forbes,
rootulp and
cmwaters
as code owners
|
Warning Rate Limit Exceeded@fahimahmedx has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 7 minutes and 51 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe changes involve updates 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? TipsChat with CodeRabbit Bot (
|
|
In |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
fahimahmedx
changed the title
Good point. params.md claims that MaxValidators is not governance-modifiable So in my opinion:
|
app/app.go
Outdated
| // MaxSquareSize | ||
| {blobmoduletypes.ModuleName, string(blobmoduletypes.KeyGovMaxSquareSize)}, | ||
| // bank.SendEnabled | ||
| {banktypes.ModuleName, string(banktypes.KeySendEnabled)}, | ||
| // staking.UnbondingTime | ||
| {stakingtypes.ModuleName, string(stakingtypes.KeyUnbondingTime)}, | ||
| // staking.BondDenom | ||
| {stakingtypes.ModuleName, string(stakingtypes.KeyBondDenom)}, | ||
| // MaxBlockBytes and consensus.block.TimeIotaMs | ||
| {baseapp.Paramspace, string(baseapp.ParamStoreKeyBlockParams)}, | ||
| // consensus.validator.PubKeyTypes | ||
| {baseapp.Paramspace, string(baseapp.ParamStoreKeyValidatorParams)}, | ||
| // consensus.Version.AppVersion | ||
| {baseapp.Paramspace, string(baseapp.ParamStoreKeyVersionParams)}, | ||
| // staking.BondDenom | ||
| {stakingtypes.ModuleName, string(stakingtypes.KeyBondDenom)}, | ||
| // staking.MaxValidators | ||
| {stakingtypes.ModuleName, string(stakingtypes.KeyMaxValidators)}, | ||
| // staking.UnbondingTime | ||
| {stakingtypes.ModuleName, string(stakingtypes.KeyUnbondingTime)}, |
There was a problem hiding this comment.
[blocking] this adds more params to the list of blocked params which is potentially something we need to do so that there isn't a divergence between what is stated in params.md and what is actually modifiable. However, it's not clear if these need to be added without appropriate test coverage.
IMO we should tackle this in a few steps:
- Write a table-driven test with test cases for each param listed in params.md. The test should attempt to modify the param and see if it was actually modified. Each test case should have a boolean field that indicates if modification was expected to happen (i.e. that param is governance modifiable) or not happen (i.e. that param is not governance modifiable).
- Once we have the test above, we can compare the test results to params.md. If any divergences exist, create a GH issue with the divergence so that we can get alignment on whether the param should be governance modifiable.
- [Optional] If there are any params that are governance modifiable and shouldn't be, we may need to add them to this BlockedParams list.
@fahimahmedx is it possible you didn't push a commit? I don't see the new test cases. |
|
@rootulp Hi Rootul, thank you for your fast review!
|
This reverts commit f966d2f.
Rephrasing a bit to something I understand: does the existing test So I think we should add test cases for those params before modifying the |
fahimahmedx
changed the title
fahimahmedx
changed the title
|
Hi @rootulp, I noticed that that there isn't a Additionally, as parameters like |
|
Sorry for my delay @fahimahmedx.
celestia-app doesn't use the mint module defined in celestiaorg/cosmos-sdk so MintDenom doesn't need to be covered by these tests. celestia-app uses the x/mint module defined in this repo so
Yep. Since mint.InitialInflationRate, mint.DisinflationRate, and mint.TargetInflationRate are constants, I think they don't strictly need to be covered by tests here. |
Yes I think it is. Thanks for your thorough research! We may want to include a note in the mint module README.md that the mint module doesn't have any governance modifiable parameters for the reasons you've outlined. We can include the note in a separate PR, it doesn't have to be in this one. Update: https://github.com/celestiaorg/celestia-app/tree/main/x/mint#params exists already so we may not need to add anything. Instead we could add a note in paramfilter or params.md that the x/mint module doesn't have any governance modifiable params. |
|
Hi @rootulp, I've implemented the changes and the PR is now ready for review!
Do I proceed with opening an issue for each parameter (given this divergence) to get alignment on if they should be modifiable or not? I have added these parameters to |
|
Thanks for all your hard work here!
Yea can you please open one GH issue and reference the list of params that are governance modifiable even though the specs claim they shouldn't be modifiable. Then we can get input from the team (cc: @evan-forbes ) on how to proceed. IMO we should have tests covering all the params before we add any params to the blocked list. |
specs/src/specs/params.md
Outdated
|
|
||
| ### Tests | ||
| See [/x/paramfilter/test/governance_params_test.go](/x/paramfilter/test/governance_params_test.go) for tests that attempt to modify a parameter. | ||
| Note that `TimeIotaMs` is not in `conensus.block`, so a governance proposal can not be submitted to modify it. Additionally, the `x/mint` module does not have any governance modifiable params as justified in [/x/mint/README.md#params](/x/mint/README.md#params) |
There was a problem hiding this comment.
[question] if TimeIotaMs is not in consensus.block, should it be removed from the list of params above? Do you know if this field exists elsewhere?
There was a problem hiding this comment.
For context, ConsensusParams in celestia-core/abci/types/types.pb.go as seen here uses BlockParam from celestia-core/abci/types/types.pb.go instead of types1.BlockParams from celestia-core/proto/tendermint/types/params.pb.go.
We use this version of ConsensusParams in BaseApp.GetConsensusParams() as seen in cosmos-sdk/baseapp/baseapp.go here.
BlockParam does not contain TimeIotaMs, however I am not sure why types1.BlockParams (which contains it) isn't used instead, or if ConsensusParams from celestia-core/proto/tendermint/types/params.pb.go as seen here is another valid alternative that could be used instead in baseapp.go.
With this context in mind and knowing the field exists somewhere else (but seems to not be used), I think it would be good to clarify if the intended version of BlockParams is being used in the first place before we discuss if TimeIotaMs should be removed from params.md. Do you have any thoughts on this or know of anyone else who might?
There was a problem hiding this comment.
For further context on why I ask this, it is because I use BaseApp.GetConsensusParams() when testing if the governance modification was successful. This commit here shows this code.
| } | ||
|
|
||
| func TestHandlerTestSuite(t *testing.T) { | ||
| suite.Run(t, new(HandlerTestSuite)) |
There was a problem hiding this comment.
I expect this test to fail when running make test-short. If that's the case, we may want to skip it in short mode via something like:
if testing.Short() {
t.Skip("skipping handler test suite in short mode.")
}
suite.Run(t, new(HandlerTestSuite))| // consensus.Version.AppVersion | ||
| {baseapp.Paramspace, string(baseapp.ParamStoreKeyVersionParams)}, | ||
| // staking.BondDenom | ||
| {stakingtypes.ModuleName, string(stakingtypes.KeyBondDenom)}, | ||
| // staking.MaxValidators | ||
| {stakingtypes.ModuleName, string(stakingtypes.KeyMaxValidators)}, | ||
| // staking.UnbondingTime | ||
| {stakingtypes.ModuleName, string(stakingtypes.KeyUnbondingTime)}, |
There was a problem hiding this comment.
[blocking] this is a breaking change so if it is included in this PR then the PR title needs a ! to conform to conventional commits.
IMO we shouldn't add this to the blocked list yet. We should create a GH issue with the list of params that are governance modifiable even though the specs claim they shouldn't be. We should get team alignment on what to do for those params. Then we may update this list or update the specs accordingly. cc: @evan-forbes
There was a problem hiding this comment.
yeah any consens breaking change needs a CIP
@fahimahmedx, this is consensus breaking since nodes running this won't change a param, where nodes that are not will change the param
There was a problem hiding this comment.
the CIP doesn't have to be super in depth, since the change is pretty small, but we do still need to explain the benefits of adding or removing any params
rootulp
changed the title
|
Converting this to draft until we have a CIP with alignment. |
rootulp
added
consensus breaking
Overview
1 of 2 PRs that will address #2916. This PR adds test cases for the governance unmodifiable params.
The three params below are not added as they are already constants in
mint/types/constants.goChecklist