SDK handling of new evidence types

[clevinson]

Tendermint 0.34 introduces the following new evidence types:

• Update to the latest evidence types in Tendermint Fix links to ocap #5324
• Watch for Tendermint updates (there will be more things to integrate here)

[deprecated - the list has changed]

• DuplicateVoteEvidence
• LunaticValidatorEvidence
• AmnesiaEvidence

(new evidence types are mapped in this proto oneof here).

We need to add proper of handling of these new types in the x/evidence module so that validators are slashed accordingly (see here).

[clevinson]

@cwgoes Where is the expected behavior (in terms of what kind of slashing or jailing happens) documented for these new evidence types?

[robert-zaremba]

[self note] https://docs.cosmos.network/master/modules/evidence/

[cwgoes]

@cwgoes Where is the expected behavior (in terms of what kind of slashing or jailing happens) documented for these new evidence types?

Honestly, we haven't decided yet. Generally, I would consider all of these as "severe protocol violations committed intentionally", so I suggest we treat them all the same as duplicate-voting (slash & jail the validator).

[tac0turtle]

There will be some changes happening in Tendermint in regard to the new evidence types. This work is blocked on rc4.

ref tendermint/tendermint#5288 for the progress on the update.

cc @cmwaters, do you have anything more to add?

[cwgoes]

There will be some changes happening in Tendermint in regard to the new evidence types. This work is blocked on rc4.

Do these changes need to block the SDK? Is something changing over the ABCI interface boundary?

How the light client deals with evidence is orthogonal to this issue/question, this is just about handling evidence sent over ABCI.

[tac0turtle]

You could start the work and then just add the name to the switch when it's available.

The message will stay the same but the string names will change.

message Evidence {
  string                    type      = 1;

I'll remove blocked.

[robert-zaremba]

OK, so basically will need to rebase later, right?
Please keep posted once the tendermint part will be ready.

[alexanderbez]

Either we

1. Treat all evidence types the same in x/evidence
2. We have serious discussions about how to treat each type respectively (this will probably also need to go through governance)
3. Do nothing and just stick with handling duplicate votes as we do now and have app devs implement their own evidence module

[cwgoes]

Treat all evidence types the same in x/evidence

I think this is a much better default than (3). I don't think we want to handle duplicate votes but not other evidence types - this is misleading, other kinds of attacks can be just as dangerous - either we should force developers to always handle evidence themselves (probably suboptimal) or handle all evidence types tracked by Tendermint in a reasonable default manner, which can always be overridden.

[tac0turtle]

1. Treat all evidence types the same in `x/evidence`

I think this 1 is the needed solution but also a quick solution. IMO all supported evidences should have a set of parameters. An application may not want to tombstone Equivocation, but only do a large slash. The needed changes seem to be fairly trivial (famous last words).

* DuplicateVoteEvidence

DuplicateVoteEvidence is already handled, nothing will need to change there.

Tendermint is removing an evidence parameter from the consensus parameters, ProofOfTrialPeriod. These parameters are passed to tendermint from the application in EndBlock. Depending on how the sdk handles evidence parameters from Tendermint this may lead to a breaking change.

[cmwaters]

Yes as @marbar3778 alluded to here:

There will be some changes happening in Tendermint in regard to the new evidence types. This work is blocked on rc4.

We're still working to get the evidence finished. There will only be one new evidence LightClientAttackEvidence.

As far as I can see, I think what bez said about treating the evidence as the same is a good default.

I've opened a PR relating to what the abci.Evidence will look like including the use of an enum for the evidence type which will either be DUPLICATE_VOTE or LIGHT_CLIENT_ATTACK. Hopefully this frees up the sdk team a bit more in knowing how to implement the slashing/punishment for this new evidence type

[alexanderbez]

Ok great, if we go with that option, then this PR should be very trivial -- slash & jail/tombstone. We might have the change the parameter name for the slashing % though.

[robert-zaremba]

Should I also update the Tendermint version (using a commit syntax) or we will do it later once a new Tendermint version (tag) will be pushed?

[tac0turtle]

Should I also update the Tendermint version (using a commit syntax) or we will do it later once a new Tendermint version (tag) will be pushed?

Update commit style will allow you to work with the most recent updates.

[robert-zaremba]

I've made a PR. There are few breaking changes from the latest Tendermint.

Also, the evidence types were totally reworked, so the original task description is obsolete now. None of the evidence types listed there (DuplicateVoteEvidence, LunaticValidatorEvidence, AmnesiaEvidence) exist any more.

[robert-zaremba]

It seams that this function in tendermint:

func (tm2pb) Evidence(ev Evidence, valSet *ValidatorSet) abci.Evidence {

Doesn't handle Light Client Attack. And we don't have an evidence structure for Light Client Attack in Tendermint. Is this something that Tendermint will work on in the future?

[tac0turtle]

It seams that this function in tendermint:

func (tm2pb) Evidence(ev Evidence, valSet *ValidatorSet) abci.Evidence {

Doesn't handle Light Client Attack. And we don't have an evidence structure for Light Client Attack in Tendermint. Is this something that Tendermint will work on in the future?

The new type of evidence has not been added yet. We are working on finalizing the design for the implementation. Once it is merged we will update you.

[robert-zaremba]

Thanks. I've added a checkpoint here to OP, to have a new task once this changes will arrive.

[clevinson]

Reopening, as @robert-zaremba mentioned there will still be some todo's remaining here (waiting on tendermint adding the new evidence type)

[tac0turtle]

Spec updates are needed alongside the addition of new evidence

[alexanderbez]

What updates are needed exactly @clevinson? What updates to the spec are needed?

[robert-zaremba]

@alexanderbez I think we are missing a final spec for evidence types tendermint is planning to add.

[tac0turtle]

What updates to the spec are needed?

this section needs to state it is handling two types of evidence.

I think we are missing a final spec for evidence types tendermint is planning to add.

Why? The sdk only cares about evidence that comes through the abci. If each evidence is handled differently than you would need to check the spec but now it all the same. I think other than the SDK spec updates this issue can be closed.

[robert-zaremba]

Why? The sdk only cares about evidence that comes through the abci. If each evidence is handled differently than you would need to check the spec but now it all the same. I think other than the SDK spec updates this issue can be closed.

We have a switch in evidence BeginBlocker function. If new evidence type is added there then this function will reject them. If we don't want that checks then we can remove the switch. Let me know - I can do it quickly.

[tac0turtle]

We have a switch in evidence BeginBlocker function. If new evidence type is added there then this function will reject them. If we don't want that checks then we can remove the switch. Let me know - I can do it quickly.

The new type (abci.EvidenceType_LIGHT_CLIENT_ATTACK) is checked: https://github.com/cosmos/cosmos-sdk/blob/master/x/evidence/abci.go#L24. The switch could be removed

[robert-zaremba]

I know, but if there will be a new type, then the function will log an error rather than handling it. I would prefer to have a coordinated update, with a fallback:

1. If a new evidence arrives to tendermint we should check it and handle it.
2. If, somehow, we miss it, then in the default statement we do both: error log and handle as other evidences.

[clevinson]

@marbar3778 @robert-zaremba Is there anything remaining on this issue or can it be closed?

[tac0turtle]

It can be closed. The needed changes from the ABCI have been incorporated

[robert-zaremba]

@clevinson , @cwgoes -- I was thinking to keep it open until we will have a decision from Tendermint about your plans for new Evidence types.
But you are right, we can do it later instead of keeping this issue open. 👍