always broadcast tranche 0 assignments and add a delay before approval (#3904)

* always broadcast tranche 0 assignments

* guide: require fixed approval delay

* prevent approval by very recent assignments

* fix approval-checking tests

* fix main approval tests

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<Tick>,
+		/// The last tick at which a needed assignment was received.
+		last_assignment_tick: Option<Tick>,
 	},
 }
 
@@ -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<Tick>),
 	/// 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<Tick>,
+	/// The last tick at which a considered assignment was received.
+	last_assignment_tick: Option<Tick>,
 }
 
 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<Tick>,
+		last_assignment_tick: Option<Tick>,
 	) -> 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 {
@@ -459,7 +484,7 @@ mod tests {
 				clock_drift: 0,
 			},
 		)
-		.is_approved());
+		.is_approved(Tick::max_value()));
 	}
 
 	#[test]
@@ -502,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]
@@ -556,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,
@@ -566,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]
@@ -617,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)
+			},
 		);
 	}
 
@@ -820,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),
 			},
 		);
 
@@ -838,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),
 			},
 		);
 
@@ -905,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.
@@ -943,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),
+			},
 		);
 	}
 
@@ -1256,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,
+			},
+		);
+	}
 
-	assert_eq!(
-		state.output(0, 10, 10, 20),
-		RequiredTranches::Exact { needed: 0, tolerated_missing: 0, next_no_show: 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,
+			last_assignment_tick: None,
+		};
+
+		assert_eq!(
+			state.output(0, 10, 10, 20),
+			RequiredTranches::Exact {
+				needed: 0,
+				tolerated_missing: 0,
+				next_no_show: None,
+				last_assignment_tick: None
+			},
+		);
+	}
 }

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<Action> {
 	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,
 		));
 
@@ -1893,6 +1909,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(
@@ -1900,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);

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));