Skip to content

Commit

Permalink
refactor: modify VerifyUpgradeAndUpdateState to set upgraded client a…
Browse files Browse the repository at this point in the history 
…nd consensus state (cosmos#598)

* modify VerifyUpgradeAndUpdateState interface function to remove returned client and consensus state

removes the client and consensus state from the return values in VerifyUpgradeAndUpdateState client state interface function
Updates light client implementations to set client and consensus state in client store
Fixes and updates tests

* add changelog entry

* add migration docs

* use upgraded client to emit height in events
  • Loading branch information
@colin-axner @seunlanlege
colin-axner authored and seunlanlege committed Aug 9, 2022
1 parent 5ca6c91 commit 8942d3c
Show file tree
Hide file tree
Showing 9 changed files with 586 additions and 177 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (core) [\#709](https://github.com/cosmos/ibc-go/pull/709) Replace github.com/pkg/errors with stdlib errors

### API Breaking

* (02-client) [\#598](https://github.com/cosmos/ibc-go/pull/598) The client state and consensus state return value has been removed from `VerifyUpgradeAndUpdateState`. Light client implementations must update the client state and consensus state after verifying a valid client upgrade.
* (testing) [\#939](https://github.com/cosmos/ibc-go/pull/939) Support custom power reduction for testing.
* (modules/core/05-port) [\#1086](https://github.com/cosmos/ibc-go/pull/1086) Added `counterpartyChannelID` argument to IBCModule.OnChanOpenAck
* (06-solomachine) [\#1100](https://github.com/cosmos/ibc-go/pull/1100) Remove `GetClientID` function from 06-solomachine `Misbehaviour` type.
Expand Down
112 changes: 5 additions & 107 deletions docs/migrations/v3-to-v4.md
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Migrating from ibc-go v3 to v4
# Migrating from ibc-go v2 to v3

This document is intended to highlight significant changes which may require more information than presented in the CHANGELOG.
Any changes that must be done by a user of ibc-go should be documented here.
Expand All @@ -18,115 +18,13 @@ No genesis or in-place migrations required when upgrading from v1 or v2 of ibc-g

## Chains

### Migration to fix support for base denoms with slashes

As part of [v1.5.0](https://github.com/cosmos/ibc-go/releases/tag/v1.5.0), [v2.3.0](https://github.com/cosmos/ibc-go/releases/tag/v2.3.0) and [v3.1.0](https://github.com/cosmos/ibc-go/releases/tag/v3.1.0) some [migration handler code sample was documented](https://github.com/cosmos/ibc-go/blob/main/docs/migrations/support-denoms-with-slashes.md#upgrade-proposal) that needs to run in order to correct the trace information of coins transferred using ICS20 whose base denom contains slashes.

Based on feedback from the community we add now an improved solution to run the same migration that does not require copying a large piece of code over from the migration document, but instead requires only adding a one-line upgrade handler.

If the chain will migrate to supporting base denoms with slashes, it must set the appropriate params during the execution of the upgrade handler in `app.go`:
```go
app.UpgradeKeeper.SetUpgradeHandler("MigrateTraces",
func(ctx sdk.Context, _ upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) {
// transfer module consensus version has been bumped to 2
return app.mm.RunMigrations(ctx, app.configurator, fromVM)
})

```

If a chain receives coins of a base denom with slashes before it upgrades to supporting it, the receive may pass however the trace information will be incorrect.

E.g. If a base denom of `testcoin/testcoin/testcoin` is sent to a chain that does not support slashes in the base denom, the receive will be successful. However, the trace information stored on the receiving chain will be: `Trace: "transfer/{channel-id}/testcoin/testcoin", BaseDenom: "testcoin"`.

This incorrect trace information must be corrected when the chain does upgrade to fully supporting denominations with slashes.

## IBC Apps

### ICS03 - Connection

Crossing hellos have been removed from 03-connection handshake negotiation.
`PreviousConnectionId` in `MsgConnectionOpenTry` has been deprecated and is no longer used by core IBC.

`NewMsgConnectionOpenTry` no longer takes in the `PreviousConnectionId` as crossing hellos are no longer supported. A non-empty `PreviousConnectionId` will fail basic validation for this message.

### ICS04 - Channel
### IS04 - Channel

The `WriteAcknowledgement` API now takes the `exported.Acknowledgement` type instead of passing in the acknowledgement byte array directly.
This is an API breaking change and as such IBC application developers will have to update any calls to `WriteAcknowledgement`.

The `OnChanOpenInit` application callback has been modified.
The return signature now includes the application version as detailed in the latest IBC [spec changes](https://github.com/cosmos/ibc/pull/629).

The `NewErrorAcknowledgement` method signature has changed.
It now accepts an `error` rather than a `string`. This was done in order to prevent accidental state changes.
All error acknowledgements now contain a deterministic ABCI code and error message. It is the responsibility of the application developer to emit error details in events.

Crossing hellos have been removed from 04-channel handshake negotiation.
IBC Applications no longer need to account from already claimed capabilities in the `OnChanOpenTry` callback. The capability provided by core IBC must be able to be claimed with error.
`PreviousChannelId` in `MsgChannelOpenTry` has been deprecated and is no longer used by core IBC.

`NewMsgChannelOpenTry` no longer takes in the `PreviousChannelId` as crossing hellos are no longer supported. A non-empty `PreviousChannelId` will fail basic validation for this message.

### ICS27 - Interchain Accounts

The `RegisterInterchainAccount` API has been modified to include an additional `version` argument. This change has been made in order to support ICS29 fee middleware, for relayer incentivization of ICS27 packets.
Consumers of the `RegisterInterchainAccount` are now expected to build the appropriate JSON encoded version string themselves and pass it accordingly.
This should be constructed within the interchain accounts authentication module which leverages the APIs exposed via the interchain accounts `controllerKeeper`. If an empty string is passed in the `version` argument, then the version will be initialized to a default value in the `OnChanOpenInit` callback of the controller's handler, so that channel handshake can proceed.

The following code snippet illustrates how to construct an appropriate interchain accounts `Metadata` and encode it as a JSON bytestring:

```go
icaMetadata := icatypes.Metadata{
Version: icatypes.Version,
ControllerConnectionId: controllerConnectionID,
HostConnectionId: hostConnectionID,
Encoding: icatypes.EncodingProtobuf,
TxType: icatypes.TxTypeSDKMultiMsg,
}

appVersion, err := icatypes.ModuleCdc.MarshalJSON(&icaMetadata)
if err != nil {
return err
}

if err := k.icaControllerKeeper.RegisterInterchainAccount(ctx, msg.ConnectionId, msg.Owner, string(appVersion)); err != nil {
return err
}
```

Similarly, if the application stack is configured to route through ICS29 fee middleware and a fee enabled channel is desired, construct the appropriate ICS29 `Metadata` type:

```go
icaMetadata := icatypes.Metadata{
Version: icatypes.Version,
ControllerConnectionId: controllerConnectionID,
HostConnectionId: hostConnectionID,
Encoding: icatypes.EncodingProtobuf,
TxType: icatypes.TxTypeSDKMultiMsg,
}

appVersion, err := icatypes.ModuleCdc.MarshalJSON(&icaMetadata)
if err != nil {
return err
}

feeMetadata := feetypes.Metadata{
AppVersion: string(appVersion),
FeeVersion: feetypes.Version,
}

feeEnabledVersion, err := feetypes.ModuleCdc.MarshalJSON(&feeMetadata)
if err != nil {
return err
}

if err := k.icaControllerKeeper.RegisterInterchainAccount(ctx, msg.ConnectionId, msg.Owner, string(feeEnabledVersion)); err != nil {
return err
}
```

## Relayers
## IBC Light Clients

When using the `DenomTrace` gRPC, the full IBC denomination with the `ibc/` prefix may now be passed in.
The `VerifyUpgradeAndUpdateState` function has been modified. The client state and consensus state return value has been removed.

Crossing hellos are no longer supported by core IBC for 03-connection and 04-channel. The handshake should be completed in the logical 4 step process (INIT, TRY, ACK, CONFIRM).
Light clients **must** set the updated client state and consensus state in the client store after verifying a valid client upgrade.
16 changes: 6 additions & 10 deletions modules/core/02-client/keeper/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,30 +158,26 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e
return sdkerrors.Wrapf(types.ErrClientNotActive, "cannot upgrade client (%s) with status %s", clientID, status)
}

updatedClientState, updatedConsState, err := clientState.VerifyUpgradeAndUpdateState(ctx, k.cdc, clientStore,
upgradedClient, upgradedConsState, proofUpgradeClient, proofUpgradeConsState)
if err != nil {
if err := clientState.VerifyUpgradeAndUpdateState(ctx, k.cdc, clientStore,
upgradedClient, upgradedConsState, proofUpgradeClient, proofUpgradeConsState,
); err != nil {
return sdkerrors.Wrapf(err, "cannot upgrade client with ID %s", clientID)
}

k.SetClientState(ctx, clientID, updatedClientState)
k.SetClientConsensusState(ctx, clientID, updatedClientState.GetLatestHeight(), updatedConsState)

k.Logger(ctx).Info("client state upgraded", "client-id", clientID, "height", updatedClientState.GetLatestHeight().String())
k.Logger(ctx).Info("client state upgraded", "client-id", clientID, "height", upgradedClient.GetLatestHeight().String())

defer func() {
telemetry.IncrCounterWithLabels(
[]string{"ibc", "client", "upgrade"},
1,
[]metrics.Label{
telemetry.NewLabel(types.LabelClientType, updatedClientState.ClientType()),
telemetry.NewLabel(types.LabelClientType, upgradedClient.ClientType()),
telemetry.NewLabel(types.LabelClientID, clientID),
},
)
}()

// emitting events in the keeper emits for client upgrades
EmitUpgradeClientEvent(ctx, clientID, updatedClientState)
EmitUpgradeClientEvent(ctx, clientID, upgradedClient)

return nil
}
Expand Down
3 changes: 2 additions & 1 deletion modules/core/exported/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ type ClientState interface {
// height of the current revision is somehow encoded in the proof verification process.
// This is to ensure that no premature upgrades occur, since upgrade plans committed to by the counterparty
// may be cancelled or modified before the last planned height.
// If the upgrade is verified, the upgraded client and consensus states must be set in the client store.
VerifyUpgradeAndUpdateState(
ctx sdk.Context,
cdc codec.BinaryCodec,
Expand All @@ -76,7 +77,7 @@ type ClientState interface {
newConsState ConsensusState,
proofUpgradeClient,
proofUpgradeConsState []byte,
) (ClientState, ConsensusState, error)
) error
// Utility function that zeroes out any client customizable fields in client state
// Ledger enforced fields are maintained while all custom fields are zero values
// Used to verify upgrades
Expand Down
Loading

0 comments on commit 8942d3c

Please sign in to comment.