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

txs: indexing at runtime #6277

Closed
4 tasks
tac0turtle opened this issue May 24, 2020 · 14 comments · Fixed by #7121
Closed
4 tasks

txs: indexing at runtime #6277

tac0turtle opened this issue May 24, 2020 · 14 comments · Fixed by #7121
Assignees

Comments

@tac0turtle
Copy link
Member

tac0turtle commented May 24, 2020

Summary

KV.Pair has been replaced by EventAttribute in the upcoming 0.34 release of Tendermint to enable tx indexing at runtime.

ref tendermint/tendermint#4466

Proposal

Enable indexing at runtime.

I'm not sure the best design to do this, a simple approach would be to do it on the msg but I dont think everyone running a node would want this.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez
Copy link
Contributor

alexanderbez commented May 25, 2020

Thanks for raising this issue @marbar3778. I'm not clear on the best approach to take either. Before, node operators were in charge of what to index and to even index at all. Now, we expand that model to allow the application to do this as well. So it's a mix of concerns. As an operator, I can disable indexing all events, yet I will still index events if the EventAttribute specifies it.

Then, the question becomes if and what should the SDK set index: true to for any given event?


One approach could be to hook into the EventManager somehow. There could be a package-level variable, IndexAllEvents. If true, the event manager will inject index: true into all the events.

Thoughts @aaronc?

@tac0turtle
Copy link
Member Author

I opened an issue in Tendermint tendermint/tendermint#4877 because this will cause confusion and it seems the application will take precedent over node operator settings.

Comment with your thoughts @alexanderbez @aaronc.

@tac0turtle
Copy link
Member Author

Could the sdk expose a section the app.toml that allows node operators to have control over which txs to index?

Currently, if the application wants to index something it will be indexed but in the Tendermint config.toml they can also set what to index, this feature could be brought up to the sdk and the node operator set which txs to index the same way it is done in tendermint then baseapp or somewhere just checks the list of keys to make sure index: bool is set to true on those txs.

@alexanderbez
Copy link
Contributor

I agree that there should be a single place where this is configured, as multiple touchpoints for it will just make it more difficult to debug and provide a less seamless UX. That single place as it stands right now is Tendermint.

Being that Tendermint is what actually does in the indexing, it seems weird to me to have the SDK decide on what to index, so I'd vote to keep that config in Tendermint. In addition, what's the difference between setting a whitelist in Tendermint vs the SDK semantically speaking?

then baseapp or somewhere just checks the list of keys to make sure index: bool is set to true on those txs.

I don't follow. How do you set index: true on a tx? Those are set on events.

@tac0turtle
Copy link
Member Author

tac0turtle commented Jun 10, 2020

Before an event that was sent over from the application to Tendermint consisted of:

message Pair {
  bytes key   = 1;
  bytes value = 2;
}

now:

// EventAttribute represents an event to the indexing service.
message EventAttribute {
  bytes key   = 1;
  bytes value = 2;
  bool  index = 3;
}

--

If an application sets some to true but in Tendermint the node operator does not have anything specified it will index the txs anyways. The proposal I was making is to bring this granularity to the sdk. Within events.go when creating Events here:

- func (a Attribute) ToKVPair() tmkv.Pair {
+ func (a Attribute) ToEventAttribute() abci.EventAttribute {
	- return tmkv.Pair{Key: toBytes(a.Key), Value: toBytes(a.Value)}
	+ if tmstring.StringInSlice(compositeTag, txi.compositeKeysToIndex) {
	+ 	return abci.EventAttribute{Key: toBytes(a.Key), Value: toBytes(a.Value), Index: true}
	+ }
	+ return abci.EventAttribute{Key: toBytes(a.Key), Value: toBytes(a.Value), Index: false}
}

(the if is copied from tendermint but it would look something like that)

you would just loop through some values defined in the app.toml and set index to true on what is needed

@alexanderbez
Copy link
Contributor

+ if tmstring.StringInSlice(compositeTag, txi.compositeKeysToIndex) {
	+ 	return abci.EventAttribute{Key: toBytes(a.Key), Value: toBytes(a.Value), Index: true}
	+ }

What is compositeTag? What is txi? We don't have those in the SDK. Just the event itself. I don't think the SDK should manually set index: true.

@tac0turtle
Copy link
Member Author

meant it as an example. it would be the values set in app.toml. We can discuss in the morning. Will the sdk not adopt runtime indexing in the next rrelease?

@alexanderbez
Copy link
Contributor

alexanderbez commented Jun 10, 2020

What will be set? What values? I don't understand. Can you give concrete example of what the config would look like?

Will the sdk not adopt runtime indexing in the next rrelease?

What does this mean? The SDK does not index at all. It simply returns events. Tendermint does the indexing.

@alexanderbez
Copy link
Contributor

alexanderbez commented Jun 10, 2020

I think I see what you're getting at. Are you suggesting that Tendermint remove indexing config/filtering altogether? That could work.

This means you'd solely rely on index: true in order to determine if you should index a tx or not in Tendermint. The point being, you have a single place to define this.

Then, the config would look something like:

index_events: [
 "message.sender",
 "message.recipient",
  ...
]

@tac0turtle
Copy link
Member Author

tac0turtle commented Jun 10, 2020

yes that is the idea, Tendermint only relies on applications telling which txs to index

the config you mention would just live in the apps toml or whatever they use

@alexanderbez
Copy link
Contributor

Ok, I think that simplifies the mental model. In order to accomplish this, Tendermint needs to solely rely on index: bool AND remove index_events list.

@alexanderbez alexanderbez self-assigned this Jun 24, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@alexanderbez
Copy link
Contributor

@marbar3778 why was this closed even though I added the pinned label?

@alexanderbez alexanderbez reopened this Jul 12, 2020
@tac0turtle
Copy link
Member Author

I think since the stale label wasn't removed. I'll look into it tomorrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants