ledger: Require leader schedules to be repeat-aligned#10047
Conversation
|
If this PR represents a change to the public RPC API:
Thank you for keeping the RPC clients in sync with the server API @vadorovsky. |
| @@ -192,7 +196,6 @@ mod tests { | |||
| #[test_case(457470, &[10, 20, 30], 12, 1, &[2, 1, 1, 1, 1, 1, 1, 1, 1, 2, 0, 2])] | |||
| #[test_case(3466545, &[10, 20, 30], 12, 1, &[2, 2, 0, 0, 2, 1, 1, 1, 0, 0, 2, 2])] | |||
| #[test_case(3466545, &[10, 20, 30], 13, 1, &[2, 2, 0, 0, 2, 1, 1, 1, 0, 0, 2, 2, 1])] | |||
| #[test_case(3466545, &[10, 20, 30], 13, 2, &[2, 2, 2, 2, 0, 0, 0, 0, 2, 2, 1, 1, 1])] | |||
There was a problem hiding this comment.
This test case makes no sense - the last 1 is repeated 3 times and the whole thing is not divisible by 2. Given that there are many other cases which are correct, I'm just removing it.
There was a problem hiding this comment.
The test case was supposed to test exactly the scenario you are forbidding - using len that is not a multiple of repeat. I agree though, that is impractical "feature", so should be fine to remove.
| @@ -4608,7 +4608,7 @@ pub mod tests { | |||
|
|
|||
| const TEST_MINT_LAMPORTS: u64 = 1_000_000_000; | |||
| const TEST_SIGNATURE_FEE: u64 = 5_000; | |||
| const TEST_SLOTS_PER_EPOCH: u64 = DELINQUENT_VALIDATOR_SLOT_DISTANCE + 1; | |||
| const TEST_SLOTS_PER_EPOCH: u64 = 256; | |||
There was a problem hiding this comment.
Happy to change it to some other value if anyone has strong feelings. What I know for sure is that using the validator's default (432_000) would make the tests running multiple minutes each.
Tests were constructing schedules with `repeat > 1` but feeding arrays whose length wasn’t a multiple of `repeat`, so some leaders never repeated the expected number of times. That inconsistency also blocks follow-up work on optimizing the schedule calculation (see anza-xyz#9126). Add a debug assert that the schedule length is divisible by `repeat`, and fix the tests to generate properly aligned schedules. Change `TEST_SLOTS_PER_EPOCH` in rpc from 129 (an incorrect, unaligned value) to 256, which stays above the delinquent-slot threshold while keeping the tests fast. Ref: anza-xyz#8280
vadorovsky
force-pushed
the
validate-epoch-slots
branch
from
dfdfbd2 to
a77055a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10047 +/- ##
=========================================
- Coverage 82.6% 82.6% -0.1%
=========================================
Files 844 844
Lines 316630 316633 +3
=========================================
- Hits 261595 261593 -2
- Misses 55035 55040 +5 🚀 New features to boost your workflow:
|
| @@ -192,7 +196,6 @@ mod tests { | |||
| #[test_case(457470, &[10, 20, 30], 12, 1, &[2, 1, 1, 1, 1, 1, 1, 1, 1, 2, 0, 2])] | |||
| #[test_case(3466545, &[10, 20, 30], 12, 1, &[2, 2, 0, 0, 2, 1, 1, 1, 0, 0, 2, 2])] | |||
| #[test_case(3466545, &[10, 20, 30], 13, 1, &[2, 2, 0, 0, 2, 1, 1, 1, 0, 0, 2, 2, 1])] | |||
| #[test_case(3466545, &[10, 20, 30], 13, 2, &[2, 2, 2, 2, 0, 0, 0, 0, 2, 2, 1, 1, 1])] | |||
There was a problem hiding this comment.
The test case was supposed to test exactly the scenario you are forbidding - using len that is not a multiple of repeat. I agree though, that is impractical "feature", so should be fine to remove.
brooksprumo
left a comment
brooksprumo
left a comment
There was a problem hiding this comment.
Looks good to me too. I'll defer final approval to @jstarry.
5 participants
Problem
Tests were constructing schedules with
repeat > 1but feeding arrays whose length wasn’t a multiple ofrepeat, so some leaders never repeated the expected number of times. That inconsistency also blocks follow-up work on optimizing the schedule calculation (see #9126).Summary of Changes
Add a debug assert that the schedule length is divisible by
repeat, and fix the tests to generate properly aligned schedules.Change
TEST_SLOTS_PER_EPOCHin rpc from 129 (an incorrect, unaligned value) to 256, which stays above the delinquent-slot threshold while keeping the tests fast.Ref: #8280