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

Allow support for changing trusting period on client upgrades #8184

Open
4 tasks
damiannolan opened this issue Mar 14, 2025 · 3 comments · May be fixed by #8185
Open
4 tasks

Allow support for changing trusting period on client upgrades #8184

damiannolan opened this issue Mar 14, 2025 · 3 comments · May be fixed by #8185
Assignees

Comments

@damiannolan
Copy link
Contributor

damiannolan commented Mar 14, 2025

Summary

Provide support for changing TrustingPeriod for 07-tendermint clients when performing client upgrades via MsgUpgradeClient.

Problem Definition

As it exists today, the only possible way to change the trusting period of an ibc client is by letting it expire and performing MsgRecoverClient which will allow changing the trusting period using the substitute client. https://github.com/cosmos/ibc-go/blob/main/modules/light-clients/07-tendermint/proposal_handle.go#L74-L75

Ideally, the TrustingPeriod can be changed via ibc client upgrades, in the same manner in which the UnbondingPeriod is changed.

TrustingPeriod and UnbondingPeriod in tendermint light clients are two tightly coupled fields, however, UnbondingPeriod is practically unused throughout the implementation.
There exists a single check in the Validate function of the 07-tendermint client state which asserts that TrustingPeriod < UnbondingPeriod. However, on client creation there is no mechanism to ensure the UnbondingPeriod is actually the correct unbonding period of the counterparty the client represents.
Thus, this a vague guideline and in practise the TrustingPeriod field is treated as the main time keeping mechanism for accepting new headers.

Changing UnbondingPeriod is classed as partially supported and TrustingPeriod is classed as unsupported for ibc client upgrades (I believe this is due to it being a "client chosen parameter", which is somewhat fair but creates holes and caveats in upgrading of unbonding period).
ref: https://ibc.cosmos.network/v8/ibc/upgrades/quick-guide/#ibc-client-breaking-upgrades

Decreasing the UnbondingPeriod is supported but if it is decreased to the point such that it is equal to or less than the TrustingPeriod then validation will fail, and the upgrade will not be accepted.

Use cases

Allow chains to change (decrease) their unbonding period while maintaining ibc safety.
For example, changing the unbonding period parameter of a chain from 21 days to 14(or even 7) days.

Allow counterparties to upgrade their ibc light clients to account for the change in UnbondingPeriod and thus, also in TrustingPeriod.

Proposal

I would like to propose that on MsgUpgradeClient, if the counterparty has modified their UnbondingPeriod then we calculate the percentage difference and scale down the TrustingPeriod by the same amount.
This would allow chains to change their unbonding period and issue a MsgIBCSoftwareUpgrade to upgrade all counterparty light clients, having both their unbonding period and trusting periods decreased in tandem.

Something like:

--- a/modules/light-clients/07-tendermint/upgrade.go
+++ b/modules/light-clients/07-tendermint/upgrade.go
@@ -93,6 +93,15 @@ func (cs ClientState) VerifyUpgradeAndUpdateState(
                return errorsmod.Wrapf(err, "consensus state proof failed. Path: %s", upgradeConsStatePath.GetKeyPath())
        }
 
+       // if the unbonding period has decreased then we must scale down the trusting period accordingly
+       if tmUpgradeClient.UnbondingPeriod < cs.UnbondingPeriod {
+               changeDiff := (cs.UnbondingPeriod - tmUpgradeClient.UnbondingPeriod) / cs.UnbondingPeriod
+               cs.TrustingPeriod *= (1-changeDiff)
+       }

We can optionally scale up the trusting period if there is an increase in unbonding period. This would maybe be more consistent.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
  • Estimate provided
@gjermundgaraba
Copy link
Contributor

Seems reasonable to me, at least! :)

FYI @AdityaSripal @srdtrk

@AdityaSripal
Copy link
Member

Im agreed on this

@srdtrk
Copy link
Member

srdtrk commented Mar 14, 2025

lgtm

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 a pull request may close this issue.

4 participants