validator slash event stored by period and height

[rigelrozanski]

Followup to fuzzer bug Icarus issue:
ref #4653

The solution (thanks @cwgoes) is to no longer average slash events by height:

I believe there is a bug in consolidation of slash events by height.
Consider the following sequence of slashes:

1. Validator A is slashed. A new slash event is created.
2. A redelegation from validator A to B is slashed. The validator's period is incremented.
3. Validator A is again slashed. The previous slash event is updated, using the period from (2).
4. A redelegation from validator A to B is again slashed. The second slash event is now skipped > when it ought to have been included, since the ended period of the slash event is the same as the starting period of the modified delegation (after (2)).
   Part of this was conjecture, so we should confirm in the actual simulation run.
   I suggest what should be a straightforward fix of no longer consolidating slashes and instead keying by validator address, height, and period in that order. As slashes are expensive to create, the iteration overhead shouldn't be a concern.

---

Summary of Bug

Running go test -race -covermode=atomic -coverprofile=769332655.1.profile.out -coverpkg=./... -mod=readonly github.com/cosmos/cosmos-sdk/simapp -run TestFullAppSimulation -SimulationEnabled=true -SimulationNumBlocks=400 -SimulationBlockSize=200 -SimulationGenesis= -SimulationVerbose=false -SimulationCommit=true -SimulationSeed=769332655 -SimulationPeriod=5 -v -timeout 6h on current HEAD (commit ac2e765607a64f9dacb475244895ba3e99ec927d) fails with:

Simulating... block 29/400, operation 50/78. panic with err: calculated final stake for delegator cosmos1y7wu8egpm6w2zkzdmtdf6tgu0gnz70mdryuq3v greater than current stake
	final stake:	367984183.087167908909931651
	current stake:	325658474.336187830811153603

Full log: 769332655.log

Version

$ git log -1
commit ac2e765607a64f9dacb475244895ba3e99ec927d (HEAD, origin/master, origin/HEAD)
Author: Alexander Bezobchuk <[email protected]>
Date:   Sat Jun 15 16:17:39 2019 +0200

    Merge PR #4561: Add account JSON tag to AccountWithHeight

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

[rigelrozanski]

CC @sabau this is going effect the state upgrade, the event period now is required to be included in the ValidatorSlashEvent object, however for historical records to migrate the Event.ValidatorPeriod can just be used. #4471

[alexanderbez]

ACK

[alexanderbez]

/cc @sabau

[alexanderbez]

Any reason why all this logic was removed? Was calculating updatedFraction unnecessary?

[rigelrozanski]

yeah I think this design decision was overlooked at inception seeing as the revised code eliminates the need for this complexity. By introducing the period into the validator-slash-record-key the fraction no longer needs to be "flattened" with the other fractions in that height, hence we remove all this flattening logic in this function

[cwgoes]

Yes, this is better and simpler. Mea culpa. I think originally we were concerned about iteration costs.

[cwgoes]

utACK

[cwgoes]

Yes, this is better and simpler. Mea culpa. I think originally we were concerned about iteration costs.