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

R4R: Update slashing spec for slashing-by-period #2001

Merged
merged 27 commits into from
Aug 24, 2018
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
4f8c9e4
Update transactions.md
cwgoes Aug 13, 2018
84e9b21
Fix typo
cwgoes Aug 13, 2018
98a278d
Reorganize sections
cwgoes Aug 13, 2018
ff01cbb
Update state.md
cwgoes Aug 13, 2018
53fa4a2
Start update of state-machine.md
cwgoes Aug 13, 2018
a8af4a4
End block to begin block, add README
cwgoes Aug 13, 2018
07a7db7
Update links
cwgoes Aug 13, 2018
2445718
State machine contd.
cwgoes Aug 13, 2018
8b7d6e0
Update state machine, contd.
cwgoes Aug 13, 2018
52475b1
Fix minor typos
cwgoes Aug 14, 2018
32263ff
Merge branch 'develop' into cwgoes/slashing-period-spec
cwgoes Aug 14, 2018
a2463d0
Clarify points from PR review
cwgoes Aug 14, 2018
21be609
Changes WIP
cwgoes Aug 14, 2018
79e3c05
Revert "Changes WIP" - we decided not to do this
cwgoes Aug 20, 2018
94dc512
Fix typos
cwgoes Aug 20, 2018
e3cb1e1
Add safety note
cwgoes Aug 20, 2018
b8d6465
Conceptual overview & ASCII diagrams of slashing period
cwgoes Aug 20, 2018
da92b1b
h_n => h_n-1
cwgoes Aug 20, 2018
d0c87ff
Go => pseudocode
cwgoes Aug 20, 2018
c9e5745
Clarify example
cwgoes Aug 20, 2018
f188955
Clarify which offenses utilize the slashing period
cwgoes Aug 20, 2018
41df6db
Merge branch 'develop' into cwgoes/slashing-period-spec
cwgoes Aug 22, 2018
73e1965
Update PENDING.md
cwgoes Aug 22, 2018
bb9c265
Merge branch 'develop' into cwgoes/slashing-period-spec
cwgoes Aug 22, 2018
4cc2054
Address @rigelrozanski comments
cwgoes Aug 23, 2018
4475a8c
Fixup links
cwgoes Aug 23, 2018
63a85aa
Change ASCII diagram slightly
cwgoes Aug 23, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions docs/spec/slashing/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Slashing module specification

## Abstract

This section specifies the slashing module of the Cosmos SDK, which implements functionality
first outlined in the [Cosmos Whitepaper](https://cosmos.network/about/whitepaper) in June 2016.

The slashing module enables Cosmos SDK-based blockchains to disincentivize any attributable action
by a protocol-recognized actor with value at stake by "slashing" them: burning some amount of their
stake - and possibly also removing their ability to vote on future blocks for a period of time.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be more clear to include these two items as numbered bullets - also use of "possibly" seems a bit out of place - I think slashing and jailing are independant penalties which are both optional to a penalty

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, changed to a list.


This module will be used by the Cosmos Hub, the first hub in the Cosmos ecosystem.

## Contents

1. **[State](state.md)**
1. [SigningInfo](state.md#signing-info)
1. [SlashingPeriod](state.md#slashing-period)
1. **[State Machine](state-machine.md)**
1. [Transactions](state-machine.md#transactions)
1. Unjail
1. [Interactions](state-machine.md#interactions)
1. Validator Bonded
1. Validator Unbonding
1. Validator Slashed
1. [State Cleanup](state-machine.md#state-cleanup)
1. **[Begin Block](begin-block.md)**
1. [Evidence handling](begin-block.md#evidence-handling)
1. [Uptime tracking](begin-block.md#uptime-tracking)
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
# End-Block
# Begin-Block

## Slashing
## Evidence handling

Tendermint blocks can include
[Evidence](https://github.com/tendermint/tendermint/blob/develop/docs/spec/blockchain/blockchain.md#evidence), which indicates that a validator
committed malicious behaviour. The relevant information is forwarded to the
application as [ABCI
Evidence](https://github.com/tendermint/tendermint/blob/develop/abci/types/types.proto#L259), so the validator an be accordingly punished.
Evidence](https://github.com/tendermint/tendermint/blob/develop/abci/types/types.proto#L259) in `abci.RequestBeginBlock`
so that the validator an be accordingly punished.

For some `evidence` to be valid, it must satisfy:

Expand Down Expand Up @@ -75,7 +76,7 @@ This ensures that offending validators are punished the same amount whether they
act as a single validator with X stake or as N validators with collectively X
stake.

## Automatic Unbonding
## Uptime tracking

At the beginning of each block, we update the signing info for each validator and check if they should be automatically unbonded:

Expand Down
106 changes: 106 additions & 0 deletions docs/spec/slashing/state-machine.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
## Transaction & State Machine Interaction Overview
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be an h1 heading and and all subsequent headers increased but not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure; updated & also updated in state.md.


### Transactions

In this section we describe the processing of transactions for the `slashing` module.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary intro, transactions should be its own file (transactions.md) to stay consistent with other specs - I'd then rename state-machine.md to something like conceptual-overview.md or something of the like - just a thought

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure; moved the intro to overview.md and the transactions to transactions.md.


#### TxUnjail

If a validator was automatically unbonded due to downtime and wishes to come back online &
possibly rejoin the bonded set, it must send `TxUnjail`:

```golang
type TxUnjail struct {
ValidatorAddr sdk.AccAddress
}

handleMsgUnjail(tx TxUnjail)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a mixed bag of pseudo-code and Go -- I think we should just stick to one. I guess in this case, can we just add braces?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, thanks - I'd rather it be pseudocode, updated to be less Go-like, let me know if it's better now.


validator := getValidator(tx.ValidatorAddr)
if validator == nil
fail with "No validator found"

if !validator.Jailed
fail with "Validator not jailed, cannot unjail"

info := getValidatorSigningInfo(operator)
if BlockHeader.Time.Before(info.JailedUntil)
fail with "Validator still jailed, cannot unjail until period has expired"

// Update the start height so the validator won't be immediately unbonded again
info.StartHeight = BlockHeight
setValidatorSigningInfo(info)

validator.Jailed = false
setValidator(validator)

return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the return statement is implied here and thus can be removed (and all the other pseudo functions which do not have early returns in this file)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this I did to maintain consistency with the other specs, e.g. https://github.com/cosmos/cosmos-sdk/blob/develop/docs/spec/staking/transactions.md

```

If the validater has enough stake to be in the top hundred, they will be automatically rebonded,
and all delegators still delegated to the validator will be rebonded and begin to again collect
provisions and rewards.

### Interactions

In this section we describe the "hooks" - slashing module code that runs when other events happen.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Interactions or "hooks" should maybe be separated into a new file - in distribution I use "triggers" - not sure if that's the most ideal - but we should def stay consistent between the two specs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Within piggy bank, I also included "triggered-by" section for each "trigger" could be good to outline these here as well for each hook

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think I'm using the standard terminology here, e.g. https://en.wikipedia.org/wiki/Hooking

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to hooks.md

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh interesting - I'll update piggy bank to use a hooks.md too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also lol
screen shot 2018-08-23 at 3 45 34 pm


#### Validator Bonded

Upon successful bonding of a validator (a given validator changing from "unbonded" state to "bonded" state,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing from "unbonded" state to "bonded" state, not necessarily true, can also be coming from unbonding state. I'd just say "entering the bonded state"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that we have two terms: "unbonded" == "not bonded", and "unbonded == sdk.Unbonded state", because we call the state itself unbonded it's unclear what to refer to the former by.

I like your wording better anyways but maybe we should consider renaming the state.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with "unbonded" == "not bonded" I think by definition:
"unbonded" || "unbonding" == "not bonded"

Copy link
Contributor Author

@cwgoes cwgoes Aug 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but "unbonded" is also an English word, e.g. https://www.dictionary.com/browse/unbonded or https://www.collinsdictionary.com/dictionary/english/unbonded, which means "not bonded" (is the antonym of "bonded") - and I presume this etymology is the same root as for Cosmos; e.g. https://en.wikipedia.org/wiki/Bonded_warehouse (perhaps)?

What do you think we should call the state of "not bonded", if not "unbonded"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I don't use "unbonded" this way in the spec anymore though)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay - yeah confusing. We should probably just completely avoid the language unbonded and only refer to things AS bonded/unbonding/unbonded - and define this strictly as meaning one of the 3 states of tokens.

However I will draw the subtle generalized distinction (as I understand it) between the prefix "un-" and the the adverb "not". The word "not" is rooted in the idea of exclusion "exclude a person or part of a group", whereas "un-" when used as prefix forming verbs and verbal derivatives (unbonding = a state = a verb) is used to refer to the deviation or movement from that original state. SO in this context I would sustain that the state which is excluding the bonded state is unique from the state which is the deviation from the bonded state - aka
superset(unbonded, unbonding) == not bonded
subset(not bonded) == unbonded || unbonding

"subtle" lol

which may happen on delegation, on unjailing, etc), we create a new `SlashingPeriod` structure for the
now-bonded validator, wich `StartHeight` of the current block, `EndHeight` of `0` (sentinel value for not-yet-ended),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wich -> with?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed

and `SlashedSoFar` of `0`:

```golang
onValidatorBonded(address sdk.ValAddress)

slashingPeriod := SlashingPeriod{
ValidatorAddr : address,
StartHeight : CurrentHeight,
EndHeight : 0,
SlashedSoFar : 0,
}
setSlashingPeriod(slashingPeriod)

return
```

#### Validator Unbonded

When a validator is unbonded, we update the in-progress `SlashingPeriod` with the current block as the `EndHeight`:

```golang
onValidatorUnbonded(address sdk.ValAddress)

slashingPeriod = getSlashingPeriod(address, CurrentHeight)
slashingPeriod.EndHeight = CurrentHeight
setSlashingPeriod(slashingPeriod)

return
```

#### Validator Slashed

When a validator is slashed, we look up the appropriate `SlashingPeriod` based on the validator
address and the time of infraction, cap the fraction slashed as `max(SlashFraction, SlashedSoFar)`
(which may be `0`), and update the `SlashingPeriod` with the increased `SlashedSoFar`:

```golang
beforeValidatorSlashed(address sdk.ValAddress, fraction sdk.Rat, infractionHeight int64)

slashingPeriod = getSlashingPeriod(address, infractionHeight)
totalToSlash = max(slashingPeriod.SlashedSoFar, fraction)
slashingPeriod.SlashedSoFar = totalToSlash
setSlashingPeriod(slashingPeriod)

remainderToSlash = slashingPeriod.SlashedSoFar - totalToSlash
fraction = remainderToSlash

continue with slashing
```

### State Cleanup

Once no evidence for a given slashing period can possibly be valid (the end time plus the unbonding period is less than the current time),
old slashing periods should be cleaned up. This will be implemented post-launch.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section State Cleanup should probably be moved to a new file future-improvements.md to stay consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

37 changes: 33 additions & 4 deletions docs/spec/slashing/state.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,11 @@ The information stored for tracking validator liveness is as follows:

```go
type ValidatorSigningInfo struct {
StartHeight int64
IndexOffset int64
JailedUntil int64
SignedBlocksCounter int64
StartHeight int64 // Height at which the validator became able to sign blocks
IndexOffset int64 // Offset into the signed block bit array
JailedUntil int64 // Block height until which the validator is jailed,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't JailedUntilHeight make a better name? I usually assume things like Until/After/Before are times

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, now JailedUntilHeight

// or sentinel value of 0 for not jailed
SignedBlocksCounter int64 // Running counter of signed blocks
}

```
Expand All @@ -49,3 +50,31 @@ Where:
* `IndexOffset` is incremented each time the candidate was a bonded validator in a block (and may have signed a precommit or not).
* `JailedUntil` is set whenever the candidate is revoked due to downtime
* `SignedBlocksCounter` is a counter kept to avoid unnecessary array reads. `SignedBlocksBitArray.Sum() == SignedBlocksCounter` always.

### Slashing Period

A slashing period is a start and end time associated with a particular validator,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's really a start and end block, right?

Copy link
Contributor Author

@cwgoes cwgoes Aug 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks, updated

within which only the "worst infraction counts": the total amount of slashing for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "worst-infraction-counts" is a good principal to outline in the conceptual-overview.md doc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then it could just be linked here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So linked.

infractions committed within the period (and discovered whenever) is capped at the
penalty for the worst offense.

This period starts when a validator is first bonded and ends when a validator is slashed & jailed
for double-signing (but does not end if they are slashed & jailed for just missing blocks).
Copy link
Contributor

@alexanderbez alexanderbez Aug 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the last part is pretty important and long -- it doesn't seem to belong in parentheses. Thoughts? In other words, I think it'll read easier with just a comma :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below comment.

When the validator voluntarily unjails themselves (and possibly changes signing keys), they reset the period.
Copy link
Contributor

@alexanderbez alexanderbez Aug 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help clarify for me one case? What if a validator misses enough blocks to be slashed and jailed -- the slashing period doesn't end. The validator can then, after some time, unjail themselves, correct? If so, is the period reset even though it never ended (they never double signed)? Does that mean they never get penalized for liveliness cost?

I'm sure I'm missing something here, sorry.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump @cwgoes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup! you are right, this is unclear - addressing

Copy link
Contributor Author

@cwgoes cwgoes Aug 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I don't think we need this, we should just end the period when they're unbonded for any reason.

If you think we do need it, please comment further...


Slashing periods are indexed in the store as follows:

- SlashingPeriod: ` 0x03 | ValTendermintAddr | StartHeight -> amino(slashingPeriod) `

This allows us to look up slashing period by validator address, the only lookup necessary,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight rewording, This allows us to look up a slashing period by the validator's address ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

and iterate over start height to efficiently retrieve the most recent slashing period(s)
or those beginning after a given height.

```go
type SlashingPeriod struct {
ValidatorAddr sdk.ValAddress // Tendermint address of the validator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this is the Tendermint Addr - I almost think the variable should be renamed to something a bit more explicit

Copy link
Contributor Author

@cwgoes cwgoes Aug 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like what? It's called ValidatorAddr and has type sdk.ValAddress, that seems pretty explicit, we call the operator address validator.Operator

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack.

StartHeight int64 // Block height at which slashing period begin
EndHeight int64 // Block height at which slashing period ended
SlashedSoFar sdk.Rat // Fraction slashed so far, cumulative
}
```
19 changes: 0 additions & 19 deletions docs/spec/slashing/transactions.md

This file was deleted.