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

polkadot/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
+	});
+}