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

Jump to bottom

chore: making changes based on nits from 02-client refactor audit #1585

Merged
merged 5 commits into from
Jun 30, 2022
Merged

chore: making changes based on nits from 02-client refactor audit #1585

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
be977e8
chore: making changes based on nits from 02-client refactor audit
chatton Jun 27, 2022
798012f
chore: removing redundant anonymous functions in defer statements, an…
chatton Jun 27, 2022
fae6cba
chore: remove unrequired if statement
chatton Jun 28, 2022
52d37d8
Merge branch '02-client-refactor' into cian/issue#1581-nits-from-firs…
chatton Jun 30, 2022
5faddd7
Update modules/core/02-client/keeper/events.go
chatton Jun 30, 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
81 changes: 33 additions & 48 deletions modules/core/02-client/keeper/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package keeper

import (
"encoding/hex"

"github.com/armon/go-metrics"
"github.com/cosmos/cosmos-sdk/telemetry"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand All @@ -27,7 +25,6 @@ func (k Keeper) CreateClient(

clientID := k.GenerateClientIdentifier(ctx, clientState.ClientType())

k.SetClientState(ctx, clientID, clientState)
k.Logger(ctx).Info("client created at height", "client-id", clientID, "height", clientState.GetLatestHeight().String())

// verifies initial consensus state against client state and initializes client store with any client-specific metadata
Expand All @@ -36,17 +33,16 @@ func (k Keeper) CreateClient(
return "", err
}

k.SetClientState(ctx, clientID, clientState)
k.SetClientConsensusState(ctx, clientID, clientState.GetLatestHeight(), consensusState)

k.Logger(ctx).Info("client created at height", "client-id", clientID, "height", clientState.GetLatestHeight().String())

defer func() {
telemetry.IncrCounterWithLabels(
[]string{"ibc", "client", "create"},
1,
[]metrics.Label{telemetry.NewLabel(types.LabelClientType, clientState.ClientType())},
)
}()
defer telemetry.IncrCounterWithLabels(
Copy link
Contributor Author

@chatton chatton Jun 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

this was not mentioned in the audit however we are wrapping these calls with an anonymous function which is not required unless we want to not evaluate clientState.ClientType() until function execution for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, nice catch. I think then this improvement could be applied to other places of the codebase as well.

[]string{"ibc", "client", "create"},
1,
[]metrics.Label{telemetry.NewLabel(types.LabelClientType, clientState.ClientType())},
)

EmitCreateClientEvent(ctx, clientID, clientState)

Expand Down Expand Up @@ -76,17 +72,15 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, clientMsg exporte

k.Logger(ctx).Info("client frozen due to misbehaviour", "client-id", clientID)

defer func() {
telemetry.IncrCounterWithLabels(
[]string{"ibc", "client", "misbehaviour"},
1,
[]metrics.Label{
telemetry.NewLabel(types.LabelClientType, clientState.ClientType()),
telemetry.NewLabel(types.LabelClientID, clientID),
telemetry.NewLabel(types.LabelMsgType, "update"),
},
)
}()
defer telemetry.IncrCounterWithLabels(
[]string{"ibc", "client", "misbehaviour"},
1,
[]metrics.Label{
telemetry.NewLabel(types.LabelClientType, clientState.ClientType()),
telemetry.NewLabel(types.LabelClientID, clientID),
telemetry.NewLabel(types.LabelMsgType, "update"),
},
)

EmitSubmitMisbehaviourEvent(ctx, clientID, clientState)

Expand All @@ -97,25 +91,18 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, clientMsg exporte

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

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))
defer 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"),
},
)

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

return nil
}
Expand Down Expand Up @@ -143,16 +130,14 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e

k.Logger(ctx).Info("client state upgraded", "client-id", clientID, "height", upgradedClient.GetLatestHeight().String())

defer func() {
telemetry.IncrCounterWithLabels(
[]string{"ibc", "client", "upgrade"},
1,
[]metrics.Label{
telemetry.NewLabel(types.LabelClientType, upgradedClient.ClientType()),
telemetry.NewLabel(types.LabelClientID, clientID),
},
)
}()
defer telemetry.IncrCounterWithLabels(
[]string{"ibc", "client", "upgrade"},
1,
[]metrics.Label{
telemetry.NewLabel(types.LabelClientType, upgradedClient.ClientType()),
telemetry.NewLabel(types.LabelClientID, clientID),
},
)

EmitUpgradeClientEvent(ctx, clientID, upgradedClient)

Expand Down
10 changes: 9 additions & 1 deletion 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 (
"encoding/hex"
"github.com/cosmos/cosmos-sdk/codec"
"strings"

sdk "github.com/cosmos/cosmos-sdk/types"
chatton marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -26,7 +28,13 @@ func EmitCreateClientEvent(ctx sdk.Context, clientID string, clientState exporte
}

// EmitUpdateClientEvent emits an update client event
func EmitUpdateClientEvent(ctx sdk.Context, clientID string, clientType string, consensusHeights []exported.Height, clientMsgStr string) {
func EmitUpdateClientEvent(ctx sdk.Context, clientID string, clientType string, consensusHeights []exported.Height, cdc codec.BinaryCodec, clientMsg exported.ClientMessage) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cdc codec.BinaryCodec was added as an argument to prevent this function being added to the Keeper or accepting a keeper as an argument


// 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(cdc, clientMsg))

var consensusHeightAttr string
if len(consensusHeights) != 0 {
consensusHeightAttr = consensusHeights[0].String()
Expand Down
52 changes: 26 additions & 26 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 @@ -17,26 +17,6 @@ import (
"github.com/cosmos/ibc-go/v3/modules/core/exported"
)

// checkTrustedHeader checks that consensus state matches trusted fields of Header
func checkTrustedHeader(header *Header, consState *ConsensusState) error {
tmTrustedValidators, err := tmtypes.ValidatorSetFromProto(header.TrustedValidators)
if err != nil {
return sdkerrors.Wrap(err, "trusted validator set in not tendermint validator set type")
}

// assert that trustedVals is NextValidators of last trusted header
// to do this, we check that trustedVals.Hash() == consState.NextValidatorsHash
tvalHash := tmTrustedValidators.Hash()
if !bytes.Equal(consState.NextValidatorsHash, tvalHash) {
return sdkerrors.Wrapf(
ErrInvalidValidatorSet,
"trusted validators %s, does not hash to latest trusted validators. Expected: %X, got: %X",
header.TrustedValidators, consState.NextValidatorsHash, tvalHash,
)
}
return nil
}

// VerifyClientMessage checks if the clientMessage is of type Header or Misbehaviour and verifies the message
func (cs *ClientState) VerifyClientMessage(
ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore,
Expand Down Expand Up @@ -198,6 +178,9 @@ func (cs ClientState) pruneOldestConsensusState(ctx sdk.Context, cdc codec.Binar
// this error should never occur
if !found {
pruneError = sdkerrors.Wrapf(clienttypes.ErrConsensusStateNotFound, "failed to retrieve consensus state at height: %s", height)
if pruneError != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can safely remove the if condition because pruneError will always be not nil here, right?

panic(pruneError)
}
return true
}

Expand All @@ -209,9 +192,6 @@ func (cs ClientState) pruneOldestConsensusState(ctx sdk.Context, cdc codec.Binar
}

IterateConsensusStateAscending(clientStore, pruneCb)
if pruneError != nil {
panic(pruneError)
}

// if pruneHeight is set, delete consensus state and metadata
if pruneHeight != nil {
Expand All @@ -230,11 +210,11 @@ func (cs ClientState) CheckForMisbehaviour(ctx sdk.Context, cdc codec.BinaryCode
// Check if the Client store already has a consensus state for the header's height
// If the consensus state exists, and it matches the header then we return early
// since header has already been submitted in a previous UpdateClient.
prevConsState, _ := GetConsensusState(clientStore, cdc, tmHeader.GetHeight())
if prevConsState != nil {
existingConsState, _ := GetConsensusState(clientStore, cdc, tmHeader.GetHeight())
if existingConsState != nil {
// This header has already been submitted and the necessary state is already stored
// in client store, thus we can return early without further validation.
if reflect.DeepEqual(prevConsState, tmHeader.ConsensusState()) {
if reflect.DeepEqual(existingConsState, tmHeader.ConsensusState()) {
return false
}

Expand Down Expand Up @@ -272,3 +252,23 @@ func (cs ClientState) UpdateStateOnMisbehaviour(ctx sdk.Context, cdc codec.Binar

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

// checkTrustedHeader checks that consensus state matches trusted fields of Header
func checkTrustedHeader(header *Header, consState *ConsensusState) error {
tmTrustedValidators, err := tmtypes.ValidatorSetFromProto(header.TrustedValidators)
if err != nil {
return sdkerrors.Wrap(err, "trusted validator set in not tendermint validator set type")
}

// assert that trustedVals is NextValidators of last trusted header
// to do this, we check that trustedVals.Hash() == consState.NextValidatorsHash
tvalHash := tmTrustedValidators.Hash()
if !bytes.Equal(consState.NextValidatorsHash, tvalHash) {
return sdkerrors.Wrapf(
ErrInvalidValidatorSet,
"trusted validators %s, does not hash to latest trusted validators. Expected: %X, got: %X",
header.TrustedValidators, consState.NextValidatorsHash, tvalHash,
)
}
return nil
}