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

Conversation

colin-axner
Copy link
Contributor

@colin-axner colin-axner commented Feb 18, 2021

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

@colin-axner colin-axner added x/ibc T: Dev UX UX for SDK developers (i.e. how to call our code) labels Feb 18, 2021
@alessio
Copy link
Contributor

alessio commented Feb 18, 2021

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

@alessio
Copy link
Contributor

alessio commented Feb 18, 2021

Tests are failing

@colin-axner
Copy link
Contributor Author

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
Copy link
Contributor Author

Tests are failing

ah yes, I will look into it

@colin-axner
Copy link
Contributor Author

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
Copy link

codecov bot commented Feb 18, 2021

Codecov Report

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

Impacted file tree graph

@@           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
Copy link
Contributor

cwgoes commented Feb 18, 2021

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
Copy link
Contributor Author

colin-axner commented Feb 18, 2021

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
Copy link

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

@ancazamfir I can update it to use JSON

@ancazamfir
Copy link

Looks good now, thanks @colin-axner!

@ancazamfir
Copy link

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
Copy link
Contributor Author

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

@colin-axner colin-axner mentioned this pull request Mar 3, 2021
9 tasks
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

ACK, modulo resolving the question I asked

CHANGELOG.md Outdated Show resolved Hide resolved
// 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

Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @colin-axner

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
@colin-axner colin-axner added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 3, 2021
@mergify mergify bot merged commit a9b034b into master Mar 3, 2021
@mergify mergify bot deleted the colin/8598-update-header-event branch March 3, 2021 16:40
mergify bot pushed a commit that referenced this pull request Mar 3, 2021
* 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
@colin-axner colin-axner mentioned this pull request Mar 4, 2021
9 tasks
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
(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 pushed a commit that referenced this pull request Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. T: Dev UX UX for SDK developers (i.e. how to call our code)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alter event on client updates to emit full header
10 participants