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 (bp #8624) #8770

Merged
merged 2 commits into from
Mar 4, 2021

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Mar 3, 2021

This is an automatic backport of pull request #8624 done by Mergify.

Cherry-pick of a9b034b has failed:

On branch mergify/bp/release/v0.41.x/pr-8624
Your branch is up to date with 'origin/release/v0.41.x'.

You are currently cherry-picking commit a9b034b5f.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   x/ibc/core/02-client/keeper/client.go
	modified:   x/ibc/core/02-client/keeper/client_test.go
	modified:   x/ibc/core/02-client/types/encoding.go
	new file:   x/ibc/core/02-client/types/encoding_test.go
	modified:   x/ibc/core/02-client/types/events.go
	modified:   x/ibc/core/spec/06_events.md

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   CHANGELOG.md

To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/


Mergify commands and options

More conditions and actions can be found in the documentation.

You can also trigger Mergify actions by commenting on this pull request:

  • @Mergifyio refresh will re-evaluate the rules
  • @Mergifyio rebase will rebase this PR on its base branch
  • @Mergifyio update will merge the base branch into this PR
  • @Mergifyio backport <destination> will backport this PR on <destination> branch

Additionally, on Mergify dashboard you can:

  • look at your merge queues
  • generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.io/

* emit header in update client msg

* update CHANGELOG

* update spec

* fix nil header bug

* use JSON encoding for emitting header

* Update x/ibc/core/spec/06_events.md

* use proto for encoding

* add tests

* encode to hex before emitting header in event

* add comment addressing reasoning for hex encoding

* Update CHANGELOG.md

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

Co-authored-by: Alessio Treglia <alessio@tendermint.com>
Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
(cherry picked from commit a9b034b)

# Conflicts:
#	CHANGELOG.md
@codecov
Copy link

codecov bot commented Mar 3, 2021

Codecov Report

Merging #8770 (2500474) into release/v0.41.x (3aaf1bc) will increase coverage by 0.00%.
The diff coverage is 84.61%.

Impacted file tree graph

@@               Coverage Diff                @@
##           release/v0.41.x    #8770   +/-   ##
================================================
  Coverage            61.95%   61.96%           
================================================
  Files                  612      612           
  Lines                35417    35430   +13     
================================================
+ Hits                 21942    21953   +11     
- Misses               11155    11156    +1     
- Partials              2320     2321    +1     
Impacted Files Coverage Δ
x/ibc/core/02-client/types/encoding.go 60.00% <77.77%> (+6.15%) ⬆️
x/ibc/core/02-client/keeper/client.go 98.27% <100.00%> (+0.06%) ⬆️

@alessio alessio removed the conflicts label Mar 3, 2021
@amaury1093 amaury1093 mentioned this pull request Mar 4, 2021
9 tasks
@alessio alessio merged commit fe1f4c8 into release/v0.41.x Mar 4, 2021
@alessio alessio deleted the mergify/bp/release/v0.41.x/pr-8624 branch March 4, 2021 12:05
alessio pushed a commit that referenced this pull request Mar 4, 2021
(cherry picked from commit a9b034b)

Co-authored-by: Alessio Treglia <alessio@tendermint.com>
Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
alessio pushed a commit that referenced this pull request Mar 4, 2021
@alessio
Copy link
Contributor

alessio commented Mar 4, 2021

This has been reverted due to concerns that it could be state machine breaking

@alessio alessio removed this from the v0.41.5 milestone Mar 4, 2021
@colin-axner
Copy link
Contributor

This has been reverted due to concerns that it could be state machine breaking

What are the concerns?

@aaronc
Copy link
Member

aaronc commented Mar 4, 2021

AFAIK events are part of consensus and this is thus state machine breaking. @marbar3778 would know for sure from the TM side

@colin-axner
Copy link
Contributor

We will test the merged commit again against the hub and/or figure out why there was no consensus failure during the first test.

@colin-axner
Copy link
Contributor

This has been reverted due to concerns that it could be state machine breaking

see comment

@alessio
Copy link
Contributor

alessio commented Mar 5, 2021

@zmanian @marbar3778 @shahankhatch can we make a decision on this, please?

@tac0turtle
Copy link
Member

Its fine to merge IMO. If it helps why not.

@zmanian
Copy link
Member

zmanian commented Mar 5, 2021

I think Events are not hashed into gaia on the Hub right but I am only 70% confident this is the case.

Some investigation would be good. @marbar3778 are you confident?

@tac0turtle
Copy link
Member

tac0turtle commented Mar 5, 2021

It is safe to merge this, in the spec https://docs.tendermint.com/master/spec/core/data_structures.html#header we mention not hashing the events. I missed it earlier. Colin did a deep dive in the code as well, we confirmed it is not.

alessio pushed a commit that referenced this pull request Mar 5, 2021
@alessio alessio added this to the v0.42.0 milestone Mar 5, 2021
@alessio
Copy link
Contributor

alessio commented Mar 5, 2021

Commit has been reintroduced. This is back in v0.42.0 milestone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants