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.

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

chore: update 07-tendermint GetConsensusState to return bool over error #1180

Merged
10 changes: 5 additions & 5 deletions modules/light-clients/07-tendermint/types/client_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ func (cs ClientState) Status(
}

// get latest consensus state from clientStore to check for expiry
consState, err := GetConsensusState(clientStore, cdc, cs.GetLatestHeight())
if err != nil {
consState, found := GetConsensusState(clientStore, cdc, cs.GetLatestHeight())
if !found {
// if the client state does not have an associated consensus state for its latest height
// then it must be expired
return exported.Expired
Expand Down Expand Up @@ -574,9 +574,9 @@ func produceVerificationArgs(
return commitmenttypes.MerkleProof{}, nil, sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "failed to unmarshal proof into commitment merkle proof")
}

consensusState, err = GetConsensusState(store, cdc, height)
if err != nil {
return commitmenttypes.MerkleProof{}, nil, sdkerrors.Wrap(err, "please ensure the proof was constructed against a height that exists on the client")
consensusState, found := GetConsensusState(store, cdc, height)
if !found {
return commitmenttypes.MerkleProof{}, nil, sdkerrors.Wrap(clienttypes.ErrConsensusStateNotFound, "please ensure the proof was constructed against a height that exists on the client")
}

return merkleProof, consensusState, nil
Expand Down
16 changes: 10 additions & 6 deletions modules/light-clients/07-tendermint/types/misbehaviour_handle.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,18 @@ func (cs *ClientState) verifyMisbehaviour(ctx sdk.Context, clientStore sdk.KVSto
// Regardless of the type of misbehaviour, ensure that both headers are valid and would have been accepted by light-client

// Retrieve trusted consensus states for each Header in misbehaviour
tmConsensusState1, err := GetConsensusState(clientStore, cdc, misbehaviour.Header1.TrustedHeight)
if err != nil {
return sdkerrors.Wrapf(err, "could not get trusted consensus state from clientStore for Header1 at TrustedHeight: %s", misbehaviour.Header1.TrustedHeight)
// and unmarshal from clientStore

// Get consensus bytes from clientStore
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
tmConsensusState1, found := GetConsensusState(clientStore, cdc, misbehaviour.Header1.TrustedHeight)
if !found {
return sdkerrors.Wrapf(clienttypes.ErrConsensusStateNotFound, "could not get trusted consensus state from clientStore for Header1 at TrustedHeight: %s", misbehaviour.Header1)
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
}

tmConsensusState2, err := GetConsensusState(clientStore, cdc, misbehaviour.Header2.TrustedHeight)
if err != nil {
return sdkerrors.Wrapf(err, "could not get trusted consensus state from clientStore for Header2 at TrustedHeight: %s", misbehaviour.Header2.TrustedHeight)
// Get consensus bytes from clientStore
tmConsensusState2, found := GetConsensusState(clientStore, cdc, misbehaviour.Header2.TrustedHeight)
if !found {
return sdkerrors.Wrapf(clienttypes.ErrConsensusStateNotFound, "could not get trusted consensus state from clientStore for Header2 at TrustedHeight: %s", misbehaviour.Header2)
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
}

// Check the validity of the two conflicting headers against their respective
Expand Down
6 changes: 3 additions & 3 deletions modules/light-clients/07-tendermint/types/proposal_handle.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ func (cs ClientState) CheckSubstituteAndUpdateState(
// starting from initial height and ending on the latest height (inclusive)
height := substituteClientState.GetLatestHeight()

consensusState, err := GetConsensusState(substituteClientStore, cdc, height)
if err != nil {
return nil, sdkerrors.Wrap(err, "unable to retrieve latest consensus state for substitute client")
consensusState, found := GetConsensusState(substituteClientStore, cdc, height)
if !found {
return nil, sdkerrors.Wrap(clienttypes.ErrConsensusStateNotFound, "unable to retrieve latest consensus state for substitute client")
}

SetConsensusState(subjectClientStore, cdc, consensusState, height)
Expand Down
29 changes: 6 additions & 23 deletions modules/light-clients/07-tendermint/types/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/store/prefix"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"

clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types"
host "github.com/cosmos/ibc-go/v3/modules/core/24-host"
Expand Down Expand Up @@ -59,29 +58,14 @@ func SetConsensusState(clientStore sdk.KVStore, cdc codec.BinaryCodec, consensus

// GetConsensusState retrieves the consensus state from the client prefixed
// store. An error is returned if the consensus state does not exist.
Copy link
Contributor

Choose a reason for hiding this comment

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

godoc needs to be updated. Should we make this function private?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good spot

Copy link
Member Author

Choose a reason for hiding this comment

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

Making private makes a bit more awkward for testing. We would actually need to remove the test function for this.
getConsensusState will still get coverage from a number of other tests but testing the panic scenarios then isn't really possible.

What do you think? I don't really have any strong feelings about making it public/private

Copy link
Contributor

Choose a reason for hiding this comment

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

lets leave for now

func GetConsensusState(store sdk.KVStore, cdc codec.BinaryCodec, height exported.Height) (*ConsensusState, error) {
func GetConsensusState(store sdk.KVStore, cdc codec.BinaryCodec, height exported.Height) (*ConsensusState, bool) {
bz := store.Get(host.ConsensusStateKey(height))
if bz == nil {
return nil, sdkerrors.Wrapf(
clienttypes.ErrConsensusStateNotFound,
"consensus state does not exist for height %s", height,
)
}

consensusStateI, err := clienttypes.UnmarshalConsensusState(cdc, bz)
if err != nil {
return nil, sdkerrors.Wrapf(clienttypes.ErrInvalidConsensus, "unmarshal error: %v", err)
}

consensusState, ok := consensusStateI.(*ConsensusState)
if !ok {
return nil, sdkerrors.Wrapf(
clienttypes.ErrInvalidConsensus,
"invalid consensus type %T, expected %T", consensusState, &ConsensusState{},
)
return nil, false
}

return consensusState, nil
consensusStateI := clienttypes.MustUnmarshalConsensusState(cdc, bz)
return consensusStateI.(*ConsensusState), true
}

// deleteConsensusState deletes the consensus state at the given height
Expand Down Expand Up @@ -299,9 +283,8 @@ func PruneAllExpiredConsensusStates(
var heights []exported.Height

pruneCb := func(height exported.Height) bool {
consState, err := GetConsensusState(clientStore, cdc, height)
// this error should never occur
if err != nil {
consState, found := GetConsensusState(clientStore, cdc, height)
if !found { // consensus state should always be found
return true
}

Expand Down
25 changes: 18 additions & 7 deletions modules/light-clients/07-tendermint/types/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,31 +23,32 @@ func (suite *TendermintTestSuite) TestGetConsensusState() {
name string
malleate func()
expPass bool
expPanic bool
}{
{
"success", func() {}, true,
"success", func() {}, true, false,
},
{
"consensus state not found", func() {
// use height with no consensus state set
height = height.(clienttypes.Height).Increment()
}, false,
}, false, false,
},
{
"not a consensus state interface", func() {
// marshal an empty client state and set as consensus state
store := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), path.EndpointA.ClientID)
clientStateBz := suite.chainA.App.GetIBCKeeper().ClientKeeper.MustMarshalClientState(&types.ClientState{})
store.Set(host.ConsensusStateKey(height), clientStateBz)
}, false,
}, false, true,
},
{
"invalid consensus state (solomachine)", func() {
// marshal and set solomachine consensus state
store := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), path.EndpointA.ClientID)
consensusStateBz := suite.chainA.App.GetIBCKeeper().ClientKeeper.MustMarshalConsensusState(&solomachinetypes.ConsensusState{})
store.Set(host.ConsensusStateKey(height), consensusStateBz)
}, false,
}, false, true,
},
}

Expand All @@ -64,16 +65,26 @@ func (suite *TendermintTestSuite) TestGetConsensusState() {

tc.malleate() // change vars as necessary

if tc.expPanic {
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
suite.Require().Panics(func() {
store := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), path.EndpointA.ClientID)
types.GetConsensusState(store, suite.chainA.Codec, height)
})

return
}

store := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), path.EndpointA.ClientID)
consensusState, err := types.GetConsensusState(store, suite.chainA.Codec, height)
consensusState, found := types.GetConsensusState(store, suite.chainA.Codec, height)

if tc.expPass {
suite.Require().NoError(err)
suite.Require().True(found)

expConsensusState, found := suite.chainA.GetConsensusState(path.EndpointA.ClientID, height)
suite.Require().True(found)
suite.Require().Equal(expConsensusState, consensusState)
} else {
suite.Require().Error(err)
suite.Require().False(found)
suite.Require().Nil(consensusState)
}
})
Expand Down
12 changes: 6 additions & 6 deletions modules/light-clients/07-tendermint/types/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,9 @@ func (cs *ClientState) verifyHeader(
currentTimestamp := ctx.BlockTime()

// Retrieve trusted consensus states for each Header in misbehaviour
consState, err := GetConsensusState(clientStore, cdc, header.TrustedHeight)
if err != nil {
return sdkerrors.Wrapf(err, "could not get trusted consensus state from clientStore for Header at TrustedHeight: %s", header.TrustedHeight)
consState, found := GetConsensusState(clientStore, cdc, header.TrustedHeight)
if !found {
return sdkerrors.Wrapf(clienttypes.ErrConsensusStateNotFound, "could not get trusted consensus state from clientStore for Header at TrustedHeight: %s", header.TrustedHeight)
}

if err := checkTrustedHeader(header, consState); err != nil {
Expand Down Expand Up @@ -280,10 +280,10 @@ func (cs ClientState) pruneOldestConsensusState(ctx sdk.Context, cdc codec.Binar
)

pruneCb := func(height exported.Height) bool {
consState, err := GetConsensusState(clientStore, cdc, height)
consState, found := GetConsensusState(clientStore, cdc, height)
// this error should never occur
if err != nil {
pruneError = err
if !found {
pruneError = sdkerrors.Wrapf(clienttypes.ErrConsensusStateNotFound, "failed to retrieve consensus state at height: %s", height)
return true
}

Expand Down
6 changes: 3 additions & 3 deletions modules/light-clients/07-tendermint/types/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ func (cs ClientState) VerifyUpgradeAndUpdateState(
// Must prove against latest consensus state to ensure we are verifying against latest upgrade plan
// This verifies that upgrade is intended for the provided revision, since committed client must exist
// at this consensus state
consState, err := GetConsensusState(clientStore, cdc, lastHeight)
if err != nil {
return sdkerrors.Wrap(err, "could not retrieve consensus state for lastHeight")
consState, found := GetConsensusState(clientStore, cdc, lastHeight)
if !found {
return sdkerrors.Wrap(clienttypes.ErrConsensusStateNotFound, "could not retrieve consensus state for lastHeight")
}

// Verify client proof
Expand Down