Document IBC misbehaviour handling in an ADR

[colin-axner]

Summary

confusion has continued to arise around the notion of misbehaviour (formerly referred to as evidence) for IBC light clients. Some of this discussion is well documented below. We should translate these notions, decisions and understanding into an ADR

ref: https://github.com/cosmos/cosmos-sdk/pull/7145#discussion_r475583334

---

For Admin Use

• [ ] Not duplicate issue
• [ ] Appropriate labels applied
• [ ] Appropriate contributors tagged
• [ ] Contributor assigned/self-assigned

[AdityaSripal]

I've mentioned this before in a Slack channel, but I still don't see why we would need to support any of the Tendermint evidence apart from ConflictingHeaderEvidence (which is something we've reimplemented and we may wish to reuse the TM ConflictinHeadersEvidence type)?

// copied from Slack
core IBC does not need to understand any Tendermint specific evidence (including DuplicateVoteEvidence). 
It just needs to check that there hasn't been a fork

Or a Fork-Light if the ibc client is bisecting

So it just needs to check if its light client can verify two seperate headers for the same height 
(which is already fully implemented)

From my understanding, the tendermint abci evidence works at the abstraction level of faulty 
individual validators, while core IBC is not concerned with faulty individual validators 
but faulty validator sets

Think the new forms of evidence are more relevant for x/slashing and future cross-chain 
validation applications 
(who would encode any evidence into packet data and thus be opaque to core IBC code), 
unless im missing something here 

Of the Evidence types located here, the only one I see as relevant to core IBC is ConflictingHeadersEvidence.

All other forms of evidence are indicative of a particular validator being malicious rather than the entire validatorset, thus IBC should not take any action on its light client on the basis of Validator-misbehaviour. It should only freeze the light-client on the basis of ValidatorSet-misbehaviour (ie a successful lite- or full-fork of the chain), which is already handled in the 07-tendermint misbehaviour logic.

@cwgoes @cmwaters

[cwgoes]

Yes, sorry, my comment was badly phrased, we only want to freeze the client in cases where the client would have possibly updated to one of two headers, but I think this can happen without double-signing, it can happen in the case of lunatic validators for example. I guess we don't need more types than we currently have, just the ability to submit the two headers, but we should change the comment above, it's not about wrapping DuplicateVoteEvidence, it's about any two headers at the same height which both validate.

[AdityaSripal]

the client would have possibly updated to one of two headers, but I think this can happen without double-signing, it can happen in the case of lunatic validators for example

Yes, I think most misbehavior is the case where the light-client would have accepted one of two headers for a given block height. All of those examples, regardless of how specifically they are pulled off, can already be handled by using the Misbehaviour logic and structs currently implemented in IBC.

---

I can only imagine one other far-fetched scenario where this is not the case but we still might want to handle. Suppose there is a Lunatic validator attack being pulled off by (>2/3 of the validator set) such that they can produce a single header for a given height, but this header happens to be invalid. (Note with >2/3 power, they can prevent any conflicting valid headers from being committed). Of course, in the cases where the invalidity is state-machine specific we can't do anything. Let's say the validators signed off on invalid state transitions, the AppHash will be "invalid", but as a light client there is nothing the IBC client can do about this. And since there's only one header for this height, the IBC client can't be frozen in this way

The only invalid fields, we could reasonably do something about is if the attack changes ValidatorsHash or LastResultsHash as this can be proven invalid by submitting the previous Header at h-1. Though, for an attack that is so powerful that they can coordinate to avoid conflicting headers, there is no necessity to change these fields at all as they can arbitrarily fool light clients by just changing app hash so in my opinion it's probably not worth bothering about

[cwgoes]

I can only imagine one other far-fetched scenario where this is not the case but we still might want to handle. Suppose there is a Lunatic validator attack being pulled off by (>2/3 of the validator set) such that they can produce a single header for a given height, but this header happens to be invalid. (Note with >2/3 power, they can prevent any conflicting valid headers from being committed). Of course, in the cases where the invalidity is state-machine specific we can't do anything. Let's say the validators signed off on invalid state transitions, the AppHash will be "invalid", but as a light client there is nothing the IBC client can do about this. And since there's only one header for this height, the IBC client can't be frozen in this way

The only invalid fields, we could reasonably do something about is if the attack changes ValidatorsHash or LastResultsHash as this can be proven invalid by submitting the previous Header at h-1. Though, for an attack that is so powerful that they can coordinate to avoid conflicting headers, there is no necessity to change these fields at all as they can arbitrarily fool light clients by just changing app hash so in my opinion it's probably not worth bothering about

This is an interesting point; I agree that we probably don't need to bother about it for now, worth keeping in mind for the future.