From 7d30dd4406808bc69d7a0770585bac8978dfb19f Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 22 Sep 2021 12:55:34 -0400 Subject: [PATCH 1/5] always broadcast tranche 0 assignments --- node/core/approval-voting/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index b7588710b1a3..fc9a271c2cee 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -1893,6 +1893,7 @@ fn should_trigger_assignment( match approval_entry.our_assignment() { None => false, Some(ref assignment) if assignment.triggered() => false, + Some(ref assignment) if assignment.tranche() == 0 => true, Some(ref assignment) => { match required_tranches { RequiredTranches::All => !approval_checking::check_approval( From f0b2f4ecbdf709205adea19d0df285b06a054884 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 29 Sep 2021 22:13:11 +0200 Subject: [PATCH 2/5] guide: require fixed approval delay --- .../src/node/approval/approval-voting.md | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/roadmap/implementers-guide/src/node/approval/approval-voting.md b/roadmap/implementers-guide/src/node/approval/approval-voting.md index 2366d7281d9e..c6367c050a04 100644 --- a/roadmap/implementers-guide/src/node/approval/approval-voting.md +++ b/roadmap/implementers-guide/src/node/approval/approval-voting.md @@ -111,6 +111,9 @@ CandidateHash => CandidateEntry ```rust const APPROVAL_SESSIONS: SessionIndex = 6; + +// The minimum amount of ticks that an assignment must have been known for. +const APPROVAL_DELAY: Tick = 2; ``` In-memory state: @@ -268,7 +271,7 @@ On receiving an `ApprovedAncestor(Hash, BlockNumber, response_channel)`: * If the `approval_entry` is approved, this doesn't need to be woken up again. * If `RequiredTranches::All` - no wakeup. We assume other incoming votes will trigger wakeup and potentially re-schedule. * If `RequiredTranches::Pending { considered, next_no_show, uncovered, maximum_broadcast, clock_drift }` - schedule at the lesser of the next no-show tick, or the tick, offset positively by `clock_drift` of the next non-empty tranche we are aware of after `considered`, including any tranche containing our own unbroadcast assignment. This can lead to no wakeup in the case that we have already broadcast our assignment and there are no pending no-shows; that is, we have approval votes for every assignment we've received that is not already a no-show. In this case, we will be re-triggered by other validators broadcasting their assignments. - * If `RequiredTranches::Exact { next_no_show, .. }` - set a wakeup for the next no-show tick. + * If `RequiredTranches::Exact { next_no_show, latest_assignment_tick, .. }` - set a wakeup for the earlier of the next no-show tick or the latest assignment tick + `APPROVAL_DELAY`. #### Launch Approval Work @@ -335,6 +338,8 @@ enum RequiredTranches { /// event that there are some assignments that don't have corresponding approval votes. If this /// is `None`, all assignments have approvals. next_no_show: Option, + /// The last tick at which a needed assignment was received. + last_assignment_tick: Option, } } ``` @@ -355,8 +360,9 @@ Likewise, when considering how many tranches to take, the no-show depth should b * Set a clock drift of `depth * no_show_duration` * Take tranches up to `tranche_now - clock_drift` until all needed assignments are met. * Keep track of the `next_no_show` according to the clock drift, as we go. + * Keep track of the `last_assignment_tick` as we go. * If running out of tranches before then, return `Pending { considered, next_no_show, maximum_broadcast, clock_drift }` - * If there are no no-shows, return `Exact { needed, tolerated_missing, next_no_show }` + * If there are no no-shows, return `Exact { needed, tolerated_missing, next_no_show, last_assignment_tick }` * `maximum_broadcast` is either `DelayTranche::max_value()` at tranche 0 or otherwise by the last considered tranche + the number of uncovered no-shows at this point. * If there are no-shows, return to the beginning, incrementing `depth` and attempting to cover the number of no-shows. Each no-show must be covered by a non-empty tranche, which are tranches that have at least one assignment. Each non-empty tranche covers exactly one no-show. * If at any point, it seems that all validators are required, do an early return with `RequiredTranches::All` which indicates that everyone should broadcast. @@ -367,7 +373,9 @@ Likewise, when considering how many tranches to take, the no-show depth should b * If we have `3 * n_approvals > n_validators`, return true. This is because any set with f+1 validators must have at least one honest validator, who has approved the candidate. * If `n_tranches` is `RequiredTranches::Pending`, return false * If `n_tranches` is `RequiredTranches::All`, return false. - * If `n_tranches` is `RequiredTranches::Exact { tranche, tolerated_missing, .. }`, then we return whether all assigned validators up to `tranche` less `tolerated_missing` have approved. e.g. if we had 5 tranches and 1 tolerated missing, we would accept only if all but 1 of assigned validators in tranches 0..=5 have approved. In that example, we also accept all validators in tranches 0..=5 having approved, but that would indicate that the `RequiredTranches` value was incorrectly constructed, so it is not realistic. `tolerated_missing` actually represents covered no-shows. If there are more missing approvals than there are tolerated missing, that indicates that there are some assignments which are not yet no-shows, but may become no-shows, and we should wait for the validators to either approve or become no-shows. + * If `n_tranches` is `RequiredTranches::Exact { tranche, tolerated_missing, latest_assignment_tick, .. }`, then we return whether all assigned validators up to `tranche` less `tolerated_missing` have approved and `latest_assignment_tick + APPROVAL_DELAY >= tick_now`. + * e.g. if we had 5 tranches and 1 tolerated missing, we would accept only if all but 1 of assigned validators in tranches 0..=5 have approved. In that example, we also accept all validators in tranches 0..=5 having approved, but that would indicate that the `RequiredTranches` value was incorrectly constructed, so it is not realistic. `tolerated_missing` actually represents covered no-shows. If there are more missing approvals than there are tolerated missing, that indicates that there are some assignments which are not yet no-shows, but may become no-shows, and we should wait for the validators to either approve or become no-shows. + * e.g. If the above passes and the `latest_assignment_tick` was 5 and the current tick was 6, then we'd return false. ### Time From 2ed97baba8e9af4e9e80105a02d4400bca6c4e98 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 29 Sep 2021 22:13:50 +0200 Subject: [PATCH 3/5] prevent approval by very recent assignments --- .../approval-voting/src/approval_checking.rs | 45 ++++++++++++++----- node/core/approval-voting/src/lib.rs | 24 ++++++++-- 2 files changed, 55 insertions(+), 14 deletions(-) diff --git a/node/core/approval-voting/src/approval_checking.rs b/node/core/approval-voting/src/approval_checking.rs index 17a5cda64c87..a4eb050e808e 100644 --- a/node/core/approval-voting/src/approval_checking.rs +++ b/node/core/approval-voting/src/approval_checking.rs @@ -58,6 +58,8 @@ pub enum RequiredTranches { /// event that there are some assignments that don't have corresponding approval votes. If this /// is `None`, all assignments have approvals. next_no_show: Option, + /// The last tick at which a needed assignment was received. + last_assignment_tick: Option, }, } @@ -66,18 +68,22 @@ pub enum RequiredTranches { pub enum Check { /// The candidate is unapproved. Unapproved, - /// The candidate is approved, with the given amount of no-shows. - Approved(usize), + /// The candidate is approved, with the given amount of no-shows, + /// with the last counted assignment being received at the given + /// tick. + Approved(usize, Option), /// The candidate is approved by one third of all validators. ApprovedOneThird, } impl Check { - /// Whether the candidate is approved. - pub fn is_approved(&self) -> bool { + /// Whether the candidate is approved and all relevant assignments + /// have at most the given assignment tick. + pub fn is_approved(&self, max_assignment_tick: Tick) -> bool { match *self { Check::Unapproved => false, - Check::Approved(_) => true, + Check::Approved(_, last_assignment_tick) => + last_assignment_tick.map_or(true, |t| t <= max_assignment_tick), Check::ApprovedOneThird => true, } } @@ -85,7 +91,7 @@ impl Check { /// The number of known no-shows in this computation. pub fn known_no_shows(&self) -> usize { match *self { - Check::Approved(n) => n, + Check::Approved(n, _) => n, _ => 0, } } @@ -107,7 +113,7 @@ pub fn check_approval( match required { RequiredTranches::Pending { .. } => Check::Unapproved, RequiredTranches::All => Check::Unapproved, - RequiredTranches::Exact { needed, tolerated_missing, .. } => { + RequiredTranches::Exact { needed, tolerated_missing, last_assignment_tick, .. } => { // whether all assigned validators up to `needed` less no_shows have approved. // e.g. if we had 5 tranches and 1 no-show, we would accept all validators in // tranches 0..=5 except for 1 approving. In that example, we also accept all @@ -130,7 +136,7 @@ pub fn check_approval( // that will surpass a minimum amount of checks. // shouldn't typically go above, since all no-shows are supposed to be covered. if n_approved + tolerated_missing >= n_assigned { - Check::Approved(tolerated_missing) + Check::Approved(tolerated_missing, last_assignment_tick) } else { Check::Unapproved } @@ -170,6 +176,8 @@ struct State { uncovered: usize, /// The next tick at which a no-show would occur, if any. next_no_show: Option, + /// The last tick at which a considered assignment was received. + last_assignment_tick: Option, } impl State { @@ -192,6 +200,7 @@ impl State { needed: tranche, tolerated_missing: self.covered, next_no_show: self.next_no_show, + last_assignment_tick: self.last_assignment_tick, } } @@ -226,6 +235,7 @@ impl State { new_assignments: usize, new_no_shows: usize, next_no_show: Option, + last_assignment_tick: Option, ) -> State { let new_covered = if self.depth == 0 { new_assignments @@ -246,6 +256,7 @@ impl State { }; let uncovered = self.uncovered + new_no_shows; let next_no_show = super::min_prefer_some(self.next_no_show, next_no_show); + let last_assignment_tick = std::cmp::max(self.last_assignment_tick, last_assignment_tick); let (depth, covering, uncovered) = if covering == 0 { if uncovered == 0 { @@ -257,7 +268,15 @@ impl State { (self.depth, covering, uncovered) }; - State { assignments, depth, covered, covering, uncovered, next_no_show } + State { + assignments, + depth, + covered, + covering, + uncovered, + next_no_show, + last_assignment_tick, + } } } @@ -356,6 +375,7 @@ pub fn tranches_to_approve( covering: needed_approvals, uncovered: 0, next_no_show: None, + last_assignment_tick: None, }; // The `ApprovalEntry` doesn't have any data for empty tranches. We still want to iterate over @@ -384,6 +404,11 @@ pub fn tranches_to_approve( .filter(|(v_index, _)| v_index.0 < n_validators as u32) .count(); + // Get the latest tick of valid validator assignments. + let last_assignment_tick = assignments.iter() + .map(|(_, t)| *t) + .max(); + // count no-shows. An assignment is a no-show if there is no corresponding approval vote // after a fixed duration. // @@ -397,7 +422,7 @@ pub fn tranches_to_approve( drifted_tick_now, ); - let s = s.advance(n_assignments, no_shows, next_no_show); + let s = s.advance(n_assignments, no_shows, next_no_show, last_assignment_tick); let output = s.output(tranche, needed_approvals, n_validators, no_show_duration); *state = match output { diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index fc9a271c2cee..90c61bf4a057 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -96,6 +96,7 @@ const APPROVAL_SESSIONS: SessionIndex = 6; const APPROVAL_CHECKING_TIMEOUT: Duration = Duration::from_secs(120); const APPROVAL_CACHE_SIZE: usize = 1024; const TICK_TOO_FAR_IN_FUTURE: Tick = 20; // 10 seconds. +const APPROVAL_DELAY: Tick = 2; const LOG_TARGET: &str = "parachain::approval-voting"; /// Configuration for the approval voting subsystem @@ -1399,13 +1400,21 @@ fn schedule_wakeup_action( block_number: BlockNumber, candidate_hash: CandidateHash, block_tick: Tick, + tick_now: Tick, required_tranches: RequiredTranches, ) -> Option { let maybe_action = match required_tranches { _ if approval_entry.is_approved() => None, RequiredTranches::All => None, - RequiredTranches::Exact { next_no_show, .. } => next_no_show - .map(|tick| Action::ScheduleWakeup { block_hash, block_number, candidate_hash, tick }), + RequiredTranches::Exact { next_no_show, last_assignment_tick, .. } => { + // Take the earlier of the next no show or the last assignment tick + required delay, + // only considering the latter if it is after the current moment. + min_prefer_some( + last_assignment_tick.map(|l| l + APPROVAL_DELAY).filter(|t| t > &tick_now), + next_no_show, + ) + .map(|tick| Action::ScheduleWakeup { block_hash, block_number, candidate_hash, tick }) + }, RequiredTranches::Pending { considered, next_no_show, clock_drift, .. } => { // select the minimum of `next_no_show`, or the tick of the next non-empty tranche // after `considered`, including any tranche that might contain our own untriggered @@ -1586,6 +1595,7 @@ fn check_and_import_assignment( block_entry.block_number(), assigned_candidate_hash, status.block_tick, + tick_now, status.required_tranches, )); } @@ -1790,6 +1800,8 @@ fn advance_approval_state( let block_hash = block_entry.block_hash(); let block_number = block_entry.block_number(); + let tick_now = state.clock.tick_now(); + let (is_approved, status) = if let Some((approval_entry, status)) = state.approval_status(&block_entry, &candidate_entry) { @@ -1799,7 +1811,10 @@ fn advance_approval_state( status.required_tranches.clone(), ); - let is_approved = check.is_approved(); + // Check whether this is approved, while allowing a maximum + // assignment tick of `now - APPROVAL_DELAY` - that is, that + // all counted assignments are at least `APPROVAL_DELAY` ticks old. + let is_approved = check.is_approved(tick_now.saturating_sub(APPROVAL_DELAY)); if is_approved { tracing::trace!( @@ -1864,6 +1879,7 @@ fn advance_approval_state( block_number, candidate_hash, status.block_tick, + tick_now, status.required_tranches, )); @@ -1901,7 +1917,7 @@ fn should_trigger_assignment( &approval_entry, RequiredTranches::All, ) - .is_approved(), + .is_approved(Tick::max_value()), // when all are required, we are just waiting for the first 1/3+ RequiredTranches::Pending { maximum_broadcast, clock_drift, .. } => { let drifted_tranche_now = tranche_now.saturating_sub(clock_drift as DelayTranche); From 1ae652fd892e369a57533d3954b92d301543e246 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 29 Sep 2021 22:40:09 +0200 Subject: [PATCH 4/5] fix approval-checking tests --- .../approval-voting/src/approval_checking.rs | 156 ++++++++++++------ 1 file changed, 103 insertions(+), 53 deletions(-) diff --git a/node/core/approval-voting/src/approval_checking.rs b/node/core/approval-voting/src/approval_checking.rs index a4eb050e808e..644d820f38e5 100644 --- a/node/core/approval-voting/src/approval_checking.rs +++ b/node/core/approval-voting/src/approval_checking.rs @@ -484,7 +484,7 @@ mod tests { clock_drift: 0, }, ) - .is_approved()); + .is_approved(Tick::max_value())); } #[test] @@ -527,21 +527,36 @@ mod tests { assert!(check_approval( &candidate, &approval_entry, - RequiredTranches::Exact { needed: 0, tolerated_missing: 0, next_no_show: None }, + RequiredTranches::Exact { + needed: 0, + tolerated_missing: 0, + next_no_show: None, + last_assignment_tick: None + }, ) - .is_approved()); + .is_approved(Tick::max_value())); assert!(!check_approval( &candidate, &approval_entry, - RequiredTranches::Exact { needed: 1, tolerated_missing: 0, next_no_show: None }, + RequiredTranches::Exact { + needed: 1, + tolerated_missing: 0, + next_no_show: None, + last_assignment_tick: None + }, ) - .is_approved()); + .is_approved(Tick::max_value())); assert!(check_approval( &candidate, &approval_entry, - RequiredTranches::Exact { needed: 1, tolerated_missing: 2, next_no_show: None }, + RequiredTranches::Exact { + needed: 1, + tolerated_missing: 2, + next_no_show: None, + last_assignment_tick: None + }, ) - .is_approved()); + .is_approved(Tick::max_value())); } #[test] @@ -581,8 +596,12 @@ mod tests { } .into(); - let exact_all = - RequiredTranches::Exact { needed: 10, tolerated_missing: 0, next_no_show: None }; + let exact_all = RequiredTranches::Exact { + needed: 10, + tolerated_missing: 0, + next_no_show: None, + last_assignment_tick: None, + }; let pending_all = RequiredTranches::Pending { considered: 5, @@ -591,20 +610,27 @@ mod tests { clock_drift: 12, }; - assert!(!check_approval(&candidate, &approval_entry, RequiredTranches::All,).is_approved()); + assert!(!check_approval(&candidate, &approval_entry, RequiredTranches::All,) + .is_approved(Tick::max_value())); - assert!(!check_approval(&candidate, &approval_entry, exact_all.clone(),).is_approved()); + assert!(!check_approval(&candidate, &approval_entry, exact_all.clone(),) + .is_approved(Tick::max_value())); - assert!(!check_approval(&candidate, &approval_entry, pending_all.clone(),).is_approved()); + assert!(!check_approval(&candidate, &approval_entry, pending_all.clone(),) + .is_approved(Tick::max_value())); // This creates a set of 4/10 approvals, which is always an approval. candidate.mark_approval(ValidatorIndex(3)); - assert!(check_approval(&candidate, &approval_entry, RequiredTranches::All,).is_approved()); + assert!(check_approval(&candidate, &approval_entry, RequiredTranches::All,) + .is_approved(Tick::max_value())); - assert!(check_approval(&candidate, &approval_entry, exact_all,).is_approved()); + assert!( + check_approval(&candidate, &approval_entry, exact_all,).is_approved(Tick::max_value()) + ); - assert!(check_approval(&candidate, &approval_entry, pending_all,).is_approved()); + assert!(check_approval(&candidate, &approval_entry, pending_all,) + .is_approved(Tick::max_value())); } #[test] @@ -642,7 +668,12 @@ mod tests { no_show_duration, needed_approvals, ), - RequiredTranches::Exact { needed: 1, tolerated_missing: 0, next_no_show: None }, + RequiredTranches::Exact { + needed: 1, + tolerated_missing: 0, + next_no_show: None, + last_assignment_tick: Some(1) + }, ); } @@ -845,6 +876,7 @@ mod tests { needed: 1, tolerated_missing: 0, next_no_show: Some(block_tick + no_show_duration + 1), + last_assignment_tick: Some(block_tick + 1), }, ); @@ -863,6 +895,7 @@ mod tests { needed: 2, tolerated_missing: 1, next_no_show: Some(block_tick + 2 * no_show_duration + 2), + last_assignment_tick: Some(block_tick + no_show_duration + 2), }, ); @@ -930,7 +963,12 @@ mod tests { no_show_duration, needed_approvals, ), - RequiredTranches::Exact { needed: 2, tolerated_missing: 1, next_no_show: None }, + RequiredTranches::Exact { + needed: 2, + tolerated_missing: 1, + next_no_show: None, + last_assignment_tick: Some(block_tick + no_show_duration + 2) + }, ); // Even though tranche 2 has 2 validators, it only covers 1 no-show. @@ -968,7 +1006,12 @@ mod tests { no_show_duration, needed_approvals, ), - RequiredTranches::Exact { needed: 3, tolerated_missing: 2, next_no_show: None }, + RequiredTranches::Exact { + needed: 3, + tolerated_missing: 2, + next_no_show: None, + last_assignment_tick: Some(block_tick + no_show_duration + 2), + }, ); } @@ -1281,43 +1324,50 @@ mod tests { exp_next_no_show: None, }) } -} -#[test] -fn depth_0_covering_not_treated_as_such() { - let state = State { - assignments: 0, - depth: 0, - covered: 0, - covering: 10, - uncovered: 0, - next_no_show: None, - }; - - assert_eq!( - state.output(0, 10, 10, 20), - RequiredTranches::Pending { - considered: 0, + #[test] + fn depth_0_covering_not_treated_as_such() { + let state = State { + assignments: 0, + depth: 0, + covered: 0, + covering: 10, + uncovered: 0, next_no_show: None, - maximum_broadcast: DelayTranche::max_value(), - clock_drift: 0, - }, - ); -} + last_assignment_tick: None, + }; -#[test] -fn depth_0_issued_as_exact_even_when_all() { - let state = State { - assignments: 10, - depth: 0, - covered: 0, - covering: 0, - uncovered: 0, - next_no_show: None, - }; + assert_eq!( + state.output(0, 10, 10, 20), + RequiredTranches::Pending { + considered: 0, + next_no_show: None, + maximum_broadcast: DelayTranche::max_value(), + clock_drift: 0, + }, + ); + } + + #[test] + fn depth_0_issued_as_exact_even_when_all() { + let state = State { + assignments: 10, + depth: 0, + covered: 0, + covering: 0, + uncovered: 0, + next_no_show: None, + last_assignment_tick: None, + }; - assert_eq!( - state.output(0, 10, 10, 20), - RequiredTranches::Exact { needed: 0, tolerated_missing: 0, next_no_show: None }, - ); + assert_eq!( + state.output(0, 10, 10, 20), + RequiredTranches::Exact { + needed: 0, + tolerated_missing: 0, + next_no_show: None, + last_assignment_tick: None + }, + ); + } } From 29334596dcd62812c9a136ae1ed2dbcff9bcb109 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Thu, 30 Sep 2021 00:05:23 +0200 Subject: [PATCH 5/5] fix main approval tests --- node/core/approval-voting/src/tests.rs | 245 ++++++++++++++++++++++--- 1 file changed, 220 insertions(+), 25 deletions(-) diff --git a/node/core/approval-voting/src/tests.rs b/node/core/approval-voting/src/tests.rs index fe0725384ba7..2582db301a60 100644 --- a/node/core/approval-voting/src/tests.rs +++ b/node/core/approval-voting/src/tests.rs @@ -172,7 +172,13 @@ impl MockClockInner { self.wakeups.iter().map(|w| w.0).next() } - fn has_wakeup(&self, tick: Tick) -> bool { + fn current_wakeup_is(&mut self, tick: Tick) -> bool { + // first, prune away all wakeups which aren't actually being awaited + // on. + self.wakeups.retain(|(_, tx)| !tx.is_canceled()); + + // Then see if any remaining wakeups match the tick. + // This should be the only wakeup. self.wakeups.binary_search_by_key(&tick, |w| w.0).is_ok() } @@ -1412,6 +1418,8 @@ fn subsystem_second_approval_import_only_schedules_wakeups() { .. } = test_harness; + clock.inner.lock().set_tick(APPROVAL_DELAY); + let block_hash = Hash::repeat_byte(0x01); let candidate_hash = { @@ -1425,13 +1433,41 @@ fn subsystem_second_approval_import_only_schedules_wakeups() { let validator = ValidatorIndex(0); let session_index = 1; + let validators = vec![ + Sr25519Keyring::Alice, + Sr25519Keyring::Bob, + Sr25519Keyring::Charlie, + Sr25519Keyring::Dave, + Sr25519Keyring::Eve, + ]; + let session_info = SessionInfo { + validators: validators.iter().map(|v| v.public().into()).collect(), + validator_groups: vec![ + vec![ValidatorIndex(0), ValidatorIndex(1)], + vec![ValidatorIndex(2)], + vec![ValidatorIndex(3), ValidatorIndex(4)], + ], + needed_approvals: 1, + discovery_keys: validators.iter().map(|v| v.public().into()).collect(), + assignment_keys: validators.iter().map(|v| v.public().into()).collect(), + n_cores: validators.len() as _, + zeroth_delay_tranche_width: 5, + relay_vrf_modulo_samples: 3, + n_delay_tranches: 50, + no_show_slots: 2, + }; + // Add block hash 0x01... ChainBuilder::new() .add_block( block_hash, ChainBuilder::GENESIS_HASH, 1, - BlockConfig { slot: Slot::from(0), candidates: None, session_info: None }, + BlockConfig { + slot: Slot::from(0), + candidates: None, + session_info: Some(session_info), + }, ) .build(&mut virtual_overseer) .await; @@ -1446,6 +1482,8 @@ fn subsystem_second_approval_import_only_schedules_wakeups() { assert_eq!(rx.await, Ok(AssignmentCheckResult::Accepted)); + assert!(clock.inner.lock().current_wakeup_is(APPROVAL_DELAY + 2)); + let rx = check_and_import_approval( &mut virtual_overseer, block_hash, @@ -1453,7 +1491,7 @@ fn subsystem_second_approval_import_only_schedules_wakeups() { validator, candidate_hash, session_index, - true, + false, true, None, ) @@ -1461,11 +1499,8 @@ fn subsystem_second_approval_import_only_schedules_wakeups() { assert_eq!(rx.await, Ok(ApprovalCheckResult::Accepted)); - // The clock should already have wakeups from the prior operations. Clear them to assert - // that the second approval adds more wakeups. - assert!(clock.inner.lock().has_wakeup(20)); - clock.inner.lock().wakeup_all(20); - assert!(!clock.inner.lock().has_wakeup(20)); + futures_timer::Delay::new(Duration::from_millis(100)).await; + assert!(clock.inner.lock().current_wakeup_is(APPROVAL_DELAY + 2)); let rx = check_and_import_approval( &mut virtual_overseer, @@ -1482,7 +1517,8 @@ fn subsystem_second_approval_import_only_schedules_wakeups() { assert_eq!(rx.await, Ok(ApprovalCheckResult::Accepted)); - assert!(clock.inner.lock().has_wakeup(20)); + futures_timer::Delay::new(Duration::from_millis(100)).await; + assert!(clock.inner.lock().current_wakeup_is(APPROVAL_DELAY + 2)); virtual_overseer }); @@ -1524,7 +1560,7 @@ fn subsystem_assignment_import_updates_candidate_entry_and_schedules_wakeup() { assert_eq!(rx.await, Ok(AssignmentCheckResult::Accepted)); - assert!(clock.inner.lock().has_wakeup(20)); + assert!(clock.inner.lock().current_wakeup_is(2)); virtual_overseer }); @@ -1566,14 +1602,14 @@ fn subsystem_process_wakeup_schedules_wakeup() { assert_eq!(rx.await, Ok(AssignmentCheckResult::Accepted)); - assert!(clock.inner.lock().has_wakeup(20)); + assert!(clock.inner.lock().current_wakeup_is(2)); // Activate the wakeup present above, and sleep to allow process_wakeups to execute.. - clock.inner.lock().wakeup_all(20); + clock.inner.lock().set_tick(2); futures_timer::Delay::new(Duration::from_millis(100)).await; // The wakeup should have been rescheduled. - assert!(clock.inner.lock().has_wakeup(20)); + assert!(clock.inner.lock().current_wakeup_is(20)); virtual_overseer }); @@ -1772,10 +1808,6 @@ fn import_checked_approval_updates_entries_and_schedules() { assert_eq!(rx.await, Ok(AssignmentCheckResult::Accepted),); - // Clear any wake ups from the assignment imports. - assert!(clock.inner.lock().has_wakeup(20)); - clock.inner.lock().wakeup_all(20); - let session_index = 1; let sig_a = sign_approval(Sr25519Keyring::Alice, candidate_hash, session_index); @@ -1801,10 +1833,10 @@ fn import_checked_approval_updates_entries_and_schedules() { // approval. let candidate_entry = store.load_candidate_entry(&candidate_hash).unwrap().unwrap(); assert!(!candidate_entry.approval_entry(&block_hash).unwrap().is_approved()); - assert!(clock.inner.lock().has_wakeup(20)); + assert!(clock.inner.lock().current_wakeup_is(2)); // Clear the wake ups to assert that later approval also schedule wakeups. - clock.inner.lock().wakeup_all(20); + clock.inner.lock().wakeup_all(2); let sig_b = sign_approval(Sr25519Keyring::Bob, candidate_hash, session_index); let rx = check_and_import_approval( @@ -1838,7 +1870,6 @@ fn import_checked_approval_updates_entries_and_schedules() { // The candidate should now be approved. let candidate_entry = store.load_candidate_entry(&candidate_hash).unwrap().unwrap(); assert!(candidate_entry.approval_entry(&block_hash).unwrap().is_approved()); - assert!(clock.inner.lock().has_wakeup(20)); virtual_overseer }); @@ -2195,15 +2226,15 @@ fn subsystem_process_wakeup_trigger_assignment_launch_approval() { .build(&mut virtual_overseer) .await; - assert!(!clock.inner.lock().has_wakeup(1)); + assert!(!clock.inner.lock().current_wakeup_is(1)); clock.inner.lock().wakeup_all(1); - assert!(clock.inner.lock().has_wakeup(slot_to_tick(slot))); + assert!(clock.inner.lock().current_wakeup_is(slot_to_tick(slot))); clock.inner.lock().wakeup_all(slot_to_tick(slot)); futures_timer::Delay::new(Duration::from_millis(200)).await; - assert!(clock.inner.lock().has_wakeup(slot_to_tick(slot + 1))); + assert!(clock.inner.lock().current_wakeup_is(slot_to_tick(slot + 1))); clock.inner.lock().wakeup_all(slot_to_tick(slot + 1)); assert_matches!( @@ -2435,9 +2466,10 @@ fn subsystem_assignment_triggered_by_all_with_less_than_threshold() { assignments_to_import: vec![1, 2, 3, 4, 5], approvals_to_import: vec![2, 4], ticks: vec![ + 2, // APPROVAL_DELAY 20, // Check for no shows ], - should_be_triggered: |_| true, + should_be_triggered: |t| t == 20, }); } @@ -2450,6 +2482,7 @@ fn subsystem_assignment_not_triggered_by_all_with_threshold() { assignments_to_import: vec![1, 2, 3, 4, 5], approvals_to_import: vec![1, 3, 5], ticks: vec![ + 2, // APPROVAL_DELAY 20, // Check no shows ], should_be_triggered: |_| false, @@ -2481,6 +2514,7 @@ fn subsystem_assignment_not_triggered_more_than_maximum() { assignments_to_import: vec![2, 3], approvals_to_import: vec![], ticks: vec![ + 2, // APPROVAL_DELAY 13, // Alice wakeup 20, // Check no shows ], @@ -2717,7 +2751,7 @@ fn pre_covers_dont_stall_approval() { // The candidate should not be approved. let candidate_entry = store.load_candidate_entry(&candidate_hash).unwrap().unwrap(); assert!(!candidate_entry.approval_entry(&block_hash).unwrap().is_approved()); - assert!(clock.inner.lock().has_wakeup(20)); + assert!(clock.inner.lock().current_wakeup_is(2)); // Wait for the no-show timer to observe the approval from // tranche 0 and set a wakeup for tranche 1. @@ -2749,3 +2783,164 @@ fn pre_covers_dont_stall_approval() { virtual_overseer }); } + +#[test] +fn waits_until_approving_assignments_are_old_enough() { + // A, B are tranche 0. + + let assignment_criteria = Box::new(MockAssignmentCriteria::check_only(|_| Ok(0))); + + let config = HarnessConfigBuilder::default().assignment_criteria(assignment_criteria).build(); + let store = config.backend(); + test_harness(config, |test_harness| async move { + let TestHarness { + mut virtual_overseer, + clock, + sync_oracle_handle: _sync_oracle_handle, + .. + } = test_harness; + + clock.inner.lock().set_tick(APPROVAL_DELAY); + + let block_hash = Hash::repeat_byte(0x01); + let validator_index_a = ValidatorIndex(0); + let validator_index_b = ValidatorIndex(1); + + let validators = vec![ + Sr25519Keyring::Alice, + Sr25519Keyring::Bob, + Sr25519Keyring::Charlie, + Sr25519Keyring::Dave, + Sr25519Keyring::Eve, + Sr25519Keyring::One, + ]; + let session_info = SessionInfo { + validators: validators.iter().map(|v| v.public().into()).collect(), + validator_groups: vec![ + vec![ValidatorIndex(0), ValidatorIndex(1)], + vec![ValidatorIndex(2), ValidatorIndex(5)], + vec![ValidatorIndex(3), ValidatorIndex(4)], + ], + needed_approvals: 2, + discovery_keys: validators.iter().map(|v| v.public().into()).collect(), + assignment_keys: validators.iter().map(|v| v.public().into()).collect(), + n_cores: validators.len() as _, + zeroth_delay_tranche_width: 5, + relay_vrf_modulo_samples: 3, + n_delay_tranches: 50, + no_show_slots: 2, + }; + + let candidate_descriptor = make_candidate(1.into(), &block_hash); + let candidate_hash = candidate_descriptor.hash(); + + let head: Hash = ChainBuilder::GENESIS_HASH; + let mut builder = ChainBuilder::new(); + let slot = Slot::from(1 as u64); + builder.add_block( + block_hash, + head, + 1, + BlockConfig { + slot, + candidates: Some(vec![(candidate_descriptor, CoreIndex(0), GroupIndex(0))]), + session_info: Some(session_info), + }, + ); + builder.build(&mut virtual_overseer).await; + + let candidate_index = 0; + + let rx = check_and_import_assignment( + &mut virtual_overseer, + block_hash, + candidate_index, + validator_index_a, + ) + .await; + + assert_eq!(rx.await, Ok(AssignmentCheckResult::Accepted),); + + let rx = check_and_import_assignment( + &mut virtual_overseer, + block_hash, + candidate_index, + validator_index_b, + ) + .await; + + assert_eq!(rx.await, Ok(AssignmentCheckResult::Accepted),); + + assert!(clock.inner.lock().current_wakeup_is(APPROVAL_DELAY + APPROVAL_DELAY)); + + let session_index = 1; + + let sig_a = sign_approval(Sr25519Keyring::Alice, candidate_hash, session_index); + let rx = check_and_import_approval( + &mut virtual_overseer, + block_hash, + candidate_index, + validator_index_a, + candidate_hash, + session_index, + false, + true, + Some(sig_a), + ) + .await; + + assert_eq!(rx.await, Ok(ApprovalCheckResult::Accepted),); + + let sig_b = sign_approval(Sr25519Keyring::Bob, candidate_hash, session_index); + + let rx = check_and_import_approval( + &mut virtual_overseer, + block_hash, + candidate_index, + validator_index_b, + candidate_hash, + session_index, + false, + true, + Some(sig_b), + ) + .await; + + assert_eq!(rx.await, Ok(ApprovalCheckResult::Accepted),); + + // Sleep to ensure we get a consistent read on the database. + futures_timer::Delay::new(Duration::from_millis(100)).await; + + // The candidate should not be approved, even though at this + // point in time we have 2 assignments and 2 approvals. + // + // This is because the assignments were imported at tick `APPROVAL_DELAY` + // and won't be considered until `APPROVAL_DELAY` more ticks have passed. + let candidate_entry = store.load_candidate_entry(&candidate_hash).unwrap().unwrap(); + assert!(!candidate_entry.approval_entry(&block_hash).unwrap().is_approved()); + assert!(clock.inner.lock().current_wakeup_is(APPROVAL_DELAY + APPROVAL_DELAY)); + + // Trigger the wakeup. + clock.inner.lock().set_tick(APPROVAL_DELAY + APPROVAL_DELAY); + + // Sleep to ensure we get a consistent read on the database. + futures_timer::Delay::new(Duration::from_millis(100)).await; + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::ChainSelection(ChainSelectionMessage::Approved(b_hash)) => { + assert_eq!(b_hash, block_hash); + } + ); + + // The candidate and block should now be approved. + let candidate_entry = store.load_candidate_entry(&candidate_hash).unwrap().unwrap(); + assert!(candidate_entry.approval_entry(&block_hash).unwrap().is_approved()); + assert!(clock.inner.lock().next_wakeup().is_none()); + + let block_entry = store.load_block_entry(&block_hash).unwrap().unwrap(); + assert!(block_entry.is_fully_approved()); + + virtual_overseer + }); +}