Remove IBC logic from x/upgrade (cosmos#8673)

* add zeroed custom fields check to tm client

* remove custom fields function from x/upgrade and fix tests

* use []byte in x/upgrade, move abci to 02-client

* remove x/ibc from types

* whoops, delete testing files

* fix upgrade tests

* fix tm tests

* fix tests

* update CHANGELOG

* revert proto breakage, use reserved field cc @AmauryM

* add IBC Upgrade Proposal type

* remove IBC from upgrade types

* add IBC upgrade logic to 02-client

* fix all tests for x/upgrade

* Add CLI for IBC Upgrade Proposal

* Update x/ibc/core/02-client/types/proposal_test.go

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

* add gRPC for upgraded client state

* test fixes

* add HandleUpgradeProposal tests

* update docs and remove unnecessary code

* self review bug and test fixes

* neatness

* construct empty rest handler

* fix tests

* fix stringer tests

* Update docs/core/proto-docs.md

Co-authored-by: Christopher Goes <cwgoes@pluranimity.org>

* add key in ibc store tracking ibc upgrade heights

Add a new Key to the IBC client store to track the IBC Upgrade Height. This allows IBC upgrades to correctly remove old IBC upgrade states

* update abci and tests

* revert key storage after discussion with @AdityaSripal

Revert using a key to track IBC upgrades. By clearing any IBC state using an old plan in ScheduleUpgrade, IBC upgrades do not need to be tracked by IBC. This reduces code complexity and reduces potential for bugs.

* clear IBC states on cancelled upgrades

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-authored-by: Christopher Goes <cwgoes@pluranimity.org>

CHANGELOG.md

@@ -51,6 +51,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (x/distribution) [\#8473](https://github.com/cosmos/cosmos-sdk/pull/8473) On genesis init, if the distribution module account balance, coming from bank module state, does not match the one in distribution module state, the initialization will panic.
 * (client/keys) [\#8500](https://github.com/cosmos/cosmos-sdk/pull/8500) `InfoImporter` interface is removed from legacy keybase.
 * [\#8629](https://github.com/cosmos/cosmos-sdk/pull/8629) Deprecated `SetFullFundraiserPath` from `Config` in favor of `SetPurpose` and `SetCoinType`.
+* (x/upgrade) [\#8673](https://github.com/cosmos/cosmos-sdk/pull/8673) Remove IBC logic from x/upgrade. Deprecates IBC fields in an Upgrade Plan. IBC upgrade logic moved to 02-client and an IBC UpgradeProposal is added. 
 
 ### State Machine Breaking
 

docs/core/proto-docs.md

@@ -567,6 +567,7 @@
     - [Height](#ibc.core.client.v1.Height)
     - [IdentifiedClientState](#ibc.core.client.v1.IdentifiedClientState)
     - [Params](#ibc.core.client.v1.Params)
+    - [UpgradeProposal](#ibc.core.client.v1.UpgradeProposal)
 
 - [ibc/applications/transfer/v1/tx.proto](#ibc/applications/transfer/v1/tx.proto)
     - [MsgTransfer](#ibc.applications.transfer.v1.MsgTransfer)
@@ -659,6 +660,8 @@
     - [QueryConsensusStateResponse](#ibc.core.client.v1.QueryConsensusStateResponse)
     - [QueryConsensusStatesRequest](#ibc.core.client.v1.QueryConsensusStatesRequest)
     - [QueryConsensusStatesResponse](#ibc.core.client.v1.QueryConsensusStatesResponse)
+    - [QueryUpgradedClientStateRequest](#ibc.core.client.v1.QueryUpgradedClientStateRequest)
+    - [QueryUpgradedClientStateResponse](#ibc.core.client.v1.QueryUpgradedClientStateResponse)
 
     - [Query](#ibc.core.client.v1.Query)
 
@@ -7509,7 +7512,6 @@ Plan specifies information about a planned upgrade and when it should occur.
 | `time` | [google.protobuf.Timestamp](#google.protobuf.Timestamp) |  | The time after which the upgrade must be performed. Leave set to its zero value to use a pre-defined Height instead. |
 | `height` | [int64](#int64) |  | The height at which the upgrade must be performed. Only used if Time is not set. |
 | `info` | [string](#string) |  | Any application specific upgrade info to be included on-chain such as a git commit that validators could automatically upgrade to |
-| `upgraded_client_state` | [google.protobuf.Any](#google.protobuf.Any) |  | IBC-enabled chains can opt-in to including the upgraded client state in its upgrade plan This will make the chain commit to the correct upgraded (self) client state before the upgrade occurs, so that connecting chains can verify that the new upgraded client is valid by verifying a proof on the previous version of the chain. This will allow IBC connections to persist smoothly across planned chain upgrades |
 
 
 
@@ -7634,7 +7636,7 @@ RPC method.
 
 | Field | Type | Label | Description |
 | ----- | ---- | ----- | ----------- |
-| `upgraded_consensus_state` | [google.protobuf.Any](#google.protobuf.Any) |  |  |
+| `upgraded_consensus_state` | [bytes](#bytes) |  |  |
 
 
 
@@ -8163,6 +8165,25 @@ Params defines the set of IBC light client parameters.
 
 
 
+
+<a name="ibc.core.client.v1.UpgradeProposal"></a>
+
+### UpgradeProposal
+UpgradeProposal is a gov Content type for initiating an IBC breaking
+upgrade.
+
+
+| Field | Type | Label | Description |
+| ----- | ---- | ----- | ----------- |
+| `title` | [string](#string) |  |  |
+| `description` | [string](#string) |  |  |
+| `plan` | [cosmos.upgrade.v1beta1.Plan](#cosmos.upgrade.v1beta1.Plan) |  |  |
+| `upgraded_client_state` | [google.protobuf.Any](#google.protobuf.Any) |  | An UpgradedClientState must be provided to perform an IBC breaking upgrade. This will make the chain commit to the correct upgraded (self) client state before the upgrade occurs, so that connected chains can verify that the new upgraded client is valid by verifying a proof of the intended new client state on the previous version of the chain. This will allow IBC connections to persist smoothly across planned chain upgrades. |
+
+
+
+
+
  <!-- end messages -->
 
  <!-- end enums -->
@@ -9529,6 +9550,39 @@ Query/ConsensusStates RPC method
 
 
 
+
+<a name="ibc.core.client.v1.QueryUpgradedClientStateRequest"></a>
+
+### QueryUpgradedClientStateRequest
+QueryUpgradedClientStateRequest is the request type for the Query/UpgradedClientState RPC
+method
+
+
+| Field | Type | Label | Description |
+| ----- | ---- | ----- | ----------- |
+| `client_id` | [string](#string) |  | client state unique identifier |
+| `plan_height` | [int64](#int64) |  | plan height of the current chain must be sent in request as this is the height under which upgraded client state is stored |
+
+
+
+
+
+
+<a name="ibc.core.client.v1.QueryUpgradedClientStateResponse"></a>
+
+### QueryUpgradedClientStateResponse
+QueryUpgradedClientStateResponse is the response type for the Query/UpgradedClientState RPC
+method.
+
+
+| Field | Type | Label | Description |
+| ----- | ---- | ----- | ----------- |
+| `upgraded_client_state` | [google.protobuf.Any](#google.protobuf.Any) |  | client state associated with the request identifier |
+
+
+
+
+
  <!-- end messages -->
 
  <!-- end enums -->
@@ -9548,6 +9602,7 @@ Query provides defines the gRPC querier service
 | `ConsensusState` | [QueryConsensusStateRequest](#ibc.core.client.v1.QueryConsensusStateRequest) | [QueryConsensusStateResponse](#ibc.core.client.v1.QueryConsensusStateResponse) | ConsensusState queries a consensus state associated with a client state at a given height. | GET|/ibc/core/client/v1beta1/consensus_states/{client_id}/revision/{revision_number}/height/{revision_height}|
 | `ConsensusStates` | [QueryConsensusStatesRequest](#ibc.core.client.v1.QueryConsensusStatesRequest) | [QueryConsensusStatesResponse](#ibc.core.client.v1.QueryConsensusStatesResponse) | ConsensusStates queries all the consensus state associated with a given client. | GET|/ibc/core/client/v1beta1/consensus_states/{client_id}|
 | `ClientParams` | [QueryClientParamsRequest](#ibc.core.client.v1.QueryClientParamsRequest) | [QueryClientParamsResponse](#ibc.core.client.v1.QueryClientParamsResponse) | ClientParams queries all parameters of the ibc client. | GET|/ibc/client/v1beta1/params|
+| `UpgradedClientState` | [QueryUpgradedClientStateRequest](#ibc.core.client.v1.QueryUpgradedClientStateRequest) | [QueryUpgradedClientStateResponse](#ibc.core.client.v1.QueryUpgradedClientStateResponse) | UpgradedClientState queries an Upgraded IBC light client. | GET|/ibc/core/client/v1/upgraded_client_states/{client_id}|
 
  <!-- end services -->
 
@@ -10840,4 +10895,3 @@ that implements Misbehaviour interface expected by ICS-02
 | <a name="bool" /> bool |  | bool | boolean | boolean | bool | bool | boolean | TrueClass/FalseClass |
 | <a name="string" /> string | A string must always contain UTF-8 encoded or 7-bit ASCII text. | string | String | str/unicode | string | string | string | String (UTF-8) |
 | <a name="bytes" /> bytes | May contain any arbitrary sequence of bytes. | string | ByteString | str | []byte | ByteString | string | String (ASCII-8BIT) |
-

docs/ibc/upgrades/quick-guide.md

@@ -30,10 +30,10 @@ Note: Since upgrades are only implemented for Tendermint clients, this doc only
 
 If the IBC-connected chain is conducting an upgrade that will break counterparty clients, it must ensure that the upgrade is first supported by IBC using the list above and then execute the upgrade process described below in order to prevent counterparty clients from breaking.
 
-1. Create a `SoftwareUpgradeProposal` with an `UpgradePlan` that includes the new IBC ClientState in the `UpgradedClientState`. Note that the `UpgradePlan` must specify an upgrade height **only** (no upgrade time), and the `ClientState` should only include the fields common to all valid clients and zero out any client-customizable fields (such as TrustingPeriod).
-2. Vote on and pass the `SoftwareUpgradeProposal`
+1. Create an `UpgradeProposal` with an IBC ClientState in the `UpgradedClientState` field and a `UpgradePlan` in the `Plan` field. Note that the proposal `Plan` must specify an upgrade height **only** (no upgrade time), and the `ClientState` should only include the fields common to all valid clients and zero out any client-customizable fields (such as TrustingPeriod).
+2. Vote on and pass the `UpgradeProposal`
 
-Upon the `SoftwareUpgradeProposal` passing, the upgrade module will commit the UpgradedClient under the key: `upgrade/UpgradedIBCState/{upgradeHeight}/upgradedClient`. On the block right before the upgrade height, the upgrade module will also commit an initial consensus state for the next chain under the key: `upgrade/UpgradedIBCState/{upgradeHeight}/upgradedConsState`.
+Upon the `UpgradeProposal` passing, the upgrade module will commit the UpgradedClient under the key: `upgrade/UpgradedIBCState/{upgradeHeight}/upgradedClient`. On the block right before the upgrade height, the upgrade module will also commit an initial consensus state for the next chain under the key: `upgrade/UpgradedIBCState/{upgradeHeight}/upgradedConsState`.
 
 Once the chain reaches the upgrade height and halts, a relayer can upgrade the counterparty clients to the last block of the old chain. They can then submit the proofs of the `UpgradedClient` and `UpgradedConsensusState` against this last block and upgrade the counterparty client.
 
@@ -51,4 +51,4 @@ Thus, the upgrade process for relayers trying to upgrade the counterparty client
 
 The Tendermint client on the counterparty chain will verify that the upgrading chain did indeed commit to the upgraded client and upgraded consensus state at the upgrade height (since the upgrade height is included in the key). If the proofs are verified against the upgrade height, then the client will upgrade to the new client while retaining all of its client-customized fields. Thus, it will retain its old TrustingPeriod, TrustLevel, MaxClockDrift, etc; while adopting the new chain-specified fields such as UnbondingPeriod, ChainId, UpgradePath, etc. Note, this can lead to an invalid client since the old client-chosen fields may no longer be valid given the new chain-chosen fields. Upgrading chains should try to avoid these situations by not altering parameters that can break old clients. For an example, see the UnbondingPeriod example in the supported upgrades section.
 
-The upgraded consensus state will serve purely as a basis of trust for future `UpdateClientMsgs` and will not contain a consensus root to perform proof verification against. Thus, relayers must submit an `UpdateClientMsg` with a header from the new chain so that the connection can be used for proof verification again.
+The upgraded consensus state will serve purely as a basis of trust for future `UpdateClientMsgs` and will not contain a consensus root to perform proof verification against. Thus, relayers must submit an `UpdateClientMsg` with a header from the new chain so that the connection can be used for proof verification again.

proto/cosmos/bank/v1beta1/bank.proto

@@ -86,5 +86,5 @@ message Metadata {
   string name = 5;
   // symbol is the token symbol usually shown on exchanges (eg: ATOM). This can
   // be the same as the display.
-  string symbol = 6; 
+  string symbol = 6;
 }

proto/cosmos/upgrade/v1beta1/query.proto

@@ -64,5 +64,7 @@ message QueryUpgradedConsensusStateRequest {
 // QueryUpgradedConsensusStateResponse is the response type for the Query/UpgradedConsensusState
 // RPC method.
 message QueryUpgradedConsensusStateResponse {
-  google.protobuf.Any upgraded_consensus_state = 1;
+  reserved 1;
+
+  bytes upgraded_consensus_state = 2;
 }

proto/cosmos/upgrade/v1beta1/upgrade.proto

@@ -34,12 +34,10 @@ message Plan {
   // such as a git commit that validators could automatically upgrade to
   string info = 4;
 
-  // IBC-enabled chains can opt-in to including the upgraded client state in its upgrade plan
-  // This will make the chain commit to the correct upgraded (self) client state before the upgrade occurs,
-  // so that connecting chains can verify that the new upgraded client is valid by verifying a proof on the
-  // previous version of the chain.
-  // This will allow IBC connections to persist smoothly across planned chain upgrades
-  google.protobuf.Any upgraded_client_state = 5 [(gogoproto.moretags) = "yaml:\"upgraded_client_state\""];
+  // UpgradedClientState field has been deprecated. IBC upgrade logic has been
+  // moved to the IBC module in the sub module 02-client.
+  reserved 5;
+  reserved "option";
 }
 
 // SoftwareUpgradeProposal is a gov Content type for initiating a software

proto/ibc/core/client/v1/client.proto

@@ -5,6 +5,7 @@ option go_package = "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/types";
 
 import "gogoproto/gogo.proto";
 import "google/protobuf/any.proto";
+import "cosmos/upgrade/v1beta1/upgrade.proto";
 
 // IdentifiedClientState defines a client state with an additional client
 // identifier field.
@@ -52,6 +53,25 @@ message ClientUpdateProposal {
   Height initial_height = 5 [(gogoproto.moretags) = "yaml:\"initial_height\"", (gogoproto.nullable) = false];
 }
 
+// UpgradeProposal is a gov Content type for initiating an IBC breaking
+// upgrade.
+message UpgradeProposal {
+  option (gogoproto.goproto_getters)  = false;
+  option (gogoproto.goproto_stringer) = false;
+  option (gogoproto.equal)            = true;
+
+  string                      title       = 1;
+  string                      description = 2;
+  cosmos.upgrade.v1beta1.Plan plan        = 3 [(gogoproto.nullable) = false];
+
+  // An UpgradedClientState must be provided to perform an IBC breaking upgrade.
+  // This will make the chain commit to the correct upgraded (self) client state before the upgrade occurs,
+  // so that connecting chains can verify that the new upgraded client is valid by verifying a proof on the
+  // previous version of the chain.
+  // This will allow IBC connections to persist smoothly across planned chain upgrades
+  google.protobuf.Any upgraded_client_state = 4 [(gogoproto.moretags) = "yaml:\"upgraded_client_state\""];
+}
+
 // Height is a monotonically increasing data type
 // that can be compared against another Height for the purposes of updating and
 // freezing clients

proto/ibc/core/client/v1/query.proto

@@ -38,6 +38,11 @@ service Query {
   rpc ClientParams(QueryClientParamsRequest) returns (QueryClientParamsResponse) {
     option (google.api.http).get = "/ibc/client/v1beta1/params";
   }
+
+  // UpgradedClientState queries an Upgraded IBC light client.
+  rpc UpgradedClientState(QueryUpgradedClientStateRequest) returns (QueryUpgradedClientStateResponse) {
+    option (google.api.http).get = "/ibc/core/client/v1/upgraded_client_states/{client_id}";
+  }
 }
 
 // QueryClientStateRequest is the request type for the Query/ClientState RPC
@@ -128,3 +133,20 @@ message QueryClientParamsResponse {
   // params defines the parameters of the module.
   Params params = 1;
 }
+
+// QueryUpgradedClientStateRequest is the request type for the Query/UpgradedClientState RPC
+// method
+message QueryUpgradedClientStateRequest {
+  // client state unique identifier
+  string client_id = 1;
+  // plan height of the current chain must be sent in request
+  // as this is the height under which upgraded client state is stored
+  int64 plan_height = 2;
+}
+
+// QueryUpgradedClientStateResponse is the response type for the Query/UpgradedClientState RPC
+// method.
+message QueryUpgradedClientStateResponse {
+  // client state associated with the request identifier
+  google.protobuf.Any upgraded_client_state = 1;
+}

simapp/app.go

@@ -69,6 +69,7 @@ import (
 	ibctransfertypes "github.com/cosmos/cosmos-sdk/x/ibc/applications/transfer/types"
 	ibc "github.com/cosmos/cosmos-sdk/x/ibc/core"
 	ibcclient "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client"
+	ibcclientclient "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/client"
 	ibcclienttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/types"
 	porttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/05-port/types"
 	ibchost "github.com/cosmos/cosmos-sdk/x/ibc/core/24-host"
@@ -120,6 +121,7 @@ var (
 		distr.AppModuleBasic{},
 		gov.NewAppModuleBasic(
 			paramsclient.ProposalHandler, distrclient.ProposalHandler, upgradeclient.ProposalHandler, upgradeclient.CancelProposalHandler,
+			ibcclientclient.UpdateClientProposalHandler, ibcclientclient.UpgradeProposalHandler,
 		),
 		params.AppModuleBasic{},
 		crisis.AppModuleBasic{},
@@ -299,7 +301,7 @@ func NewSimApp(
 
 	// Create IBC Keeper
 	app.IBCKeeper = ibckeeper.NewKeeper(
-		appCodec, keys[ibchost.StoreKey], app.GetSubspace(ibchost.ModuleName), app.StakingKeeper, scopedIBCKeeper,
+		appCodec, keys[ibchost.StoreKey], app.GetSubspace(ibchost.ModuleName), app.StakingKeeper, app.UpgradeKeeper, scopedIBCKeeper,
 	)
 
 	app.AuthzKeeper = authzkeeper.NewKeeper(keys[authztypes.StoreKey], appCodec, app.BaseApp.MsgServiceRouter())
@@ -310,7 +312,7 @@ func NewSimApp(
 		AddRoute(paramproposal.RouterKey, params.NewParamChangeProposalHandler(app.ParamsKeeper)).
 		AddRoute(distrtypes.RouterKey, distr.NewCommunityPoolSpendProposalHandler(app.DistrKeeper)).
 		AddRoute(upgradetypes.RouterKey, upgrade.NewSoftwareUpgradeProposalHandler(app.UpgradeKeeper)).
-		AddRoute(ibcclienttypes.RouterKey, ibcclient.NewClientUpdateProposalHandler(app.IBCKeeper.ClientKeeper))
+		AddRoute(ibcclienttypes.RouterKey, ibcclient.NewClientProposalHandler(app.IBCKeeper.ClientKeeper))
 	app.GovKeeper = govkeeper.NewKeeper(
 		appCodec, keys[govtypes.StoreKey], app.GetSubspace(govtypes.ModuleName), app.AccountKeeper, app.BankKeeper,
 		&stakingKeeper, govRouter,

x/gov/legacy/v040/migrate_test.go

@@ -176,8 +176,7 @@ func TestMigrate(t *testing.T) {
 					"height": "123",
 					"info": "foo_upgrade_info",
 					"name": "foo_upgrade_name",
-					"time": "0001-01-01T00:00:00Z",
-					"upgraded_client_state": null
+					"time": "0001-01-01T00:00:00Z"
 				},
 				"title": "foo_software_upgrade"
 			},

x/ibc/core/02-client/abci.go

@@ -4,11 +4,32 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/keeper"
 	"github.com/cosmos/cosmos-sdk/x/ibc/core/exported"
+	ibctmtypes "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/07-tendermint/types"
 )
 
 // BeginBlocker updates an existing localhost client with the latest block height.
 func BeginBlocker(ctx sdk.Context, k keeper.Keeper) {
-	_, found := k.GetClientState(ctx, exported.Localhost)
+	plan, found := k.GetUpgradePlan(ctx)
+	if found {
+		// Once we are at the last block this chain will commit, set the upgraded consensus state
+		// so that IBC clients can use the last NextValidatorsHash as a trusted kernel for verifying
+		// headers on the next version of the chain.
+		// Set the time to the last block time of the current chain.
+		// In order for a client to upgrade successfully, the first block of the new chain must be committed
+		// within the trusting period of the last block time on this chain.
+		_, exists := k.GetUpgradedClient(ctx, plan.Height)
+		if exists && ctx.BlockHeight() == plan.Height-1 {
+			upgradedConsState := &ibctmtypes.ConsensusState{
+				Timestamp:          ctx.BlockTime(),
+				NextValidatorsHash: ctx.BlockHeader().NextValidatorsHash,
+			}
+			bz := k.MustMarshalConsensusState(upgradedConsState)
+
+			k.SetUpgradedConsensusState(ctx, plan.Height, bz)
+		}
+	}
+
+	_, found = k.GetClientState(ctx, exported.Localhost)
 	if !found {
 		return
 	}