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

feat: add optional migration pruning for tendermint consensus states #2800

Merged
merged 16 commits into from
Nov 29, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 25 additions & 36 deletions docs/migrations/v6-to-v7.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,31 @@ There are four sections based on the four potential user groups of this document

## Chains
Copy link
Contributor

Choose a reason for hiding this comment

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

the headline above needs to be updated (currently it's Migrating from ibc-go v5 to v6)


- No relevant changes were made in this release.
Chains will perform automatic migrations to remove a localhost client, if it exists and to migrate the solomachine to the v3 of the protobuf definition.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

An optional upgrade handler has been added to prune expired tendermint consensus states.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
Add the following to the function call to the upgrade handler in `app/app.go`, to perform the optional state pruning.

```go
import (
// ...
ibcclientmigrations "github.com/cosmos/ibc-go/v6/modules/core/02-client/migrations"
)

// ...

app.UpgradeKeeper.SetUpgradeHandler(
upgradeName,
func(ctx sdk.Context, _ upgradetypes.Plan, _ module.VersionMap) (module.VersionMap, error) {
// prune expired tendermint consensus states
ibcclientmigrations.PruneTendermintExpiredConsensusStates(ctx, app.Codec, appCodec, keys[ibchost.StoreKey])

return app.mm.RunMigrations(ctx, app.configurator, fromVM)
},
)
```

Checkout the logs to see how many consensus states are pruned.

## IBC Apps

Expand Down Expand Up @@ -55,41 +79,6 @@ The function `GetTimestampAtHeight` has been added to the `ClientState` interfac

The `GetRoot` function has been removed from consensus state interface since it was not used by core IBC.

### Light client implementations

The `09-localhost` light client implementation has been removed because it is currently non-functional.

An upgrade handler has been added to supply chain developers with the logic needed to prune the ibc client store and successfully complete the removal of `09-localhost`.
Add the following to the application upgrade handler in `app/app.go`, calling `MigrateToV6` to perform store migration logic.

```go
import (
// ...
ibcv6 "github.com/cosmos/ibc-go/v6/modules/core/migrations/v6"
)

// ...

app.UpgradeKeeper.SetUpgradeHandler(
upgradeName,
func(ctx sdk.Context, _ upgradetypes.Plan, _ module.VersionMap) (module.VersionMap, error) {
// prune the 09-localhost client from the ibc client store
ibcv6.MigrateToV6(ctx, app.IBCKeeper.ClientKeeper)

return app.mm.RunMigrations(ctx, app.configurator, fromVM)
},
)
```

Please note the above upgrade handler is optional and should only be run if chains have an existing `09-localhost` client stored in state.
A simple query can be performed to check for a `09-localhost` client on chain.

For example:

```
simd query ibc client states | grep 09-localhost
```

### Client Keeper

Keeper function `CheckMisbehaviourAndUpdateState` has been removed since function `UpdateClient` can now handle updating `ClientState` on `ClientMessage` type which can be any `Misbehaviour` implementations.
Expand Down
77 changes: 77 additions & 0 deletions modules/core/02-client/migrations/store.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package migrations

import (
"fmt"
"strings"

"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/store/prefix"
storetypes "github.com/cosmos/cosmos-sdk/store/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"

"github.com/cosmos/ibc-go/v6/modules/core/02-client/types"
host "github.com/cosmos/ibc-go/v6/modules/core/24-host"
"github.com/cosmos/ibc-go/v6/modules/core/exported"
ibctm "github.com/cosmos/ibc-go/v6/modules/light-clients/07-tendermint"
)

// PruneTendermintConsensusStates prunes all expired tendermint consensus states. This function
// may optionally be called during in-place store migrations. The 02-client store key must be provided.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
func PruneTendermintConsensusStates(ctx sdk.Context, cdc codec.BinaryCodec, storeKey storetypes.StoreKey) error {
store := ctx.KVStore(storeKey)

// iterate over 02-client store with prefix: clients/07-tendermint,
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
// example iterator.Key = -100/clientState (second half of the clientID + clientStore specific keys)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
tendermintClientPrefix := []byte(fmt.Sprintf("%s/%s", host.KeyClientStorePrefix, exported.Tendermint))
iterator := sdk.KVStorePrefixIterator(store, tendermintClientPrefix)

var clients []string

// collect all clients to avoid performing store state changes during iteration
defer iterator.Close()
for ; iterator.Valid(); iterator.Next() {
path := string(iterator.Key())
if !strings.Contains(path, host.KeyClientState) {
// skip non client state keys
continue
}

clientID, err := host.ParseClientStatePath(path)
if err != nil {
return err
}

clients = append(clients, clientID)
}

var totalPruned int

for _, clientID := range clients {
clientPrefix := []byte(fmt.Sprintf("%s/%s/", host.KeyClientStorePrefix, clientID))
Copy link
Member

Choose a reason for hiding this comment

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

Is the trailing slash required here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I think so. That's how it is in 02-client. If the slash wasn't there, I believe every query would need to prefix the key with a slash

clientStore := prefix.NewStore(ctx.KVStore(storeKey), clientPrefix)

bz := clientStore.Get(host.ClientStateKey())
if bz == nil {
return types.ErrClientNotFound
}

var clientState exported.ClientState
if err := cdc.UnmarshalInterface(bz, &clientState); err != nil {
return sdkerrors.Wrap(err, "failed to unmarshal client state bytes into tendermint client state")
}

tmClientState, ok := clientState.(*ibctm.ClientState)
if !ok {
return sdkerrors.Wrap(types.ErrInvalidClient, "client state is not tendermint even though client id contains 07-tendermint")
}

amtPruned := ibctm.PruneAllExpiredConsensusStates(ctx, clientStore, cdc, tmClientState)
totalPruned = totalPruned + amtPruned
}

clientLogger := ctx.Logger().With("module", "x/"+host.ModuleName+"/"+types.SubModuleName)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

copied from 02-client keeper

clientLogger.Info("pruned expired tendermint consensus states", "total", totalPruned)

return nil
}
178 changes: 178 additions & 0 deletions modules/core/02-client/migrations/store_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
package migrations_test

import (
"testing"
"time"

"github.com/stretchr/testify/suite"

"github.com/cosmos/ibc-go/v6/modules/core/02-client/migrations"
"github.com/cosmos/ibc-go/v6/modules/core/02-client/types"
host "github.com/cosmos/ibc-go/v6/modules/core/24-host"
"github.com/cosmos/ibc-go/v6/modules/core/exported"
ibctm "github.com/cosmos/ibc-go/v6/modules/light-clients/07-tendermint"
ibctesting "github.com/cosmos/ibc-go/v6/testing"
)

type MigrationsTestSuite struct {
suite.Suite

coordinator *ibctesting.Coordinator

chainA *ibctesting.TestChain
chainB *ibctesting.TestChain
}

// TestMigrationsTestSuite runs all the tests within this package.
func TestMigrationsTestSuite(t *testing.T) {
suite.Run(t, new(MigrationsTestSuite))
}

// SetupTest creates a coordinator with 2 test chains.
func (suite *MigrationsTestSuite) SetupTest() {
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2)
suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1))
suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(2))
}

// test pruning of multiple expired tendermint consensus states
func (suite *MigrationsTestSuite) TestPruneTendermintConsensusStates() {
// create multiple tendermint clients and a solo machine client
// the solo machine is used to verify this pruning function only modifies
// the tendermint store.

numTMClients := 3
paths := make([]*ibctesting.Path, numTMClients)

for i := 0; i < numTMClients; i++ {
path := ibctesting.NewPath(suite.chainA, suite.chainB)
suite.coordinator.SetupClients(path)

paths[i] = path
}

solomachine := ibctesting.NewSolomachine(suite.T(), suite.chainA.Codec, "06-solomachine-0", "testing", 1)
smClientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), solomachine.ClientID)

// set client state
bz, err := suite.chainA.App.AppCodec().MarshalInterface(solomachine.ClientState())
suite.Require().NoError(err)
smClientStore.Set(host.ClientStateKey(), bz)

bz, err = suite.chainA.App.AppCodec().MarshalInterface(solomachine.ConsensusState())
suite.Require().NoError(err)
smHeight := types.NewHeight(0, 1)
smClientStore.Set(host.ConsensusStateKey(smHeight), bz)

pruneHeightMap := make(map[*ibctesting.Path][]exported.Height)
unexpiredHeightMap := make(map[*ibctesting.Path][]exported.Height)

for _, path := range paths {
// collect all heights expected to be pruned
var pruneHeights []exported.Height
pruneHeights = append(pruneHeights, path.EndpointA.GetClientState().GetLatestHeight())

// these heights will be expired and also pruned
for i := 0; i < 3; i++ {
err := path.EndpointA.UpdateClient()
suite.Require().NoError(err)

pruneHeights = append(pruneHeights, path.EndpointA.GetClientState().GetLatestHeight())
}

// double chedck all information is currently stored
for _, pruneHeight := range pruneHeights {
consState, ok := suite.chainA.GetConsensusState(path.EndpointA.ClientID, pruneHeight)
suite.Require().True(ok)
suite.Require().NotNil(consState)

ctx := suite.chainA.GetContext()
clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(ctx, path.EndpointA.ClientID)

processedTime, ok := ibctm.GetProcessedTime(clientStore, pruneHeight)
suite.Require().True(ok)
suite.Require().NotNil(processedTime)

processedHeight, ok := ibctm.GetProcessedHeight(clientStore, pruneHeight)
suite.Require().True(ok)
suite.Require().NotNil(processedHeight)

expectedConsKey := ibctm.GetIterationKey(clientStore, pruneHeight)
suite.Require().NotNil(expectedConsKey)
}
pruneHeightMap[path] = pruneHeights
}

// Increment the time by a week
suite.coordinator.IncrementTimeBy(7 * 24 * time.Hour)

for _, path := range paths {
// create the consensus state that can be used as trusted height for next update
var unexpiredHeights []exported.Height
err := path.EndpointA.UpdateClient()
suite.Require().NoError(err)
unexpiredHeights = append(unexpiredHeights, path.EndpointA.GetClientState().GetLatestHeight())

err = path.EndpointA.UpdateClient()
suite.Require().NoError(err)
unexpiredHeights = append(unexpiredHeights, path.EndpointA.GetClientState().GetLatestHeight())

unexpiredHeightMap[path] = unexpiredHeights
}

// Increment the time by another week, then update the client.
// This will cause the consensus states created before the first time increment
// to be expired
suite.coordinator.IncrementTimeBy(7 * 24 * time.Hour)
err = migrations.PruneTendermintConsensusStates(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), suite.chainA.GetSimApp().GetKey(host.StoreKey))
suite.Require().NoError(err)

for _, path := range paths {
ctx := suite.chainA.GetContext()
clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(ctx, path.EndpointA.ClientID)

// ensure everything has been pruned
for i, pruneHeight := range pruneHeightMap[path] {
consState, ok := suite.chainA.GetConsensusState(path.EndpointA.ClientID, pruneHeight)
suite.Require().False(ok, i)
suite.Require().Nil(consState, i)

processedTime, ok := ibctm.GetProcessedTime(clientStore, pruneHeight)
suite.Require().False(ok, i)
suite.Require().Equal(uint64(0), processedTime, i)

processedHeight, ok := ibctm.GetProcessedHeight(clientStore, pruneHeight)
suite.Require().False(ok, i)
suite.Require().Nil(processedHeight, i)

expectedConsKey := ibctm.GetIterationKey(clientStore, pruneHeight)
suite.Require().Nil(expectedConsKey, i)
}

// ensure metadata is set for unexpired consensus state
for _, height := range unexpiredHeightMap[path] {
consState, ok := suite.chainA.GetConsensusState(path.EndpointA.ClientID, height)
suite.Require().True(ok)
suite.Require().NotNil(consState)

processedTime, ok := ibctm.GetProcessedTime(clientStore, height)
suite.Require().True(ok)
suite.Require().NotEqual(uint64(0), processedTime)

processedHeight, ok := ibctm.GetProcessedHeight(clientStore, height)
suite.Require().True(ok)
suite.Require().NotEqual(types.ZeroHeight(), processedHeight)

consKey := ibctm.GetIterationKey(clientStore, height)
suite.Require().Equal(host.ConsensusStateKey(height), consKey)
}
}

// verify that solomachine client and consensus state were not removed
smClientStore = suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), solomachine.ClientID)
bz = smClientStore.Get(host.ClientStateKey())
suite.Require().NotEmpty(bz)

bz = smClientStore.Get(host.ConsensusStateKey(smHeight))
suite.Require().NotEmpty(bz)
}
23 changes: 23 additions & 0 deletions modules/core/24-host/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,29 @@ func ParseIdentifier(identifier, prefix string) (uint64, error) {
return sequence, nil
}

// ParseClientStatePath returns the client ID from a client state path. It returns
// an error if the provided path is invalid.
func ParseClientStatePath(path string) (string, error) {
split := strings.Split(path, "/")
if len(split) != 3 {
return "", sdkerrors.Wrapf(ErrInvalidPath, "cannot parse client state path %s", path)
}

if split[0] != string(KeyClientStorePrefix) {
return "", sdkerrors.Wrapf(ErrInvalidPath, "path does not begin with client store prefix: expected %s, got %s", KeyClientStorePrefix, split[0])
}

if split[2] != KeyClientState {
return "", sdkerrors.Wrapf(ErrInvalidPath, "path does not end with client state key: expected %s, got %s", KeyClientState, split[2])
}

if strings.TrimSpace(split[1]) == "" {
return "", sdkerrors.Wrap(ErrInvalidPath, "clientID is empty")
}

return split[1], nil
}

// ParseConnectionPath returns the connection ID from a full path. It returns
// an error if the provided path is invalid.
func ParseConnectionPath(path string) (string, error) {
Expand Down
Loading