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

Query Evidence doesn't return evidences #8360

Closed
4 tasks
sahith-narahari opened this issue Jan 18, 2021 · 12 comments · Fixed by #8502
Closed
4 tasks

Query Evidence doesn't return evidences #8360

sahith-narahari opened this issue Jan 18, 2021 · 12 comments · Fixed by #8502
Assignees

Comments

@sahith-narahari
Copy link
Contributor

Summary of Bug

The existing query evidence doesn't return handled evidences.

The issue may be because we're not storing the evidence here, it's only being handled but not committed to the store

func (k Keeper) HandleEquivocationEvidence(ctx sdk.Context, evidence *types.Equivocation) {

Version

master

Steps to Reproduce

Simulate a validator double sign on a network using simapp

cc @alessio @tessr


For Admin Use

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

Yes this was confusing to me. In reading the spec it says all submitted valid evidence is persisted. This is not the case for tendermint based evidence but is the case when it's submitted through the SubmitEvidence method. Not sure how important this is for stargate since the evidence from Tendermint can be queried through blocks but it would be a breaking change (I believe)

@sahith-narahari
Copy link
Contributor Author

yes, this seems to be inconsistent with the behavior of SubmitEvidence and looks like a breaking change.

@alexanderbez
Copy link
Contributor

Note there are two means of evidence handling and submission:

  1. Automatically which is sent by Tendermint and handled in BeginBlock. This is NOT persisted as it's not externally submitted.
  2. Manually handled by external submission. If valid, these will be persisted.

I suppose the doc should make this distinction clear.

@sahith-narahari
Copy link
Contributor Author

thanks, in that case this seems to be intended behavior and not a bug

@sahith-narahari
Copy link
Contributor Author

One question, from application standpoint wouldn't we want sdk to offer a way to query such incidents?

@aaronc
Copy link
Member

aaronc commented Jan 18, 2021

There grpc query method could just query tendermint for this evidence. I'd rather not 1) make a state breaking change for this now and 2) unnecessarily duplicate tendermint state in SDK modules.

@zmanian
Copy link
Member

zmanian commented Jan 18, 2021

This seems like something that could/ should be in a post stargate upgrade to me.

@alexanderbez
Copy link
Contributor

alexanderbez commented Jan 18, 2021

thanks, in that case this seems to be intended behavior and not a bug

Correct. This was always the behavior. All versions of the hub never persisted evidence submitted automatically by Tendermint.

@sahith-narahari
Copy link
Contributor Author

If there's consensus that we need this post stargate, I can make a PR for the same

@tessr
Copy link
Contributor

tessr commented Jan 19, 2021

I think that, even if this is the intended behavior, there's probably something to be done to make it less confusing/surprising.

@alexanderbez
Copy link
Contributor

I think that, even if this is the intended behavior, there's probably something to be done to make it less confusing/surprising.

Yes, just also persist evidence in HandleEquivocationEvidence (any all infraction handlers).

@amaury1093
Copy link
Contributor

amaury1093 commented Feb 5, 2021

I'd like to propose an alternate solution: tendermint/tendermint#5977.

So the gRPC query handler would:

  • read all evidences from TM db (assuming there's no issues with pruning on that db)
  • read all evidences from state
  • return the combined array

(related #8360 (comment))

@mergify mergify bot closed this as completed in #8502 Feb 5, 2021
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.

7 participants