docs: event hashing ADR 058

[melekes]

👉 Rendered 👈

Closes #5113

[auto-comment]

👋 Thanks for creating a PR!

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.

• Wrote tests
• Updated CHANGELOG_PENDING.md
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Updated relevant documentation (docs/) and code comments
• Re-reviewed Files changed in the Github PR explorer
• Applied Appropriate Labels

Thank you for your contribution to Tendermint! 🚀

[cwgoes]

Looks fine, just a note that we should make sure it is easy in the SDK to specify per-event, e.g. using the ctx.EmitEvents() function, whether or not an event should be hashed into the header.

[alexanderbez]

Thanks for writing this formally @melekes. LGTM!

[tac0turtle]

LGTM

[erikgrinaker]

It might be nice to specify how they are to be hashed, e.g. Merkle tree vs. sequential, ordering, and hash algo.

[melekes]

cc @ebuchman

[ebuchman]

Thanks. I think this looks fine. We could add more clarity but it's spelled out in more detail in the corresponding spec changes.

Some loose thoughts: Having the event types in the tendermint consensus params does feel a bit weird though - is it really better than having an extra field in the event? We could probably do a better job of weighing the difference here. With the current proposal it means the ConsensusParams could get a lot bigger and there's more we need to track in the Tendermint State. With an extra field we'd have more flexibility, like an app could sometimes decide to include an event of some type and sometimes not, but maybe that's a bad thing, and too implicit. Though I'd expect apps to have some stateful representation of what events they are hashing.

Anyways, I'd also seek input from @ValarDragon @ethanfrey @liamsi @adlerjohn @mappum @zmanian if they have time to think about it... ideally in the future these kinds of changes will happen in more formal RFCs :)

[melekes]

Having the event types in the tendermint consensus params does feel a bit weird though

agree here

[ValarDragon]

Thanks for writing this up!

However I'm overall confused as to why adding / removing event names breaks "LastResultsHash" (and thereby, why we need this as a consensus parameter). Why do I need to know all the event types in order to verify or create proofs?

As I understand it, the procedure should be as follows:
Each Event has a type, and some other additional data. In each of these responses I am given a list of events that should be merkelized to get provability. To merkelize, I set leaf_i := serialize(events[i]). Then the verifier can still get lite client proofs against this as before, without needing to know the name of all events, they only must know the name of the event they care about.

Even if you want tproofs that you are given all events of a certain type or proofs of non-inclusion, you still don't need to know all the event names beforehand to verify or create such proofs. (This just requires the property that you can 'order' the event types, e.g. ordering them alphabetically, since you sort the event types before merkelizing)

In particular, I feel like the application should be able to define a new event type every block and use it immediately, without causing Tendermint issues.

[ethanfrey]

Complexity is always nice in the design phase (have your cake and eat it too), but leads to lots of tech debt, confusion, and often misleading docs down the line.

I see 3 simple options:

1. Keep the event hash out of the block header (same as 0.33)
2. Add all events hash to the block header (current behavior on master)
3. One global toggle to switch between (1) and (2) - a simpler version of this proposal

I think the whole event search/subscribe behavior needs to be rethought anyway with the Begin/EndBlockers.

My personal preference is: stick with (1) for 0.34. Go to (2) when sdk team and others in the ecosystem are ready.

If you are stuck in the position where some clients want (1) and some want (2) now, then my option (3) - global swtich can address that, and is much simpler than the full proposed solution

[ebuchman]

However I'm overall confused as to why adding / removing event names breaks "LastResultsHash" (and thereby, why we need this as a consensus parameter). Why do I need to know all the event types in order to verify or create proofs?

@ValarDragon you don't - the motivation here is that the event system as an API has a large surface area and is a significant integration point that may take a long time to stabilize. Hence you want to be able to make updates to the kinds of events that are fired and their fields/structure to enable newer/better integrations and pub-sub behaviour, but you don't want those kinds of changes to break the blockchain.

So there's this tension between using events as (1) a non-consensus critical pub-sub mechanism to integrate against and (2) a consensus critical "proof of action" system for light clients.

The proposal allows the application to control which subset of its event system is consensus critical (ie hashed into the header) and which are not. Those that are not can continue to evolve (add/remove fields, add new events, etc.) without breaking the chain.

My personal preference is: stick with (1) for 0.34. Go to (2) when sdk team and others in the ecosystem are ready.

@ethanfrey the challenge here is again around the idea that event systems will completely stabilize. In so far as events are used for pub-sub and as a major integration point, it seems likely that apps may always want to be able to emit new events that aren't consensus critical, even if some events are.

Another possibility here which is perhaps preferred for now until there's more stability/motivation/use-cases/demand here is to push this entirely application side and just have apps which want events to be provable to insert them into their application-side merkle trees. Of course this puts more pressure on their application state and makes event proving application specific, but it might help built up a better sense of use-cases and how this ought to ultimately be done by Tendermint.

[ValarDragon]

So there's this tension between using events as (1) a non-consensus critical pub-sub mechanism to integrate against and (2) a consensus critical "proof of action" system for light clients.

The proposal allows the application to control which subset of its event system is consensus critical (ie hashed into the header) and which are not. Those that are not can continue to evolve (add/remove fields, add new events, etc.) without breaking the chain.

Thanks for the expl, I think i get it now. Handling the filter seems like something the app should handle.

If these non-merkelized txs should be added to pre-existing pub-sub logic, perhaps it makes sense for there to be two lists of events in these responses? This way Tendermint doesn't need to filter where the events go, instead the SDK filters them before handing them off to ABCI.

[ebuchman]

This way Tendermint doesn't need to filter where the events go, instead the SDK filters them before handing them off to ABCI.

Right, the alternative proposal being considered was to add a hash bool field to the Event to indicate whether Tendermint should hash the event or not, and then it would be fully controlled by the app and Tendermint wouldn't know anything up front, but this has a downside of being somewhat less explicit about what events get hashed. With the list in the consensus params, it's pretty up front and clear, though it is a bit awkward ...

[ValarDragon]

I see, the downside being now the pub-sub module of Tendermint can't return nice lists for what events are provably queryable, and which aren't. Now I get the ADR, thanks for the expl! (Perhaps more context should be in the intro? Though I'm also the only one who was confused lol)

Instead of having a list of event strings that specify whether or not to merkelize, I'd prefer a KV-map if its easy w/ protobuf. The key being the string, and the value being a 'merkelization type' enum. Seems plausible to me that there me be multiple desired ways to merkelize/accumulate event attributes for different querying modes the app may want (in line with #1007 (comment))

[ValarDragon]

The easy to upgrade via governance bit doesn't seem true to me. Presumably the SDK would have a governance updatable filter.

[melekes]

Are u saying neither proposal makes it easy to upgrade on the SDK side? (cc @alexanderbez )

[ValarDragon]

I'm a bit surprised by this. My impression of events is that the following 3 query cases would be most common:

1. Just track my tx's
2. Gather data for all tx's of a certain type
3. Gather some per-block stats about the txs

If (3) is indeed a notable use case for many lite clients, then perhaps these should be merkelized? (Or at least merkelized within end-block).

[melekes]

we can do that, sure

[mappum]

Another possibility here which is perhaps preferred for now until there's more stability/motivation/use-cases/demand here is to push this entirely application side and just have apps which want events to be provable to insert them into their application-side merkle trees. Of course this puts more pressure on their application state and makes event proving application specific, but it might help built up a better sense of use-cases and how this ought to ultimately be done by Tendermint.

Just want to give a +1 for this. I've always thought the beauty of Tendermint is the clean separation between app and consensus - in essence just a state hash and some transaction data. Anything else is just extra complexity, and I feel like things have slightly overfitted to the Cosmos SDK, leaving alternative stacks with more features to integrate or locking them into less flexible solutions.

In this case we are pretty agnostic to whatever the decision is, but if we were to use some sort of events we would probably keep them within our own merkle tree so we can do it in a way that fits with the rest of our stack.

[tomtau]

this alternative seems to be a simpler proposal IMHO?

[melekes]

it is

[melekes]

Thanks all for your input! ❤️ It looks like leaving it to the application is a way to go for now. I'm going to revert some of the changes made in https://github.com/tendermint/tendermint/pull/4845/files, specifically adding BeginBlock#Events, EndBlock#Events and ResponseDeliverTx#Events. NOTE: GasWanted/GasUsed will not be reverted.

[ebuchman]

Do we have rationale for GasWanted / GasUsed or do we have a similar argument for leaving it to the application?

GasWanted would already be accessible and provable directly in the tx (at least eg. in an sdk tx and prob in others like an eth tx has a max gas.

GasUsed can currently be adjusted downwards without breaking the chain, and may in some cases be adjusted upwards. Not clear if that's a useful degree of freedom, but in any case we'd lose it by hashing.

I guess the more general question here is how coupled the ABCI interface ought to be with the light client provability ...