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

docs: CIP for make security related governance parameters immutable #74

Merged
merged 5 commits into from
Feb 16, 2024

Conversation

caomingpei
Copy link
Contributor

Add a CIP to make two parameters unchangeable for online governance.

Motivation: celestiaorg/CIPs/pull#68

Forum: https://forum.celestia.org/t/cip-make-security-related-governance-parameters-immutable/1566

cips/cip-draft_msrgpi_abbrev.md Show resolved Hide resolved
cips/cip-draft_msrgpi_abbrev.md Outdated Show resolved Hide resolved
cips/cip-draft_msrgpi_abbrev.md Outdated Show resolved Hide resolved
cips/cip-draft_msrgpi_abbrev.md Show resolved Hide resolved
cips/cip-draft_msrgpi_abbrev.md Outdated Show resolved Hide resolved
cips/cip-draft_msrgpi_abbrev.md Outdated Show resolved Hide resolved
cips/cip-draft_msrgpi_abbrev.md Outdated Show resolved Hide resolved
cips/cip-draft_msrgpi_abbrev.md Show resolved Hide resolved
cips/cip-draft_msrgpi_abbrev.md Show resolved Hide resolved
cips/cip-draft_msrgpi_abbrev.md Outdated Show resolved Hide resolved
| ...... | ...... | ...... | ...... |
| `staking.UnbondingTime` | 1814400 (21 days) | Duration of time for unbonding in seconds. | False |

This is a part of the table introduced in [CIP-13](cip-13.md). The summary of parameter `consensus.evidence.MaxAgeDuration` states *"...This value should be identical to the unbonding period"*. Meanwhile, the parameter `staking.UnbondingTime` is NOT changeable since the `Changeable via Governance` is set to False. Suppose an on-chain governance proposal tries to modify the default value of `consensus.evidence.MaxAgeDuration` from `1814400000000000 (21 days)` to a different value. It would create an inconsistency between the description and implementation because the modified value would no longer be identical to the unbonding period.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[clarifying question] I think our specs are actually wrong here. I think the MaxAgeDuration doesn't need to be identical to the unbonding time but just >= unbonding time. For example, could the unbonding time be set to 30 days?

Update: I see the point about resource consumption below. Given evidence is usually empty, the resource consumption on nodes for a longer unboding time seems negligible to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good question. It is well known that setting evidence.maxAge shorter than the unbonding period could introduce security risks. In the previous description, setting both values as equal seems reasonable. Although a longer time will not consume too much storage space, the meaning of evidence is lost if it is longer than the unbonding period. Suppose there is an attack, even if the evidence is still valid after a period longer than the unbonding period, but the attacker has already unstaked. Different value settings in evidence expiration and unbonding periods could also lead to confusion. It is unclear which value should be chosen for the "challenge period" for users. If the evidence always equals the unbonding period, we can also simplify the Celestia description, i.e., we need to confirm the validity of consensus within the unbonding period.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your thorough response. It seems like we want evidnece.MaxAgeDuration to == unbonding period for simplicity and because the protocol doesn't need to retain evidence longer than the unbonding period because a malicious validator could unbond and exit the protocol in that time. Seems reasonable to me.

If we wanted to provide a bit more info in this section, we could analyze the MaxAgeDuration and unboding period of other Cosmos chains (like Cosmos Hub, Osmosis). Something like:

Chain MaxAgeDuration Unbonding time
Celestia 1814400000000000 (21 days) 1814400 (21 days)
Cosmos Hub TBD 21 days
Osmosis TBD 14 days

and then if they're identical (on a per-chain basis), that could be supporting evidence for making the param governance unmodifiable and exactly match the unbonding time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chain MaxAgeDuration Unbonding time
Celestia 1814400000000000 (21 days) 1814400 (21 days)
Cosmos Hub Complex Situation* 21 days
Osmosis 14 days (UnbondingTime) 14 days

The MaxAgeDuration of Osmosis is the same as its unbonding time.

However, for Cosmos Hub, the situation is complex. For the genesis, the max_evidence_age in slashing is set to 1814400000000000 (21 days), which equals to the unbonding time. Moreover, this parameter remains unchanged until the genesis of cosmoshub-4. We can check the #PR5952, and the L191 in Changelog states that "Remove parameters from x/evidence genesis and module state. The x/evidence module now solely uses Tendermint consensus parameters to determine of evidence is valid or not." In fact, there seems to still remain a problem (Inconsistency between design and implementation) in the parameter setting because after PR5952, the setting in ComosHub adopts the default setting of Tendermint. But if we check the current-parameters.js in gaia, we can find the max_age_num_blocks is 10 times than the default value in Tendermint. Besides, the average block creation interval for Comos Hub is 6 seconds (which is different from the default value). So, the final validity period for evidence in gaia is about MAX(70 days, 2 days), this is a strange situation because these two values are expected to be close.

I believe the security guarantee for Celestia should be close to that for Osmosis. This is because for Cosmos Hub, the exploit of malicious behaviour usually gains the native token, and the punishment mechanism will make it not profitable. However, the potential consensus instability risk for Osmosis or Celestia may lead to other losses. Overall, the genesis settings indicate that the maxAgeDuration equaling the unbonding time is a typical implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for your thorough investigation! Agreed the Cosmos Hub situation seems complex. @mpoke is there any rationale for the inconsistency?

Given all that info, it seems simpler to set both evidnece.MaxAgeDuration and unbonding time to the same values (21 days) like you initially proposed.

cips/cip-draft_msrgpi_abbrev.md Outdated Show resolved Hide resolved

## Backwards Compatibility

It maintains backward compatibility provided that NO on-chain governance proposal to change the two parameters has been approved at the time of this CIP implementation.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[question] I think this CIP is actually backwards incompatible because a change needs to be made to celestia-app to make these params unmodifiable. So this proposal needs to be introduced via a hard-fork.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The section I wrote needs to be clearer. What I meant to convey is that if the two parameters have not been modified, the setting for the validity period of evidence is still compatible after being banned from on-chain governance. However, I did not clearly state that the client proposing on-chain governance is no longer backwards compatible. My current thought is to amend this content to be backwards incompatible. And add a note explaining that if the parameters have never been changed, the security impact on consensus is not significant. Do you have any suggestions or comments more?

Copy link
Collaborator

@rootulp rootulp Feb 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My current thought is to amend this content to be backwards incompatible.

Agreed.

And add a note explaining that if the parameters have never been changed, the security impact on consensus is not significant.

Hmm I think this may belong in the Security Considerations section rather than here in Backwards Compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Updated.

Copy link
Member

@jcstein jcstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, cc @rootulp for a last review of relevant changes.

@jcstein jcstein merged commit 8658f2f into celestiaorg:main Feb 16, 2024
1 check passed
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