-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Conversation
Codecov Report
@@ 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 |
fb4f1c8
to
2b8c670
Compare
There was a problem hiding this 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 👍
Co-Authored-By: sunnya97 <sunnya97@gmail.com>
There was a problem hiding this 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 👍
There was a problem hiding this 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!
There was a problem hiding this 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.
Keeping them permanently in jail (never deleting their |
Yeah I believe so @sunnya97. |
There was a problem hiding this 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, |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
There was a problem hiding this 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 👍
ACK pending @rigelrozanski comments. |
Co-Authored-By: sunnya97 <sunnya97@gmail.com>
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 explorerFor Admin Use: