Tendermint: ABCI: Switch to list of lists of tags for transaction results, begin block, and end block

[zramsay]

original issue: https://github.com/tendermint/abci/issues/273

[cwgoes]

Ref https://github.com/tendermint/tendermint/pull/1803

[cwgoes]

Also, community suggestion: easy way via API to query all events for a particular block (which can then be parsed through and stored elsewhere, e.g. SQL DB).

[cwgoes]

(thanks @ajcronk for the suggestion!)

[ValarDragon]

Is this needed prelaunch? AFAICT this is to increase indexing speed. (Which I think can be punted to postlaunch, unless we don't want to break ABCI soon after launch. If this is true, I think we should make stubs for more ABCI changes). Per Anton's comment in the original issue, I agree that we shouldn't re-invent the wheel here, and should aim to be compatible with existing databases. e.g. https://github.com/tendermint/tendermint/issues/1161

[cwgoes]

Is this needed prelaunch? AFAICT this is to increase indexing speed. (Which I think can be punted to postlaunch, unless we don't want to break ABCI soon after launch. If this is true, I think we should make stubs for more ABCI changes). Per Anton's comment in the original issue, I agree that we shouldn't re-invent the wheel here, and should aim to be compatible with existing databases. e.g. #1161

Why would this increase indexing speed?

The reason on the SDK side is to allow prefixed error codes for particular modules and avoid the necessity of inventing a complex codespacing system to deal with potential module conflicts.

[ValarDragon]

Oh woops your right, I completely misunderstood the comments on the prev issue.

If this is the goal though, why don't we make the results a list of structs consisting of a kv pair, k being byte slice and v being list of byte slices. Then we could also sort the results by key to get log n searching.

[cwgoes]

If this is the goal though, why don't we make the results a list of structs consisting of a kv pair, k being byte slice and v being list of byte slices. Then we could also sort the results by key to get log n searching.

How would that preserve the connection between tags associated with a particular event?

It's not that we need to support multiple values-per-tag-key, it's that we want to be able to return multiple indexable/searchable "events" per transaction / BeginBlock / EndBlock, e.g. for multiple validators being slashed for downtime in the same BeginBlock.

[ebuchman]

@cwgoes @faboweb thoughts on prioritizing this pre/post launch?

[cwgoes]

@cwgoes @faboweb thoughts on prioritizing this pre/post launch?

Probably the most important use case for this at launch is slashing events, multiple of which can happen per abci.RequestBeginBlock. Without this, I don't see an easy way to report any number of slashing events in an easy-to-query manner. I guess we could prefix tags or try another disambiguation method.

[ebuchman]

Noting https://github.com/tendermint/tendermint/pull/2314#discussion_r222155615

Also not entirely sure what @jaekwon is saying in https://github.com/tendermint/tendermint/pull/2314#issuecomment-423740293

[melekes]

Didn't we explicitly get rid of types here at one point?

yes, but I'd vouch to rethink if the current design does not satisfy our needs

This is cool, but I think it means we'd need to unmarshal the JSON and index the keys in there too if we want to support eg. a query for "validator X slashed"

that won't not be a problem for Postgresql indexer, KV indexer will require modification

[melekes]

Do you know if the reasoning for this was documented anywhere?

https://github.com/tendermint/tendermint/issues/1369#issuecomment-377169038 I think the reasoning was it's just simpler and we don't loose that much in term of performance ([]byte string requires parsing) https://github.com/tendermint/tendermint/issues/1369#issuecomment-383884334

I wasn't able to find anything else, sorry.

[ebuchman]

Removing this as a launch requirement. The current indexing system will do for now, but we may want to consider some change to the Tags data structure as we better flesh out the full indexing story (eg. how it will integrate with external indexers like mongo or postgres)

[cwgoes]

Removing this as a launch requirement. The current indexing system will do for now, but we may want to consider some change to the Tags data structure as we better flesh out the full indexing story (eg. how it will integrate with external indexers like mongo or postgres)

What was the conclusion - was it this comment or somewhere else? I'm not clear on what we should do SDK/Gaia-side in the case of multi-Msg transactions, multiple slashing events in BeginBlock/EndBlock, etc.

[ebuchman]

was it this comment

I didn't entirely understand that comment TBH.

I'm not clear on what we should do SDK/Gaia-side in the case of multi-Msg transactions, multiple slashing events in BeginBlock/EndBlock, etc.

Seems there's still a bit of uncertainty on how best to do this - we probably want to support types in the tags, and either do lists of lists, or some other kind of thing. I think we should work towards addressing this in the first hard fork.

In the meantime, SDK should do the simplest thing that will meet MVP Hub goals. That could mean just using the order to delineate, like []Tags{ {"validator", X}, {"slashed", 5}, {"validator", Y}, {"slashed", 10} } or even putting each event in JSON. Let's go with something dead simple that we expect to change later.

[cwgoes]

I didn't entirely understand that comment TBH.

No, nor did I, hence my confusion.

In the meantime, SDK should do the simplest thing that will meet MVP Hub goals. That could mean just using the order to delineate, like []Tags{ {"validator", X}, {"slashed", 5}, {"validator", Y}, {"slashed", 10} } or even putting each event in JSON. Let's go with something dead simple that we expect to change later.

OK! https://github.com/cosmos/cosmos-sdk/issues/2448

[jackzampolin]

So @alexanderbez spent some time trying to implement this in the SDK and his judgement was that we needed it to be a tendermint change. https://github.com/cosmos/cosmos-sdk/pull/3171

Is it possible to start work on this feature?

[melekes]

Linking https://github.com/tendermint/tendermint/issues/1385#issuecomment-441190176 for other ways to solve the core problem here.

[ebuchman]

See the original PR here: https://github.com/tendermint/tendermint/pull/1803

Shouldn't be too difficult to resuscitate it.

Also see https://github.com/tendermint/tendermint/pull/1803#discussion_r245723160 - an event should look like:

message Event {
  string type
  repeated common.KVPair attributes
}

Then Responses can include repeated Event events

[ebuchman]

This is still an important feature, but has been deemed less critical from a Cosmos launch perspective. Since we're no longer planning to hash tags into the header at this point (#1007), making this change will not break existing blockchains, so we can make it more leisurely. I will move it to a slightly later milestone

[ethanfrey]

1385 was closed in favor of this, but I don't understand how this works for the client queries and suscriptions. This proposes a solution (list of list of tags), but the rationale behind it, or specific examples of how it will make life easier for the client queries, are missing.

Can you please add a summary of the changes to the issue description?

[melekes]

Sorry for confusion. We'll write a spec. There are two distinct issues, but new events format will require modifying indexing and querying code anyway. So I was hoping we could fix both issues.

[alexanderbez]

@ethanfrey mind taking a look/review of #3643? It accomplishes what is outlined in this issue. Namely, using list-of-lists so we can support aggregate and more complex queries in ABCI responses. In addition, it also utilizes your suggestion from #1385 in using map[string][]string, so you can have subscriptions like you've mentioned in that issue.

[melekes]

Merged https://github.com/tendermint/tendermint/pull/3643 to develop. Thanks @alexanderbez 👍