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

cwgoes · 2018-08-13T12:51:50Z

Ref #1256
Closes #1896

To complete before merge:

Include ASCII timeline diagrams

Standard checklist:

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Updated all relevant documentation (docs/)
~~Updated all relevant code comments~~
~~Wrote tests~~
Added entries in PENDING.md that include links to the relevant issue or PR that most accurately describes the change.
~~Updated cmd/gaia and examples/~~

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2018-08-13T13:03:05Z

Codecov Report

Merging #2001 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           develop   #2001   +/-   ##
=======================================
  Coverage     62.2%   62.2%           
=======================================
  Files          115     115           
  Lines         6874    6874           
=======================================
  Hits          4276    4276           
  Misses        2314    2314           
  Partials       284     284

cwgoes · 2018-08-13T16:40:01Z

The slashing-by-period part of this PR is ready for review. Working on the NextValSet updates.

"PR4R" = "partially ready for review"

alexanderbez

Thanks for the updates @cwgoes -- I left a question and a tiny remark.

alexanderbez · 2018-08-13T16:57:45Z

docs/spec/slashing/state.md

+
+- SlashingPeriod: ` 0x03 | ValTendermintAddr | StartHeight -> amino(slashingPeriod) `
+
+This allows us to look up slashing period by validator address, the only lookup necessary,


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

alexanderbez · 2018-08-13T16:58:48Z

docs/spec/slashing/state.md

+
+### Slashing Period
+
+A slashing period is a start and end time associated with a particular validator,


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

Yes, thanks, updated

alexanderbez · 2018-08-13T19:56:32Z

docs/spec/slashing/state.md

+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).


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 :)

See below comment.

alexanderbez · 2018-08-13T20:00:53Z

docs/spec/slashing/state.md

+
+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).
+When the validator voluntarily unjails themselves (and possibly changes signing keys), they reset the period.


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.

bump @cwgoes

yup! you are right, this is unclear - addressing

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

melekes · 2018-08-14T06:56:37Z

docs/spec/slashing/state-machine.md

+
+Upon successful bonding of a validator (a given validator changing from "unbonded" state to "bonded" state,
+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),


wich -> with?

Thanks, fixed

melekes · 2018-08-14T06:58:39Z

docs/spec/slashing/state.md

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


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

Agreed, now JailedUntilHeight

cwgoes · 2018-08-14T16:30:24Z

Implementation of NextValSet depends on whether or not we change the SDK state machine to track the validator set differently. Likely it's just a simple offset of 1.

I think point 2.i in #1896 (comment) is a serious exploit and we need to prevent it, working on that now - unless we're OK with limiting ourselves to revoking-while-slashing.

alexanderbez · 2018-08-14T18:01:21Z

docs/spec/slashing/state.md

+
+### Slashing Period
+
+A slashing period is a start and end block height associated with a particular validator,


alexanderbez · 2018-08-14T18:28:13Z

@cwgoes

I think point 2.i in #1896 (comment) is a serious exploit and we need to prevent it, working on that now - unless we're OK with limiting ourselves to revoking-while-slashing.

Can you elaborate on why?

This reverts commit 21be609.

cwgoes · 2018-08-20T13:02:38Z

Can you elaborate on why?

Because otherwise a validator could start with a very small bond and intentionally slash themselves but remain within the slashing period, meaning that they aren't at stake in the future. I'll add a line to the spec about this.

Also see #1896 (comment).

cwgoes · 2018-08-20T13:03:52Z

Going to leave NextValSet out of this PR as that's blocked on some documentation Tendermint-side.

alexanderbez

@cwgoes thanks for the updates -- very straightforward to follow and the ASCII diagrams help. I left a few questions and suggestions 👍

alexanderbez · 2018-08-20T15:24:15Z

docs/spec/slashing/state-machine.md

@@ -0,0 +1,174 @@
+## Transaction & State Machine Interaction Overview


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

Sure; updated & also updated in state.md.

alexanderbez · 2018-08-20T15:31:45Z

docs/spec/slashing/state-machine.md

+
+*Multiple infractions*
+
+<---------------------------->   


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

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

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

alexanderbez · 2018-08-20T15:34:26Z

docs/spec/slashing/state-machine.md

+    ValidatorAddr sdk.AccAddress
+}
+
+handleMsgUnjail(tx TxUnjail)


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?

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

alexanderbez · 2018-08-20T15:41:20Z

docs/spec/slashing/state-machine.md

+
+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`.


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

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

Clarified the sentence.

alexanderbez · 2018-08-20T15:44:23Z

docs/spec/slashing/state-machine.md

+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`.
+
+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.


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)?

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.

cwgoes · 2018-08-20T16:39:00Z

@alexanderbez Addressed your comments I think.

jaekwon · 2018-08-20T17:59:57Z

I wonder if the ascii diagram might look different on different browsers due to font issues.

alexanderbez · 2018-08-20T18:24:23Z

Good point @jaekwon. I think this could be solved by simply throwing them into code blocks.

alexanderbez

Changes LGTM 👍

cwgoes · 2018-08-20T18:56:53Z

I wonder if the ascii diagram might look different on different browsers due to font issues.

Maybe so; does it look offset in yours? I can try code blocks although I'm not sure if that would make them consistent either; maybe images are a better option.

alexanderbez · 2018-08-20T19:10:55Z

No, look great in Chrome!

cwgoes · 2018-08-22T16:04:34Z

@jaekwon Any further comments or ready to merge?

rigelrozanski

cool

rigelrozanski · 2018-08-22T21:06:46Z

docs/spec/slashing/README.md

+
+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.


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

Sure, changed to a list.

rigelrozanski · 2018-08-22T21:07:51Z

docs/spec/slashing/begin-block.md


 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


darn 'mericans

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

rigelrozanski · 2018-08-22T21:10:49Z

docs/spec/slashing/begin-block.md

@@ -75,7 +76,9 @@ 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
+Double signature slashes are capped by the slashing period as described in [state-machine.md](state-machine.md).


what do you mean by slashes are capped by the slashing period - I thought that "cap" referred to the amount one could be slashed, not by the period in which discovered actions where slashable by. Maybe we could say Double signature slashes are time bound by or something like that

The amount slashed for all infractions committed in a single slashing period is capped. Clarified.

rigelrozanski · 2018-08-22T21:11:14Z

docs/spec/slashing/begin-block.md

@@ -113,3 +116,5 @@ for val in block.Validators:

  SigningInfo.Set(val.Address, signInfo)
 ```
+
+Downtime slashes are *not* capped by the slashing period, although they do reset it (since the validator is unbonded).


again clarification on capped would be good

rigelrozanski · 2018-08-22T21:12:07Z

docs/spec/slashing/state-machine.md

+
+### States
+
+At any given time, there are any number of validator candidates registered in the state machine.


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

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

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

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

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).

rigelrozanski · 2018-08-22T21:38:43Z

docs/spec/slashing/state-machine.md

+## 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.


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

rigelrozanski · 2018-08-22T21:39:33Z

docs/spec/slashing/state-machine.md

+  continue with slashing
+```
+
+##### Safety note


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

Sure, done.

rigelrozanski · 2018-08-22T21:40:00Z

docs/spec/slashing/state-machine.md

+then later slashed at `0.4` when they have `1000` stake bonded, the total amount slashed is just `40 + 100 = 140` (since the latter slash is capped at `0.1`) - 
+whereas if they had `1000` stake bonded initially, the first offense would have been slashed for `400` stake and the total amount slashed would have been `400 + 100 = 500`.
+
+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.


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

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

rigelrozanski · 2018-08-22T21:42:10Z

docs/spec/slashing/state.md

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


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

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

rigelrozanski · 2018-08-22T21:47:41Z

docs/spec/slashing/state.md

+## Slashing Period
+
+A slashing period is a start and end block height associated with a particular validator,
+within which only the "worst infraction counts": the total amount of slashing for


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

then it could just be linked here

rigelrozanski · 2018-08-23T19:53:29Z

Thanks cwgoes! highlighted comment responses:
#2001 (comment)
#2001 (comment)
#2001 (comment)

rigelrozanski

good stuff, merge now? additional comments from Jae can be in another PR?

Also see added discussion on "un" vs. "not"

#2001 (comment)

Update transactions.md

4f8c9e4

cwgoes requested a review from zramsay as a code owner August 13, 2018 12:51

cwgoes added the wip label Aug 13, 2018

cwgoes added 2 commits August 13, 2018 14:54

Fix typo

84e9b21

Reorganize sections

98a278d

cwgoes added 5 commits August 13, 2018 15:48

Update state.md

ff01cbb

Start update of state-machine.md

53fa4a2

End block to begin block, add README

a8af4a4

Update links

07a7db7

State machine contd.

2445718

cwgoes changed the title ~~WIP: Update slashing spec for slashing-by-period~~ WIP: Update slashing spec for slashing-by-period and NextValSet Aug 13, 2018

Update state machine, contd.

8b7d6e0

cwgoes changed the title ~~WIP: Update slashing spec for slashing-by-period and NextValSet~~ PR4R: Update slashing spec for slashing-by-period and NextValSet Aug 13, 2018

cwgoes added the ready-for-review label Aug 13, 2018

alexanderbez reviewed Aug 13, 2018

View reviewed changes

melekes reviewed Aug 14, 2018

View reviewed changes

cwgoes added 3 commits August 14, 2018 11:31

Fix minor typos

52475b1

Merge branch 'develop' into cwgoes/slashing-period-spec

32263ff

Clarify points from PR review

a2463d0

Changes WIP

21be609

alexanderbez reviewed Aug 14, 2018

View reviewed changes

Revert "Changes WIP" - we decided not to do this

79e3c05

This reverts commit 21be609.

cwgoes changed the title ~~PR4R: Update slashing spec for slashing-by-period and NextValSet~~ PR4R: Update slashing spec for slashing-by-period Aug 20, 2018

cwgoes added the C:x/slashing label Aug 20, 2018

alexanderbez reviewed Aug 20, 2018

View reviewed changes

cwgoes added 4 commits August 20, 2018 18:28

h_n => h_n-1

da92b1b

Go => pseudocode

d0c87ff

Clarify example

c9e5745

Clarify which offenses utilize the slashing period

f188955

alexanderbez approved these changes Aug 20, 2018

View reviewed changes

This was referenced Aug 22, 2018

WIP: Implement slashing period #2121

Merged

R4R: Implement slashing period #2122

Merged

cwgoes added 2 commits August 22, 2018 18:29

Merge branch 'develop' into cwgoes/slashing-period-spec

41df6db

Update PENDING.md

73e1965

cwgoes requested review from ebuchman and rigelrozanski as code owners August 22, 2018 16:30

Merge branch 'develop' into cwgoes/slashing-period-spec

bb9c265

rigelrozanski reviewed Aug 22, 2018

View reviewed changes

cwgoes added 2 commits August 23, 2018 13:43

Address @rigelrozanski comments

4cc2054

Fixup links

4475a8c

Change ASCII diagram slightly

63a85aa

rigelrozanski approved these changes Aug 23, 2018

View reviewed changes

cwgoes merged commit c5d44bc into develop Aug 24, 2018

cwgoes deleted the cwgoes/slashing-period-spec branch August 24, 2018 00:09

cwgoes mentioned this pull request Sep 3, 2018

SDK 0.25 / Gaia-9000 release checklist #2220

Closed

23 tasks


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

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


		### Slashing Period

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


		### Slashing Period

		A slashing period is a start and end block height associated with a particular validator,

		@@ -0,0 +1,174 @@
		## Transaction & State Machine Interaction Overview


		### States

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


		Multiple infractions

		<---------------------------->

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

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

Conversation

cwgoes commented Aug 13, 2018 • edited Loading

codecov bot commented Aug 13, 2018 • edited Loading

Codecov Report

cwgoes commented Aug 13, 2018 • edited Loading

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

alexanderbez Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes commented Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

alexanderbez commented Aug 14, 2018

cwgoes commented Aug 20, 2018

cwgoes commented Aug 20, 2018

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes commented Aug 20, 2018

jaekwon commented Aug 20, 2018

alexanderbez commented Aug 20, 2018

alexanderbez left a comment

Choose a reason for hiding this comment

cwgoes commented Aug 20, 2018

alexanderbez commented Aug 20, 2018

cwgoes commented Aug 22, 2018

rigelrozanski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes Aug 23, 2018 • edited Loading

cwgoes commented Aug 13, 2018 •

edited

Loading

codecov bot commented Aug 13, 2018 •

edited

Loading

cwgoes commented Aug 13, 2018 •

edited

Loading

cwgoes Aug 14, 2018 •

edited

Loading

alexanderbez Aug 13, 2018 •

edited

Loading

alexanderbez Aug 13, 2018 •

edited

Loading

cwgoes Aug 14, 2018 •

edited

Loading

cwgoes commented Aug 14, 2018 •

edited

Loading

cwgoes Aug 20, 2018 •

edited

Loading

cwgoes Aug 23, 2018 •

edited

Loading

cwgoes Aug 23, 2018 •

edited

Loading

cwgoes Aug 23, 2018 •

edited

Loading

rigelrozanski commented Aug 23, 2018 •

edited

Loading