Outdated SlashPackets are not dropped

[mpoke]

Problem

A corrupt consumer chain could send SlashPackets with data.vscId after sending a VSCMaturedPacket for the same vscId. These packets should be dropped.

After discussions with @MSalopek, we realized that it's correct behavior to send SlashPackets with data.vscId after sending a VSCMaturedPacket for the same vscId. This is because the data.vscId in a SlashPacket is the vscId of the VSCPacket that last updated the validator set of the block when the infraction was committed. The misbehaving validator must be punished regardless of the infraction height. data.vscId is mapped on a height on the provider that is used to determined what undelegations and redelegations should be slashed (see Slash in the staking module).

Thus, this issue is about reducing as much as possible the scenarios in which a corrupt consumer chain ends up slashing a validator without following the rules. A more complete solution will be added as part of future versions, i.e., the provider can verify the evidence for all the SlashPackets.

Closing criteria

Drop SlashPackets for which a corresponding VSCMaturedPacket was already received.

• Drop SlashPackets for downtime infraction for the same validator that were received from the same consumer without the validator having the chance to Unjail itself.
• Drop SlashPackets for downtime infraction were received from the same consumer for infraction heights smaller than the latest downtime infraction height received from that consumer.
• RunSlashPacketData.ValidateBasic on receiving SlashPackets

TODOs

• Labels have been added for issue
• Issue has been added to the ICS project

[shaspitz]

Nice! This logic can be added to validateSlashPacket after #542 is merged. I won't address this issue in 542 however, as I want to make sure that PR does not introduce any functionality changes.

[mpoke]

This logic can be added to validateSlashPacket after #542 is merged.

If we add it to validateSlashPacket, we need to check VSCMaturedPackets in the queue. Alternatively, we can drop the packets when we handle them, but then we don't sent back err ack.

[shaspitz]

Reading the new Closing criteria, the proposed validations seem safe. My concern is that these validations, along with 1-4 defined in #594, will make the slash packet validation logic pretty complex, and hard to reason about. Could we consider validations (as a part of ValidateSlashPacket) that are more broad, such as:

1. Drop double-sign slash packets (from any consumer) for vals that're already tombstoned. Note this logic already exists once the packet is handled, but not yet in ValidateSlashPacket.
2. Drop downtime slash packets (from any consumer) in ValidateSlashPacket for vals that are already jailed. Note this would change the protocol's slashing behavior, but could make our protocol implementation more simple and practical.

I'd prefer not to worry about SlashAcks, infraction heights, etc. in these validations, as I believe it can create more weird edge cases where spamming is still possible. See this comment as an example

[shaspitz]

This issue is a subtask of solving #594, and I think we need to be considering the big picture of what 594 is trying to achieve. My comment above could easily have flaws, and maybe we just need to brainstorm a wholistic solution that checks all the boxes but doesn't introduce a large amount of validation complexity or edge cases.

The Don't stop iteration through queue strategy described in the parent issue (with some tweaks) may be worth considering again

[mpoke]

Closing in favor of #635 and #634