Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider adding evidence parameters to the paramfilter #2418

Closed
evan-forbes opened this issue Sep 4, 2023 · 6 comments
Closed

Consider adding evidence parameters to the paramfilter #2418

evan-forbes opened this issue Sep 4, 2023 · 6 comments
Labels
consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules param

Comments

@evan-forbes
Copy link
Member

We've noticed that there are potentially 3 values that are currently modifiable via token voting that we might not want to be changed. These are the two evidence paramters, evidence.MaxAgeNumBlocks and evidence.MaxAgeDuration, along with the mint module's mint.BondDenom (the denomination sent to the distribution module's account)

@evan-forbes evan-forbes added consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules param labels Sep 4, 2023
@evan-forbes
Copy link
Member Author

cc @adlerjohn and @liamsi

simlar to our latest last minute change, while technically consensus breaking if no one successfully changes these values in governance we should be clear to update by simply upgrading

@rootulp rootulp self-assigned this Sep 5, 2023
@rootulp
Copy link
Collaborator

rootulp commented Sep 5, 2023

I'm still investigating if governance can modify any of these 3 params but it doesn't look like it.

  • I thinkmint.BondDenom is a genesis config and isn't a governance modifiable param. The x/mint module doesn't appear to use the params module (b/c no param subspace is provided here).
  • For the evidence parameters, this file is mostly empty. Cosmos SDK docs claim: "The evidence module does not contain any parameters."

@rootulp
Copy link
Collaborator

rootulp commented Sep 5, 2023

The evidence parameters are a subset of the consensus parameters. cosmos/cosmos-sdk#6197 and cosmos/cosmos-sdk#5952 leads me to think that consensus parameters are modifiable through abci.EndBlocker but I don't think they're modifiable through the governance module. The behavior of modifying consensus params isn't documented in the gov docs and I couldn't find references to consensus params or evidence params in the x/gov module so they still seem non governance modifiable.

Update: created cosmos/cosmos-sdk#17630

@rootulp rootulp added this to the Mainnet milestone Sep 5, 2023
@rootulp rootulp modified the milestones: Mainnet, Post-mainnet Sep 21, 2023
@rootulp
Copy link
Collaborator

rootulp commented Oct 9, 2023

Update: we're not including this in the v1.x release of celestia-app. We do intend on including it in a subsequent breaking change.

@rootulp rootulp modified the milestones: Post-mainnet, v2 Oct 10, 2023
@rootulp rootulp removed their assignment Oct 10, 2023
@rootulp
Copy link
Collaborator

rootulp commented Jan 21, 2024

I'm not sure why the OP calls out those 3 params. IMO all the params that are currently governance modifiable should be made governance unmodifiable. Instead we should determine the values for those params off-chain via rough consensus and then update them (if necessary) via hard-forks.

@rootulp rootulp changed the title Consider adding missing parameters to the paramfilter Consider adding evidence parameters to the paramfilter Feb 5, 2024
@rootulp rootulp removed this from the v2 milestone Feb 5, 2024
@rootulp
Copy link
Collaborator

rootulp commented Feb 22, 2024

Update: moving any param from governance modifiable to unmodifiable requires a CIP. CIP-16 proposes making the two evidence params gov unmodifiable so I think we can close this issue as won't do until that CIP has reached Final status.

@rootulp rootulp closed this as not planned Won't fix, can't repro, duplicate, stale Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules param
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants