cherry-pick 8624 for v0.41.x

[colin-axner]

Description

Cherry-pick #8624 for backport

closes: #XXXX

---

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]

In case anyone has any concerns about event changes in a backport, @marbar3778 tested this change against the cosmos-hub and the node was able to sync from genesis no problem (and MsgUpdateClient msgs have been committed)

[colin-axner]

This change is added to enable relayers to do easier misbehaviour monitoring. We don't need a point release immediately, but within the next week or so would be great. Definitely before transfer are enabled

[amaury1093]

Dup of #8770?

Yes, we have a bot now! 🎉

[tac0turtle]

The other pr already has approvals. Can we merge that one and close this one?

[alessio]

Am I right in thinking that we need to backport this for v0.42 @AmauryM ?

[aaronc]

Wait this is state machine breaking right?

[amaury1093]

No, i don't think so #8624 (comment), didn't have a look at the code though

[colin-axner]

Wait this is state machine breaking right?

We checked gas and larger event consideration for this. Gas costs are based on KV store read/writes and this pr does neither. We also checked if adding another event would cause a divergence in the header hash, see comment. Running these changes against the hub did not cause a consensus failure.

Is there another consideration we are missing?

Edit: for reference, the branch used was colin/8598-testing, it used slightly outdated code but still emitted another event attribute

[tac0turtle]

Wait this is state machine breaking right?

I thought the same. I was able to sync a node with a branch colin prepared for me. He said this message was executed already, and I was able to sync, so I guess it's not.

[colin-axner]

I think going forward, it could be useful to automate syncing a point release to the hub to ensure no consensus failure happens. The SDK is pretty big and understanding exactly what might cause a state-machine breaking change is sometimes unclear or hard to determine. It would be a good preventative measure to ensure we don't do a release which is trivially state-machine breaking

[aaronc]

@marbar3778 can you clarify how events affect consensus generally in TM? It would be nice to have a definitive answer.

[tac0turtle]

it is defined here:

	// root hash of all results from the txs from the previous block
	LastResultsHash tmbytes.HexBytes `json:"last_results_hash"`

When delivertxResponse is received we build a tree and use the roothash as the last resultshash. If an event is in beginblock or endblock it is not hashed.

From my understanding this is executed as a tx, we should see divergence in this but for some reason I didnt observe this when testing, its unclear if this is a sdk issue or tendermint issue. Will spin up a node tonight to double-check.

[aaronc]

From what I see in the spec just log and info can be non-deterministic. It would be useful to fine the lines of code where that's all handled in TM...

[colin-axner]

From what I see in the spec just log and info can be non-deterministic. It would be useful to fine the lines of code where that's all handled in TM...

Here is where it is in tendermint. Note, events are not included in the hash. Only the Data, so as long as we don't put events in the Data as well, then it is safe

And it looks like events are not added into the data (which logically follows)

Could we get this backported?

[aaronc]

From what I see in the spec just log and info can be non-deterministic. It would be useful to fine the lines of code where that's all handled in TM...

Here is where it is in tendermint. Note, events are not included in the hash. Only the Data, so as long as we don't put events in the Data as well, then it is safe

And it looks like events are not added into the data (which logically follows)

Thanks @colin-axner ! @marbar3778 could we get the ABCI documentation updated to flag events as non-deterministic as well?

Could we get this backported?

Ping @clevinson @AmauryM

[tac0turtle]

It's already there: https://docs.tendermint.com/master/spec/core/data_structures.html#header. I just never checked the specs I rewrote.

adding a note in the proto files as well