Emit header in MsgUpdateClient events

[colin-axner]

Description

cc @ancazamfir

closes: #8598

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[alessio]

This looks like a non breaking fix for the v0.41 branch - correct @colin-axner?

[alessio]

Tests are failing

[colin-axner]

This looks like a non breaking fix for the v0.41 branch - correct @colin-axner?

correct. Not urgent. It would just be very useful to include in the next point release, but no need to cut a point release just for this fix. It makes misbehaviour monitoring a lot easier

[colin-axner]

Tests are failing

ah yes, I will look into it

[colin-axner]

fixed, though I have a few questions now and I must admit I don't fully understand the SDK backport policy.

I'm not entirely sure how gas is calculated, but if this code increases the gas costs of a transaction, then it is possible a successful transaction on the old code, could run out of gas on the new code, causing a consensus failure. This seems like good reason not to backport this fix, but it also seems like most SDK fixes would fit this criteria.

Maybe an extra if statement and var don't increase gas costs?

[codecov]

Codecov Report

Merging #8624 (bcaab1a) into master (91affb5) will increase coverage by 0.00%.
The diff coverage is 90.90%.

@@           Coverage Diff           @@
##           master    #8624   +/-   ##
=======================================
  Coverage   61.38%   61.39%           
=======================================
  Files         672      672           
  Lines       38390    38404   +14     
=======================================
+ Hits        23565    23577   +12     
- Misses      12341    12342    +1     
- Partials     2484     2485    +1     

Impacted Files	Coverage Δ
x/ibc/core/02-client/types/encoding.go	60.00% <77.77%> (+6.15%)	⬆️
types/address.go	66.00% <100.00%> (+0.13%)	⬆️
x/ibc/core/02-client/keeper/client.go	98.27% <100.00%> (+0.06%)	⬆️

[cwgoes]

Is gas charged for emitting events, in a way which is dependent on the contents of an event (e.g. size)?

I suppose it probably should be, but I'm not sure if it is.

[colin-axner]

Based on the docs I read, it seems gas is only consumed for each read/write to the KVStore, in which case, this pr shouldn't change gas costs at all (since events aren't stored?)

[ancazamfir]

Not clear how the header is encoded, is this protobuf? Could we do the same as for send_packet.packet_data (json string)?

[colin-axner]

Not clear how the header is encoded, is this protobuf? Could we do the same as for send_packet.packet_data (json string)?

Yes this is protobuf. Emitting packet data as protobuf requires changing how the packet data is encoded before it is hashed. We could do this, the data is never stored so I don't think gas costs should change. I also don't think it is IBC breaking even though the hash would change. It might be nice to be standardized about this. I think it would be good if ICS had a recommendation here so most applications use the same encoding format. thoughts @cwgoes ?

cc @ethanfrey

[ancazamfir]

Emitting packet data as protobuf requires...

I actually meant NOT doing protobuf for the header but rather follow the same encoding as send_packet.packet_data.
On a side note, I tried to use the new update event (used your branch) and couldn't decode the header. It is very likely I don't know what I am doing, just tried to reuse the decoders we have for the header in the client update messages (btw, those are the value fields of the Any, Vec<u8> and not String).

This is what I get for the update client header:

"update_client.header": ["signed_header:<header:<version:<block:11 > chain_id:\"ibc-1\" height:360 time:<seconds:1613655287 nanos:38513000 > last_block_id:<hash:\"\\177*\\371\\274}\\025\\035\\311\\253\\226\\273\\264\\271\\244\\312\\360x&\\276\\037\\352\\224\\177\\327\\235\\207@\\304\\276\\224P_\" part_set_header:<total:1 hash:\"Eb\\341\\270n\\273Nx\\266(&\\247E\\366\\362\\245/\\252\\360iR\\3534\\317\\257Y\\325\\354\\332\\372E\\032\" > > last_commit_hash:\"I\\345\\004\\365\\246T[ \\304`#\\244#T\\244\\207\\221w\\004\\304\\257^\\217\\350j;\\301\\351\\273y\\234\\266\" data_hash:\"\\343\\260\\304B\\230\\374\\034\\024\\232\\373\\364\\310\\231o\\271$\'\\256A\\344d\\233\\223L\\244\\225\\231\\033xR\\270U\" validators_hash:\"q\\3576\\257\\336k\\003\\211\\241\\360\\224>\\204i4\\237\\323\\322#\\320W\\241\\177\\375\\200\\202\\001\\262\\016\\020\\035\\361\" next_validators_hash:\"q\\3576\\257\\336k\\003\\211\\241\\360\\224>\\204i4\\237\\323\\322#\\320W\\241\\177\\375\\200\\202\\001\\262\\016\\020\\035\\361\" consensus_hash:\"\\004\\200\\221\\274}\\334(?w\\277\\277\\221\\327<D\\332X\\303\\337\\212\\234\\274\\206t\\005\\330\\267\\363\\332\\255\\242/\" app_hash:\"\\213\\205\\355\\235?\\253\\005QrrILn\\373h\\rs\\221\\301\'5\\r!\\217o\\346n\\260\\360\\203\\347x\" last_results_hash:\"\\343\\260\\304B\\230\\374\\034\\024\\232\\373\\364\\310\\231o\\271$\'\\256A\\344d\\233\\223L\\244\\225\\231\\033xR\\270U\" evidence_hash:\"\\343\\260\\304B\\230\\374\\034\\024\\232\\373\\364\\310\\231o\\271$\'\\256A\\344d\\233\\223L\\244\\225\\231\\033xR\\270U\" proposer_address:\"\\305\\221Hn\\377\\2030$//\\017-vC.\\030\\306\\022\\201`\" > commit:<height:360 block_id:<hash:\"\\256\\256\\356\\221\\351LB\\004\\235GfY\\te\\275\\303\\374\\215\\376\\217\\212\\216:G}\\330\\343\\327Uf1\\212\" part_set_header:<total:1 hash:\"O\\254\\376q\\033\\217Qrc\\264e\\262\\num\\034\\311N\\240\\317\\201\\2206\\237\\366\\343\\325\'>.\\372\\327\" > > signatures:<block_id_flag:BLOCK_ID_FLAG_COMMIT validator_address:\"\\305\\221Hn\\377\\2030$//\\017-vC.\\030\\306\\022\\201`\" timestamp:<seconds:1613655288 nanos:108274000 > signature:\"NaR:\\302/<6a|Q\\267\\021\\222\\203\\031\\242JN (\\255~\\2160\\211\\336D\\332\\355I\\223\\2420p\\273\\017d\\224\\377Cg\\366\\302\\\"\\267\\375iH\\244/\\364\\264\\177\\025\\362t\\t\\220\\367\'\'0\\005\" > > > validator_set:<validators:<address:\"\\305\\221Hn\\377\\2030$//\\017-vC.\\030\\306\\022\\201`\" pub_key:<ed25519:\"c>\\3247z5\\370\\301\\321\\244\\016\\351\\335\\221\\240\\030*\\\\\\363\\362\\233\\354\\246\\345n\\220I:\\230H\\2047\" > voting_power:100000 > proposer:<address:\"\\305\\221Hn\\377\\2030$//\\017-vC.\\030\\306\\022\\201`\" pub_key:<ed25519:\"c>\\3247z5\\370\\301\\321\\244\\016\\351\\335\\221\\240\\030*\\\\\\363\\362\\233\\354\\246\\345n\\220I:\\230H\\2047\" > voting_power:100000 > total_voting_power:100000 > trusted_height:<revision_number:1 revision_height:342 > trusted_validators:<validators:<address:\"\\305\\221Hn\\377\\2030$//\\017-vC.\\030\\306\\022\\201`\" pub_key:<ed25519:\"c>\\3247z5\\370\\301\\321\\244\\016\\351\\335\\221\\240\\030*\\\\\\363\\362\\233\\354\\246\\345n\\220I:\\230H\\2047\" > voting_power:100000 > proposer:<address:\"\\305\\221Hn\\377\\2030$//\\017-vC.\\030\\306\\022\\201`\" pub_key:<ed25519:\"c>\\3247z5\\370\\301\\321\\244\\016\\351\\335\\221\\240\\030*\\\\\\363\\362\\233\\354\\246\\345n\\220I:\\230H\\2047\" > voting_power:100000 > total_voting_power:100000 > "],

and here is an example for the packet_data:

"send_packet.packet_data": ["{\"amount\":\"1000\",\"denom\":\"samoleans\",\"receiver\":\"cosmos15lce6zu4k6f29ncfvnltuqc87ygm63l4tfdfn0\",\"sender\":\"cosmos142vru9qlvdpa3g5l58lqj9y5wfqsucqdzt335r\"}"]

[colin-axner]

@ancazamfir I can update it to use JSON

[ancazamfir]

Looks good now, thanks @colin-axner!

[ancazamfir]

One more thing, it looks like headers are not stored in IBC. So, on (re)start the relayer would have to get them from the Tx events.

[colin-axner]

Awesome @ancazamfir! That's great to hear!

One more thing, it looks like headers are not stored in IBC. So, on (re)start the relayer would have to get them from the Tx events.

Yes, this is the unfortunate tradeoff made for the minimalist design of IBC, I think it falls into the same category as #8593.

Perhaps one partial work around is to use the consensus state comparison on re(start) and only attempt to obtain the header from Tx events if the consensus states do not match. You'd still need to parse the tx events, but maybe it'd be a little more efficient. I think this is probably what the tendermint light client would need to do if it incorporates misbehaviour monitoring. Or perhaps the tendermint light client simply notifies the relayer that there is misbehaviour at height X and provides one of the misbehaving headers

[fedekunze]

ACK, modulo resolving the question I asked

[fedekunze]

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

[colin-axner]

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

[AdityaSripal]

Looks good, thanks @colin-axner