Slashing v2 #1278

cwgoes · 2018-06-16T01:54:50Z

Implement semifinal Gaia slashing spec (#1263), less #1348, #1378, and #1440 which are TBD.

Closes #1117
~~Mostly blocked on finalization of #1119~~ Merged!
Wants tendermint/tendermint#1803, but doesn't need to block merge

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG.md
Updated Gaia/Examples
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

Addressed comments & added an extra test in x/stake/handler_test.go

ValarDragon

WiP review, overall looks good, just want a bit more clarification in some of the comments!

ValarDragon · 2018-06-30T02:28:40Z

client/lcd/lcd_test.go

+	defer cleanup()
+
+	signingInfo := getSigningInfo(t, port, pks[0].Address())
+	tests.WaitForHeight(4, port)


Would be better to use tests.WaitForNextNBlocksTM

ValarDragon · 2018-06-30T02:30:44Z

examples/democoin/mock/validator.go

@@ -38,6 +38,11 @@ func (v Validator) GetDelegatorShares() sdk.Rat {
 	return sdk.ZeroRat()
 }

+// Implements sdk.Validator


Is this sdk.Validator, or x/stake/types.Validator?`

It is sdk.Validator (types/stake.go), to which GetRevoked() was added.

ValarDragon · 2018-06-30T02:37:52Z

x/slashing/tick.go

+	// Iterate over all the validators  which *should* have signed this block
+	for _, signingValidator := range req.Validators {
+		present := signingValidator.SignedLastBlock
+		pubkey, err := tmtypes.PB2TM.PubKey(signingValidator.Validator.PubKey)


Can there be a comment here regarding what the goal of this loop is? Is it just to figure out which validators signed the block? (wasn't clear to me) If so, is this for liveness slashing?

Yes; for downtime slashing, added a comment.

ValarDragon · 2018-06-30T02:41:46Z

x/stake/keeper/slash.go

-// slash a validator
-func (k Keeper) Slash(ctx sdk.Context, pubkey crypto.PubKey, height int64, fraction sdk.Rat) {
+// slash an unbonding delegation and update the pool
+func (k Keeper) slashUnbondingDelegation(ctx sdk.Context, unbondingDelegation types.UnbondingDelegation, infractionHeight int64, fraction sdk.Rat) (slashAmount sdk.Int) {


can fraction be renamed to something more descriptive, perhaps slashFactor?

Changed to slashFraction.

ValarDragon · 2018-06-30T02:53:08Z

x/stake/keeper/slash.go

+	// Calculate slash amount proportional to stake contributing to infraction
+	slashAmount = sdk.NewRatFromInt(unbondingDelegation.InitialBalance.Amount, sdk.OneInt()).Mul(fraction).EvaluateInt()
+
+	// Don't slash more tokens than held


Write a note here, that this is a concern since this unbonding delegation may have already been slashed once, and we slash based on initial balance.

Added comment, here and for the same check on redelegation.

ValarDragon · 2018-06-30T02:55:05Z

x/stake/keeper/slash.go

+	}
+
+	// Calculate slash amount proportional to stake contributing to infraction
+	slashAmount = sdk.NewRatFromInt(unbondingDelegation.InitialBalance.Amount, sdk.OneInt()).Mul(fraction).EvaluateInt()


Do we want a sanity check that this is a non-negative number? I think that would improve overall safety, and it shouldn't be an expensive check.

Added a sanity check for the sign of slashFraction.

ValarDragon · 2018-06-30T02:56:15Z

x/stake/keeper/slash.go

+	// Calculate slash amount proportional to stake contributing to infraction
+	slashAmount = sdk.NewRatFromInt(redelegation.InitialBalance.Amount, sdk.OneInt()).Mul(fraction).EvaluateInt()
+
+	// Don't slash more tokens than held


Same two comments as above. (sanity check for positive slashAmount, indicate why slashing more tokens than held would be a concern)

Added comment, added sanity check in the top-level Slash() function.

ValarDragon · 2018-06-30T03:00:14Z

x/stake/keeper/slash.go

+	slashAmount = sdk.NewRatFromInt(redelegation.InitialBalance.Amount, sdk.OneInt()).Mul(fraction).EvaluateInt()
+
+	// Don't slash more tokens than held
+	redelegationSlashAmount := slashAmount


Can more indication be given as to the justification for this rename?

Added comment.

ValarDragon · 2018-06-30T03:01:51Z

x/stake/keeper/slash.go

+}
+
+// slash a redelegation and update the pool
+func (k Keeper) slashRedelegation(ctx sdk.Context, validator types.Validator, redelegation types.Redelegation, infractionHeight int64, fraction sdk.Rat) (slashAmount sdk.Int) {


Can fraction be renamed to slashFactor

Changed to slashFraction.

ValarDragon · 2018-06-30T03:05:09Z

x/stake/keeper/slash.go

+// CONTRACT:
+//    Infraction committed at the current height or at a past height,
+//    not at a height in the future
+func (k Keeper) Slash(ctx sdk.Context, pubkey crypto.PubKey, infractionHeight int64, power int64, fraction sdk.Rat) {


Could this function be placed above the previous two? (Sorry for the extremely minor nitpick here. I think it would be useful structurally to see the contract first though)

Also can fraction be renamed to slashFactor

Moved, now slashFactor.

ValarDragon · 2018-06-30T03:10:08Z

x/stake/keeper/slash.go

 	logger.Info(fmt.Sprintf("Validator %s slashed by fraction %v, removed %v shares and burned %d tokens", pubkey.Address(), fraction, sharesToRemove, burned))
+


Can there be a sanity check that that the remaining slash amount = 0?

It's not necessarily zero; most of the stake may still be bonded.

Addressed (I think)

ValarDragon

lgtm! slashFactor is so much cooler than fraction :)

Implement semifinal Gaia slashing spec (#1263), less #1348, #1378, and #1440 which are TBD.

rigelrozanski added 29 commits June 5, 2018 09:56

stake/fees spec updates

3d0e7ff

staking overview.md revisions, moving files

43a0ed9

docs reorganization

a49f9cb

staking spec state revisions

f972cab

transaction stake updates

752f084

complete staking spec update

5a77d7d

WIP adding unbonding/redelegation commands

26681a8

added msg types for unbonding, redelegation

8e677e8

stake sub-package reorg

43429ef

...

482a3c5

working stake reorg

fd1eb5f

...

63a0dcc

...

7eaa715

modify lcd tests to not use hardcoded json strings

3fac834

add description update

d69d179

index keys

0efb0a4

...

2153e0d

...

f6b9893

key managment for unbonding redelegation complete

e460c06

...

8639f91

update stake errors

9ee70ea

...

bd41980

completed handleMsgCompleteUnbonding fn

40ad0a5

updated to use begin/complete unbonding/redelegation

4be5e41

...

03c6804

fix token shares bug

98dad7d

develop docs into unbonding

4601bd5

...

fc6ee07

...

ffc8cc4

cwgoes added the wip label Jun 16, 2018

cwgoes added 4 commits June 30, 2018 00:56

Linter fixes

cb85459

Add slashing test to x/stake handler

4edb773

Clarify a few comments

78ac9ae

Unify comment format

b00a0a1

Remove redundant sdk.Coin{}

f8dd859

cwgoes force-pushed the cwgoes/slashing-version-two branch 2 times, most recently from d0a7017 to 68406dd Compare June 30, 2018 00:07

Update changelog

c1987da

cwgoes force-pushed the cwgoes/slashing-version-two branch from 68406dd to c1987da Compare June 30, 2018 00:13

Merge branch 'develop' into cwgoes/slashing-version-two

9bc99f1

cwgoes force-pushed the cwgoes/slashing-version-two branch from 200a8b0 to 9bc99f1 Compare June 30, 2018 01:19

Change CircleCI config slightly

30d9d90

cwgoes force-pushed the cwgoes/slashing-version-two branch from b88f82c to 30d9d90 Compare June 30, 2018 01:38

Merge branch 'develop' into cwgoes/slashing-version-two

aeea789

Change jail durations to five minutes

9ff81a6

ValarDragon requested changes Jun 30, 2018

View reviewed changes

ValarDragon reviewed Jun 30, 2018

View reviewed changes

ValarDragon previously requested changes Jun 30, 2018

View reviewed changes

Address PR comments

5a479e0

Add additional comment

e46e687

cwgoes force-pushed the cwgoes/slashing-version-two branch from bbe1ea2 to e46e687 Compare June 30, 2018 03:23

ValarDragon approved these changes Jun 30, 2018

View reviewed changes

cwgoes merged commit 3654579 into develop Jun 30, 2018

cwgoes deleted the cwgoes/slashing-version-two branch June 30, 2018 03:34

adrianbrink pushed a commit that referenced this pull request Jul 2, 2018

Merge PR #1278: Slashing v2

3d2834a

Implement semifinal Gaia slashing spec (#1263), less #1348, #1378, and #1440 which are TBD.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slashing v2 #1278

Slashing v2 #1278

cwgoes commented Jun 16, 2018 •

edited

Loading

ValarDragon left a comment

ValarDragon Jun 30, 2018

ValarDragon Jun 30, 2018

cwgoes Jun 30, 2018

ValarDragon Jun 30, 2018

cwgoes Jun 30, 2018

ValarDragon Jun 30, 2018

cwgoes Jun 30, 2018

ValarDragon Jun 30, 2018

cwgoes Jun 30, 2018

ValarDragon Jun 30, 2018

cwgoes Jun 30, 2018

ValarDragon Jun 30, 2018

cwgoes Jun 30, 2018

ValarDragon Jun 30, 2018

cwgoes Jun 30, 2018

ValarDragon Jun 30, 2018

cwgoes Jun 30, 2018

ValarDragon Jun 30, 2018

cwgoes Jun 30, 2018

ValarDragon Jun 30, 2018

cwgoes Jun 30, 2018

ValarDragon left a comment

		logger.Info(fmt.Sprintf("Validator %s slashed by fraction %v, removed %v shares and burned %d tokens", pubkey.Address(), fraction, sharesToRemove, burned))

Slashing v2 #1278

Slashing v2 #1278

Conversation

cwgoes commented Jun 16, 2018 • edited Loading

ValarDragon 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

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

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

ValarDragon left a comment

Choose a reason for hiding this comment

cwgoes commented Jun 16, 2018 •

edited

Loading