From 9e7a3179938b0bdd5054a116bc77f10264d68c33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 24 Mar 2022 15:29:29 +0100 Subject: [PATCH] 02-client-refactor: rename update to UpdateState for 07-tendermint (#1117) * rename update to UpdateState rename 07-tendermint update function to UpdateState add pruneOldestConsensusState function add check for duplicate update * fix: duplicate update check was performing incorrect logic * update godoc * add UpdateState tests * update godoc * chore: fix code spacing Co-authored-by: Sean King --- .../07-tendermint/types/update.go | 112 ++++++++------ .../07-tendermint/types/update_test.go | 143 ++++++++++++++++++ 2 files changed, 210 insertions(+), 45 deletions(-) diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go index 2699ebc2cc9..7497307ae84 100644 --- a/modules/light-clients/07-tendermint/types/update.go +++ b/modules/light-clients/07-tendermint/types/update.go @@ -28,14 +28,6 @@ import ( // - header timestamp is past the trusting period in relation to the consensus state // - header timestamp is less than or equal to the consensus state timestamp // -// UpdateClient may be used to either create a consensus state for: -// - a future height greater than the latest client state height -// - a past height that was skipped during bisection -// If we are updating to a past height, a consensus state is created for that height to be persisted in client store -// If we are updating to a future height, the consensus state is created and the client state is updated to reflect -// the new latest height -// UpdateClient must only be used to update within a single revision, thus header revision number and trusted height's revision -// number must be the same. To update to a new revision, use a separate upgrade path // Tendermint client validity checking uses the bisection algorithm described // in the [Tendermint spec](https://github.com/tendermint/spec/blob/master/spec/consensus/light-client.md). // @@ -45,10 +37,6 @@ import ( // 2. Any valid update that breaks time monotonicity with respect to its neighboring consensus states is evidence of misbehaviour and will freeze client. // Misbehaviour sets frozen height to {0, 1} since it is only used as a boolean value (zero or non-zero). // -// Pruning: -// UpdateClient will additionally retrieve the earliest consensus state for this clientID and check if it is expired. If it is, -// that consensus state will be pruned from store along with all associated metadata. This will prevent the client store from -// becoming bloated with expired consensus states that can no longer be used for updates and packet verification. func (cs ClientState) CheckHeaderAndUpdateState( ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, header exported.ClientMessage, @@ -110,35 +98,10 @@ func (cs ClientState) CheckHeaderAndUpdateState( return &cs, consState, nil } - // Check the earliest consensus state to see if it is expired, if so then set the prune height - // so that we can delete consensus state and all associated metadata. - var ( - pruneHeight exported.Height - pruneError error - ) - pruneCb := func(height exported.Height) bool { - consState, err := GetConsensusState(clientStore, cdc, height) - // this error should never occur - if err != nil { - pruneError = err - return true - } - if cs.IsExpired(consState.Timestamp, ctx.BlockTime()) { - pruneHeight = height - } - return true - } - IterateConsensusStateAscending(clientStore, pruneCb) - if pruneError != nil { - return nil, nil, pruneError - } - // if pruneHeight is set, delete consensus state and metadata - if pruneHeight != nil { - deleteConsensusState(clientStore, pruneHeight) - deleteConsensusMetadata(clientStore, pruneHeight) + newClientState, consensusState, err := cs.UpdateState(ctx, cdc, clientStore, tmHeader) + if err != nil { + return nil, nil, err } - - newClientState, consensusState := update(ctx, clientStore, &cs, tmHeader) return newClientState, consensusState, nil } @@ -244,11 +207,32 @@ func checkValidity( return nil } -// update the consensus state from a new header and set processed time metadata -func update(ctx sdk.Context, clientStore sdk.KVStore, clientState *ClientState, header *Header) (*ClientState, *ConsensusState) { +// UpdateState may be used to either create a consensus state for: +// - a future height greater than the latest client state height +// - a past height that was skipped during bisection +// If we are updating to a past height, a consensus state is created for that height to be persisted in client store +// If we are updating to a future height, the consensus state is created and the client state is updated to reflect +// the new latest height +// UpdateState must only be used to update within a single revision, thus header revision number and trusted height's revision +// number must be the same. To update to a new revision, use a separate upgrade path +// UpdateState will prune the oldest consensus state if it is expired. +func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, clientMsg exported.ClientMessage) (*ClientState, *ConsensusState, error) { + header, ok := clientMsg.(*Header) + if !ok { + return nil, nil, sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "expected type %T, got %T", &Header{}, header) + } + + // check for duplicate update + if consensusState, _ := GetConsensusState(clientStore, cdc, header.GetHeight()); consensusState != nil { + // perform no-op + return &cs, consensusState, nil + } + + cs.pruneOldestConsensusState(ctx, cdc, clientStore) + height := header.GetHeight().(clienttypes.Height) - if height.GT(clientState.LatestHeight) { - clientState.LatestHeight = height + if height.GT(cs.LatestHeight) { + cs.LatestHeight = height } consensusState := &ConsensusState{ Timestamp: header.GetTime(), @@ -259,5 +243,43 @@ func update(ctx sdk.Context, clientStore sdk.KVStore, clientState *ClientState, // set metadata for this consensus state setConsensusMetadata(ctx, clientStore, header.GetHeight()) - return clientState, consensusState + return &cs, consensusState, nil +} + +// pruneOldestConsensusState will retrieve the earliest consensus state for this clientID and check if it is expired. If it is, +// that consensus state will be pruned from store along with all associated metadata. This will prevent the client store from +// becoming bloated with expired consensus states that can no longer be used for updates and packet verification. +func (cs ClientState) pruneOldestConsensusState(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore) { + // Check the earliest consensus state to see if it is expired, if so then set the prune height + // so that we can delete consensus state and all associated metadata. + var ( + pruneHeight exported.Height + pruneError error + ) + + pruneCb := func(height exported.Height) bool { + consState, err := GetConsensusState(clientStore, cdc, height) + // this error should never occur + if err != nil { + pruneError = err + return true + } + + if cs.IsExpired(consState.Timestamp, ctx.BlockTime()) { + pruneHeight = height + } + + return true + } + + IterateConsensusStateAscending(clientStore, pruneCb) + if pruneError != nil { + panic(pruneError) + } + + // if pruneHeight is set, delete consensus state and metadata + if pruneHeight != nil { + deleteConsensusState(clientStore, pruneHeight) + deleteConsensusMetadata(clientStore, pruneHeight) + } } diff --git a/modules/light-clients/07-tendermint/types/update_test.go b/modules/light-clients/07-tendermint/types/update_test.go index 379ccba5e00..1fd5a5253d1 100644 --- a/modules/light-clients/07-tendermint/types/update_test.go +++ b/modules/light-clients/07-tendermint/types/update_test.go @@ -367,6 +367,149 @@ func (suite *TendermintTestSuite) TestCheckHeaderAndUpdateState() { } } +func (suite *TendermintTestSuite) TestUpdateState() { + var ( + path *ibctesting.Path + clientMessage exported.ClientMessage + pruneHeight clienttypes.Height + updatedClientState *types.ClientState // TODO: retrieve from state after 'UpdateState' call + updatedConsensusState *types.ConsensusState // TODO: retrieve from state after 'UpdateState' call + ) + + testCases := []struct { + name string + malleate func() + expResult func() + expPass bool + }{ + { + "success with height later than latest height", func() { + suite.Require().True(path.EndpointA.GetClientState().GetLatestHeight().LT(clientMessage.GetHeight())) + }, + func() { + suite.Require().True(path.EndpointA.GetClientState().GetLatestHeight().LT(updatedClientState.GetLatestHeight())) // new update, updated client state should have changed + }, true, + }, + { + "success with height earlier than latest height", func() { + // commit a block so the pre-created ClientMessage + // isn't used to update the client to a newer height + suite.coordinator.CommitBlock(suite.chainB) + err := path.EndpointA.UpdateClient() + suite.Require().NoError(err) + + suite.Require().True(path.EndpointA.GetClientState().GetLatestHeight().GT(clientMessage.GetHeight())) + }, + func() { + suite.Require().Equal(path.EndpointA.GetClientState(), updatedClientState) // fill in height, no change to client state + }, true, + }, + { + "success with duplicate header", func() { + // update client in advance + err := path.EndpointA.UpdateClient() + suite.Require().NoError(err) + + // use the same header which just updated the client + clientMessage, err = path.EndpointA.Chain.ConstructUpdateTMClientHeader(path.EndpointA.Counterparty.Chain, path.EndpointA.ClientID) + suite.Require().NoError(err) + suite.Require().Equal(path.EndpointA.GetClientState().GetLatestHeight(), clientMessage.GetHeight()) + }, + func() { + suite.Require().Equal(path.EndpointA.GetClientState(), updatedClientState) + suite.Require().Equal(path.EndpointA.GetConsensusState(clientMessage.GetHeight()), updatedConsensusState) + }, true, + }, + { + "success with pruned consensus state", func() { + // this height will be expired and pruned + err := path.EndpointA.UpdateClient() + suite.Require().NoError(err) + pruneHeight = path.EndpointA.GetClientState().GetLatestHeight().(clienttypes.Height) + + // Increment the time by a week + suite.coordinator.IncrementTimeBy(7 * 24 * time.Hour) + + // create the consensus state that can be used as trusted height for next update + err = path.EndpointA.UpdateClient() + suite.Require().NoError(err) + + // Increment the time by another week, then update the client. + // This will cause the first two consensus states to become expired. + suite.coordinator.IncrementTimeBy(7 * 24 * time.Hour) + err = path.EndpointA.UpdateClient() + suite.Require().NoError(err) + + // ensure counterparty state is committed + suite.coordinator.CommitBlock(suite.chainB) + clientMessage, err = path.EndpointA.Chain.ConstructUpdateTMClientHeader(path.EndpointA.Counterparty.Chain, path.EndpointA.ClientID) + suite.Require().NoError(err) + }, + func() { + suite.Require().True(path.EndpointA.GetClientState().GetLatestHeight().LT(updatedClientState.GetLatestHeight())) // new update, updated client state should have changed + + // ensure consensus state was pruned + _, found := path.EndpointA.Chain.GetConsensusState(path.EndpointA.ClientID, pruneHeight) + suite.Require().False(found) + }, true, + }, + { + "invalid ClientMessage type", func() { + clientMessage = &types.Misbehaviour{} + }, + func() {}, false, + }, + } + for _, tc := range testCases { + tc := tc + suite.Run(tc.name, func() { + suite.SetupTest() // reset + pruneHeight = clienttypes.ZeroHeight() + path = ibctesting.NewPath(suite.chainA, suite.chainB) + + err := path.EndpointA.CreateClient() + suite.Require().NoError(err) + + // ensure counterparty state is committed + suite.coordinator.CommitBlock(suite.chainB) + clientMessage, err = path.EndpointA.Chain.ConstructUpdateTMClientHeader(path.EndpointA.Counterparty.Chain, path.EndpointA.ClientID) + suite.Require().NoError(err) + + tc.malleate() + + clientState := path.EndpointA.GetClientState() + + // TODO: remove casting when 'UpdateState' is an interface function. + tmClientState, ok := clientState.(*types.ClientState) + suite.Require().True(ok) + + clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), path.EndpointA.ClientID) + updatedClientState, updatedConsensusState, err = tmClientState.UpdateState(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), clientStore, clientMessage) + + if tc.expPass { + suite.Require().NoError(err) + + header := clientMessage.(*types.Header) + expConsensusState := &types.ConsensusState{ + Timestamp: header.GetTime(), + Root: commitmenttypes.NewMerkleRoot(header.Header.GetAppHash()), + NextValidatorsHash: header.Header.NextValidatorsHash, + } + suite.Require().Equal(expConsensusState, updatedConsensusState) + + } else { + suite.Require().Error(err) + suite.Require().Nil(updatedClientState) + suite.Require().Nil(updatedConsensusState) + + } + + // perform custom checks + tc.expResult() + }) + } +} + func (suite *TendermintTestSuite) TestPruneConsensusState() { // create path and setup clients path := ibctesting.NewPath(suite.chainA, suite.chainB)