R4R: Fix signing info handling bugs & faulty slashing #2480

cwgoes · 2018-10-11T20:50:09Z

Closes #2372
Closes #1867

Addressing both issues at once is more efficient.

Signing info is now initialized when a validator is bonded, using the existing validator bond hook
Signing info array & counter are now reset when a validator is jailed, giving them a chance to sign blocks again afterwards without being immediately slashed

Nothing is reset if a validator leaves the bonded set as a result of any reason other than being slashed for downtime, so it shouldn't be possible to reset the counter without also incurring the penalties of being slashed & jailed.

We must iterate over SignedBlocksWindow store keys and delete them when a validator is slashed for downtime, but that's probably a relatively rare occurrence and incurs a real cost, so I don't think spam is too much of a concern.

Standard checklist:

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
rereviewed Files changed in the github PR explorer

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-10-11T21:08:08Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@2803830). Click here to learn what that means.
The diff coverage is 83.67%.

@@            Coverage Diff            @@
##             develop   #2480   +/-   ##
=========================================
  Coverage           ?     60%           
=========================================
  Files              ?     151           
  Lines              ?    8844           
  Branches           ?       0           
=========================================
  Hits               ?    5307           
  Misses             ?    3167           
  Partials           ?     370

cwgoes · 2018-10-11T22:17:45Z

OK, this should basically do what we want, just need to make sure the jail duration is greater than the signed blocks window. Needs testcases & spec update.

cwgoes · 2018-10-12T19:23:48Z

cc @SLAMPER @HaoyangLiu want to review?

hendrikhofstadt

utACK however please note minimal change requests.

x/slashing/keeper_test.go

hendrikhofstadt · 2018-10-13T11:23:01Z

x/slashing/keeper.go

 	}
 	minHeight := signInfo.StartHeight + k.SignedBlocksWindow(ctx)
-	if height > minHeight && signInfo.SignedBlocksCounter < k.MinSignedPerWindow(ctx) {
+	maxMissed := k.SignedBlocksWindow(ctx) - k.MinSignedPerWindow(ctx)
+	if height > minHeight && signInfo.MissedBlocksCounter > maxMissed {


Do we really want to keep this dependent on the StartHeight ?

That way a validator could unbond every SignedBlocksWindow-2 blocks and then rebond to avoid slashing since the StartHeight is reset now.

Ah yes, of course, in the case when they aren't jailed at all.

I think I'll try the other strategy, deleting the array shouldn't be too expensive.

Now we never reset the start height, and instead reset the counter & clear the array when the validator is slashed for downtime.

x/slashing/keeper.go

cwgoes · 2018-10-15T21:15:03Z

Ready for review again.

rigelrozanski

looks good! few lil comments

x/slashing/keeper.go

x/slashing/keeper_test.go

cwgoes · 2018-10-15T22:19:45Z

Thanks @rigelrozanski - do you want to review this @jaekwon?

Create signing info struct on validator bond hook

f6e92df

cwgoes added C:x/staking C:x/slashing wip labels Oct 11, 2018

cwgoes requested review from ebuchman and rigelrozanski as code owners October 11, 2018 20:50

cwgoes mentioned this pull request Oct 11, 2018

Failed to slash a validator for double sign at the first block #2372

Closed

4 tasks

Update PENDING.md

b55f29f

cwgoes changed the title ~~WIP: Fix signing info handling bugs~~ WIP: Fix signing info handling bugs & faulty slashing Oct 11, 2018

cwgoes added 2 commits October 12, 2018 00:04

Count missed blocks instead of signed blocks

5fd7297

Update block height on validator bonding

8c2c9db

Update spec

6e4a8d9

cwgoes requested a review from zramsay as a code owner October 11, 2018 23:05

cwgoes added 3 commits October 12, 2018 19:57

Merge branch 'develop' into cwgoes/fix-signing-info-bugs

69b7519

Greater than to greater than or equal to

a83535a

Clarify testcase

bdfb10f

cwgoes changed the title ~~WIP: Fix signing info handling bugs & faulty slashing~~ R4R: Fix signing info handling bugs & faulty slashing Oct 12, 2018

Back to greater than

1ff2e86

cwgoes added ready-for-review and removed wip labels Oct 12, 2018

hendrikhofstadt suggested changes Oct 13, 2018

View reviewed changes

cwgoes added wip and removed ready-for-review labels Oct 14, 2018

cwgoes changed the title ~~R4R: Fix signing info handling bugs & faulty slashing~~ WIP: Fix signing info handling bugs & faulty slashing Oct 14, 2018

HaoyangLiu reviewed Oct 14, 2018

View reviewed changes

x/slashing/keeper.go Outdated Show resolved Hide resolved

HaoyangLiu reviewed Oct 14, 2018

View reviewed changes

x/slashing/keeper.go Outdated Show resolved Hide resolved

cwgoes added 5 commits October 15, 2018 21:04

Merge branch 'develop' into cwgoes/fix-signing-info-bugs

2c4d9a0

Test correct JailedUntil

fa372d3

Update function names to clarify semantics

6e0f3d6

Further clarification of nomenclature

096a8bb

Clear array when validator is jailed

9ec35a0

cwgoes changed the title ~~WIP: Fix signing info handling bugs & faulty slashing~~ R4R: Fix signing info handling bugs & faulty slashing Oct 15, 2018

Fix testcases

ec53a14

cwgoes mentioned this pull request Oct 15, 2018

Faulty slashing when ex-validator is reintroduced into the validator set #1867

Closed

4 tasks

cwgoes added ready-for-review and removed wip labels Oct 15, 2018

cwgoes added 4 commits October 15, 2018 23:01

Bugfix; update slashing spec

83f7a4c

Fix linter warning

e3fa982

Fix typo

7bf8a41

iter.Close()

c0c5e29

rigelrozanski reviewed Oct 15, 2018

View reviewed changes

x/slashing/keeper.go Outdated Show resolved Hide resolved

x/slashing/keeper_test.go Show resolved Hide resolved

x/slashing/keeper_test.go Outdated Show resolved Hide resolved

x/slashing/keeper_test.go Outdated Show resolved Hide resolved

x/slashing/keeper_test.go Show resolved Hide resolved

cwgoes mentioned this pull request Oct 15, 2018

Separate slashing tests into smaller distinct cases with more explicit state setup #2499

Closed

Address @rigelrozanski comments

f987750

rigelrozanski approved these changes Oct 15, 2018

View reviewed changes

HaoyangLiu approved these changes Oct 16, 2018

View reviewed changes

hendrikhofstadt approved these changes Oct 16, 2018

View reviewed changes

cwgoes added 3 commits October 16, 2018 19:23

Merge branch 'develop' into cwgoes/fix-signing-info-bugs

3f59cf1

Alphanumeric parameter keys

7d46b8b

Fix merge rename

0d11043

cwgoes merged commit 55f4f61 into develop Oct 16, 2018

cwgoes deleted the cwgoes/fix-signing-info-bugs branch October 16, 2018 18:21

sunnya97 mentioned this pull request Oct 24, 2018

Mystery validator unbond/bond #1865

Closed

4 tasks

rigelrozanski mentioned this pull request Nov 30, 2018

WIP: Initial window downtime slash fix #1792

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Fix signing info handling bugs & faulty slashing #2480

R4R: Fix signing info handling bugs & faulty slashing #2480

cwgoes commented Oct 11, 2018 •

edited

Loading

codecov bot commented Oct 11, 2018 •

edited

Loading

cwgoes commented Oct 11, 2018

cwgoes commented Oct 12, 2018

hendrikhofstadt left a comment

hendrikhofstadt Oct 13, 2018

cwgoes Oct 14, 2018 •

edited

Loading

cwgoes Oct 15, 2018

cwgoes commented Oct 15, 2018

rigelrozanski left a comment

cwgoes commented Oct 15, 2018

R4R: Fix signing info handling bugs & faulty slashing #2480

R4R: Fix signing info handling bugs & faulty slashing #2480

Conversation

cwgoes commented Oct 11, 2018 • edited Loading

codecov bot commented Oct 11, 2018 • edited Loading

Codecov Report

cwgoes commented Oct 11, 2018

cwgoes commented Oct 12, 2018

hendrikhofstadt left a comment

Choose a reason for hiding this comment

hendrikhofstadt Oct 13, 2018

Choose a reason for hiding this comment

cwgoes Oct 14, 2018 • edited Loading

Choose a reason for hiding this comment

cwgoes Oct 15, 2018

Choose a reason for hiding this comment

cwgoes commented Oct 15, 2018

rigelrozanski left a comment

Choose a reason for hiding this comment

cwgoes commented Oct 15, 2018

cwgoes commented Oct 11, 2018 •

edited

Loading

codecov bot commented Oct 11, 2018 •

edited

Loading

cwgoes Oct 14, 2018 •

edited

Loading