Unverified Commit 7beea9ff authored by asynchronous rob's avatar asynchronous rob Committed by GitHub
Browse files

fix clock drift for assignments issued before the block (#3851)



* fix clock drift for assignments issued before the block

* always broadcast tranche 0 assignments

* Update tests

* Change from min to max

* Remove erronious print statement

* guide: require fixed approval delay

* prevent approval by very recent assignments

* fix approval-checking tests

* fix main approval tests

* Fix tests

* Fix looked over test

* fix test
Co-authored-by: Lldenaurois's avatarLldenaurois <Ljdenaurois@gmail.com>
parent 9e4ee69a
Pipeline #160367 passed with stages
in 40 minutes and 21 seconds
......@@ -315,6 +315,11 @@ fn filled_tranche_iterator<'a>(
/// and tick parameters. This method also returns the next tick at which a `no_show` will occur
/// amongst the set of validators that have not submitted an approval.
///
/// This also bounds the earliest tick of all assignments to be equal to the
/// block tick for the purposes of the calculation, so no assignment can be treated
/// as being received before the block itself. This is unlikely if not impossible
/// in practice, but can occur during test code.
///
/// If the returned `next_no_show` is not None, there are two possible cases for the value of
/// based on the earliest assignment `tick` of a non-approving, yet-to-be-no-show validator:
/// - if `tick` <= `clock_drift`: the value will always be `clock_drift` + `no_show_duration`.
......@@ -323,13 +328,16 @@ fn count_no_shows(
assignments: &[(ValidatorIndex, Tick)],
approvals: &BitSlice<BitOrderLsb0, u8>,
clock_drift: Tick,
block_tick: Tick,
no_show_duration: Tick,
drifted_tick_now: Tick,
) -> (usize, Option<u64>) {
let mut next_no_show = None;
let no_shows = assignments
.iter()
.map(|(v_index, tick)| (v_index, tick.saturating_sub(clock_drift) + no_show_duration))
.map(|(v_index, tick)| {
(v_index, tick.max(&block_tick).saturating_sub(clock_drift) + no_show_duration)
})
.filter(|&(v_index, no_show_at)| {
let has_approved = if let Some(approved) = approvals.get(v_index.0 as usize) {
*approved
......@@ -418,6 +426,7 @@ pub fn tranches_to_approve(
assignments,
approvals,
clock_drift,
block_tick,
no_show_duration,
drifted_tick_now,
);
......@@ -635,7 +644,7 @@ mod tests {
#[test]
fn tranches_to_approve_everyone_present() {
let block_tick = 0;
let block_tick = 20;
let no_show_duration = 10;
let needed_approvals = 4;
......@@ -672,7 +681,7 @@ mod tests {
needed: 1,
tolerated_missing: 0,
next_no_show: None,
last_assignment_tick: Some(1)
last_assignment_tick: Some(21)
},
);
}
......@@ -1127,6 +1136,7 @@ mod tests {
fn test_count_no_shows(test: NoShowTest) {
let n_validators = 4;
let block_tick = 20;
let mut approvals = bitvec![BitOrderLsb0, u8; 0; n_validators];
for &v_index in &test.approvals {
......@@ -1137,6 +1147,7 @@ mod tests {
&test.assignments,
&approvals,
test.clock_drift,
block_tick,
test.no_show_duration,
test.drifted_tick_now,
);
......@@ -1160,13 +1171,13 @@ mod tests {
#[test]
fn count_no_shows_single_validator_is_next_no_show() {
test_count_no_shows(NoShowTest {
assignments: vec![(ValidatorIndex(1), 21)],
assignments: vec![(ValidatorIndex(1), 31)],
approvals: vec![],
clock_drift: 10,
no_show_duration: 10,
drifted_tick_now: 20,
exp_no_shows: 0,
exp_next_no_show: Some(31),
exp_next_no_show: Some(41),
})
}
......@@ -1199,26 +1210,26 @@ mod tests {
#[test]
fn count_no_shows_two_validators_next_no_show_ordered_first() {
test_count_no_shows(NoShowTest {
assignments: vec![(ValidatorIndex(1), 21), (ValidatorIndex(2), 22)],
assignments: vec![(ValidatorIndex(1), 31), (ValidatorIndex(2), 32)],
approvals: vec![],
clock_drift: 10,
no_show_duration: 10,
drifted_tick_now: 20,
exp_no_shows: 0,
exp_next_no_show: Some(31),
exp_next_no_show: Some(41),
})
}
#[test]
fn count_no_shows_two_validators_next_no_show_ordered_last() {
test_count_no_shows(NoShowTest {
assignments: vec![(ValidatorIndex(1), 22), (ValidatorIndex(2), 21)],
assignments: vec![(ValidatorIndex(1), 32), (ValidatorIndex(2), 31)],
approvals: vec![],
clock_drift: 10,
no_show_duration: 10,
drifted_tick_now: 20,
exp_no_shows: 0,
exp_next_no_show: Some(31),
exp_next_no_show: Some(41),
})
}
......@@ -1226,16 +1237,16 @@ mod tests {
fn count_no_shows_three_validators_one_almost_late_one_no_show_one_approving() {
test_count_no_shows(NoShowTest {
assignments: vec![
(ValidatorIndex(1), 21),
(ValidatorIndex(2), 20),
(ValidatorIndex(3), 20),
(ValidatorIndex(1), 31),
(ValidatorIndex(2), 19),
(ValidatorIndex(3), 19),
],
approvals: vec![3],
clock_drift: 10,
no_show_duration: 10,
drifted_tick_now: 20,
exp_no_shows: 1,
exp_next_no_show: Some(31),
exp_next_no_show: Some(41),
})
}
......@@ -1282,7 +1293,7 @@ mod tests {
no_show_duration: 20,
drifted_tick_now: 0,
exp_no_shows: 0,
exp_next_no_show: Some(30),
exp_next_no_show: Some(40),
})
}
......@@ -1295,7 +1306,7 @@ mod tests {
no_show_duration: 20,
drifted_tick_now: 0,
exp_no_shows: 0,
exp_next_no_show: Some(30),
exp_next_no_show: Some(40),
})
}
......
......@@ -1609,7 +1609,7 @@ fn subsystem_process_wakeup_schedules_wakeup() {
futures_timer::Delay::new(Duration::from_millis(100)).await;
// The wakeup should have been rescheduled.
assert!(clock.inner.lock().current_wakeup_is(20));
assert!(clock.inner.lock().current_wakeup_is(30));
virtual_overseer
});
......@@ -2234,8 +2234,8 @@ fn subsystem_process_wakeup_trigger_assignment_launch_approval() {
futures_timer::Delay::new(Duration::from_millis(200)).await;
assert!(clock.inner.lock().current_wakeup_is(slot_to_tick(slot + 1)));
clock.inner.lock().wakeup_all(slot_to_tick(slot + 1));
assert!(clock.inner.lock().current_wakeup_is(slot_to_tick(slot + 2)));
clock.inner.lock().wakeup_all(slot_to_tick(slot + 2));
assert_matches!(
overseer_recv(&mut virtual_overseer).await,
......@@ -2467,7 +2467,7 @@ fn subsystem_assignment_triggered_by_all_with_less_than_threshold() {
approvals_to_import: vec![2, 4],
ticks: vec![
2, // APPROVAL_DELAY
20, // Check for no shows
21, // Check for no shows
],
should_be_triggered: |t| t == 20,
});
......@@ -2483,7 +2483,7 @@ fn subsystem_assignment_not_triggered_by_all_with_threshold() {
approvals_to_import: vec![1, 3, 5],
ticks: vec![
2, // APPROVAL_DELAY
20, // Check no shows
21, // Check no shows
],
should_be_triggered: |_| false,
});
......@@ -2498,8 +2498,8 @@ fn subsystem_assignment_triggered_if_below_maximum_and_clock_is_equal() {
assignments_to_import: vec![1],
approvals_to_import: vec![],
ticks: vec![
20, // Check no shows
21, // Alice wakeup, assignment triggered
21, // Check no shows
23, // Alice wakeup, assignment triggered
],
should_be_triggered: |tick| tick >= 21,
});
......@@ -2516,7 +2516,7 @@ fn subsystem_assignment_not_triggered_more_than_maximum() {
ticks: vec![
2, // APPROVAL_DELAY
13, // Alice wakeup
20, // Check no shows
30, // Check no shows
],
should_be_triggered: |_| false,
});
......@@ -2524,16 +2524,15 @@ fn subsystem_assignment_not_triggered_more_than_maximum() {
#[test]
fn subsystem_assignment_triggered_if_at_maximum() {
// TODO(ladi): is this possible?
triggers_assignment_test(TriggersAssignmentConfig {
our_assigned_tranche: 11,
our_assigned_tranche: 21,
assign_validator_tranche: |_| Ok(2),
no_show_slots: 2,
assignments_to_import: vec![1],
approvals_to_import: vec![],
ticks: vec![
12, // Bob wakeup
20, // Check no shows
30, // Check no shows
],
should_be_triggered: |_| false,
});
......@@ -2583,7 +2582,7 @@ fn subsystem_assignment_not_triggered_if_at_maximum_but_clock_is_before_with_dri
12, // Charlie wakeup
13, // Dave wakeup
15, // Alice wakeup, noop
20, // Check no shows
30, // Check no shows
34, // Eve wakeup
],
should_be_triggered: |_| false,
......@@ -2755,7 +2754,7 @@ fn pre_covers_dont_stall_approval() {
// Wait for the no-show timer to observe the approval from
// tranche 0 and set a wakeup for tranche 1.
clock.inner.lock().set_tick(20);
clock.inner.lock().set_tick(30);
// Sleep to ensure we get a consistent read on the database.
futures_timer::Delay::new(Duration::from_millis(100)).await;
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment