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.

Sign up for GitHub

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

IsFrozen() changed to Status() #140

Merged
merged 23 commits into from
May 4, 2021
Merged

IsFrozen() changed to Status() #140

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
8a5ab72
initial work, pause for feedback
colin-axner Apr 9, 2021
7afeca9
Merge branch 'main' of github.com:cosmos/ibc-go into colin/98-client-…
colin-axner Apr 22, 2021
0db05e6
IsFrozen() -> Status()
colin-axner Apr 26, 2021
47448e1
Merge branch 'main' of github.com:cosmos/ibc-go into colin/98-client-…
colin-axner Apr 26, 2021
7899060
fix bug
colin-axner Apr 26, 2021
400e719
fix tests
colin-axner Apr 26, 2021
e34a9af
remove typo
colin-axner Apr 26, 2021
9c4c899
add verify tests
colin-axner Apr 27, 2021
dc26cd3
error message and code cleanup
colin-axner Apr 27, 2021
94cafbe
self review fixes
colin-axner Apr 27, 2021
3ed46b2
Update modules/core/02-client/keeper/client.go
colin-axner Apr 27, 2021
2ae5339
Merge branch 'main' into colin/98-client-status
colin-axner Apr 27, 2021
ffd9e1b
add gRPC route to proto
colin-axner Apr 28, 2021
2f67828
add gRPC route and tests
colin-axner Apr 28, 2021
ea8644a
update changelog
colin-axner Apr 28, 2021
cb0eb3f
apply review suggestions
colin-axner Apr 28, 2021
0ac6588
Update modules/light-clients/06-solomachine/types/client_state_test.go
colin-axner Apr 28, 2021
8c4b1f1
code ordering
colin-axner Apr 28, 2021
d3e203c
Merge branch 'colin/98-client-status' of github.com:cosmos/ibc-go int…
colin-axner Apr 28, 2021
470dff8
Merge branch 'main' into colin/98-client-status
colin-axner Apr 29, 2021
90c9f29
add set consensus state helper function
colin-axner Apr 29, 2021
cca717d
use typed string for status
colin-axner May 3, 2021
2ad2e25
Merge branch 'main' into colin/98-client-status
colin-axner May 4, 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
26 changes: 15 additions & 11 deletions modules/core/02-client/keeper/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,13 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
return sdkerrors.Wrapf(types.ErrClientNotFound, "cannot update client with ID %s", clientID)
}

// prevent update if the client is frozen before or at header height
if clientState.IsFrozen() && clientState.GetFrozenHeight().LTE(header.GetHeight()) {
return sdkerrors.Wrapf(types.ErrClientFrozen, "cannot update client with ID %s", clientID)
clientStore := k.ClientStore(ctx, clientID)

if status := clientState.Status(ctx, clientStore, k.cdc); status != exported.Active {
return sdkerrors.Wrapf(types.ErrClientNotActive, "cannot update client (%s) with status %s", clientID, status)
}

clientState, consensusState, err := clientState.CheckHeaderAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, clientID), header)
clientState, consensusState, err := clientState.CheckHeaderAndUpdateState(ctx, k.cdc, clientStore, header)
if err != nil {
return sdkerrors.Wrapf(err, "cannot update client with ID %s", clientID)
}
Expand Down Expand Up @@ -130,12 +131,13 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e
return sdkerrors.Wrapf(types.ErrClientNotFound, "cannot update client with ID %s", clientID)
}

// prevent upgrade if current client is frozen
if clientState.IsFrozen() {
return sdkerrors.Wrapf(types.ErrClientFrozen, "cannot update client with ID %s", clientID)
clientStore := k.ClientStore(ctx, clientID)

if status := clientState.Status(ctx, clientStore, k.cdc); status != exported.Active {
return sdkerrors.Wrapf(types.ErrClientNotActive, "cannot upgrade client (%s) with status %s", clientID, status)
}

updatedClientState, updatedConsState, err := clientState.VerifyUpgradeAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, clientID),
updatedClientState, updatedConsState, err := clientState.VerifyUpgradeAndUpdateState(ctx, k.cdc, clientStore,
upgradedClient, upgradedConsState, proofUpgradeClient, proofUpgradeConsState)
if err != nil {
return sdkerrors.Wrapf(err, "cannot upgrade client with ID %s", clientID)
Expand Down Expand Up @@ -178,11 +180,13 @@ func (k Keeper) CheckMisbehaviourAndUpdateState(ctx sdk.Context, misbehaviour ex
return sdkerrors.Wrapf(types.ErrClientNotFound, "cannot check misbehaviour for client with ID %s", misbehaviour.GetClientID())
}

if clientState.IsFrozen() && clientState.GetFrozenHeight().LTE(misbehaviour.GetHeight()) {
return sdkerrors.Wrapf(types.ErrInvalidMisbehaviour, "client is already frozen at height ≤ misbehaviour height (%s ≤ %s)", clientState.GetFrozenHeight(), misbehaviour.GetHeight())
clientStore := k.ClientStore(ctx, misbehaviour.GetClientID())

if status := clientState.Status(ctx, clientStore, k.cdc); status != exported.Active {
return sdkerrors.Wrapf(types.ErrClientNotActive, "cannot process misbehaviour for client (%s) with status %s", misbehaviour.GetClientID(), status)
}

clientState, err := clientState.CheckMisbehaviourAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, misbehaviour.GetClientID()), misbehaviour)
clientState, err := clientState.CheckMisbehaviourAndUpdateState(ctx, k.cdc, clientStore, misbehaviour)
if err != nil {
return err
}
Expand Down
183 changes: 72 additions & 111 deletions modules/core/02-client/keeper/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,141 +42,101 @@ func (suite *KeeperTestSuite) TestCreateClient() {
}

func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
// Must create header creation functions since suite.header gets recreated on each test case
createFutureUpdateFn := func(s *KeeperTestSuite) *ibctmtypes.Header {
heightPlus3 := clienttypes.NewHeight(suite.header.GetHeight().GetRevisionNumber(), suite.header.GetHeight().GetRevisionHeight()+3)
height := suite.header.GetHeight().(clienttypes.Height)

return suite.chainA.CreateTMClientHeader(testChainID, int64(heightPlus3.RevisionHeight), height, suite.header.Header.Time.Add(time.Hour),
suite.valSet, suite.valSet, []tmtypes.PrivValidator{suite.privVal})
}
createPastUpdateFn := func(s *KeeperTestSuite) *ibctmtypes.Header {
heightMinus2 := clienttypes.NewHeight(suite.header.GetHeight().GetRevisionNumber(), suite.header.GetHeight().GetRevisionHeight()-2)
heightMinus4 := clienttypes.NewHeight(suite.header.GetHeight().GetRevisionNumber(), suite.header.GetHeight().GetRevisionHeight()-4)

return suite.chainA.CreateTMClientHeader(testChainID, int64(heightMinus2.RevisionHeight), heightMinus4, suite.header.Header.Time,
suite.valSet, suite.valSet, []tmtypes.PrivValidator{suite.privVal})
}
var (
path *ibctesting.Path
updateHeader *ibctmtypes.Header
clientState *ibctmtypes.ClientState
clientID string
err error
)

// Must create header creation functions since suite.header gets recreated on each test case
createFutureUpdateFn := func(trustedHeight clienttypes.Height) *ibctmtypes.Header {
header, err := suite.chainA.ConstructUpdateTMClientHeaderWithTrustedHeight(path.EndpointB.Chain, path.EndpointA.ClientID, trustedHeight)
suite.Require().NoError(err)
return header
}

createPastUpdateFn := func(fillHeight, trustedHeight clienttypes.Height) *ibctmtypes.Header {
consState, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientConsensusState(suite.chainA.GetContext(), path.EndpointA.ClientID, trustedHeight)
suite.Require().True(found)
Comment on lines +58 to +59
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you have this check in future 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.

I added this because the consensus state is required to know the correct timestamp. The future function doesn't need to pass in a timestamp and thus the consensus state isn't needed. Checking for the existence of the consensus state would break a test case below, I'm fairly sure - "consensus state not found"


return suite.chainB.CreateTMClientHeader(suite.chainB.ChainID, int64(fillHeight.RevisionHeight), trustedHeight, consState.(*ibctmtypes.ConsensusState).Timestamp.Add(time.Second*5),
suite.chainB.Vals, suite.chainB.Vals, suite.chainB.Signers)
}
cases := []struct {
name string
malleate func() error
malleate func()
expPass bool
}{
{"valid update", func() error {
clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
{"valid update", func() {
clientState := path.EndpointA.GetClientState().(*ibctmtypes.ClientState)
trustHeight := clientState.GetLatestHeight().(types.Height)

// store intermediate consensus state to check that trustedHeight does not need to be highest consensus state before header height
incrementedClientHeight := testClientHeight.Increment().(types.Height)
intermediateConsState := &ibctmtypes.ConsensusState{
Timestamp: suite.now.Add(time.Minute),
NextValidatorsHash: suite.valSetHash,
}
suite.keeper.SetClientConsensusState(suite.ctx, clientID, incrementedClientHeight, intermediateConsState)

clientState.LatestHeight = incrementedClientHeight
suite.keeper.SetClientState(suite.ctx, clientID, clientState)
path.EndpointA.UpdateClient()

updateHeader = createFutureUpdateFn(suite)
return err
updateHeader = createFutureUpdateFn(trustHeight)
}, true},
{"valid past update", func() error {
clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
suite.Require().NoError(err)
{"valid past update", func() {
clientState := path.EndpointA.GetClientState()
trustedHeight := clientState.GetLatestHeight().(types.Height)

height1 := types.NewHeight(0, 1)
currHeight := suite.chainB.CurrentHeader.Height
fillHeight := types.NewHeight(clientState.GetLatestHeight().GetRevisionNumber(), uint64(currHeight))

// store previous consensus state
prevConsState := &ibctmtypes.ConsensusState{
Timestamp: suite.past,
NextValidatorsHash: suite.valSetHash,
}
suite.keeper.SetClientConsensusState(suite.ctx, clientID, height1, prevConsState)
// commit a couple blocks to allow client to fill in gaps
suite.coordinator.CommitBlock(suite.chainB) // this height is not filled in yet
suite.coordinator.CommitBlock(suite.chainB) // this height is filled in by the update below

height2 := types.NewHeight(0, 2)
path.EndpointA.UpdateClient()

// store intermediate consensus state to check that trustedHeight does not need to be hightest consensus state before header height
intermediateConsState := &ibctmtypes.ConsensusState{
Timestamp: suite.past.Add(time.Minute),
NextValidatorsHash: suite.valSetHash,
}
suite.keeper.SetClientConsensusState(suite.ctx, clientID, height2, intermediateConsState)
// ensure fill height not set
_, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientConsensusState(suite.chainA.GetContext(), path.EndpointA.ClientID, fillHeight)
suite.Require().False(found)

// updateHeader will fill in consensus state between prevConsState and suite.consState
// clientState should not be updated
updateHeader = createPastUpdateFn(suite)
return nil
updateHeader = createPastUpdateFn(fillHeight, trustedHeight)
}, true},
{"client state not found", func() error {
updateHeader = createFutureUpdateFn(suite)
{"client state not found", func() {
updateHeader = createFutureUpdateFn(path.EndpointA.GetClientState().GetLatestHeight().(types.Height))

return nil
path.EndpointA.ClientID = ibctesting.InvalidID
}, false},
{"consensus state not found", func() error {
clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
suite.keeper.SetClientState(suite.ctx, testClientID, clientState)
updateHeader = createFutureUpdateFn(suite)

return nil
{"consensus state not found", func() {
clientState := path.EndpointA.GetClientState()
tmClient, ok := clientState.(*ibctmtypes.ClientState)
suite.Require().True(ok)
tmClient.LatestHeight = tmClient.LatestHeight.Increment().(types.Height)

suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID, clientState)
updateHeader = createFutureUpdateFn(clientState.GetLatestHeight().(types.Height))
}, false},
{"frozen client before update", func() error {
clientState = &ibctmtypes.ClientState{FrozenHeight: types.NewHeight(0, 1), LatestHeight: testClientHeight}
suite.keeper.SetClientState(suite.ctx, testClientID, clientState)
updateHeader = createFutureUpdateFn(suite)

return nil
{"client is not active", func() {
clientState := path.EndpointA.GetClientState().(*ibctmtypes.ClientState)
clientState.FrozenHeight = types.NewHeight(0, 1)
suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID, clientState)
updateHeader = createFutureUpdateFn(clientState.GetLatestHeight().(types.Height))
}, false},
{"valid past update before client was frozen", func() error {
clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
clientState.FrozenHeight = types.NewHeight(0, testClientHeight.RevisionHeight-1)
clientID, err = suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
suite.Require().NoError(err)

height1 := types.NewHeight(0, 1)

// store previous consensus state
prevConsState := &ibctmtypes.ConsensusState{
Timestamp: suite.past,
NextValidatorsHash: suite.valSetHash,
}
suite.keeper.SetClientConsensusState(suite.ctx, clientID, height1, prevConsState)

// updateHeader will fill in consensus state between prevConsState and suite.consState
// clientState should not be updated
updateHeader = createPastUpdateFn(suite)
return nil
}, true},
{"invalid header", func() error {
clientState = ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
_, err := suite.keeper.CreateClient(suite.ctx, clientState, suite.consensusState)
suite.Require().NoError(err)
updateHeader = createPastUpdateFn(suite)

return nil
{"invalid header", func() {
updateHeader = createFutureUpdateFn(path.EndpointA.GetClientState().GetLatestHeight().(types.Height))
updateHeader.TrustedHeight = updateHeader.TrustedHeight.Increment().(types.Height)
}, false},
}

for i, tc := range cases {
for _, tc := range cases {
tc := tc
i := i
suite.Run(fmt.Sprintf("Case %s", tc.name), func() {
suite.SetupTest()
clientID = testClientID // must be explicitly changed
path = ibctesting.NewPath(suite.chainA, suite.chainB)
suite.coordinator.SetupClients(path)

err := tc.malleate()
suite.Require().NoError(err)
tc.malleate()

suite.ctx = suite.ctx.WithBlockTime(updateHeader.Header.Time.Add(time.Minute))
var clientState exported.ClientState
if tc.expPass {
clientState = path.EndpointA.GetClientState()
}

err = suite.keeper.UpdateClient(suite.ctx, clientID, updateHeader)
err := suite.chainA.App.GetIBCKeeper().ClientKeeper.UpdateClient(suite.chainA.GetContext(), path.EndpointA.ClientID, updateHeader)

if tc.expPass {
suite.Require().NoError(err, err)
Expand All @@ -187,11 +147,10 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
NextValidatorsHash: updateHeader.Header.NextValidatorsHash,
}

newClientState, found := suite.keeper.GetClientState(suite.ctx, clientID)
suite.Require().True(found, "valid test case %d failed: %s", i, tc.name)
newClientState := path.EndpointA.GetClientState()

consensusState, found := suite.keeper.GetClientConsensusState(suite.ctx, clientID, updateHeader.GetHeight())
suite.Require().True(found, "valid test case %d failed: %s", i, tc.name)
consensusState, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientConsensusState(suite.chainA.GetContext(), path.EndpointA.ClientID, updateHeader.GetHeight())
suite.Require().True(found)

// Determine if clientState should be updated or not
if updateHeader.GetHeight().GT(clientState.GetLatestHeight()) {
Expand All @@ -202,10 +161,10 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() {
suite.Require().Equal(clientState.GetLatestHeight(), newClientState.GetLatestHeight(), "client state height updated for past header")
}

suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.name)
suite.Require().NoError(err)
suite.Require().Equal(expConsensusState, consensusState, "consensus state should have been updated on case %s", tc.name)
} else {
suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.name)
suite.Require().Error(err)
}
})
}
Expand Down Expand Up @@ -291,8 +250,10 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
expPass: false,
},
{
name: "client state frozen",
name: "client state is not active",
setup: func() {
// client is frozen

// last Height is at next block
lastHeight = clienttypes.NewHeight(0, uint64(suite.chainB.GetContext().BlockHeight()+1))

Expand Down Expand Up @@ -524,7 +485,7 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
false,
},
{
"client already frozen at earlier height",
"client already is not active - client is frozen",
&ibctmtypes.Misbehaviour{
Header1: suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight), testClientHeight, altTime, bothValSet, bothValSet, bothSigners),
Header2: suite.chainA.CreateTMClientHeader(testChainID, int64(testClientHeight.RevisionHeight), testClientHeight, suite.ctx.BlockTime(), bothValSet, bothValSet, bothSigners),
Expand Down Expand Up @@ -582,7 +543,7 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {

clientState, found := suite.keeper.GetClientState(suite.ctx, clientID)
suite.Require().True(found, "valid test case %d failed: %s", i, tc.name)
suite.Require().True(clientState.IsFrozen(), "valid test case %d failed: %s", i, tc.name)
suite.Require().True(!clientState.(*ibctmtypes.ClientState).FrozenHeight.IsZero(), "valid test case %d failed: %s", i, tc.name)
suite.Require().Equal(tc.misbehaviour.GetHeight(), clientState.GetFrozenHeight(),
"valid test case %d failed: %s. Expected FrozenHeight %s got %s", tc.misbehaviour.GetHeight(), clientState.GetFrozenHeight())
} else {
Expand Down
2 changes: 1 addition & 1 deletion modules/core/02-client/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ func (k Keeper) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientS
&ibctmtypes.ClientState{}, tmClient)
}

if clientState.IsFrozen() {
if !tmClient.FrozenHeight.IsZero() {
return types.ErrClientFrozen
}

Expand Down
1 change: 1 addition & 0 deletions modules/core/02-client/types/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,5 @@ var (
ErrInvalidHeight = sdkerrors.Register(SubModuleName, 26, "invalid height")
ErrInvalidSubstitute = sdkerrors.Register(SubModuleName, 27, "invalid client state substitute")
ErrInvalidUpgradeProposal = sdkerrors.Register(SubModuleName, 28, "invalid upgrade proposal")
ErrClientNotActive = sdkerrors.Register(SubModuleName, 29, "client is not active")
)
Loading