-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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: Reset slashing periods on export #3189
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3189 +/- ##
===========================================
- Coverage 54.85% 54.82% -0.04%
===========================================
Files 133 133
Lines 9559 9552 -7
===========================================
- Hits 5244 5237 -7
Misses 3994 3994
Partials 321 321 |
Running this with a 500 block variant of import/export simulation. EDIT: Never mind, I do not have adequate memory on my machine to run such a simulation. |
} | ||
app.slashingKeeper.SetValidatorSlashingPeriod(ctx, sp) | ||
} | ||
|
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.
So @cwgoes had the perspective that the correct behavior would be to have initChain
actually set the slashing period for validator in state at the beginning of the chain.
This fix puts the responsibility on the export function to ensure that this invariant is maintained. Just checking that this is the best approach?
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.
initChain
already does this - usually - it just doesn't do it in the case when state was exported (which is a mistake). I think either mechanism is OK, it has to be "special-cased" either way.
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.
Doing reseting the slashing periods via the state export is more "backwards compatible"
I made a branch where this change was applied to |
How did the results turn out @zmanian? |
Check it out. |
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.
looks good, minor comment,
one question: does the export function actually affect the state of the blockchain (seems bad)? or does it just affect a cached state before the export? As in, if I was to run a transaction after running an export, the slashing period should still be maintained on the committed state correct?
Presently it runs on the |
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 looks good to me!
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.
tACK
closes: #3187
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: