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

chore: remove GetHeight from ClientMessage interface #1285

Merged
merged 7 commits into from
Apr 27, 2022
Merged

chore: remove GetHeight from ClientMessage interface #1285

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d72c9e1
adding new consensus heights return value from UpdateState interface,…
damiannolan Apr 21, 2022
9737a7e
removing GetHeight() from ClientMessage interface, updating tests
damiannolan Apr 21, 2022
b30380f
updating godocs to include returned consensus height data
damiannolan Apr 22, 2022
832ca2e
removing unused event emission func, adding deprecation notice, remov…
damiannolan Apr 25, 2022
9116fd1
removing error return from UpdateState, returning height on duplicate…
damiannolan Apr 26, 2022
2066ab9
Merge branch '02-client-refactor' into damian/594-remove-clientmsg-ge…
damiannolan Apr 26, 2022
aca9870
updating event emissions as per suggestions
damiannolan Apr 26, 2022
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
47 changes: 23 additions & 24 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 @@ -70,14 +70,6 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, clientMsg exporte
return err
}

// Marshal the ClientMessage as an Any and encode the resulting bytes to hex.
// This prevents the event value from containing invalid UTF-8 characters
// which may cause data to be lost when JSON encoding/decoding.
clientMsgStr := hex.EncodeToString(types.MustMarshalClientMessage(k.cdc, clientMsg))
damiannolan marked this conversation as resolved.
Show resolved Hide resolved

// set default consensus height with header height
consensusHeight := clientMsg.GetHeight()

foundMisbehaviour := clientState.CheckForMisbehaviour(ctx, k.cdc, clientStore, clientMsg)
if foundMisbehaviour {
clientState.UpdateStateOnMisbehaviour(ctx, k.cdc, clientStore, clientMsg)
Expand All @@ -96,29 +88,36 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, clientMsg exporte
)
}()

EmitSubmitMisbehaviourEventOnUpdate(ctx, clientID, clientState.ClientType(), consensusHeight, clientMsgStr)
EmitSubmitMisbehaviourEvent(ctx, clientID, clientState)

return nil
}

clientState.UpdateState(ctx, k.cdc, clientStore, clientMsg)
consensusHeights := clientState.UpdateState(ctx, k.cdc, clientStore, clientMsg)
if len(consensusHeights) != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think not emitting UpdateClientEvent on a successful MsgUpdateClient is problematic. I may be wrong though. Would be good to check with relayers what the best expected functionality is for dup updates

The main odd thing is if you query for UpdateClient events, this message wouldn't come up. To me, it'd make more sense if the event was emitted, but indicated it was a duplicate update

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'm happy to change this to return the height from the ClientState implementation on duplicate updates.
However, I don't think we can provide information about duplicate updates in the events, that would only be possible if emitting from 07-tendermint for example, but as we discussed before they should be emitted from 02-client in a somewhat standardised fashion.

If we're in agreement, I can update this PR to return the height on duplicates and also make the changes to remove the error return value and panic instead.

Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking if consensus_heights is empty, we should just return "" in the consensus heights attribute. Thoughts?

In favor of removing error return 👍

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 updated and removed the error return, and also returning the height on duplicate header updates from 07!

k.Logger(ctx).Info("client state updated", "client-id", clientID, "heights", consensusHeights)

k.Logger(ctx).Info("client state updated", "client-id", clientID, "height", consensusHeight.String())
defer func() {
telemetry.IncrCounterWithLabels(
[]string{"ibc", "client", "update"},
1,
[]metrics.Label{
telemetry.NewLabel(types.LabelClientType, clientState.ClientType()),
telemetry.NewLabel(types.LabelClientID, clientID),
telemetry.NewLabel(types.LabelUpdateType, "msg"),
},
)
}()

defer func() {
telemetry.IncrCounterWithLabels(
[]string{"ibc", "client", "update"},
1,
[]metrics.Label{
telemetry.NewLabel(types.LabelClientType, clientState.ClientType()),
telemetry.NewLabel(types.LabelClientID, clientID),
telemetry.NewLabel(types.LabelUpdateType, "msg"),
},
)
}()
// Marshal the ClientMessage as an Any and encode the resulting bytes to hex.
// This prevents the event value from containing invalid UTF-8 characters
// which may cause data to be lost when JSON encoding/decoding.
clientMsgStr := hex.EncodeToString(types.MustMarshalClientMessage(k.cdc, clientMsg))

// emitting events in the keeper emits for both begin block and handler client updates
EmitUpdateClientEvent(ctx, clientID, clientState.ClientType(), consensusHeight, clientMsgStr)
// emitting events in the keeper emits for both begin block and handler client updates
EmitUpdateClientEvent(ctx, clientID, clientState.ClientType(), consensusHeights, clientMsgStr)

}

return nil
}
Expand Down
25 changes: 10 additions & 15 deletions modules/core/02-client/keeper/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package keeper

import (
"strings"

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/v3/modules/core/02-client/types"
Expand All @@ -24,13 +26,19 @@ func EmitCreateClientEvent(ctx sdk.Context, clientID string, clientState exporte
}

// EmitUpdateClientEvent emits an update client event
func EmitUpdateClientEvent(ctx sdk.Context, clientID string, clientType string, consensusHeight exported.Height, clientMsgStr string) {
func EmitUpdateClientEvent(ctx sdk.Context, clientID string, clientType string, consensusHeights []exported.Height, clientMsgStr string) {
var consensusHeightStr []string
for _, height := range consensusHeights {
consensusHeightStr = append(consensusHeightStr, height.String())
}

ctx.EventManager().EmitEvents(sdk.Events{
sdk.NewEvent(
types.EventTypeUpdateClient,
sdk.NewAttribute(types.AttributeKeyClientID, clientID),
sdk.NewAttribute(types.AttributeKeyClientType, clientType),
sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeight.String()),
sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeights[0].String()),
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, I think we need a len(consensusHeights) > 0 check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was being checked in the UpdateClient() method of keeper.go.

if len(consensusHeights) != 0 {
    // emit events, otherwise no-op & return nil
}

return nil

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yes. The only strange part is that this function assumes you pass in a non empty array of consensus heights

Unrelated, could we add // deprecated next to the consensus height attribute

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we backport consensusHeights attribute to older releases? That way relayers could switch to it without waiting for all chains to upgrade to this version

Copy link
Contributor Author

@damiannolan damiannolan Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The only strange part is that this function assumes you pass in a non empty array of consensus heights

Agree, it is kind of strange. I'll adapt to check len(consensusHeights) inside event emission func and remove the check from the UpdateClient keeper method.

Should we backport consensusHeights attribute to older releases?

Sounds like a good idea, but this would require a separate PR to main, correct?

Edit: I'll also add deprecated comment in-line 👍 Now that I'm looking at this once again, AttributeKeyConsensusHeight is used for create and upgrade events as a single height value, and should probably be maintained. In which case, I think the deprecation notice should only be here, solely for the update events and not in types/events.go

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch!

sdk.NewAttribute(types.AttributeKeyConsensusHeight, strings.Join(consensusHeightStr, ",")),
sdk.NewAttribute(types.AttributeKeyHeader, clientMsgStr),
),
sdk.NewEvent(
Expand Down Expand Up @@ -78,16 +86,3 @@ func EmitSubmitMisbehaviourEvent(ctx sdk.Context, clientID string, clientState e
),
)
}

// EmitSubmitMisbehaviourEventOnUpdate emits a client misbehaviour event on a client update event
func EmitSubmitMisbehaviourEventOnUpdate(ctx sdk.Context, clientID string, clientType string, consensusHeight exported.Height, headerStr string) {
ctx.EventManager().EmitEvent(
sdk.NewEvent(
types.EventTypeSubmitMisbehaviour,
sdk.NewAttribute(types.AttributeKeyClientID, clientID),
sdk.NewAttribute(types.AttributeKeyClientType, clientType),
sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeight.String()),
sdk.NewAttribute(types.AttributeKeyHeader, headerStr),
),
)
}
2 changes: 1 addition & 1 deletion modules/core/02-client/legacy/v100/solomachine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func (cs *ClientState) VerifyClientMessage(
}

// UpdateState panis!
func (cs *ClientState) UpdateState(_ sdk.Context, _ codec.BinaryCodec, _ sdk.KVStore, _ exported.ClientMessage) error {
func (cs *ClientState) UpdateState(_ sdk.Context, _ codec.BinaryCodec, _ sdk.KVStore, _ exported.ClientMessage) []exported.Height {
panic("legacy solo machine is deprecated!")
}

Expand Down
8 changes: 6 additions & 2 deletions modules/core/02-client/types/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,12 @@ const (
AttributeKeyClientID = "client_id"
AttributeKeySubjectClientID = "subject_client_id"
AttributeKeyClientType = "client_type"
AttributeKeyConsensusHeight = "consensus_height"
AttributeKeyHeader = "header"

// Deprecated: AttributeKeyConsensusHeight is deprecated and will be removed in a future release.
// Please use AttributeKeyConsensusHeights instead.
AttributeKeyConsensusHeight = "consensus_height"
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
AttributeKeyConsensusHeights = "consensus_heights"
AttributeKeyHeader = "header"
)

// IBC client events vars
Expand Down
5 changes: 2 additions & 3 deletions modules/core/exported/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ type ClientState interface {
VerifyClientMessage(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, clientMsg ClientMessage) error

// UpdateState updates and stores as necessary any associated information for an IBC client, such as the ClientState and corresponding ConsensusState.
// An error is returned if ClientMessage is of type Misbehaviour
UpdateState(sdk.Context, codec.BinaryCodec, sdk.KVStore, ClientMessage) error
// Upon successful update, a list of consensus heights is returned. It assumes the ClientMessage has already been verified.
UpdateState(sdk.Context, codec.BinaryCodec, sdk.KVStore, ClientMessage) []Height

// Update and Misbehaviour functions
CheckSubstituteAndUpdateState(ctx sdk.Context, cdc codec.BinaryCodec, subjectClientStore, substituteClientStore sdk.KVStore, substituteClient ClientState) (ClientState, error)
Expand Down Expand Up @@ -203,7 +203,6 @@ type ConsensusState interface {
type ClientMessage interface {
proto.Message

GetHeight() Height
ClientType() string
ValidateBasic() error
}
Expand Down
7 changes: 0 additions & 7 deletions modules/light-clients/06-solomachine/types/header.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,6 @@ func (Header) ClientType() string {
return exported.Solomachine
}

// GetHeight returns the current sequence number as the height.
// Return clientexported.Height to satisfy interface
// Revision number is always 0 for a solo-machine
func (h Header) GetHeight() exported.Height {
return clienttypes.NewHeight(0, h.Sequence)
}

// GetPubKey unmarshals the new public key into a cryptotypes.PubKey type.
// An error is returned if the new public key is nil or the cached value
// is not a PubKey.
Expand Down
9 changes: 6 additions & 3 deletions modules/light-clients/06-solomachine/types/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package types

import (
"fmt"

"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
Expand Down Expand Up @@ -81,10 +83,11 @@ func (cs ClientState) verifyMisbehaviour(ctx sdk.Context, cdc codec.BinaryCodec,
}

// UpdateState updates the consensus state to the new public key and an incremented sequence.
func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, clientMsg exported.ClientMessage) error {
// A list containing the updated consensus height is returned.
func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, clientMsg exported.ClientMessage) []exported.Height {
smHeader, ok := clientMsg.(*Header)
if !ok {
return sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "expected %T got %T", Header{}, clientMsg)
panic(fmt.Errorf("unsupported ClientMessage: %T", clientMsg))
}

// create new solomachine ConsensusState
Expand All @@ -99,7 +102,7 @@ func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, client

clientStore.Set(host.ClientStateKey(), clienttypes.MustMarshalClientState(cdc, &cs))

return nil
return []exported.Height{clienttypes.NewHeight(0, cs.Sequence)}
}

// CheckForMisbehaviour returns true for type Misbehaviour (passed VerifyClientMessage check), otherwise returns false
Expand Down
12 changes: 8 additions & 4 deletions modules/light-clients/06-solomachine/types/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,23 +441,27 @@ func (suite *SoloMachineTestSuite) TestUpdateState() {
suite.Run(tc.name, func() {
tc.setup() // setup test

err := clientState.UpdateState(suite.chainA.GetContext(), suite.chainA.Codec, suite.store, clientMsg)

if tc.expPass {
suite.Require().NoError(err)
consensusHeights := clientState.UpdateState(suite.chainA.GetContext(), suite.chainA.Codec, suite.store, clientMsg)

clientStateBz := suite.store.Get(host.ClientStateKey())
suite.Require().NotEmpty(clientStateBz)

newClientState := clienttypes.MustUnmarshalClientState(suite.chainA.Codec, clientStateBz)

suite.Require().Len(consensusHeights, 1)
suite.Require().Equal(uint64(0), consensusHeights[0].GetRevisionNumber())
suite.Require().Equal(newClientState.(*types.ClientState).Sequence, consensusHeights[0].GetRevisionHeight())

suite.Require().False(newClientState.(*types.ClientState).IsFrozen)
suite.Require().Equal(clientMsg.(*types.Header).Sequence+1, newClientState.(*types.ClientState).Sequence)
suite.Require().Equal(clientMsg.(*types.Header).NewPublicKey, newClientState.(*types.ClientState).ConsensusState.PublicKey)
suite.Require().Equal(clientMsg.(*types.Header).NewDiversifier, newClientState.(*types.ClientState).ConsensusState.Diversifier)
suite.Require().Equal(clientMsg.(*types.Header).Timestamp, newClientState.(*types.ClientState).ConsensusState.Timestamp)
} else {
suite.Require().Error(err)
suite.Require().Panics(func() {
clientState.UpdateState(suite.chainA.GetContext(), suite.chainA.Codec, suite.store, clientMsg)
})
}

})
Expand Down
11 changes: 7 additions & 4 deletions modules/light-clients/07-tendermint/types/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package types

import (
"bytes"
"fmt"
"reflect"

"github.com/cosmos/cosmos-sdk/codec"
Expand Down Expand Up @@ -154,19 +155,20 @@ func (cs *ClientState) verifyHeader(
// 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
// A list containing the updated consensus height is returned.
// 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) error {
func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, clientMsg exported.ClientMessage) []exported.Height {
header, ok := clientMsg.(*Header)
if !ok {
return sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "expected type %T, got %T", &Header{}, header)
panic(fmt.Errorf("expected type %T, got %T", &Header{}, clientMsg))
}

// check for duplicate update
if consensusState, _ := GetConsensusState(clientStore, cdc, header.GetHeight()); consensusState != nil {
// perform no-op
return nil
return []exported.Height{header.GetHeight()}
}

cs.pruneOldestConsensusState(ctx, cdc, clientStore)
Expand All @@ -175,6 +177,7 @@ func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, client
if height.GT(cs.LatestHeight) {
cs.LatestHeight = height
}

consensusState := &ConsensusState{
Timestamp: header.GetTime(),
Root: commitmenttypes.NewMerkleRoot(header.Header.GetAppHash()),
Expand All @@ -186,7 +189,7 @@ func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, client
setConsensusState(clientStore, cdc, consensusState, header.GetHeight())
setConsensusMetadata(ctx, clientStore, header.GetHeight())

return nil
return []exported.Height{height}
}

// pruneOldestConsensusState will retrieve the earliest consensus state for this clientID and check if it is expired. If it is,
Expand Down
Loading