[wip] slashing: remove validator missed bit array from HandleValidatorSignature #4641
Conversation
defunctzombie
requested review from
alessio,
alexanderbez,
jackzampolin and
rigelrozanski
as code owners
There was a problem hiding this comment.
Thanks for the PR @defunctzombie! Would you mind rebasing off of master? I'd like to see how this fairs in simulation. Also, an issue outlining the rationale behind this PR is warranted I believe (assuming it's correct) -- I left some initial feedback. Thanks!
| maxMissed := k.SignedBlocksWindow(ctx) - k.MinSignedPerWindow(ctx) | ||
| // Only when the last miss height is past the sign window can we decrement the miss block counter | ||
| // This ensures that a missed block is tracked throughout its lifetime in the sign window | ||
| if signed && signInfo.MissedBlocksCounter > 0 && (signInfo.LastMissHeight+signWindow) < height { |
There was a problem hiding this comment.
This doesn't seem to exactly resemble the same logic as the rolling window? What if I miss a block within signed window?
(essentially what @cwgoes stated)
| // if we are past the minimum height and the validator has missed too many blocks, punish them | ||
| if height > minHeight && signInfo.MissedBlocksCounter > maxMissed { | ||
| // if the validator has missed too many blocks, punish them | ||
| if signInfo.MissedBlocksCounter > maxMissed { |
There was a problem hiding this comment.
Might be off by one here.
There was a problem hiding this comment.
I kept the same logic as before which was also strictly greater-than.
|
I do not think this works. What if I miss a single block just within the window, repeatedly? My miss counter will increment forever, even though I've only missed one block every window length. We should implement a proper inter-block cache; that will help a lot here. |
|
Not sure how or why I overlooked that simple case when working through the various cases. Closing for now since indeed there is an issue with constantly increasing miss counts. Thanks for taking a closer look! |
While working on a program for block replay, I did some cpu profiling of my application. I found that one area where it was spending a lot of time was in the
HandleValidatorSignatureroutine - specifically around getting the missed block bit array.After looking at the code and how the bit array is used, I propose this alternative implementation which maintains the same slashing logic but avoids the use of a bit array, the index offset, and the start height. These are replaced by a last miss height and the existing miss block counter.
When a validator misses signing a block, their miss counter is incremented and the last miss height is recorded. If the miss counter exceeds the maximum allowed misses in a window, then the validator is slashed and jailed (as before). The miss counter is decremented when the validator successfully signs a block AND their last miss occurred outside the sign window. The latter is important to capture a missed block for the duration of the sign window.
I imagine this code change would fall under the "hard fork" category due to the app state changes it introduces (tho the slashing condition remains unchanged). I am not sure of the appropriate approach to land such a change.
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 a relevant changelog entry:
clog add [section] [stanza] [message]rereviewed
Files changedin the github PR explorerFor Admin Use: