fix(lightclient): validate state info and timestamp eagerly, prune la…

…st valid height signer, removes buggy hard fork in progress state (#1594)

ibctesting/light_client_test.go

@@ -467,14 +467,12 @@ func (s *lightClientSuite) TestAfterUpdateState_OptimisticUpdateExists_NotCompat
 // - trigger rollback
 // - validate rollback:
 //   - check if the client is frozen
-//   - validate IsHardForkingInProgress returns true
 //   - validate client updates are blocked
 //   - validate future consensus states are cleared
 //
 // - resolve hard fork
 //   - validate client is unfrozen and hard fork is resolved
 //   - validate the client is updated
-//   - validate the client is not in hard forking state
 //
 // - validate client updates are allowed
 func (s *lightClientSuite) TestAfterUpdateState_Rollback() {
@@ -517,9 +515,14 @@ func (s *lightClientSuite) TestAfterUpdateState_Rollback() {
 	// get number of consensus states before rollback
 	csBeforeRollback := s.hubApp().IBCKeeper.ClientKeeper.GetAllConsensusStates(s.hubCtx())[0].ConsensusStates
 
-	// Trigger rollback
-	rollbackHeight := uint64(s.rollappChain().LastHeader.Header.Height) - 5
-	err := s.hubApp().LightClientKeeper.RollbackCanonicalClient(s.hubCtx(), s.rollappChain().ChainID, rollbackHeight)
+	// Trigger rollback / simulate fork
+	nRolledBack := uint64(5)
+	lastValidHeight := uint64(s.rollappChain().LastHeader.Header.Height) - nRolledBack
+	newRevisionHeight := lastValidHeight + 1
+	ra := s.hubApp().RollappKeeper.MustGetRollapp(s.hubCtx(), s.rollappChain().ChainID)
+	ra.Revisions = append(ra.Revisions, rollapptypes.Revision{StartHeight: newRevisionHeight, Number: 1})
+	s.hubApp().RollappKeeper.SetRollapp(s.hubCtx(), ra)
+	err := s.hubApp().LightClientKeeper.RollbackCanonicalClient(s.hubCtx(), s.rollappChain().ChainID, lastValidHeight)
 	s.Require().NoError(err)
 
 	clientState, found := s.hubApp().IBCKeeper.ClientKeeper.GetClientState(s.hubCtx(), s.path.EndpointA.ClientID)
@@ -530,15 +533,12 @@ func (s *lightClientSuite) TestAfterUpdateState_Rollback() {
 	// Check if the client is frozen
 	s.True(!tmClientState.FrozenHeight.IsZero(), "Client should be frozen after rollback")
 
-	// Check if IsHardForkingInProgress returns true
-	s.True(s.hubApp().LightClientKeeper.IsHardForkingInProgress(s.hubCtx(), s.rollappChain().ChainID), "Rollapp should be in hard forking state")
-
 	// Validate future consensus states are cleared
 	csAfterRollback := s.hubApp().IBCKeeper.ClientKeeper.GetAllConsensusStates(s.hubCtx())[0].ConsensusStates
 	s.Require().Less(len(csAfterRollback), len(csBeforeRollback), "Consensus states should be cleared after rollback")
 	for height := uint64(0); height <= uint64(s.rollappChain().LastHeader.Header.Height); height++ {
 		_, found := s.hubApp().IBCKeeper.ClientKeeper.GetClientConsensusState(s.hubCtx(), s.path.EndpointA.ClientID, clienttypes.NewHeight(1, height))
-		if height > rollbackHeight {
+		if height >= newRevisionHeight {
 			s.False(found, "Consensus state should be cleared for height %d", height)
 		}
 	}
@@ -547,7 +547,7 @@ func (s *lightClientSuite) TestAfterUpdateState_Rollback() {
 	cnt := 0
 	for _, height := range signerHeights {
 		_, err := s.hubApp().LightClientKeeper.GetSigner(s.hubCtx(), s.path.EndpointA.ClientID, uint64(height))
-		if height > int64(rollbackHeight) {
+		if height >= int64(lastValidHeight) {
 			s.Error(err, "Signer should be removed for height %d", height)
 		} else {
 			s.NoError(err, "Signer should not be removed for height %d", height)
@@ -578,6 +578,8 @@ func (s *lightClientSuite) TestAfterUpdateState_Rollback() {
 	s.ErrorIs(err, types.ErrorHardForkInProgress)
 
 	// submit a state info update to resolve the hard fork
+
+	bds.BD = bds.BD[len(bds.BD)-int(nRolledBack):]
 	blockDescriptors := &rollapptypes.BlockDescriptors{BD: bds.BD}
 	msgUpdateState := rollapptypes.NewMsgUpdateState(
 		s.hubChain().SenderAccount.GetAddress().String(),
@@ -587,22 +589,26 @@ func (s *lightClientSuite) TestAfterUpdateState_Rollback() {
 		uint64(len(bds.BD)),
 		blockDescriptors,
 	)
+	msgUpdateState.RollappRevision = 1
 	_, err = s.rollappMsgServer().UpdateState(s.hubCtx(), msgUpdateState)
-	s.Require().NoError(err)
+	s.Require().NoError(err, "update state")
 
 	// Test resolve hard fork
 	clientState, found = s.hubApp().IBCKeeper.ClientKeeper.GetClientState(s.hubCtx(), s.path.EndpointA.ClientID)
 	s.True(found)
 	// Verify that the client is unfrozen and hard fork is resolved
 	s.True(clientState.(*ibctm.ClientState).FrozenHeight.IsZero(), "Client should be unfrozen after hard fork resolution")
-	// Verify that the client is not in hard forking state
-	s.False(s.hubApp().LightClientKeeper.IsHardForkingInProgress(s.hubCtx(), s.rollappChain().ChainID), "Rollapp should not be in hard forking state")
 	// Verify that the client is updated with the height of the first block descriptor
 	s.Require().Equal(bds.BD[0].Height, clientState.GetLatestHeight().GetRevisionHeight())
 	_, ok = s.hubApp().IBCKeeper.ClientKeeper.GetLatestClientConsensusState(s.hubCtx(), s.path.EndpointA.ClientID)
 	s.True(ok)
 
 	// validate client updates are no longer blocked
 	s.coordinator.CommitBlock(s.rollappChain())
+
+	// a bit of a hack to make sure the ibc go testing framework can update, since we can't get inside to pass a revision
+	ra.Revisions = nil
+	s.hubApp().RollappKeeper.SetRollapp(s.hubCtx(), ra)
+
 	s.NoError(s.path.EndpointA.UpdateClient())
 }

proto/dymensionxyz/dymension/lightclient/genesis.proto

@@ -17,7 +17,6 @@ message HeaderSignerEntry {
 message GenesisState {
     repeated CanonicalClient canonical_clients = 1 [ (gogoproto.nullable) = false ];
     repeated HeaderSignerEntry header_signers = 3 [ (gogoproto.nullable) = false ];
-    repeated string hard_fork_keys = 4;
 }
 
 message CanonicalClient {

testutil/keeper/lightclient.go

@@ -182,6 +182,10 @@ func NewMockSequencerKeeper(sequencers map[string]*sequencertypes.Sequencer) *Mo
 
 type MockRollappKeeper struct{}
 
+func (m *MockRollappKeeper) IsFirstHeightOfLatestFork(ctx sdk.Context, rollappId string, revision, height uint64) bool {
+	return false
+}
+
 func (m *MockRollappKeeper) GetLatestHeight(ctx sdk.Context, rollappId string) (uint64, bool) {
 	panic("implement me")
 }

x/delayedack/keeper/fraud.go

@@ -13,11 +13,11 @@ import (
 
 var _ rollapptypes.RollappHooks = &Keeper{}
 
-func (k Keeper) OnHardFork(ctx sdk.Context, rollappID string, newRevisionHeight uint64) error {
+func (k Keeper) OnHardFork(ctx sdk.Context, rollappID string, lastValidHeight uint64) error {
 	logger := ctx.Logger().With("module", "DelayedAckMiddleware")
 
 	// Get all the pending packets from fork height inclusive
-	rollappPendingPackets := k.ListRollappPackets(ctx, types.PendingByRollappIDFromHeight(rollappID, newRevisionHeight))
+	rollappPendingPackets := k.ListRollappPackets(ctx, types.PendingByRollappIDFromHeight(rollappID, lastValidHeight+1))
 
 	// Iterate over all the pending packets and revert them
 	for _, rollappPacket := range rollappPendingPackets {

x/delayedack/keeper/fraud_test.go

@@ -35,7 +35,7 @@ func (suite *DelayedAckTestSuite) TestHandleFraud() {
 	suite.Require().Nil(err)
 
 	// call fraud on the 4 packet
-	err = keeper.OnHardFork(ctx, rollappId, 4)
+	err = keeper.OnHardFork(ctx, rollappId, 3)
 	suite.Require().NoError(err)
 
 	// expected result:

x/dymns/keeper/hooks.go

@@ -71,7 +71,7 @@ func (h rollappHooks) BeforeUpdateState(_ sdk.Context, _ string, _ string, _ boo
 	return nil
 }
 
-func (h rollappHooks) AfterUpdateState(_ sdk.Context, _ string, _ *rollapptypes.StateInfo) error {
+func (h rollappHooks) AfterUpdateState(ctx sdk.Context, stateInfo *rollapptypes.StateInfoMeta) error {
 	return nil
 }
 

x/lightclient/ante/ibc_msg_update_client.go

@@ -63,11 +63,6 @@ func (i IBCMessagesDecorator) HandleMsgUpdateClient(ctx sdk.Context, msg *ibccli
 		return gerrc.ErrInternal.Wrapf("get rollapp from sequencer: rollapp: %s", seq.RollappId)
 	}
 
-	// cannot update the LC unless fork is resolved (after receiving state post fork state update)
-	if i.k.IsHardForkingInProgress(ctx, rollapp.RollappId) {
-		return types.ErrorHardForkInProgress
-	}
-
 	// this disallows LC updates from previous revisions but should be fine since new state roots can be used to prove
 	// state older than the one in the current state root.
 	if header.Header.Version.App != rollapp.LatestRevision().Number {

x/lightclient/ante/ibc_msgs_test.go

@@ -17,8 +17,11 @@ type MockRollappKeeper struct {
 	stateInfos map[string]map[uint64]rollapptypes.StateInfo
 }
 
+func (m *MockRollappKeeper) IsFirstHeightOfLatestFork(ctx sdk.Context, rollappId string, revision, height uint64) bool {
+	panic("implement me")
+}
+
 func (m *MockRollappKeeper) GetLatestHeight(ctx sdk.Context, rollappId string) (uint64, bool) {
-	// TODO implement me
 	panic("implement me")
 }
 

x/lightclient/keeper/client_store.go

@@ -22,6 +22,13 @@ func getClientState(clientStore sdk.KVStore, cdc codec.BinaryCodec) exported.Cli
 	return clienttypes.MustUnmarshalClientState(cdc, bz)
 }
 
+// must be tendermint!
+func getClientStateTM(clientStore sdk.KVStore, cdc codec.BinaryCodec) *ibctm.ClientState {
+	c := getClientState(clientStore, cdc)
+	tmClientState, _ := c.(*ibctm.ClientState)
+	return tmClientState
+}
+
 // setClientState stores the client state
 func setClientState(clientStore sdk.KVStore, cdc codec.BinaryCodec, clientState exported.ClientState) {
 	key := host.ClientStateKey()

x/lightclient/keeper/genesis.go

@@ -19,18 +19,13 @@ func (k Keeper) InitGenesis(ctx sdk.Context, genesisState types.GenesisState) {
 			panic(err)
 		}
 	}
-	for _, rollappID := range genesisState.HardForkKeys {
-		k.SetHardForkInProgress(ctx, rollappID)
-	}
 }
 
 func (k Keeper) ExportGenesis(ctx sdk.Context) types.GenesisState {
 	clients := k.GetAllCanonicalClients(ctx)
-	hardForkKeys := k.ListHardForkKeys(ctx)
 
 	ret := types.GenesisState{
 		CanonicalClients: clients,
-		HardForkKeys:     hardForkKeys,
 	}
 
 	if err := k.headerSigners.Walk(ctx, nil,

x/lightclient/keeper/genesis_test.go

@@ -19,7 +19,6 @@ func TestInitGenesis(t *testing.T) {
 
 	keeper.InitGenesis(ctx, types.GenesisState{
 		CanonicalClients: clients,
-		HardForkKeys:     []string{"rollapp-1", "rollapp-2"},
 	})
 
 	ibc, found := keeper.GetCanonicalClient(ctx, "rollapp-1")
@@ -28,19 +27,13 @@ func TestInitGenesis(t *testing.T) {
 	ibc, found = keeper.GetCanonicalClient(ctx, "rollapp-2")
 	require.True(t, found)
 	require.Equal(t, "client-2", ibc)
-	hfks := keeper.ListHardForkKeys(ctx)
-	require.Len(t, hfks, 2)
-	require.Equal(t, "rollapp-1", hfks[0])
-	require.Equal(t, "rollapp-2", hfks[1])
 }
 
 func TestExportGenesis(t *testing.T) {
 	keeper, ctx := keepertest.LightClientKeeper(t)
 
 	keeper.SetCanonicalClient(ctx, "rollapp-1", "client-1")
 	keeper.SetCanonicalClient(ctx, "rollapp-2", "client-2")
-	keeper.SetHardForkInProgress(ctx, "rollapp-1")
-	keeper.SetHardForkInProgress(ctx, "rollapp-2")
 
 	genesis := keeper.ExportGenesis(ctx)
 
@@ -49,9 +42,6 @@ func TestExportGenesis(t *testing.T) {
 	require.Equal(t, "client-2", genesis.CanonicalClients[1].IbcClientId)
 	require.Equal(t, "rollapp-1", genesis.CanonicalClients[0].RollappId)
 	require.Equal(t, "rollapp-2", genesis.CanonicalClients[1].RollappId)
-	require.Len(t, genesis.HardForkKeys, 2)
-	require.Equal(t, "rollapp-1", genesis.HardForkKeys[0])
-	require.Equal(t, "rollapp-2", genesis.HardForkKeys[1])
 }
 
 func TestImportExportGenesis(t *testing.T) {
@@ -80,7 +70,6 @@ func TestImportExportGenesis(t *testing.T) {
 				Height:           43,
 			},
 		},
-		HardForkKeys: []string{"rollapp-1", "rollapp-2"},
 	}
 
 	k.InitGenesis(ctx, g)

x/lightclient/keeper/hook_listener.go

@@ -30,40 +30,35 @@ func (k Keeper) RollappHooks() rollapptypes.RollappHooks {
 // AfterUpdateState is called after a state update is made to a rollapp.
 // This hook checks if the rollapp has a canonical IBC light client and if the Consensus state is compatible with the state update
 // and punishes the sequencer if it is not
-func (hook rollappHook) AfterUpdateState(
-	ctx sdk.Context,
-	rollappId string,
-	stateInfo *rollapptypes.StateInfo,
-) error {
+func (hook rollappHook) AfterUpdateState(ctx sdk.Context, stateInfoM *rollapptypes.StateInfoMeta) error {
 	if !hook.k.Enabled() {
 		return nil
 	}
+	rollappID := stateInfoM.Rollapp
+	stateInfo := &stateInfoM.StateInfo
 
-	client, ok := hook.k.GetCanonicalClient(ctx, rollappId)
+	client, ok := hook.k.GetCanonicalClient(ctx, rollappID)
 	if !ok {
 		return nil
 	}
 
-	// first state after hardfork, should reset the client to active state
-	if hook.k.IsHardForkingInProgress(ctx, rollappId) {
-		err := hook.k.ResolveHardFork(ctx, rollappId)
-		if err != nil {
-			return errorsmod.Wrap(err, "resolve hard fork")
-		}
-		return nil
+	if hook.k.rollappKeeper.IsFirstHeightOfLatestFork(ctx, rollappID, stateInfoM.Revision, stateInfo.GetStartHeight()) {
+		return errorsmod.Wrap(hook.k.ResolveHardFork(ctx, rollappID), "resolve hard fork")
 	}
 
-	// TODO: check hard fork in progress here
-
 	seq, err := hook.k.SeqK.RealSequencer(ctx, stateInfo.Sequencer)
 	if err != nil {
 		return errorsmod.Wrap(errors.Join(gerrc.ErrInternal, err), "get sequencer for state info")
 	}
 
-	// [hStart-1..,hEnd) is correct because we compare against a next validators hash
-	for h := stateInfo.GetStartHeight() - 1; h < stateInfo.GetLatestHeight(); h++ {
-		if err := hook.validateOptimisticUpdate(ctx, rollappId, client, seq, stateInfo, h); err != nil {
-			return errorsmod.Wrap(err, "validate optimistic update")
+	// [hStart-1..,hEnd] is correct because we compare against a next validators hash
+	// for all heights in the range [hStart-1..hEnd), but do not for hEnd
+	for h := stateInfo.GetStartHeight() - 1; h <= stateInfo.GetLatestHeight(); h++ {
+		if err := hook.validateOptimisticUpdate(ctx, rollappID, client, seq, stateInfo, h); err != nil {
+			if errors.Is(err, types.ErrNextValHashMismatch) && h == stateInfo.GetLatestHeight() {
+				continue
+			}
+			return errorsmod.Wrapf(err, "validate optimistic update: height: %d", h)
 		}
 	}
 
@@ -92,7 +87,7 @@ func (hook rollappHook) validateOptimisticUpdate(
 	}
 	expectBD, err := hook.getBlockDescriptor(ctx, rollapp, cache, h)
 	if err != nil {
-		return err
+		return errorsmod.Wrap(err, "get block descriptor")
 	}
 	expect := types.RollappState{
 		BlockDescriptor:    expectBD,
@@ -101,7 +96,7 @@ func (hook rollappHook) validateOptimisticUpdate(
 
 	err = types.CheckCompatibility(*got, expect)
 	if err != nil {
-		return errors.Join(gerrc.ErrFault, err)
+		return errorsmod.Wrap(err, "check compatibility")
 	}
 
 	// everything is fine

x/lightclient/keeper/hook_listener_test.go

@@ -22,9 +22,6 @@ func TestAfterUpdateState(t *testing.T) {
 		prepare   func(ctx sdk.Context, k lightClientKeeper.Keeper) input
 		expectErr bool
 	}{
-		// TODO: tests need expanding
-		//  At least the following need to be added
-		//  - Client with all cons states after the state update will not be canonical
 		{
 			name: "canonical client does not exist for rollapp",
 			prepare: func(ctx sdk.Context, k lightClientKeeper.Keeper) input {
@@ -37,7 +34,7 @@ func TestAfterUpdateState(t *testing.T) {
 		},
 
 		{
-			name: "both states are not compatible - slash the sequencer who signed",
+			name: "incompatible",
 			prepare: func(ctx sdk.Context, k lightClientKeeper.Keeper) input {
 				k.SetCanonicalClient(ctx, keepertest.DefaultRollapp, keepertest.CanonClientID)
 				err := k.SaveSigner(ctx, keepertest.Alice.Address, keepertest.CanonClientID, 2)
@@ -115,7 +112,11 @@ func TestAfterUpdateState(t *testing.T) {
 			keeper, ctx := keepertest.LightClientKeeper(t)
 			input := tc.prepare(ctx, *keeper)
 
-			err := keeper.RollappHooks().AfterUpdateState(ctx, input.rollappId, input.stateInfo)
+			err := keeper.RollappHooks().AfterUpdateState(ctx, &rollapptypes.StateInfoMeta{
+				StateInfo: *input.stateInfo,
+				Revision:  0, /// TODO:
+				Rollapp:   keepertest.DefaultRollapp,
+			})
 			if tc.expectErr {
 				require.Error(t, err)
 			} else {

x/lightclient/keeper/keeper.go

@@ -9,7 +9,6 @@ import (
 	errorsmod "cosmossdk.io/errors"
 	"github.com/cometbft/cometbft/libs/log"
 	"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"
 	ibcclienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
@@ -153,31 +152,6 @@ func (k Keeper) ExpectedClientState(context.Context, *types.QueryExpectedClientS
 	return &types.QueryExpectedClientStateResponse{ClientState: anyClient}, nil
 }
 
-func (k Keeper) SetHardForkInProgress(ctx sdk.Context, rollappID string) {
-	ctx.KVStore(k.storeKey).Set(types.HardForkKey(rollappID), []byte{0x01})
-}
-
-// remove the hardfork key from the store
-func (k Keeper) setHardForkResolved(ctx sdk.Context, rollappID string) {
-	ctx.KVStore(k.storeKey).Delete(types.HardForkKey(rollappID))
-}
-
-// checks if rollapp is hard forking
-func (k Keeper) IsHardForkingInProgress(ctx sdk.Context, rollappID string) bool {
-	return ctx.KVStore(k.storeKey).Has(types.HardForkKey(rollappID))
-}
-
-func (k Keeper) ListHardForkKeys(ctx sdk.Context) []string {
-	prefixStore := prefix.NewStore(ctx.KVStore(k.storeKey), types.HardForkPrefix)
-	iter := prefixStore.Iterator(nil, nil)
-	defer iter.Close() // nolint: errcheck
-	var ret []string
-	for ; iter.Valid(); iter.Next() {
-		ret = append(ret, string(iter.Key()))
-	}
-	return ret
-}
-
 func (k Keeper) pruneSigners(ctx sdk.Context, client string, h uint64, isAbove bool) error {
 	var rng *collections.PairRange[string, uint64]
 	if isAbove {