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

Emit header in MsgUpdateClient events #8624

Merged
merged 23 commits into from
Mar 3, 2021
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
47d6a77
emit header in update client msg
colin-axner Feb 18, 2021
600e954
update CHANGELOG
colin-axner Feb 18, 2021
3d824ff
update spec
colin-axner Feb 18, 2021
5c82a98
Merge branch 'master' into colin/8598-update-header-event
Feb 18, 2021
ccea83b
fix nil header bug
colin-axner Feb 18, 2021
8ce1f53
Merge branch 'colin/8598-update-header-event' of github.com:cosmos/co…
colin-axner Feb 18, 2021
1ea4081
use JSON encoding for emitting header
colin-axner Feb 19, 2021
401abdc
Update x/ibc/core/spec/06_events.md
fedekunze Feb 27, 2021
0052d40
use proto for encoding
colin-axner Mar 1, 2021
046ae44
add tests
colin-axner Mar 1, 2021
1462ce6
Merge branch 'colin/8598-update-header-event' of github.com:cosmos/co…
colin-axner Mar 1, 2021
a08b336
fix changelog merge conflict
colin-axner Mar 1, 2021
df9dbfa
merge master
colin-axner Mar 1, 2021
732431c
Merge branch 'master' into colin/8598-update-header-event
colin-axner Mar 1, 2021
3f4773e
encode to hex before emitting header in event
colin-axner Mar 2, 2021
9ffcbe9
Merge branch 'colin/8598-update-header-event' of github.com:cosmos/co…
colin-axner Mar 2, 2021
cb28f10
add comment addressing reasoning for hex encoding
colin-axner Mar 2, 2021
cde3dad
Merge branch 'master' into colin/8598-update-header-event
colin-axner Mar 2, 2021
d639449
Merge branch 'master' into colin/8598-update-header-event
colin-axner Mar 3, 2021
75ee333
Merge branch 'master' into colin/8598-update-header-event
colin-axner Mar 3, 2021
bcaab1a
Merge branch 'master' into colin/8598-update-header-event
colin-axner Mar 3, 2021
1165dad
Update CHANGELOG.md
colin-axner Mar 3, 2021
6cad885
Merge branch 'master' into colin/8598-update-header-event
colin-axner Mar 3, 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/ibc) [\#7949](https://github.com/cosmos/cosmos-sdk/issues/7949) Standardized channel `Acknowledgement` moved to its own file. Codec registration redundancy removed.
* (crypto/types) [\#8600](https://github.com/cosmos/cosmos-sdk/pull/8600) `CompactBitArray`: optimize the `NumTrueBitsBefore` method and add an `Equal` method.
* (store/cachekv), (x/bank/types) [\#8719](https://github.com/cosmos/cosmos-sdk/pull/8719) algorithmically fix pathologically slow code
* (x/ibc)[\#8624](https://github.com/cosmos/cosmos-sdk/pull/8624) Emit full header in IBC UpdateClient message.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

### Bug Fixes

Expand Down
13 changes: 13 additions & 0 deletions x/ibc/core/02-client/keeper/client.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package keeper

import (
"encoding/hex"

"github.com/armon/go-metrics"

"github.com/cosmos/cosmos-sdk/telemetry"
Expand Down Expand Up @@ -95,13 +97,24 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H
)
}()

// emit the full header in events
var headerStr string
if header != nil {
// Marshal the Header 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.
headerStr = hex.EncodeToString(types.MustMarshalHeader(k.cdc, header))

}

// emitting events in the keeper emits for both begin block and handler client updates
ctx.EventManager().EmitEvent(
sdk.NewEvent(
types.EventTypeUpdateClient,
sdk.NewAttribute(types.AttributeKeyClientID, clientID),
sdk.NewAttribute(types.AttributeKeyClientType, clientState.ClientType()),
sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeight.String()),
sdk.NewAttribute(types.AttributeKeyHeader, headerStr),
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will emit an empty event value if the header is nil, is that expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the header is only nil for the localhost client (which currently doesn't work). I cannot imagine a non-localhost client ever using a nil header. Nil headers should be disallowed following the refactor of the localhost client. I think emitting an empty event value is ok in the short term

),
)

Expand Down
37 changes: 37 additions & 0 deletions x/ibc/core/02-client/keeper/client_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper_test

import (
"encoding/hex"
"fmt"
"time"

Expand Down Expand Up @@ -589,3 +590,39 @@ func (suite *KeeperTestSuite) TestCheckMisbehaviourAndUpdateState() {
})
}
}

func (suite *KeeperTestSuite) TestUpdateClientEventEmission() {
clientID, _ := suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint)
header, err := suite.chainA.ConstructUpdateTMClientHeader(suite.chainB, clientID)
suite.Require().NoError(err)

msg, err := clienttypes.NewMsgUpdateClient(
clientID, header,
suite.chainA.SenderAccount.GetAddress(),
)

result, err := suite.chainA.SendMsgs(msg)
suite.Require().NoError(err)
// first event type is "message"
updateEvent := result.Events[1]

suite.Require().Equal(clienttypes.EventTypeUpdateClient, updateEvent.Type)

// use a boolean to ensure the update event contains the header
contains := false
for _, attr := range updateEvent.Attributes {
if string(attr.Key) == clienttypes.AttributeKeyHeader {
contains = true

bz, err := hex.DecodeString(string(attr.Value))
suite.Require().NoError(err)

emittedHeader, err := types.UnmarshalHeader(suite.chainA.App.AppCodec(), bz)
suite.Require().NoError(err)
suite.Require().Equal(header, emittedHeader)
}

}
suite.Require().True(contains)

}
33 changes: 30 additions & 3 deletions x/ibc/core/02-client/types/encoding.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func MustUnmarshalConsensusState(cdc codec.BinaryMarshaler, bz []byte) exported.
return consensusState
}

// MustMarshalConsensusState attempts to encode an ConsensusState object and returns the
// MustMarshalConsensusState attempts to encode a ConsensusState object and returns the
// raw encoded bytes. It panics on error.
func MustMarshalConsensusState(cdc codec.BinaryMarshaler, consensusState exported.ConsensusState) []byte {
bz, err := MarshalConsensusState(cdc, consensusState)
Expand All @@ -68,12 +68,12 @@ func MustMarshalConsensusState(cdc codec.BinaryMarshaler, consensusState exporte
return bz
}

// MarshalConsensusState protobuf serializes an ConsensusState interface
// MarshalConsensusState protobuf serializes a ConsensusState interface
func MarshalConsensusState(cdc codec.BinaryMarshaler, cs exported.ConsensusState) ([]byte, error) {
return cdc.MarshalInterface(cs)
}

// UnmarshalConsensusState returns an ConsensusState interface from raw encoded clientState
// UnmarshalConsensusState returns a ConsensusState interface from raw encoded consensus state
// bytes of a Proto-based ConsensusState type. An error is returned upon decoding
// failure.
func UnmarshalConsensusState(cdc codec.BinaryMarshaler, bz []byte) (exported.ConsensusState, error) {
Expand All @@ -84,3 +84,30 @@ func UnmarshalConsensusState(cdc codec.BinaryMarshaler, bz []byte) (exported.Con

return consensusState, nil
}

// MarshalHeader protobuf serializes a Header interface
func MarshalHeader(cdc codec.BinaryMarshaler, h exported.Header) ([]byte, error) {
return cdc.MarshalInterface(h)
}

// MustMarshalHeader attempts to encode a Header object and returns the
// raw encoded bytes. It panics on error.
func MustMarshalHeader(cdc codec.BinaryMarshaler, header exported.Header) []byte {
bz, err := MarshalHeader(cdc, header)
if err != nil {
panic(fmt.Errorf("failed to encode header: %w", err))
}

return bz
}

// UnmarshalHeader returns a Header interface from raw proto encoded header bytes.
// An error is returned upon decoding failure.
func UnmarshalHeader(cdc codec.BinaryMarshaler, bz []byte) (exported.Header, error) {
var header exported.Header
if err := cdc.UnmarshalInterface(bz, &header); err != nil {
return nil, err
}

return header, nil
}
30 changes: 30 additions & 0 deletions x/ibc/core/02-client/types/encoding_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package types_test

import (
"github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/types"
ibctmtypes "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/07-tendermint/types"
)

func (suite *TypesTestSuite) TestMarshalHeader() {

cdc := suite.chainA.App.AppCodec()
h := &ibctmtypes.Header{
TrustedHeight: types.NewHeight(4, 100),
}

// marshal header
bz, err := types.MarshalHeader(cdc, h)
suite.Require().NoError(err)

// unmarshal header
newHeader, err := types.UnmarshalHeader(cdc, bz)
suite.Require().NoError(err)

suite.Require().Equal(h, newHeader)

// use invalid bytes
invalidHeader, err := types.UnmarshalHeader(cdc, []byte("invalid bytes"))
suite.Require().Error(err)
suite.Require().Nil(invalidHeader)

}
1 change: 1 addition & 0 deletions x/ibc/core/02-client/types/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const (
AttributeKeySubjectClientID = "subject_client_id"
AttributeKeyClientType = "client_type"
AttributeKeyConsensusHeight = "consensus_height"
AttributeKeyHeader = "header"
)

// IBC client events vars
Expand Down
2 changes: 1 addition & 1 deletion x/ibc/core/spec/06_events.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ callbacks to IBC applications.
| update_client | client_id | {clientId} |
| update_client | client_type | {clientType} |
| update_client | consensus_height | {consensusHeight} |
| update_client | header | {header} |
| message | action | update_client |
| message | module | ibc_client |

Expand Down Expand Up @@ -238,4 +239,3 @@ callbacks to IBC applications.
| message | action | timeout_packet |
| message | module | ibc-channel |