add additional slash packet checks

[MSalopek]

Please do not merge

Difftests are not working due to additional slash packet validation (ValidateBasic).

Description

Downtime slash packets will be dropped in HandleSlashPacket if a SlashAck already exists for a given chain and given validator. This prevents validators being spammed with downtime infractions without ever unjailing themselves.

Linked issues

EDIT: Partial fix for #544

Type of change

If you've checked more than one of the first three boxes, consider splitting this PR into multiple PRs!

• Feature: Changes and/or adds code behavior, irrelevant to bug fixes

[MSalopek]

Sending these packets would cause an error in provider.OnRecvSlashPacket since the slash packet would be considered outdated.

[shaspitz]

I was also running into problems with the tests in this file when first attempting to solve #546.

Imo we should change the test behavior instead of commenting out

[mpoke]

Uncomment the tests and use the following reasoning to populate the valsetUpdateID of consumerKeeper.QueueSlashPacket:

// a downtime infraction identified at the current height
// has the infraction height set as follows (cf. SDK slashing module)
infractionHeight := s.consumerCtx().BlockHeight() - sdk.ValidatorUpdateDelay - 1

// for a double-sing infraction at consumer height H
// has the infraction height set as follows (cf. SDK evidence module)
infractionHeight := H - sdk.ValidatorUpdateDelay

For both downtime and double-signing, send SlashPackets using the following valsetUpdateID

infractionVSCid := consumerKeeper.GetHeightValsetUpdateID(s.consumerCtx(), uint64(infractionHeight))

[mpoke]

Uncomment the tests and use the following reasoning to populate the valsetUpdateID of consumerKeeper.QueueSlashPacket:

// a downtime infraction identified at the current height
// has the infraction height set as follows (cf. SDK slashing module)
infractionHeight := s.consumerCtx().BlockHeight() - sdk.ValidatorUpdateDelay - 1

// for a double-sing infraction at consumer height H
// has the infraction height set as follows (cf. SDK evidence module)
infractionHeight := H - sdk.ValidatorUpdateDelay

For both downtime and double-signing, send SlashPackets using the following valsetUpdateID

infractionVSCid := consumerKeeper.GetHeightValsetUpdateID(s.consumerCtx(), uint64(infractionHeight))

[MSalopek]

@smarshall-spitzbart I think this needs to be redone on my part. I think the point of the test got lost, right?

In this version, the second slash cannot be applied since the SlashAck for given validator already exists. The slash acks are consumed (deleted) in endblock.

[shaspitz]

Hmm it's not completely clear what the original intention of the test was, considering the comment:

The test cases verify that only the unbonding tokens get slashed for the VSC ids mapping to the block heights before and during the undelegation otherwise not.

I think Marius may be more familiar with this functionality. @mpoke do you know what this test is validating?

[shaspitz]

Regarding slash acks as a mechanism to achieve

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.

(From #544)

Slash acks are appended for a particular chain only once that slash has been handled. Therefore I don't think slash acks work as a mechanism to achieve the closing criteria defined in the issue.

A malicious consumer could still send a bunch of downtime slash packets that are recv all in block N, those packets would all be queued during block N. During the end blocker of block N, one of the queued downtime slash packets could be handled, which would append the slash ack. Note that if the slash meter is negative in block N, no slash ack would be appended in block N. Even if a slash ack was appended in block N, this would only prevent spam until that slash ack is consumed, and the spam packets would only be dequeued if the slash ack exists at the time of handling.

To prevent spam, we need to be adding logic to ValidateSlashPacket, which runs before the slash packet is added to the queue.

[MSalopek]

Regarding slash acks as a mechanism to achieve

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.

(From #544)

Slash acks are appended for a particular chain only once that slash has been handled. Therefore I don't think slash acks work as a mechanism to achieve the closing criteria defined in the issue.

A malicious consumer could still send a bunch of downtime slash packets that are recv all in block N, those packets would all be queued during block N. During the end blocker of block N, one of the queued downtime slash packets could be handled, which would append the slash ack. Note that if the slash meter is negative in block N, no slash ack would be appended in block N. Even if a slash ack was appended in block N, this would only prevent spam until that slash ack is consumed, and the spam packets would only be dequeued if the slash ack exists at the time of handling.

To prevent spam, we need to be adding logic to ValidateSlashPacket, which runs before the slash packet is added to the queue.

After adding the requested changes I'm setting this to draft since difftests are failing because of the new functionality.

Current difftest do not account for the fact that LastDowntimeValsetUpdateId must increase monotonically and thus creates traces where the valsetUpdateId in the slash packet is less than LastDowntimeValsetUpdateId.

Updating difftests will take will take an unknown amount of time for me to handle.

[MSalopek]

Please don't merge - difftests are not working due to additional slash packet validation (ValidateBasic).

Validate basic was removed, PR can be merged if need be.

[shaspitz]

If this PR will still close #544, it'll need to address the closing criteria of

RunSlashPacketData.ValidateBasic on receiving SlashPackets

Otherwise we can make that criteria it's own issue.

@MSalopek @mpoke I still do not understand how slash acks accomplish the following behavior in this PR

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.

AFAIK slash acks have nothing to do with unjailing, and slash acks are deleted once included in a VSC packet. It seems this comment still applies, and the current changes prevent spam only in a very small subset of cases, not the cases defined in the issue

[MSalopek]

If this PR will still close #544, it'll need to address the closing criteria of

RunSlashPacketData.ValidateBasic on receiving SlashPackets

Otherwise we can make that criteria it's own issue.

@MSalopek @mpoke I still do not understand how slash acks accomplish the following behavior in this PR

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.

AFAIK slash acks have nothing to do with unjailing, and slash acks are deleted once included in a VSC packet. It seems this comment still applies, and the current changes prevent spam only in a very small subset of cases, not the cases defined in the issue

Hi, thanks for the insight.

ValidateBasic was removed since it broke difftests in a way that is not super trivial to fix since it adds new requirements that need to be reflected in the difftest setup/trace generation.

This PR insures the validator has unjailed itself in case of multiple subsequent downtime requests from the same chain.

ValidateBasic addition can be tracked in the existing issue or in a new one if you prefer. I would prefer the current issue remain open, so no context is lost.

[mpoke]

AFAIK slash acks have nothing to do with unjailing, and slash acks are deleted once included in a VSC packet.

@smarshall-spitzbart True. Which means that if two SlashPackets are received in the same block from the same chain requesting to slash the same validator for downtime, then the second packet is dropped. If the two packets are received in separate blocks, then the second packet will go through and the validator will be slashed. Thus, you are right Shawn. This doesn't fix the issue. @MSalopek

[MSalopek]

@mpoke @smarshall-spitzbart Thanks for your insights.

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.

After discussing with @smarshall-spitzbart it seems that this requirement cannot be fulfilled with basic checks that are currently proposed in this PR. We would need to keep additional state to achieve that, and that would need to be thoroughly tested.

The basic checks may still hold some value, but I am in favor of closing this PR as it stands now.

[mpoke]

Closing and opening new issues: #635 and #634