Vest a portion of block reward immediately (FIP-0004)

[ZenGround0]

• Code should be correct
• Unrelated test failures caused by those tests using applyRewards as a test state setup helper are now fixed.
• Reward tests updated to test new behavior
• Reward test exercising rewards in both versions to catch any unexpected balance table assumptions about not having consistent vesting specs applied.
• Land Clean up vesting reward use in miner tests #1251 and rebase on master
• Update PR title when FIP is published if the number changes

[anorth]

I think in general all unit tests should target the latest network version. If we have a version switch, it will generally be because the prior version is already live in the network, and so we're working to test that the new version will behave as expected. E.g. maybe there's some unexpected interaction between the change guarded by this version and other things that are being tested here.

Which means generally all these tests that are not specifically about the reward vesting should require no diff (because the runtime uses VersionMax). If they do require a diff, that diff will specifically highlight the behaviour that's changing.

Is there a diff here to pledge expectations or something? I think we should update expectations to match (hopefully we can do it just once in the deadlineCron or something).

[ZenGround0]

I think in general all unit tests should target the latest network version.

agree

Which means generally all these tests that are not specifically about the reward vesting should require no diff

agree on the should

Is there a diff here to pledge expectations or something?

Yes. The problem is that we use applyRewards as a testing setup function in a bunch of places in a fragile way, not as part of actually testing "what happens if the miners gets a reward message and then x and y happen". There is a better way to do it as we've known for a while, however designing it and then this refactor seems non-trivial to me. I didn't want fixing up our testing problems to block this change so I used the ugly approach you see in the diff. The benefit of this approach is that it makes it really obvious that there is a problem with how we setup tests, because as you point out we shouldn't be using v5 in tests.

[anorth]

I have addressed this in #1251 and will rebase this PR upon that one, removing the noise.

[codecov-io]

Codecov Report

Merging #1249 into anorth/prefactor will increase coverage by 0.0%.
The diff coverage is 81.8%.

@@               Coverage Diff                @@
##           anorth/prefactor   #1249   +/-   ##
================================================
  Coverage              70.3%   70.4%           
================================================
  Files                    72      72           
  Lines                  7169    7176    +7     
================================================
+ Hits                   5046    5053    +7     
  Misses                 1306    1306           
  Partials                817     817           

[anorth]

@ZenGround0 @acruikshank I have applied my own suggestions. Please carefully review the commits I added here, then I think it's good to go (after #1251).

[ZenGround0]

Nice demonstration of some of the benefits of pushing the network version down

[acruikshank]

LGTM