Extract and test count_no_shows method for approval voting

[Lldenaurois]

This commit extracts no_show computation into a pure function so that it can be
extensively unit tested.

This commit extracts no_show computation into a pure function so that it can be extensively unit tested. So far the tests behave mostly as I would expect, though I plan on further expanding them to better test the interactions of the Tick parameters.

The main thing that currently sticks out is the next_no_show calculation, which I suspect is adding an extra clock_drift factor. For example, in the current test harness, a validator assigned at tick 20 is counted as a no_show when drifted_tick_now is also 20. However, a validator assigned at tick 21 (which would be counted as a no_show at a drifted_tick_now of 21) is reported as a next_no_show of 31 (21 + clock_drift). If this is the correct behavior I can continue to add unit tests on this interaction, but wanted to clarify before proceeding.

[rphmeier]

style nit: I'd prefer each test to get its own #[test] function which uses a helper function as the body of this loop

[Lldenaurois]

Good to know, will take note of that going forward.

[Lldenaurois]

@rphmeier force pushed the change to create each test case individually. Should be good to go.

[rphmeier]

For example, in the current test harness, a validator assigned at tick 20 is counted as a no_show when drifted_tick_now is also 20. However, a validator assigned at tick 21 (which would be counted as a no_show at a drifted_tick_now of 21) is reported as a next_no_show of 31 (21 + clock_drift). If this is the correct behavior I can continue to add unit tests on this interaction, but wanted to clarify before proceeding.

Yes, I think this is correct. The reasoning is that the next_no_show is measured in ticks (time), not tranches, so the scheduler can schedule a wake-up at the next no-show. With clock_drift = 10, drifted_tick_now = 20, the absolute tick is 30 and so a wakeup will be scheduled 1 tick from now at tick 31 to check for the no-show.

I do think that a comment explaining logic in the API would be useful.

cc @burdges to confirm

[burdges]

Yes, we want being no-show to indicate that you'll likely not show within any sane time period, like your power when off or you got DoS. If your assignment was late then your approval vote will likely be late too, so we measure no shows in wall time since we learned about your assignment.

[Lldenaurois]

Should validator indexes past the length of the approvals vector be ignored or handled?

[rphmeier]

Should be ignored; Garbage-in-Garbage-out

[rphmeier]

I do think that a comment explaining logic in the API would be useful.

Please add a commit adding a comment to the + clock_drift line which explains the logic. Remembering why that's correct was not obvious at all so it'll be good to have it there for posterity

[Lldenaurois]

@rphmeier Added comment and modified logic such that we ignore indexes past the length of the approval vector.

Should be ready to go.

[rphmeier]

bot merge

[ghost]

Trying merge.