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

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

Closed
zramsay opened this issue Jul 2, 2018 · 24 comments
Labels
C:abci Component: Application Blockchain Interface T:breaking Type: Breaking Change T:enhancement Type: Enhancement
Milestone

Comments

@zramsay
Copy link
Contributor

zramsay commented Jul 2, 2018

original issue: tendermint/abci#273

@zramsay zramsay added the C:abci Component: Application Blockchain Interface label Jul 2, 2018
@ebuchman ebuchman added this to the launch milestone Jul 5, 2018
@xla xla added the T:enhancement Type: Enhancement label Jul 8, 2018
@cwgoes
Copy link
Contributor

cwgoes commented Jul 11, 2018

Ref #1803

@cwgoes
Copy link
Contributor

cwgoes commented Jul 24, 2018

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

cwgoes commented Jul 24, 2018

(thanks @ajcronk for the suggestion!)

@ebuchman ebuchman added API T:breaking Type: Breaking Change labels Jul 27, 2018
@xla xla unassigned cwgoes Aug 1, 2018
@ValarDragon
Copy link
Contributor

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

@cwgoes
Copy link
Contributor

cwgoes commented Aug 10, 2018

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

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

cwgoes commented Aug 10, 2018

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

ebuchman commented Sep 2, 2018

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

@cwgoes
Copy link
Contributor

cwgoes commented Sep 3, 2018

@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 ebuchman modified the milestones: v1.0, launch Sep 13, 2018
@ebuchman
Copy link
Contributor

ebuchman commented Oct 3, 2018

Noting #2314 (comment)

Also not entirely sure what @jaekwon is saying in #2314 (comment)

@melekes
Copy link
Contributor

melekes commented Oct 3, 2018

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

melekes commented Oct 3, 2018

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

#1369 (comment) I think the reasoning was it's just simpler and we don't loose that much in term of performance ([]byte string requires parsing) #1369 (comment)

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

@ebuchman ebuchman modified the milestones: launch, v1.0 Oct 4, 2018
@jackzampolin
Copy link
Contributor

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. cosmos/cosmos-sdk#3171

Is it possible to start work on this feature?

@melekes
Copy link
Contributor

melekes commented Dec 20, 2018

Linking #1385 (comment) for other ways to solve the core problem here.

@ebuchman ebuchman modified the milestones: v1.0, v0.30.0 Jan 13, 2019
@ebuchman ebuchman modified the milestones: v0.30.0, v0.29.0 Jan 13, 2019
@ebuchman
Copy link
Contributor

See the original PR here: #1803

Shouldn't be too difficult to resuscitate it.

Also see #1803 (comment) - an event should look like:

message Event {
  string type
  repeated common.KVPair attributes
}

Then Responses can include repeated Event events

@ebuchman
Copy link
Contributor

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

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

melekes commented May 7, 2019

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

@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 pushed a commit that referenced this issue Jun 12, 2019
## PR

This PR introduces a fundamental breaking change to the structure of ABCI response and tx tags and the way they're processed. Namely, the SDK can support more complex and aggregated events for distribution and slashing. In addition, block responses can include duplicate keys in events.

    Implement new Event type. An event has a type and a list of KV pairs (ie. list-of-lists). Typical events may look like:

"rewards": [{"amount": "5000uatom", "validator": "...", "recipient": "..."}]
"sender": [{"address": "...", "balance": "100uatom"}]

The events are indexed by {even.type}.{even.attribute[i].key}/.... In this case a client would subscribe or query for rewards.recipient='...'

    ABCI response types and related types now include Events []Event instead of Tags []cmn.KVPair.
    PubSub logic now publishes/matches against map[string][]string instead of map[string]string to support duplicate keys in response events (from #1385). A match is successful if the value is found in the slice of strings.

closes: #1859
closes: #2905

## Commits:

* Implement Event ABCI type and updates responses to use events

* Update messages_test.go

* Update kvstore.go

* Update event_bus.go

* Update subscription.go

* Update pubsub.go

* Update kvstore.go

* Update query logic to handle slice of strings in events

* Update Empty#Matches and unit tests

* Update pubsub logic

* Update EventBus#Publish

* Update kv tx indexer

* Update godocs

* Update ResultEvent to use slice of strings; update RPC

* Update more tests

* Update abci.md

* Check for key in validateAndStringifyEvents

* Fix KV indexer to skip empty keys

* Fix linting errors

* Update CHANGELOG_PENDING.md

* Update docs/spec/abci/abci.md

Co-Authored-By: Federico Kunze <[email protected]>

* Update abci/types/types.proto

Co-Authored-By: Ethan Buchman <[email protected]>

* Update docs/spec/abci/abci.md

Co-Authored-By: Ethan Buchman <[email protected]>

* Update libs/pubsub/query/query.go

Co-Authored-By: Ethan Buchman <[email protected]>

* Update match function to match if ANY value matches

* Implement TestSubscribeDuplicateKeys

* Update TestMatches to include multi-key test cases

* Update events.go

* Update Query interface godoc

* Update match godoc

* Add godoc for matchValue

* DRY-up tx indexing

* Return error from PublishWithEvents in EventBus#Publish

* Update PublishEventNewBlockHeader to return an error

* Fix build

* Update events doc in ABCI

* Update ABCI events godoc

* Implement TestEventBusPublishEventTxDuplicateKeys

* Update TestSubscribeDuplicateKeys to be table-driven

* Remove mod file

* Remove markdown from events godoc

* Implement TestTxSearchDeprecatedIndexing test
@melekes
Copy link
Contributor

melekes commented Jun 12, 2019

Merged #3643 to develop. Thanks @alexanderbez 👍

@melekes melekes closed this as completed Jun 12, 2019
firelizzard18 pushed a commit to AccumulateNetwork/tendermint that referenced this issue Feb 1, 2024
iKapitonau pushed a commit to scrtlabs/tendermint that referenced this issue Jul 10, 2024
…ermint#1863)

* Updates go crypto package to v0.17.0 (tendermint#1859)

(cherry picked from commit fd87fda)

# Conflicts:
#	go.mod
#	go.sum

* Solving conflict

---------

Co-authored-by: lasaro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:abci Component: Application Blockchain Interface T:breaking Type: Breaking Change T:enhancement Type: Enhancement
Projects
None yet
Development

No branches or pull requests

9 participants