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

fix: gov changeable param typo #68

Closed
wants to merge 1 commit into from

Conversation

caomingpei
Copy link
Contributor

Overview

Commit Summary:

Fix Typo in CIP-13 Regarding consensus.evidence.MaxAgeDuration

Commit Message:

This commit addresses a documentation error in cips/cip-13.md concerning the consensus.evidence.MaxAgeDuration parameter.

The current documentation incorrectly suggests that consensus.evidence.MaxAgeDuration is changeable via governance. This is a discrepancy because consensus.evidence.MaxAgeDuration should be identical with the staking.UnbondingTime, which is not changeable on-chain as indicated by its governance changeability status being set to False.

Additional comment: Allowing consensus.evidence.MaxAgeDuration to be modified on-chain poses a security risk. Such flexibility could enable a node to reduce this duration to under 3 days, which is problematic. In such a scenario, evidence older than this reduced period (less than 3 days) would be deemed invalid, potentially opening a attack window for fraud proofs to be submitted to the rollup. (Especially, the common setting of challenge period of rollup is 3-14 days).

To maintain consistency with staking.UnbondingTime, the parmeter consensus.evidence.MaxAgeDuration should remain unchanged by on-chain governance. This commit fixes the typo.

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@rootulp
Copy link
Collaborator

rootulp commented Feb 6, 2024

We shouldn't merge this PR because it isn't a typo. I agree the param likely shouldn't be changeable by governance but it currently is changeable by governance. See this test.

Instead of this PR, we should make a CIP to convert this param from governance modifiable to unmodifiable.

@jcstein
Copy link
Member

jcstein commented Feb 6, 2024

@jcstein jcstein closed this Feb 6, 2024
@caomingpei
Copy link
Contributor Author

Contributor

Yeah you are right, I agree with you. Simply modifying this CIP would cause the implementation not to match the protocol.
I am interested in these settings (such as consensus.evidence.MaxAgeDuration and consensus.evidence.MaxAgeNumBlocks), because I realized that these settings may introduce security risk (Long Range Attack for PoS). Is there any chance we can come up with a new CIP on this issue?

@jcstein
Copy link
Member

jcstein commented Feb 6, 2024

Yes, absolutely. Please feel free to do so in the forum: https://forum.celestia.org/c/celestia-improvement-proposal-cip/31

@caomingpei
Copy link
Contributor Author

Yes, absolutely. Please feel free to do so in the forum: https://forum.celestia.org/c/celestia-improvement-proposal-cip/31

Thanks. I have submitted a placeholder topic, which is under pending. And I will write a draft for discussion as soon as possible.

@jcstein
Copy link
Member

jcstein commented Feb 6, 2024

what is your ID on the forum @caomingpei ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants