From 8e1309bd7bd176495697c8608894da97f71ac2ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 14 Mar 2022 17:47:42 +0100 Subject: [PATCH 1/6] rename update to UpdateState rename 07-tendermint update function to UpdateState add pruneOldestConsensusState function add check for duplicate update --- .../07-tendermint/types/update.go | 87 ++++++++++++------- 1 file changed, 54 insertions(+), 33 deletions(-) diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go index c4d422ccb73..debd5dcef8c 100644 --- a/modules/light-clients/07-tendermint/types/update.go +++ b/modules/light-clients/07-tendermint/types/update.go @@ -2,6 +2,7 @@ package types import ( "bytes" + "fmt" "reflect" "time" @@ -110,35 +111,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 +220,24 @@ 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 sets the consensus state associated with the provided header and sets the consensus metadata. +func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, msg exported.ClientMessage) (*ClientState, *ConsensusState, error) { + header, ok := msg.(*Header) + if !ok { + panic(fmt.Sprintf("client state can only be updated with a Header: expected %T, got %T)", &Header{}, msg)) + } + + // check for duplicate update + if consensusState, err := GetConsensusState(clientStore, cdc, header.GetHeight()); err != 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 +248,37 @@ 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 attempts to prune the oldest consensus state. The consensus state will only be pruned +// if it is expired. +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) + } } From 6658bea79e4bf47c3ba5b8afe2d118025153fef1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 16 Mar 2022 13:41:52 +0100 Subject: [PATCH 2/6] fix: duplicate update check was performing incorrect logic --- modules/light-clients/07-tendermint/types/update.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go index 71e737ecb99..9012861b78e 100644 --- a/modules/light-clients/07-tendermint/types/update.go +++ b/modules/light-clients/07-tendermint/types/update.go @@ -228,7 +228,7 @@ func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, client } // check for duplicate update - if consensusState, err := GetConsensusState(clientStore, cdc, header.GetHeight()); err != nil { + if consensusState, _ := GetConsensusState(clientStore, cdc, header.GetHeight()); consensusState != nil { // perform no-op return &cs, consensusState, nil } From 4863e984855c4028010b48906dadfc547a36fa1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 16 Mar 2022 13:47:35 +0100 Subject: [PATCH 3/6] update godoc --- .../07-tendermint/types/update.go | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go index 9012861b78e..63b9cdfdba3 100644 --- a/modules/light-clients/07-tendermint/types/update.go +++ b/modules/light-clients/07-tendermint/types/update.go @@ -29,14 +29,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). // @@ -46,10 +38,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, @@ -221,6 +209,15 @@ func checkValidity( } // UpdateState sets the consensus state associated with the provided header and sets the consensus metadata. +// 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, msg exported.ClientMessage) (*ClientState, *ConsensusState, error) { header, ok := msg.(*Header) if !ok { @@ -251,8 +248,9 @@ func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, client return &cs, consensusState, nil } -// pruneOldestConsensusState attempts to prune the oldest consensus state. The consensus state will only be pruned -// if it is expired. +// 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. From 3e07080bde0a9bfc4301dbdc3b5351086a147aa5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 16 Mar 2022 14:58:10 +0100 Subject: [PATCH 4/6] add UpdateState tests --- .../07-tendermint/types/update.go | 7 +- .../07-tendermint/types/update_test.go | 143 ++++++++++++++++++ 2 files changed, 146 insertions(+), 4 deletions(-) diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go index 63b9cdfdba3..42febb50ab8 100644 --- a/modules/light-clients/07-tendermint/types/update.go +++ b/modules/light-clients/07-tendermint/types/update.go @@ -2,7 +2,6 @@ package types import ( "bytes" - "fmt" "reflect" "time" @@ -218,10 +217,10 @@ func checkValidity( // 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, msg exported.ClientMessage) (*ClientState, *ConsensusState, error) { - header, ok := msg.(*Header) +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 { - panic(fmt.Sprintf("client state can only be updated with a Header: expected %T, got %T)", &Header{}, msg)) + return nil, nil, sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "expected type %T, got %T", &Header{}, header) } // check for duplicate update diff --git a/modules/light-clients/07-tendermint/types/update_test.go b/modules/light-clients/07-tendermint/types/update_test.go index 77c993dff72..cb7cb4c5e4a 100644 --- a/modules/light-clients/07-tendermint/types/update_test.go +++ b/modules/light-clients/07-tendermint/types/update_test.go @@ -373,6 +373,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) From 8c573de5ab8d7cece188bc817b89a8e07ab225bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 16 Mar 2022 15:00:38 +0100 Subject: [PATCH 5/6] update godoc --- modules/light-clients/07-tendermint/types/update.go | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go index 42febb50ab8..77573ea8901 100644 --- a/modules/light-clients/07-tendermint/types/update.go +++ b/modules/light-clients/07-tendermint/types/update.go @@ -207,7 +207,6 @@ func checkValidity( return nil } -// UpdateState sets the consensus state associated with the provided header and sets the consensus metadata. // 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 From 6e63509e7a618f9e79ea3ea68473d7ee51dbf83a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 17 Mar 2022 13:25:21 +0100 Subject: [PATCH 6/6] chore: fix code spacing --- modules/light-clients/07-tendermint/types/update.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go index 77573ea8901..7497307ae84 100644 --- a/modules/light-clients/07-tendermint/types/update.go +++ b/modules/light-clients/07-tendermint/types/update.go @@ -256,6 +256,7 @@ func (cs ClientState) pruneOldestConsensusState(ctx sdk.Context, cdc codec.Binar pruneHeight exported.Height pruneError error ) + pruneCb := func(height exported.Height) bool { consState, err := GetConsensusState(clientStore, cdc, height) // this error should never occur @@ -263,15 +264,19 @@ func (cs ClientState) pruneOldestConsensusState(ctx sdk.Context, cdc codec.Binar 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)