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: Tombstone Implementation #3225

Merged
merged 12 commits into from
Jan 11, 2019
Merged

R4R: Tombstone Implementation #3225

merged 12 commits into from
Jan 11, 2019

Conversation

sunnya97
Copy link
Member

@sunnya97 sunnya97 commented Jan 4, 2019

closes #2363
Fixes #3225
Fixes #3253
Spec in #3103

  • Targeted PR against correct branch (see CONTRIBUTING.md)

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

@sunnya97 sunnya97 requested a review from jaekwon January 4, 2019 07:26
@sunnya97 sunnya97 changed the title R4R: Tombstone Implementation WIP: Tombstone Implementation Jan 4, 2019
@codecov
Copy link

codecov bot commented Jan 6, 2019

Codecov Report

Merging #3225 into develop will decrease coverage by 0.09%.
The diff coverage is 61.76%.

@@            Coverage Diff             @@
##           develop    #3225     +/-   ##
==========================================
- Coverage    55.34%   55.24%   -0.1%     
==========================================
  Files          135      134      -1     
  Lines         9596     9517     -79     
==========================================
- Hits          5311     5258     -53     
+ Misses        3953     3928     -25     
+ Partials       332      331      -1

@sunnya97 sunnya97 changed the title WIP: Tombstone Implementation R4R: Tombstone Implementation Jan 6, 2019
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Haven't thoroughly reviewed in full yet, but the changes look good. I left some comments 👍

docs/spec/slashing/future-improvements.md Outdated Show resolved Hide resolved
x/slashing/keeper.go Outdated Show resolved Hide resolved
x/slashing/keeper.go Outdated Show resolved Hide resolved
x/slashing/keeper.go Outdated Show resolved Hide resolved
x/slashing/keeper.go Outdated Show resolved Hide resolved
x/slashing/keeper.go Show resolved Hide resolved
x/slashing/params.go Outdated Show resolved Hide resolved
alexanderbez and others added 2 commits January 7, 2019 10:08
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Just another minor remark 👍

docs/spec/slashing/future-improvements.md Outdated Show resolved Hide resolved
docs/spec/slashing/overview.md Show resolved Hide resolved
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

JailedUntil and Tombstoned seem redundant - otherwise this mostly LGTM, a few other minor comments. Definitely simpler than slashing periods!

docs/spec/slashing/begin-block.md Show resolved Hide resolved
docs/spec/slashing/begin-block.md Show resolved Hide resolved
docs/spec/slashing/begin-block.md Show resolved Hide resolved
docs/spec/slashing/future-improvements.md Outdated Show resolved Hide resolved
docs/spec/slashing/overview.md Outdated Show resolved Hide resolved
x/slashing/handler.go Show resolved Hide resolved
x/slashing/keeper.go Show resolved Hide resolved
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

LGTM mostly 👍

I still think we should actually prevent reuse of "tomb-stoned" consensus keys.

@sunnya97
Copy link
Member Author

sunnya97 commented Jan 9, 2019

Keeping them permanently in jail (never deleting their ValidatorSigningInfo) should prevent them from ever rejoining the validator set, right? I'll double check to make sure that is correct.

@alexanderbez
Copy link
Contributor

Yeah I believe so @sunnya97.

Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

"I'm getting tombstoned tonight buddy"

@@ -17,26 +17,11 @@ func (k Keeper) AfterValidatorBonded(ctx sdk.Context, address sdk.ConsAddress, _
StartHeight: ctx.BlockHeight(),
IndexOffset: 0,
JailedUntil: time.Unix(0, 0),
Tombstoned: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

:P I love this name


// Slash validator
// `power` is the int64 power of the validator as provided to/by
// Tendermint. This value is validator.Tokens as sent to Tendermint via
// ABCI, and now received as evidence.
// The revisedFraction (which is the new fraction to be slashed) is passed
// in separately to separately slash unbonding and rebonding delegations.
k.validatorSet.Slash(ctx, consAddr, distributionHeight, power, revisedFraction)
k.validatorSet.Slash(ctx, consAddr, distributionHeight, power, fraction)
Copy link
Contributor

Choose a reason for hiding this comment

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

should update the above comment based on this code change

x/slashing/signing_info.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

This PR gets an approval from me pending addressing the two comments by @rigelrozanski 👍

@cwgoes
Copy link
Contributor

cwgoes commented Jan 10, 2019

ACK pending @rigelrozanski comments.

rigelrozanski and others added 2 commits January 10, 2019 12:46
Co-Authored-By: sunnya97 <sunnya97@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Max evidence age not effective Slashing Period -> Tombstone
5 participants