feat: Add Attribute header_hash To Update Client Event

[Pipello]

Description

This PR adds the header_hash attribute to the update client evemt

closes: #5168

• 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 and Go style guide.
• Wrote unit and integration tests.
• Updated relevant documentation (docs/) or specification (x/<module>/spec/).
• Added relevant godoc comments.
• Provide a commit message to be used for the changelog entry in the PR description for review.
• Re-reviewed Files changed in the Github PR explorer.
• Review Codecov Report in the comment section below once CI passes.

[codecov-commenter]

Codecov Report

Merging #5172 (cb47015) into main (d0cf5b6) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5172      +/-   ##
==========================================
+ Coverage   80.08%   80.11%   +0.03%     
==========================================
  Files         189      189              
  Lines       13142    13147       +5     
==========================================
+ Hits        10525    10533       +8     
+ Misses       2203     2199       -4     
- Partials      414      415       +1     

Files	Coverage Δ
modules/core/02-client/keeper/events.go	88.34% <100.00%> (+0.59%)	⬆️

... and 1 file with indirect coverage changes

[crodriguezvega]

Thank you, @Pipello! I pushed a couple of commits, one of them replacing sha1 with sha256, since I think that's the hash algorithm we use in the rest of the codebase and I guess CometBFT header is also hashed with this algorithm. Having said this, I would be very nice if you could do a sanity check and run these changes locally to make sure that the hash emitted in the events matches the hash generated by CometBFT and that can be used in the RPC endpoint header_by_hash.

[Pipello]

I pushed a couple of commits, one of them replacing sha1 with sha256, since I think that's the hash algorithm we use in the rest of the codebase and I guess CometBFT header is also hashed with this algorithm.

Thank you @crodriguezvega :)

I would be very nice if you could do a sanity check and run these changes locally to make sure that the hash emitted in the events matches the hash generated by CometBFT and that can be used in the RPC endpoint header_by_hash.

I will try to check that

[Pipello]

@crodriguezvega I was able to run 1 test but not more, I am short in time to dedicate to open source work. If you think this single test is enough then that's good. Otherwise if you or somebody would like to be safer and run more tests no problems.

[crodriguezvega]

@crodriguezvega I was able to run 1 test but not more, I am short in time to dedicate to open source work. If you think this single test is enough then that's good. Otherwise if you or somebody would like to be safer and run more tests no problems.

Thank you, @Pipello! Just for confirmation: so you were able to run a chain and after the light client was updated, you could call header_by_hash with the hash in the event and retrieve the header?

[crodriguezvega]

@Pipello I did a quick test and unfortunately the hash emitted in the event doesn't match the expected header hash. I also took a quick look at CometBFT's code and it seems this is how the hash is calculated (so it's not just a sha 256 hash of the header bytes). Therefore I am not sure how useful it is to emit the sha 256 hash, since it will not be possible to use it to query CometBFT's RPC to retrieve the header...

[crodriguezvega]

Thanks for the PR, @Pipello, but I think it's best for now to not add the attribute, since it's not as simple as we thought it would be to calculate the hash of the header that can be used with the header_by_hash RPC. Since it seems that relayers will be able to manage without the attribute, I think it's best just not spend time on this right now.