[Staking] Kill unnecessary storage bloat

[4meta5]

Changes the pay_stakers code to take() the storage items Staked and Points instead of just getting them.

Also includes a migration to remove the stored values that are older than 2 rounds ago (because any stored values less than 2 rounds old will be killed in pay_stakers during future payout which will take() these values).

I separated this from #810 because this is a distinct change and I do not want to lump too many changes into that PR.

[notlesh]

For the migration part, would it be possible/preferable to remove the storage bloat through democracy?

[JoshOrndorff]

Overall looks good. Made a few little comments. Only one I consider blocking is post_upgrade.

[JoshOrndorff]

Being a little pedantic here, but if you're intending it to be %5 of max block, you should express it as max_block / 20 or similar in code. Because the max_block actually depends on the runtime in question.

[4meta5]

This code is in the pallet so how would I pass it this information (max block weight from the runtime where it's executed)?

It is used as a safety margin so it doesn't have to be accurate in this case.

[JoshOrndorff]

<T as frame_system::Config>::BlockWeights::get().max_block

[JoshOrndorff]

I agree it doesn't really need to be that accurate.

[4meta5]

For the migration part, would it be possible/preferable to remove the storage bloat through democracy?

@notlesh It's not a heavy migration so I don't think this is necessarily preferable. Is there a test we could do to determine when this ought to be done?

With that said, we could easily do this -- we'd just add an extrinsic gated by root hotfix_purge_stale_storage and move that part of the migration into this function.

[notlesh]

@notlesh It's not a heavy migration so I don't think this is necessarily preferable. Is there a test we could do to determine when this ought to be done?

I was thinking more about simplicity and resiliency; submitting an extrinsic is probably simpler than managing a migration and might be more resilient (less chance of something catastrophic happening). But neither of these are necessarily true.

I don't have a strong opinion either way, just wanted to bring it up.

As for it perhaps being too heavy, we could test this with try-runtime and/or fork-off-substrate. I don't recall whether these things clearly indicate the weight of migrations, but that's something we could add (and, potentially, PoV size). I wouldn't worry much about it if we're talking hundreds or fewer DB reads/writes...

With that said, we could easily do this -- we'd just add an extrinsic gated by root hotfix_purge_stale_storage and move that part of the migration into this function.

Would system->killStorage() or system->killPrefix() work?

[4meta5]

Would system->killStorage() or system->killPrefix() work?

The proposal would need to be a batch of calls to killPrefix for all values > 2 rounds old for the two relevant storage maps.

I'm sure it could be done, but it's easier for me to do it in a migration.

[JoshOrndorff]

try-runtime and the unit test both look good to me!