From a358e9f4e017eab6520a96440274f8cc6b7fd2f4 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Tue, 29 Aug 2023 12:58:46 +0300 Subject: [PATCH 1/3] Add 02-client implementation for Recover client. --- modules/core/02-client/keeper/client.go | 48 ++++++ modules/core/02-client/keeper/client_test.go | 160 +++++++++++++++++++ modules/core/02-client/keeper/events.go | 15 ++ modules/core/02-client/types/errors.go | 1 + modules/core/02-client/types/events.go | 1 + 5 files changed, 225 insertions(+) diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go index 34b8ff8c528..ebcd8c307ed 100644 --- a/modules/core/02-client/keeper/client.go +++ b/modules/core/02-client/keeper/client.go @@ -149,3 +149,51 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e return nil } + +func (k Keeper) RecoverClient(ctx sdk.Context, subjectClientID, substituteClientID string) error { + subjectClientState, found := k.GetClientState(ctx, subjectClientID) + if !found { + return errorsmod.Wrapf(types.ErrClientNotFound, "subject client with ID %s", subjectClientID) + } + + subjectClientStore := k.ClientStore(ctx, subjectClientID) + + if status := k.GetClientStatus(ctx, subjectClientState, subjectClientID); status == exported.Active { + return errorsmod.Wrap(types.ErrInvalidRecoveryClient, "cannot recover Active subject client") + } + + substituteClientState, found := k.GetClientState(ctx, substituteClientID) + if !found { + return errorsmod.Wrapf(types.ErrClientNotFound, "substitute client with ID %s", substituteClientID) + } + + if subjectClientState.GetLatestHeight().GTE(substituteClientState.GetLatestHeight()) { + return errorsmod.Wrapf(types.ErrInvalidHeight, "subject client state latest height is greater or equal to substitute client state latest height (%s >= %s)", subjectClientState.GetLatestHeight(), substituteClientState.GetLatestHeight()) + } + + substituteClientStore := k.ClientStore(ctx, substituteClientID) + + if status := k.GetClientStatus(ctx, substituteClientState, substituteClientID); status != exported.Active { + return errorsmod.Wrapf(types.ErrClientNotActive, "substitute client is not Active, status is %s", status) + } + + if err := subjectClientState.CheckSubstituteAndUpdateState(ctx, k.cdc, subjectClientStore, substituteClientStore, substituteClientState); err != nil { + return err + } + + k.Logger(ctx).Info("client recovered", "client-id", subjectClientID) + + defer telemetry.IncrCounterWithLabels( + []string{"ibc", "client", "recover"}, + 1, + []metrics.Label{ + telemetry.NewLabel(types.LabelClientType, substituteClientState.ClientType()), + telemetry.NewLabel(types.LabelClientID, subjectClientID), + }, + ) + + // emitting events in the keeper for recovering clients + emitRecoverClientEvent(ctx, subjectClientID, substituteClientState.ClientType()) + + return nil +} diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go index 99dcd861647..837a3794444 100644 --- a/modules/core/02-client/keeper/client_test.go +++ b/modules/core/02-client/keeper/client_test.go @@ -508,3 +508,163 @@ func (suite *KeeperTestSuite) TestUpdateClientEventEmission() { } suite.Require().True(contains) } + +func (suite *KeeperTestSuite) TestRecoverClient() { + var ( + subject, substitute string + subjectClientState, substituteClientState exported.ClientState + ) + + testCases := []struct { + msg string + malleate func() + expErr error + }{ + { + "success", + func() {}, + nil, + }, + { + "success, subject and substitute use different revision number", + func() { + tmClientState, ok := substituteClientState.(*ibctm.ClientState) + suite.Require().True(ok) + consState, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientConsensusState(suite.chainA.GetContext(), substitute, tmClientState.LatestHeight) + suite.Require().True(found) + newRevisionNumber := tmClientState.GetLatestHeight().GetRevisionNumber() + 1 + + tmClientState.LatestHeight = clienttypes.NewHeight(newRevisionNumber, tmClientState.GetLatestHeight().GetRevisionHeight()) + + suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), substitute, tmClientState.LatestHeight, consState) + clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), substitute) + ibctm.SetProcessedTime(clientStore, tmClientState.LatestHeight, 100) + ibctm.SetProcessedHeight(clientStore, tmClientState.LatestHeight, clienttypes.NewHeight(0, 1)) + suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), substitute, tmClientState) + }, + nil, + }, + { + "subject client does not exist", + func() { + subject = ibctesting.InvalidID + }, + clienttypes.ErrClientNotFound, + }, + { + "subject is Active", + func() { + tmClientState, ok := subjectClientState.(*ibctm.ClientState) + suite.Require().True(ok) + // Set FrozenHeight to zero to ensure client is reported as Active + tmClientState.FrozenHeight = clienttypes.ZeroHeight() + suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), subject, tmClientState) + }, + clienttypes.ErrInvalidRecoveryClient, + }, + { + "substitute client does not exist", + func() { + substitute = ibctesting.InvalidID + }, + clienttypes.ErrClientNotFound, + }, + { + "subject height is equal than substitute height", + func() { + tmClientState, ok := subjectClientState.(*ibctm.ClientState) + suite.Require().True(ok) + tmClientState.LatestHeight = substituteClientState.GetLatestHeight().(clienttypes.Height) + suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), subject, tmClientState) + }, + clienttypes.ErrInvalidHeight, + }, + { + "subject height is greater than substitute height", + func() { + tmClientState, ok := subjectClientState.(*ibctm.ClientState) + suite.Require().True(ok) + tmClientState.LatestHeight = substituteClientState.GetLatestHeight().Increment().(clienttypes.Height) + suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), subject, tmClientState) + }, + clienttypes.ErrInvalidHeight, + }, + { + "substitute is frozen", + func() { + tmClientState, ok := substituteClientState.(*ibctm.ClientState) + suite.Require().True(ok) + tmClientState.FrozenHeight = clienttypes.NewHeight(0, 1) + suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), substitute, tmClientState) + }, + clienttypes.ErrClientNotActive, + }, + { + "CheckSubstituteAndUpdateState fails, substitute client trust level doesn't match subject client trust level", + func() { + tmClientState, ok := substituteClientState.(*ibctm.ClientState) + suite.Require().True(ok) + tmClientState.UnbondingPeriod += time.Minute + suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), substitute, tmClientState) + }, + clienttypes.ErrInvalidSubstitute, + }, + } + + for _, tc := range testCases { + tc := tc + + suite.Run(tc.msg, func() { + suite.SetupTest() // reset + + subjectPath := ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.SetupClients(subjectPath) + subject = subjectPath.EndpointA.ClientID + subjectClientState = suite.chainA.GetClientState(subject) + + substitutePath := ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.SetupClients(substitutePath) + substitute = substitutePath.EndpointA.ClientID + + // update substitute twice + err := substitutePath.EndpointA.UpdateClient() + suite.Require().NoError(err) + err = substitutePath.EndpointA.UpdateClient() + suite.Require().NoError(err) + substituteClientState = suite.chainA.GetClientState(substitute) + + tmClientState, ok := subjectClientState.(*ibctm.ClientState) + suite.Require().True(ok) + tmClientState.AllowUpdateAfterMisbehaviour = true + tmClientState.AllowUpdateAfterExpiry = true + tmClientState.FrozenHeight = tmClientState.LatestHeight + suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), subject, tmClientState) + + tmClientState, ok = substituteClientState.(*ibctm.ClientState) + suite.Require().True(ok) + tmClientState.AllowUpdateAfterMisbehaviour = true + tmClientState.AllowUpdateAfterExpiry = true + + tc.malleate() + + clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), subjectPath.EndpointA.ClientID) + tmClientState = subjectPath.EndpointA.GetClientState().(*ibctm.ClientState) + fmt.Printf("Client status: %s", tmClientState.Status(suite.chainA.GetContext(), clientStore, suite.chainA.App.AppCodec())) + + err = suite.chainA.App.GetIBCKeeper().ClientKeeper.RecoverClient(suite.chainA.GetContext(), subject, substitute) + + expPass := tc.expErr == nil + if expPass { + suite.Require().NoError(err) + + // Assert that client status is now Active + clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), subjectPath.EndpointA.ClientID) + tmClientState := subjectPath.EndpointA.GetClientState().(*ibctm.ClientState) + suite.Require().Equal(tmClientState.Status(suite.chainA.GetContext(), clientStore, suite.chainA.App.AppCodec()), exported.Active) + } else { + suite.Require().Error(err) + suite.Require().ErrorIs(err, tc.expErr) + } + }) + } +} diff --git a/modules/core/02-client/keeper/events.go b/modules/core/02-client/keeper/events.go index 9b0bb6841ba..ce51fe189c5 100644 --- a/modules/core/02-client/keeper/events.go +++ b/modules/core/02-client/keeper/events.go @@ -96,6 +96,21 @@ func emitUpdateClientProposalEvent(ctx sdk.Context, clientID, clientType string) }) } +// emitRecoverClientEvent emits a recover client event +func emitRecoverClientEvent(ctx sdk.Context, clientID, clientType string) { + ctx.EventManager().EmitEvents(sdk.Events{ + sdk.NewEvent( + types.EventTypeRecoverClient, + sdk.NewAttribute(types.AttributeKeySubjectClientID, clientID), + sdk.NewAttribute(types.AttributeKeyClientType, clientType), + ), + sdk.NewEvent( + sdk.EventTypeMessage, + sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory), + ), + }) +} + // emitUpgradeClientProposalEvent emits an upgrade client proposal event func emitUpgradeClientProposalEvent(ctx sdk.Context, title string, height int64) { ctx.EventManager().EmitEvents(sdk.Events{ diff --git a/modules/core/02-client/types/errors.go b/modules/core/02-client/types/errors.go index cdc84871ca0..7809f59d133 100644 --- a/modules/core/02-client/types/errors.go +++ b/modules/core/02-client/types/errors.go @@ -36,4 +36,5 @@ var ( ErrClientNotActive = errorsmod.Register(SubModuleName, 29, "client state is not active") ErrFailedMembershipVerification = errorsmod.Register(SubModuleName, 30, "membership verification failed") ErrFailedNonMembershipVerification = errorsmod.Register(SubModuleName, 31, "non-membership verification failed") + ErrInvalidRecoveryClient = errorsmod.Register(SubModuleName, 32, "invalid recovery client") ) diff --git a/modules/core/02-client/types/events.go b/modules/core/02-client/types/events.go index 9671cf9dcb9..5a32edf32c2 100644 --- a/modules/core/02-client/types/events.go +++ b/modules/core/02-client/types/events.go @@ -28,6 +28,7 @@ var ( EventTypeUpdateClientProposal = "update_client_proposal" EventTypeUpgradeChain = "upgrade_chain" EventTypeUpgradeClientProposal = "upgrade_client_proposal" + EventTypeRecoverClient = "recover_client" AttributeValueCategory = fmt.Sprintf("%s_%s", ibcexported.ModuleName, SubModuleName) ) From 9ce81874e6b3d7b552a2c36e1a8752c86dbaad9c Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Wed, 30 Aug 2023 16:23:29 +0300 Subject: [PATCH 2/3] Partially address feedback. --- modules/core/02-client/keeper/client_test.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go index 837a3794444..1b386e3572d 100644 --- a/modules/core/02-client/keeper/client_test.go +++ b/modules/core/02-client/keeper/client_test.go @@ -570,7 +570,7 @@ func (suite *KeeperTestSuite) TestRecoverClient() { clienttypes.ErrClientNotFound, }, { - "subject height is equal than substitute height", + "subject and substitute have equal latest height", func() { tmClientState, ok := subjectClientState.(*ibctm.ClientState) suite.Require().True(ok) @@ -647,10 +647,6 @@ func (suite *KeeperTestSuite) TestRecoverClient() { tc.malleate() - clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), subjectPath.EndpointA.ClientID) - tmClientState = subjectPath.EndpointA.GetClientState().(*ibctm.ClientState) - fmt.Printf("Client status: %s", tmClientState.Status(suite.chainA.GetContext(), clientStore, suite.chainA.App.AppCodec())) - err = suite.chainA.App.GetIBCKeeper().ClientKeeper.RecoverClient(suite.chainA.GetContext(), subject, substitute) expPass := tc.expErr == nil From a6a42657b9819f09b47c6ea474d6e389d74803b2 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Wed, 30 Aug 2023 18:36:43 +0300 Subject: [PATCH 3/3] Docu RecoverClient, add label, re-use error. --- modules/core/02-client/keeper/client.go | 10 +++++++++- modules/core/02-client/keeper/proposal.go | 2 +- modules/core/02-client/types/errors.go | 3 +-- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go index ebcd8c307ed..b5e49f7f077 100644 --- a/modules/core/02-client/keeper/client.go +++ b/modules/core/02-client/keeper/client.go @@ -150,6 +150,13 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e return nil } +// RecoverClient will retrieve the subject and substitute client. +// A callback will occur to the subject client state with the client +// prefixed store being provided for both the subject and the substitute client. +// The IBC client implementations are responsible for validating the parameters of the +// substitute (ensuring they match the subject's parameters) as well as copying +// the necessary consensus states from the substitute to the subject client +// store. The substitute must be Active and the subject must not be Active. func (k Keeper) RecoverClient(ctx sdk.Context, subjectClientID, substituteClientID string) error { subjectClientState, found := k.GetClientState(ctx, subjectClientID) if !found { @@ -184,11 +191,12 @@ func (k Keeper) RecoverClient(ctx sdk.Context, subjectClientID, substituteClient k.Logger(ctx).Info("client recovered", "client-id", subjectClientID) defer telemetry.IncrCounterWithLabels( - []string{"ibc", "client", "recover"}, + []string{"ibc", "client", "update"}, 1, []metrics.Label{ telemetry.NewLabel(types.LabelClientType, substituteClientState.ClientType()), telemetry.NewLabel(types.LabelClientID, subjectClientID), + telemetry.NewLabel(types.LabelUpdateType, "recovery"), }, ) diff --git a/modules/core/02-client/keeper/proposal.go b/modules/core/02-client/keeper/proposal.go index f35f00d616d..12f2685e299 100644 --- a/modules/core/02-client/keeper/proposal.go +++ b/modules/core/02-client/keeper/proposal.go @@ -28,7 +28,7 @@ func (k Keeper) ClientUpdateProposal(ctx sdk.Context, p *types.ClientUpdatePropo subjectClientStore := k.ClientStore(ctx, p.SubjectClientId) if status := k.GetClientStatus(ctx, subjectClientState, p.SubjectClientId); status == exported.Active { - return errorsmod.Wrap(types.ErrInvalidUpdateClientProposal, "cannot update Active subject client") + return errorsmod.Wrap(types.ErrInvalidRecoveryClient, "cannot update Active subject client") } substituteClientState, found := k.GetClientState(ctx, p.SubstituteClientId) diff --git a/modules/core/02-client/types/errors.go b/modules/core/02-client/types/errors.go index 7809f59d133..3a726378cbc 100644 --- a/modules/core/02-client/types/errors.go +++ b/modules/core/02-client/types/errors.go @@ -28,7 +28,7 @@ var ( ErrFailedNextSeqRecvVerification = errorsmod.Register(SubModuleName, 21, "next sequence receive verification failed") ErrSelfConsensusStateNotFound = errorsmod.Register(SubModuleName, 22, "self consensus state not found") ErrUpdateClientFailed = errorsmod.Register(SubModuleName, 23, "unable to update light client") - ErrInvalidUpdateClientProposal = errorsmod.Register(SubModuleName, 24, "invalid update client proposal") + ErrInvalidRecoveryClient = errorsmod.Register(SubModuleName, 24, "invalid recovery client") ErrInvalidUpgradeClient = errorsmod.Register(SubModuleName, 25, "invalid client upgrade") ErrInvalidHeight = errorsmod.Register(SubModuleName, 26, "invalid height") ErrInvalidSubstitute = errorsmod.Register(SubModuleName, 27, "invalid client state substitute") @@ -36,5 +36,4 @@ var ( ErrClientNotActive = errorsmod.Register(SubModuleName, 29, "client state is not active") ErrFailedMembershipVerification = errorsmod.Register(SubModuleName, 30, "membership verification failed") ErrFailedNonMembershipVerification = errorsmod.Register(SubModuleName, 31, "non-membership verification failed") - ErrInvalidRecoveryClient = errorsmod.Register(SubModuleName, 32, "invalid recovery client") )