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

Add in-place and genesis migrations #205

Merged
merged 27 commits into from
Jun 17, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
4f874d9
add in-place migrations
colin-axner May 31, 2021
f8ca013
update migrations
colin-axner Jun 2, 2021
29c1c48
migrate solomachine from v1 to v2 during in place migration
colin-axner Jun 7, 2021
db0db21
fix build
colin-axner Jun 7, 2021
972ca3c
add genesis migration
colin-axner Jun 7, 2021
314aaba
code cleanup
colin-axner Jun 8, 2021
40a30fa
Merge branch 'main' into colin/11-migration-scripts
colin-axner Jun 8, 2021
863fdbd
add store migration test for expired tendermint consensus states
colin-axner Jun 8, 2021
a36801f
Merge branch 'colin/11-migration-scripts' of github.com:cosmos/ibc-go…
colin-axner Jun 8, 2021
863b50a
finish adding in place migration store tests
colin-axner Jun 9, 2021
545056e
add genesis test for solo machines
colin-axner Jun 9, 2021
40484bb
fix genesis migration bug, add tendermint tests
colin-axner Jun 9, 2021
a24618d
test fix, changelog, migration docs
colin-axner Jun 9, 2021
6d9bcdd
Apply suggestions from code review
colin-axner Jun 9, 2021
6c6fc52
Merge branch 'main' into colin/11-migration-scripts
colin-axner Jun 9, 2021
a67102f
Update docs/migrations/ibc-migration-043.md
AdityaSripal Jun 9, 2021
7e1f40f
apply Aditya's review suggestions
colin-axner Jun 10, 2021
3896a2a
Merge branch 'colin/11-migration-scripts' of github.com:cosmos/ibc-go…
colin-axner Jun 10, 2021
7edac1e
fix tests
colin-axner Jun 10, 2021
e326f74
add genesis json unmarshal test
colin-axner Jun 10, 2021
a970700
Merge branch 'main' into colin/11-migration-scripts
colin-axner Jun 10, 2021
7ceca61
add migration support for max expected time per block
colin-axner Jun 15, 2021
ca070bd
Merge branch 'colin/11-migration-scripts' of github.com:cosmos/ibc-go…
colin-axner Jun 15, 2021
0fcd0c4
fix docs
colin-axner Jun 15, 2021
900b907
fix bug found by Aditya
colin-axner Jun 16, 2021
fed9443
remove unnecessary code
colin-axner Jun 16, 2021
e595b11
apply Aditya review suggestions, fix bug
colin-axner Jun 17, 2021
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
13 changes: 9 additions & 4 deletions modules/core/02-client/legacy/v100/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
// - Update solo machine client state protobuf definition (v1 to v2)
// - Remove all solo machine consensus states
// - Remove all expired tendermint consensus states
func MigrateGenesis(cdc codec.BinaryCodec, clientGenState *types.GenesisState, genesisBlockTime time.Time) (*types.GenesisState, error) {
func MigrateGenesis(cdc codec.BinaryCodec, clientGenState *types.GenesisState, genesisBlockTime time.Time, selfHeight exported.Height) (*types.GenesisState, error) {
// To prune the consensus states, we will create new clientsConsensus
// and clientsMetadata. These slices will be filled up with consensus states
// which should not be pruned. No solo machine consensus states should be added
Expand Down Expand Up @@ -102,9 +102,14 @@ func MigrateGenesis(cdc codec.BinaryCodec, clientGenState *types.GenesisState, g
// only unexpired consensus state heights should be added
var clientMetadata []types.GenesisMetadata
for _, metadata := range identifiedGenMetadata.ClientMetadata {
if bytes.Equal(metadata.Key, ibctmtypes.IterationKey(height)) ||
bytes.Equal(metadata.Key, ibctmtypes.ProcessedTimeKey(height)) ||
bytes.Equal(metadata.Key, ibctmtypes.ProcessedHeightKey(height)) {
// the previous version of IBC only contained the processed time metadata
// if we find the processed time metadata for an unexpired height, add the
// iteration key and processed height keys.
if bytes.Equal(metadata.Key, ibctmtypes.ProcessedTimeKey(height)) {
clientMetadata = append(clientMetadata, types.GenesisMetadata{
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't you do this with just one append call??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wow, I've always worked under the impression append() took only 2 arguments

Key: ibctmtypes.ProcessedHeightKey(height),
Copy link
Member

Choose a reason for hiding this comment

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

Add godoc saying we're setting processed height to self height, which is still safe

Value: []byte(selfHeight.String()),
})
clientMetadata = append(clientMetadata, metadata)
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
}
}
Expand Down
47 changes: 45 additions & 2 deletions modules/core/02-client/legacy/v100/genesis_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package v100_test

import (
"bytes"
"encoding/json"
"fmt"
"time"
Expand Down Expand Up @@ -120,8 +121,29 @@ func (suite *LegacyTestSuite) TestMigrateGenesisSolomachine() {
suite.Require().NoError(err)
expectedClientGenState := ibcclient.ExportGenesis(path.EndpointA.Chain.GetContext(), path.EndpointA.Chain.App.GetIBCKeeper().ClientKeeper)

// 'ExportGenesis' order metadata keys by processedheight, processedtime for all heights, then it appends all iteration keys
// In order to match the genesis migration with export genesis we must reorder the iteration keys to be last
// This isn't ideal, but it is better than modifying the genesis migration from a previous version to match the export genesis of a new version
// which provides no benefit except nicer testing
for i, clientMetadata := range expectedClientGenState.ClientsMetadata {
var updatedMetadata []types.GenesisMetadata
var iterationKeys []types.GenesisMetadata
for _, metadata := range clientMetadata.ClientMetadata {
if bytes.HasPrefix(metadata.Key, []byte(ibctmtypes.KeyIterateConsensusStatePrefix)) {
iterationKeys = append(iterationKeys, metadata)
} else {
updatedMetadata = append(updatedMetadata, metadata)
}
}
updatedMetadata = append(updatedMetadata, iterationKeys...)
expectedClientGenState.ClientsMetadata[i] = types.IdentifiedGenesisMetadata{
ClientId: clientMetadata.ClientId,
ClientMetadata: updatedMetadata,
}
}

// NOTE: genesis time isn't updated since we aren't testing for tendermint consensus state pruning
migrated, err := v100.MigrateGenesis(codec.NewProtoCodec(clientCtx.InterfaceRegistry), &clientGenState, suite.coordinator.CurrentTime)
migrated, err := v100.MigrateGenesis(codec.NewProtoCodec(clientCtx.InterfaceRegistry), &clientGenState, suite.coordinator.CurrentTime, types.GetSelfHeight(suite.chainA.GetContext()))
suite.Require().NoError(err)

bz, err := clientCtx.JSONCodec.MarshalJSON(&expectedClientGenState)
Expand Down Expand Up @@ -199,7 +221,28 @@ func (suite *LegacyTestSuite) TestMigrateGenesisTendermint() {
suite.Require().NoError(err)
expectedClientGenState := ibcclient.ExportGenesis(path1.EndpointA.Chain.GetContext(), path1.EndpointA.Chain.App.GetIBCKeeper().ClientKeeper)

migrated, err := v100.MigrateGenesis(codec.NewProtoCodec(clientCtx.InterfaceRegistry), &clientGenState, suite.coordinator.CurrentTime)
// 'ExportGenesis' order metadata keys by processedheight, processedtime for all heights, then it appends all iteration keys
// In order to match the genesis migration with export genesis we must reorder the iteration keys to be last
// This isn't ideal, but it is better than modifying the genesis migration from a previous version to match the export genesis of a new version
// which provides no benefit except nicer testing
for i, clientMetadata := range expectedClientGenState.ClientsMetadata {
var updatedMetadata []types.GenesisMetadata
var iterationKeys []types.GenesisMetadata
for _, metadata := range clientMetadata.ClientMetadata {
if bytes.HasPrefix(metadata.Key, []byte(ibctmtypes.KeyIterateConsensusStatePrefix)) {
iterationKeys = append(iterationKeys, metadata)
} else {
updatedMetadata = append(updatedMetadata, metadata)
}
}
updatedMetadata = append(updatedMetadata, iterationKeys...)
expectedClientGenState.ClientsMetadata[i] = types.IdentifiedGenesisMetadata{
ClientId: clientMetadata.ClientId,
ClientMetadata: updatedMetadata,
}
}

migrated, err := v100.MigrateGenesis(codec.NewProtoCodec(clientCtx.InterfaceRegistry), &clientGenState, suite.coordinator.CurrentTime, types.GetSelfHeight(suite.chainA.GetContext()))
suite.Require().NoError(err)

// check path 1 client pruning
Expand Down
44 changes: 43 additions & 1 deletion modules/core/02-client/legacy/v100/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,16 @@ func MigrateStore(ctx sdk.Context, storeKey sdk.StoreKey, cdc codec.BinaryCodec)
return sdkerrors.Wrap(err, "failed to unmarshal client state bytes into tendermint client state")
}

if err = ibctmtypes.PruneAllExpiredConsensusStates(ctx, clientStore, cdc, clientState.(*ibctmtypes.ClientState)); err != nil {
tmClientState, ok := clientState.(*ibctmtypes.ClientState)
if !ok {
return sdkerrors.Wrap(types.ErrInvalidClient, "client state is not tendermint even though client id contains 07-tendermint")
}

if err = ibctmtypes.PruneAllExpiredConsensusStates(ctx, clientStore, cdc, tmClientState); err != nil {
return err
}

if err = addConsensusMetadata(ctx, clientStore, cdc, tmClientState); err != nil {
return err
}

Expand Down Expand Up @@ -139,3 +148,36 @@ func pruneSolomachineConsensusStates(clientStore sdk.KVStore) {
clientStore.Delete(host.ConsensusStateKey(height))
}
}

// addConsensusMetadata adds the iteration key and processed height for all unexpired tendermint consensus states
// These keys were not included in the previous release of the IBC module.
func addConsensusMetadata(ctx sdk.Context, clientStore sdk.KVStore, cdc codec.BinaryCodec, clientState *ibctmtypes.ClientState) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided not to modify PruneAllExpiredConsensusStates as that seems like a useful ibctmtypes function by itself. Since the insertion of the iteration key and processed height key is logic specific to this migration, I decided to iterate the consensus state again and do it in a separate iteration. It is less efficient, but I think better code wise. Open to changing if efficiency is a big concern

var heights []exported.Height

metadataCb := func(height exported.Height) bool {
consState, err := ibctmtypes.GetConsensusState(clientStore, cdc, height)
// this error should never occur
if err != nil {
return true
}
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

if !clientState.IsExpired(consState.Timestamp, ctx.BlockTime()) {
heights = append(heights, height)
}

return false
}

if err := ibctmtypes.IterateConsensusStateAscending(clientStore, metadataCb); err != nil {
return err
}

for _, height := range heights {
// set the iteration key and processed height
// these keys were not included in the SDK v0.42.0 release
ibctmtypes.SetProcessedHeight(clientStore, height, clienttypes.GetSelfHeight(ctx))
ibctmtypes.SetIterationKey(clientStore, height)
}

return nil
}
17 changes: 17 additions & 0 deletions modules/core/02-client/legacy/v100/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -185,4 +185,21 @@ func (suite *LegacyTestSuite) TestMigrateStoreTendermint() {
expectedConsKey := ibctmtypes.GetIterationKey(clientStore, pruneHeight)
suite.Require().Nil(expectedConsKey, i)
}

// ensure metadata is set for unexpired consensus state
height := path.EndpointA.GetClientState().GetLatestHeight()
consState, ok := path.EndpointA.Chain.GetConsensusState(path.EndpointA.ClientID, height)
suite.Require().True(ok)
suite.Require().NotNil(consState)

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

processedHeight, ok := ibctmtypes.GetProcessedHeight(clientStore, height)
suite.Require().True(ok)
suite.Require().Equal(types.GetSelfHeight(path.EndpointA.Chain.GetContext()), processedHeight)

consKey := ibctmtypes.GetIterationKey(clientStore, height)
Copy link
Member

Choose a reason for hiding this comment

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

Didn't this metadata already get created in original client update?

I guess you proved metadata did something by checking value of processed height

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, the purpose of this to ensure the unexpired consensus state metadata did not get pruned. I wrote this before adding the addConsensusMetadata function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

found a bug, since I was using the iteration keys for iterating, it wasn't updating the metadata since the iteration keys hadn't been set

changed the structure, set new metadata for all consensus states (using traditional iteration), then prune expired consensus states

suite.Require().Equal(host.ConsensusStateKey(height), consKey)
}