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 17 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
committed malicious behavior. The relevant information is forwarded to the
Copy link
Contributor

Choose a reason for hiding this comment

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

darn 'mericans

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.

Heh, I originally spelled it the British way, but the spellchecker wasn't a fan.

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
174 changes: 174 additions & 0 deletions docs/spec/slashing/state-machine.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
## 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.


### Conceptual overview

#### States

At any given time, there are any number of validator candidates registered in the state machine.
Copy link
Contributor

Choose a reason for hiding this comment

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

validator candidates -> bonded validators
also generally remove use of candidates throughout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Validator candidates aren't necessarily bonded, do you mean "validators / bonded validators"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got rid of "validator candidates" in this paragraph, which was the only location.

Copy link
Contributor

Choose a reason for hiding this comment

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

the term validators includes validators in all states - so if you want to say validators/bonded validators you might as well just say validators

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.

Exactly; I mean "validators" instead of "validator candidates", and "bonded validators", for the top n - which I did I think (lmk if I missed any).

Each block, the top `n` candidates who are not jailed become *bonded*, meaning that they may propose and vote on blocks.
Copy link
Contributor

Choose a reason for hiding this comment

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

clarify what n means - if that is the maximum bonded validators param then we should say this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 , clarified

Validators who are *bonded* are *at stake*, meaning that part or all of their stake is at risk if they commit a protocol fault.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something that all their stake and ALL of their delegators stake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.


#### Slashing period

In order to mitigate the impact of initially likely categories of non-malicious protocol faults, the Cosmos Hub implements for each validator
a *slashing period*, in which the amount by which a validator can be slashed is capped at the punishment for the worst violation. For example,
Copy link
Contributor

Choose a reason for hiding this comment

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

is the slashing period a different length than the unbonding period? Let's draw the distinction

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 slashing period doesn't have a particular length, it's determined by when the validator bonds or unbonds. Clarified that difference.

if you misconfigure your HSM and double-sign a bunch of old blocks, you'll only be punished for the first double-sign (and then immediately jailed,
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a realistic example? (fine if so) I just don't want to scare people into thinking if they slip up somehow in configuration they get messed - we should make hsm configuration pretty robust

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 think it is a realistic example, that's why the slashing period exists.

Agreed that we want to develop secure HSM software though!

so that you have a chance to reconfigure your setup). This will still be quite expensive and desirable to avoid, but slashing periods somewhat blunt
the economic impact of unintentional misconfiguration.

A new slashing period starts whenever a validator is bonded and ends whenever the validator is unbonded (which will happen if the validator is jailed).
The amount of tokens slashed relative to validator power for infractions committed within the slashing period, whenever they are discovered, is capped
at the punishment for the worst infraction (which for the Cosmos Hub at launch will be double-signing a block).

##### ASCII timelines

*Code*

*[* : timeline start
*]* : timeline end
*<* : slashing period start
*>* : slashing period end
*C<sub>n</sub>* : infraction `n` committed
*D<sub>n</sub>* : infraction `n` discovered
*V<sub>b</sub>* : validator bonded
*V<sub>u</sub>* : validator unbonded

*Single infraction*

<----------------->
[----------C<sub>1</sub>----D<sub>1</sub>,V<sub>u</sub>-----]
Copy link
Contributor

Choose a reason for hiding this comment

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

Although I like how compact it is, I think that a numbered list would more clearly and understandably depict timelines... pretty confusing to have to look back and forth from the codes and timeline here, especially when rendered in text like here in the PR diff - I'd suggest we switch even if this becomes more verbose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A numbered list doesn't really allow for parallel tracks; maybe a table? Leaving for now, but I agree that this could be cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh geez that's right - yeah we should upgrade to use a table then

Copy link
Contributor

@rigelrozanski rigelrozanski Aug 23, 2018

Choose a reason for hiding this comment

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

Okay - I reverse my opinion - I actually really like the markdown rendering - I think the ultimate solution is to just include graphics however

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 agree, maybe a separate PR though?


A single infraction is committed then later discovered, at which point the validator is unbonded and slashed at the full amount for the infraction.

*Multiple infractions*

<---------------------------->
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the slashing period ends when the validator is jailed & unbonded (Vu), wouldn't that mean the slashing period should extend to Vu?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those two events are simultaneous, not quite sure how to express that best in ASCII, maybe I'll try stacking them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think stacking them might help -- either way, not a big deal because the spec describes this well enough.

[----------C<sub>1</sub>--C<sub>2</sub>---C<sub>3</sub>---D<sub>1</sub>,D<sub>2</sub>,D<sub>3</sub>V<sub>u</sub>-----]

Multiple infractions are committed within a single slashing period then later discovered, at which point the validator is unbonded and slashed for only the worst infraction.

*Multiple infractions after rebonding*


<---------------------------->&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<-------------->
[----------C<sub>1</sub>--C<sub>2</sub>---C<sub>3</sub>---D<sub>1</sub>,D<sub>2</sub>,D<sub>3</sub>V<sub>u</sub>---V<sub>b</sub>---C<sub>4</sub>----D<sub>4</sub>,V<sub>u</sub>--]
Copy link
Contributor

Choose a reason for hiding this comment

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

hellish

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I like the rendered output, but if there's a better way to do this in Markdown I'm all ears

Copy link
Contributor

@rigelrozanski rigelrozanski Aug 23, 2018

Choose a reason for hiding this comment

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

see previous comment - but also I think that the arrows are out bit (see lower most <------------>)

screen shot 2018-08-23 at 3 39 15 pm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the ASCII diagrams a bit but I'm not sure if they'll render exactly the same on different browsers, feel free to update if they still look off.


Multiple infractions are committed within a single slashing period then later discovered, at which point the validator is unbonded and slashed for only the worst infraction.
The validator then unjails themself and rebonds, then commits a fourth infraction - which is discovered and punished at the full amount, since a new slashing period started
when they unjailed and rebonded.
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 that protocol actions can be added directly into the bulleted list I've suggested above -> this means that each of these scenarios can JUST be the bulleted list, all explanations should happen within the list

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 think a list is sufficient, see the above comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, agreed


### 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 validator has enough stake to be in the top hundred, they will be automatically rebonded,
Copy link
Contributor

Choose a reason for hiding this comment

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

top n?

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 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, which `StartHeight` of the current block, `EndHeight` of `0` (sentinel value for not-yet-ended),
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
```

##### Safety note
Copy link
Contributor

Choose a reason for hiding this comment

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

this safety note section should probably be moved into the conceptual-overview.md doc I suggested in an earlier comment

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, done.


Slashing is capped fractionally per period, but the amount of total bonded stake associated with any given validator can change (by an unbounded amount) over that period.

For example, with MaxFractionSlashedPerPeriod = `0.5`, if a validator is initially slashed at `0.4` near the start of a period when they have 100 steak bonded,
then later slashed at `0.4` when they have `1000` steak bonded, the total amount slashed is just `40 + 100 = 140` (since the latter slash is capped at `0.1`) -
whereas if they had `1000` steak bonded initially, the total amount slashed would have been `500`.
Copy link
Contributor

Choose a reason for hiding this comment

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

... if they had 1000 steak bonded initially ... and slashed at what fraction? 0.4? Wouldn't that be 400?

Copy link
Contributor Author

@cwgoes cwgoes Aug 20, 2018

Choose a reason for hiding this comment

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

0.4 * 1000 = 400 for the first offense and 0.1 * 1000 = 100 for the second, total 500.

Clarified the sentence.


This means that any slashing events which utilize the slashing period (are capped-per-period) **must** *also* jail the validator when the infraction is discovered.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this essentially saying (at least at launch), that a validator can only be slashed, in a given slashing period, at most the cost of a double sign OR missing enough signatures (at which point they'd be jailed + 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.

Only the former, downtime is not capped by the slashing period (although being unbonded for downtime does reset the slashing period).

Clarified in begin-block.md.

Copy link
Contributor

Choose a reason for hiding this comment

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

**must** *also* is funny formatting but okay

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't achieve the desired effect of extra-special-emphasis? Ah well, changed to just bold.

Copy link
Contributor

Choose a reason for hiding this comment

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

lol

Otherwise it would be possible for a validator to slash themselves intentionally at a low bond, then increase their bond but no longer be at stake since they would have already hit the `SlashedSoFar` cap.

### 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
JailedUntilHeight int64 // Block height until which the validator is jailed,
// 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 block height 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.

👍

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 any reason. When the validator rejoins the validator set (perhaps through unjailing themselves,
and perhaps also changing signing keys), they enter into a new period.

Slashing periods are indexed in the store as follows:

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

This allows us to look up slashing period by a validator's address, the only lookup necessary,
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.