Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Divergence spec and impl: slashing after tombstoning. #220

Closed
danwt opened this issue Jul 8, 2022 · 12 comments
Closed

Divergence spec and impl: slashing after tombstoning. #220

danwt opened this issue Jul 8, 2022 · 12 comments
Assignees
Labels
type: bug Issues that need priority attention -- something isn't working

Comments

@danwt
Copy link
Contributor

danwt commented Jul 8, 2022

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

if k.slashingKeeper.IsTombstoned(ctx, consAddr) {
return false, nil
}
// slash and jail validator according to their infraction type
// and using the provider chain parameters
var (
jailTime time.Time
slashFraction sdk.Dec
)
switch data.Infraction {
case stakingtypes.Downtime:
// set the downtime slash fraction and duration
// then append the validator address to the slash ack for its chain id
slashFraction = k.slashingKeeper.SlashFractionDowntime(ctx)
jailTime = ctx.BlockTime().Add(k.slashingKeeper.DowntimeJailDuration(ctx))
k.AppendSlashAck(ctx, chainID, consAddr.String())
case stakingtypes.DoubleSign:
// set double-signing slash fraction and infinite jail duration
// then tombstone the validator
slashFraction = k.slashingKeeper.SlashFractionDoubleSign(ctx)
jailTime = evidencetypes.DoubleSignJailEndTime
k.slashingKeeper.Tombstone(ctx, consAddr)

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?

if k.slashingKeeper.IsTombstoned(ctx, consAddr) {
return false, nil
}

or not?

@danwt danwt added type: bug Issues that need priority attention -- something isn't working ccv-core labels Jul 8, 2022
@mpoke
Copy link
Contributor

mpoke commented Jul 8, 2022

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
Copy link
Contributor Author

danwt commented Jul 8, 2022

@danwt danwt closed this as completed Jul 8, 2022
@danwt
Copy link
Contributor Author

danwt commented Jul 11, 2022

Reopened :)

@danwt danwt reopened this Jul 11, 2022
@mpoke
Copy link
Contributor

mpoke commented Jul 11, 2022

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
Copy link
Contributor Author

danwt commented Jul 14, 2022

@danwt danwt added the spec label Jul 14, 2022
@danwt
Copy link
Contributor Author

danwt commented Jul 14, 2022

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

@okwme
Copy link
Contributor

okwme commented Aug 4, 2022

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
Copy link
Contributor

jtremback commented Aug 4, 2022

How it currently works in the implementation:

Downtime:

  • Once you get slashed for downtime, you will be jailed and slash.
  • If you get downtime on other chains you can get slashed for downtime there too
  • The more consumer chains there are, the more validators are going to get slashed (if they have correlated downtime)

Double Sign:

  • You can only get slashed once, regardless of how many times you double sign

@mpoke mpoke assigned mpoke and unassigned okwme Aug 8, 2022
@danwt
Copy link
Contributor Author

danwt commented Aug 15, 2022

Can we close this?

@mpoke
Copy link
Contributor

mpoke commented Aug 15, 2022

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

@danwt
Copy link
Contributor Author

danwt commented Aug 15, 2022

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
Copy link
Contributor

mpoke commented Aug 16, 2022

I opened an issue on cosmos/ibc (cosmos/ibc#820). Thus, we can close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Issues that need priority attention -- something isn't working
Projects
No open projects
Status: Done
Development

No branches or pull requests

4 participants