From 65d0cd83878778fe8346a781b5bfa856ac3ee7a1 Mon Sep 17 00:00:00 2001 From: Charly Date: Fri, 15 Jul 2022 14:09:54 +0200 Subject: [PATCH 1/6] initial commit --- .../adr-026-ibc-client-recovery-mechanisms.md | 4 ++++ .../07-tendermint/types/proposal_handle.go | 8 +++++++- .../07-tendermint/types/proposal_handle_test.go | 13 +++++++++++-- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md b/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md index 516002ed1b9..ec6f9b994b9 100644 --- a/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md +++ b/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md @@ -7,6 +7,7 @@ - 2021/01/15: Revision to support substitute clients for unfreezing - 2021/05/20: Revision to simplify consensus state copying, remove initial height - 2022/04/08: Revision to deprecate AllowUpdateAfterExpiry and AllowUpdateAfterMisbehaviour +- 2022/07/15: Revision to allow updating of TrustingPeriod ## Status @@ -51,6 +52,9 @@ We elect not to deal with chains which have actually halted, which is necessaril Previously, `AllowUpdateAfterExpiry` and `AllowUpdateAfterMisbehaviour` were used to signal the recovery options for an expired or frozen client, and governance proposals were not allowed to overwrite the client if these parameters were set to false. However, this has now been deprecated because a code migration can overwrite the client and consensus states regardless of the value of these parameters. If governance would vote to overwrite a client or consensus state, it is likely that governance would also be willing to perform a code migration to do the same. + In addition, `TrustingPeriod` was initally not allowed to be updated by a client upgrade proposal. However, due to the number of situations experienced in production where the trusting period of a client should be allowed to be updated because of ie: initial misconfiguration for a canonical channel, governance should be allowed to upgrade this parameter. + + Note that this should NOT be lightly updated, as there may be a gap in time between when misbehaviour has occured and when the evidence of misbehaviour is submitted. For example, if the unbonding period is 2 weeks and the trusting period has been set to two weeks, a validator could wait until right before unbonding period finishes, submit false information, then unbond and exit without being slashed for misbehaviour. Therefore, we recommend that the trusting period be set at a minimum to 2/3 of the unbonding period. Note that clients frozen due to misbehaviour must wait for the evidence to expire to avoid becoming refrozen. diff --git a/modules/light-clients/07-tendermint/types/proposal_handle.go b/modules/light-clients/07-tendermint/types/proposal_handle.go index b1ba592453e..85720f7eaeb 100644 --- a/modules/light-clients/07-tendermint/types/proposal_handle.go +++ b/modules/light-clients/07-tendermint/types/proposal_handle.go @@ -2,6 +2,7 @@ package types import ( "reflect" + "time" "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" @@ -70,6 +71,9 @@ func (cs ClientState) CheckSubstituteAndUpdateState( cs.LatestHeight = substituteClientState.LatestHeight cs.ChainId = substituteClientState.ChainId + // set new trusting period based on the substitute client state + cs.TrustingPeriod = substituteClientState.TrustingPeriod + // no validation is necessary since the substitute is verified to be Active // in 02-client. @@ -77,13 +81,15 @@ func (cs ClientState) CheckSubstituteAndUpdateState( } // IsMatchingClientState returns true if all the client state parameters match -// except for frozen height, latest height, and chain-id. +// except for frozen height, latest height, trusting period, chain-id. func IsMatchingClientState(subject, substitute ClientState) bool { // zero out parameters which do not need to match subject.LatestHeight = clienttypes.ZeroHeight() subject.FrozenHeight = clienttypes.ZeroHeight() + subject.TrustingPeriod = time.Duration(0) substitute.LatestHeight = clienttypes.ZeroHeight() substitute.FrozenHeight = clienttypes.ZeroHeight() + substitute.TrustingPeriod = time.Duration(0) subject.ChainId = "" substitute.ChainId = "" // sets both sets of flags to true as these flags have been DEPRECATED, see ADR-026 for more information diff --git a/modules/light-clients/07-tendermint/types/proposal_handle_test.go b/modules/light-clients/07-tendermint/types/proposal_handle_test.go index 459f38187ea..4a59078708d 100644 --- a/modules/light-clients/07-tendermint/types/proposal_handle_test.go +++ b/modules/light-clients/07-tendermint/types/proposal_handle_test.go @@ -141,6 +141,8 @@ func (suite *TendermintTestSuite) TestCheckSubstituteAndUpdateState() { substitutePath := ibctesting.NewPath(suite.chainA, suite.chainB) suite.coordinator.SetupClients(substitutePath) substituteClientState := suite.chainA.GetClientState(substitutePath.EndpointA.ClientID).(*types.ClientState) + // update trusting period of substitute client state + substituteClientState.TrustingPeriod = time.Hour * 24 * 7 suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), substitutePath.EndpointA.ClientID, substituteClientState) // update substitute a few times @@ -189,8 +191,9 @@ func (suite *TendermintTestSuite) TestCheckSubstituteAndUpdateState() { suite.Require().Equal(expectedProcessedTime, subjectProcessedTime) suite.Require().Equal(expectedProcessedHeight, subjectProcessedHeight) suite.Require().Equal(expectedIterationKey, subjectIterationKey) - + suite.Require().Equal(newChainID, updatedClient.(*types.ClientState).ChainId) + suite.Require().Equal(time.Hour * 24 * 7, updatedClient.(*types.ClientState).TrustingPeriod) } else { suite.Require().Error(err) suite.Require().Nil(updatedClient) @@ -235,9 +238,15 @@ func (suite *TendermintTestSuite) TestIsMatchingClientState() { }, true, }, { - "not matching, trusting period is different", func() { + "matching, trusting period is different", func() { subjectClientState.TrustingPeriod = time.Duration(time.Hour * 10) substituteClientState.TrustingPeriod = time.Duration(time.Hour * 1) + }, true, + }, + { + "not matching, trust level is different", func() { + subjectClientState.TrustLevel = types.Fraction{2, 3} + substituteClientState.TrustLevel = types.Fraction{1, 3} }, false, }, } From 08a72a7332b4264471a5cc4d3d89826d65e1c200 Mon Sep 17 00:00:00 2001 From: Charly Date: Fri, 15 Jul 2022 14:14:57 +0200 Subject: [PATCH 2/6] format imports --- .../light-clients/07-tendermint/types/proposal_handle_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/light-clients/07-tendermint/types/proposal_handle_test.go b/modules/light-clients/07-tendermint/types/proposal_handle_test.go index 4a59078708d..bdd2d98ead5 100644 --- a/modules/light-clients/07-tendermint/types/proposal_handle_test.go +++ b/modules/light-clients/07-tendermint/types/proposal_handle_test.go @@ -191,9 +191,9 @@ func (suite *TendermintTestSuite) TestCheckSubstituteAndUpdateState() { suite.Require().Equal(expectedProcessedTime, subjectProcessedTime) suite.Require().Equal(expectedProcessedHeight, subjectProcessedHeight) suite.Require().Equal(expectedIterationKey, subjectIterationKey) - + suite.Require().Equal(newChainID, updatedClient.(*types.ClientState).ChainId) - suite.Require().Equal(time.Hour * 24 * 7, updatedClient.(*types.ClientState).TrustingPeriod) + suite.Require().Equal(time.Hour*24*7, updatedClient.(*types.ClientState).TrustingPeriod) } else { suite.Require().Error(err) suite.Require().Nil(updatedClient) From eba7e2c445ad0430f84df540060b5756ef0792b4 Mon Sep 17 00:00:00 2001 From: Charly Date: Fri, 15 Jul 2022 14:33:16 +0200 Subject: [PATCH 3/6] update docs --- docs/architecture/adr-026-ibc-client-recovery-mechanisms.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md b/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md index ec6f9b994b9..ab0b907ccbc 100644 --- a/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md +++ b/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md @@ -52,9 +52,9 @@ We elect not to deal with chains which have actually halted, which is necessaril Previously, `AllowUpdateAfterExpiry` and `AllowUpdateAfterMisbehaviour` were used to signal the recovery options for an expired or frozen client, and governance proposals were not allowed to overwrite the client if these parameters were set to false. However, this has now been deprecated because a code migration can overwrite the client and consensus states regardless of the value of these parameters. If governance would vote to overwrite a client or consensus state, it is likely that governance would also be willing to perform a code migration to do the same. - In addition, `TrustingPeriod` was initally not allowed to be updated by a client upgrade proposal. However, due to the number of situations experienced in production where the trusting period of a client should be allowed to be updated because of ie: initial misconfiguration for a canonical channel, governance should be allowed to upgrade this parameter. + In addition, `TrustingPeriod` was initally not allowed to be updated by a client upgrade proposal. However, due to the number of situations experienced in production where the `TrustingPeriod` of a client should be allowed to be updated because of ie: initial misconfiguration for a canonical channel, governance should be allowed to update this client parameter. - Note that this should NOT be lightly updated, as there may be a gap in time between when misbehaviour has occured and when the evidence of misbehaviour is submitted. For example, if the unbonding period is 2 weeks and the trusting period has been set to two weeks, a validator could wait until right before unbonding period finishes, submit false information, then unbond and exit without being slashed for misbehaviour. Therefore, we recommend that the trusting period be set at a minimum to 2/3 of the unbonding period. + Note that this should NOT be lightly updated, as there may be a gap in time between when misbehaviour has occured and when the evidence of misbehaviour is submitted. For example, if the `UnbondingPeriod` is 2 weeks and the `TrustingPeriod` has also been set to two weeks, a validator could wait until right before `UnbondingPeriod` finishes, submit false information, then unbond and exit without being slashed for misbehaviour. Therefore, we recommend that the trusting period for the 07-tendermint client be set at a minimum to 2/3 of the `UnbondingPeriod`. Note that clients frozen due to misbehaviour must wait for the evidence to expire to avoid becoming refrozen. From 35751f8c7078a3cac31e37353937d4ee79107205 Mon Sep 17 00:00:00 2001 From: Charly Date: Fri, 15 Jul 2022 14:45:56 +0200 Subject: [PATCH 4/6] update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index db771acd750..526b4e25fb9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -99,6 +99,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (transfer) [\#1414](https://github.com/cosmos/ibc-go/pull/1414) Emitting Sender address from `fungible_token_packet` events in `OnRecvPacket` and `OnAcknowledgementPacket`. * (modules/core/04-channel) [\#1464](https://github.com/cosmos/ibc-go/pull/1464) Emit a channel close event when an ordered channel is closed. * (modules/light-clients/07-tendermint) [\#1118](https://github.com/cosmos/ibc-go/pull/1118) Deprecating `AllowUpdateAfterExpiry` and `AllowUpdateAfterMisbehaviour`. See ADR-026 for context. +* (modules/light-clients/07-tendermint) [\#1713](https://github.com/cosmos/ibc-go/pull/1713) Allow client upgrade proposals to update `TrustingPeriod`. See ADR-026 for context. ### Features From 6c217cb152f81e2192c0bcc3b67ae93c0d26e05f Mon Sep 17 00:00:00 2001 From: Charly Date: Mon, 18 Jul 2022 13:22:58 +0200 Subject: [PATCH 5/6] update upgrade dev docs --- docs/ibc/upgrades/developer-guide.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/ibc/upgrades/developer-guide.md b/docs/ibc/upgrades/developer-guide.md index d41b3346d4f..73a19b93664 100644 --- a/docs/ibc/upgrades/developer-guide.md +++ b/docs/ibc/upgrades/developer-guide.md @@ -36,7 +36,7 @@ Developers must ensure that the new client adopts all of the new Client paramete Upgrades must adhere to the IBC Security Model. IBC does not rely on the assumption of honest relayers for correctness. Thus users should not have to rely on relayers to maintain client correctness and security (though honest relayers must exist to maintain relayer liveness). While relayers may choose any set of client parameters while creating a new `ClientState`, this still holds under the security model since users can always choose a relayer-created client that suits their security and correctness needs or create a Client with their desired parameters if no such client exists. -However, when upgrading an existing client, one must keep in mind that there are already many users who depend on this client's particular parameters. We cannot give the upgrading relayer free choice over these parameters once they have already been chosen. This would violate the security model since users who rely on the client would have to rely on the upgrading relayer to maintain the same level of security. Thus, developers must make sure that their upgrade mechanism allows clients to upgrade the chain-specified parameters whenever a chain upgrade changes these parameters (examples in the Tendermint client include `UnbondingPeriod`, `ChainID`, `UpgradePath`, etc.), while ensuring that the relayer submitting the `UpgradeClientMsg` cannot alter the client-chosen parameters that the users are relying upon (examples in Tendermint client include `TrustingPeriod`, `TrustLevel`, `MaxClockDrift`, etc). +However, when upgrading an existing client, one must keep in mind that there are already many users who depend on this client's particular parameters. We cannot give the upgrading relayer free choice over these parameters once they have already been chosen. This would violate the security model since users who rely on the client would have to rely on the upgrading relayer to maintain the same level of security. Thus, developers must make sure that their upgrade mechanism allows clients to upgrade the chain-specified parameters whenever a chain upgrade changes these parameters (examples in the Tendermint client include `UnbondingPeriod`, `TrustingPeriod`, `ChainID`, `UpgradePath`, etc.), while ensuring that the relayer submitting the `UpgradeClientMsg` cannot alter the client-chosen parameters that the users are relying upon (examples in Tendermint client include `TrustLevel`, `MaxClockDrift`, etc). Developers should maintain the distinction between Client parameters that are uniform across every valid light client of a chain (chain-chosen parameters), and Client parameters that are customizable by each individual client (client-chosen parameters); since this distinction is necessary to implement the `ZeroCustomFields` method in the `ClientState` interface: From 361be8c91f944dbf14d83249c7a297e5124eff1d Mon Sep 17 00:00:00 2001 From: Charly Date: Thu, 21 Jul 2022 14:57:13 +0200 Subject: [PATCH 6/6] update re: pr comments --- docs/architecture/adr-026-ibc-client-recovery-mechanisms.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md b/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md index ab0b907ccbc..bec25a3aad9 100644 --- a/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md +++ b/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md @@ -54,7 +54,7 @@ We elect not to deal with chains which have actually halted, which is necessaril In addition, `TrustingPeriod` was initally not allowed to be updated by a client upgrade proposal. However, due to the number of situations experienced in production where the `TrustingPeriod` of a client should be allowed to be updated because of ie: initial misconfiguration for a canonical channel, governance should be allowed to update this client parameter. - Note that this should NOT be lightly updated, as there may be a gap in time between when misbehaviour has occured and when the evidence of misbehaviour is submitted. For example, if the `UnbondingPeriod` is 2 weeks and the `TrustingPeriod` has also been set to two weeks, a validator could wait until right before `UnbondingPeriod` finishes, submit false information, then unbond and exit without being slashed for misbehaviour. Therefore, we recommend that the trusting period for the 07-tendermint client be set at a minimum to 2/3 of the `UnbondingPeriod`. + Note that this should NOT be lightly updated, as there may be a gap in time between when misbehaviour has occured and when the evidence of misbehaviour is submitted. For example, if the `UnbondingPeriod` is 2 weeks and the `TrustingPeriod` has also been set to two weeks, a validator could wait until right before `UnbondingPeriod` finishes, submit false information, then unbond and exit without being slashed for misbehaviour. Therefore, we recommend that the trusting period for the 07-tendermint client be set to 2/3 of the `UnbondingPeriod`. Note that clients frozen due to misbehaviour must wait for the evidence to expire to avoid becoming refrozen.