cosmos / interchain-security

Interchain Security is an open sourced IBC application which allows cosmos blockchains to lease their proof-of-stake security to one another.
https://cosmos.github.io/interchain-security/
Other
154 stars 116 forks source link

Divergence spec and impl: slashing after tombstoning. #220

Closed danwt closed 2 years ago

danwt commented 2 years ago

The spec has not mention of tombstoning (see here) but the code does use tombstoning

https://github.com/cosmos/interchain-security/blob/4e40e093e6b682732057c06ce41fe32715f24478/x/ccv/provider/keeper/relay.go#L217-L240

There is a mismatch between the code and the spec.

What is the intended behavior?

@mpoke we said today that after a double sign is received the validator shall be tombstoned. But what if downtime slashing packets are subsequently received? Should they be ignored, as they are in the code?

https://github.com/cosmos/interchain-security/blob/4e40e093e6b682732057c06ce41fe32715f24478/x/ccv/provider/keeper/relay.go#L217-L219

or not?

mpoke commented 2 years ago

what if downtime slashing packets are subsequently received? Should they be ignored, as they are in the code?

Yes, that's the expected behavior.

danwt commented 2 years ago

See

danwt commented 2 years ago

Reopened :)

mpoke commented 2 years ago

what if downtime slashing packets are subsequently received? Should they be ignored, as they are in the code?

Yes, that's the expected behavior.

I'm not sure actually that we want this. Should a validator that is already slashed and jailed for double-signing be also slashed for downtime on another consumer chain? This would be important as a deterrent for tombstoned validators to not stop validating on the consumer chains (especially during channel initialization). @okwme What do you think?

danwt commented 2 years ago

See also

danwt commented 2 years ago

For the record to proceed with testing I will take the code as correct, until we have an update from product side.

okwme commented 2 years ago

once a validator is tombstoned on the hub it is just a matter of time until the validator is removed from all consumer chains right? if the validator stops validating on all consumer chains before the packet is received that removes them from the validator set i don't think it makes sense for trying to slash them for this. if anything it's "more" correct. they should not be validating after being tombstoned, it's just a matter of time for that to be propogated. it seems that it would also complicate the code to have them further slashed. i think it is unnecessary to slash them further for missed blocks after being tombstoned.

jtremback commented 2 years ago

How it currently works in the implementation:

Downtime:

Double Sign:

danwt commented 2 years ago

Can we close this?

mpoke commented 2 years ago

The spec and the code are still inconsistent. The question is, do we need to mention tombstoning in the spec?

danwt commented 2 years ago

Hmmm good question. I think we should. If not, we should explicitly mention in the spec that there are divergent details w.r.t slashing module.

mpoke commented 2 years ago

I opened an issue on cosmos/ibc (https://github.com/cosmos/ibc/issues/820). Thus, we can close this.