approval-voting: Make sure we always mark approved candidates approve…

…d in a different relay chain context (#4153)

... see for more detail why this is needed

#4149 (comment)

## TODO:
- [x] Unittests
- [x] Replicate scenario from
#4149 and confirm this
fixes it: #4149 [
Replicated on a zombienet with some hacked nodes, that we can end up in
this state where no-wake is schedule and the nodes are pending new
assignments]

---------

Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
Co-authored-by: Andrei Sandu <54316454+sandreim@users.noreply.github.com>

polkadot/node/core/approval-voting/src/lib.rs

@@ -978,6 +978,7 @@ where
 					woken_block,
 					woken_candidate,
 					&subsystem.metrics,
+					&wakeups,
 				).await?
 			}
 			next_msg = ctx.recv().fuse() => {
@@ -1152,6 +1153,7 @@ async fn handle_actions<Context>(
 					candidate_hash,
 					delayed_approvals_timers,
 					approval_request,
+					&wakeups,
 				)
 				.await?
 				.into_iter()
@@ -1663,6 +1665,7 @@ async fn handle_from_overseer<Context>(
 					|r| {
 						let _ = res.send(r);
 					},
+					&wakeups,
 				)
 				.await?
 				.0,
@@ -2477,6 +2480,7 @@ async fn check_and_import_approval<T, Sender>(
 	metrics: &Metrics,
 	approval: IndirectSignedApprovalVoteV2,
 	with_response: impl FnOnce(ApprovalCheckResult) -> T,
+	wakeups: &Wakeups,
 ) -> SubsystemResult<(Vec<Action>, T)>
 where
 	Sender: SubsystemSender<RuntimeApiMessage>,
@@ -2655,6 +2659,7 @@ where
 			approved_candidate_hash,
 			candidate_entry,
 			ApprovalStateTransition::RemoteApproval(approval.validator),
+			wakeups,
 		)
 		.await;
 		actions.extend(new_actions);
@@ -2689,6 +2694,10 @@ impl ApprovalStateTransition {
 			ApprovalStateTransition::WakeupProcessed => false,
 		}
 	}
+
+	fn is_remote_approval(&self) -> bool {
+		matches!(*self, ApprovalStateTransition::RemoteApproval(_))
+	}
 }
 
 // Advance the approval state, either by importing an approval vote which is already checked to be
@@ -2705,6 +2714,7 @@ async fn advance_approval_state<Sender>(
 	candidate_hash: CandidateHash,
 	mut candidate_entry: CandidateEntry,
 	transition: ApprovalStateTransition,
+	wakeups: &Wakeups,
 ) -> Vec<Action>
 where
 	Sender: SubsystemSender<RuntimeApiMessage>,
@@ -2835,6 +2845,43 @@ where
 			status.required_tranches,
 		));
 
+		if is_approved && transition.is_remote_approval() {
+			// Make sure we wake other blocks in case they have
+			// a no-show that might be covered by this approval.
+			for (fork_block_hash, fork_approval_entry) in candidate_entry
+				.block_assignments
+				.iter()
+				.filter(|(hash, _)| **hash != block_hash)
+			{
+				let assigned_on_fork_block = validator_index
+					.as_ref()
+					.map(|validator_index| fork_approval_entry.is_assigned(*validator_index))
+					.unwrap_or_default();
+				if wakeups.wakeup_for(*fork_block_hash, candidate_hash).is_none() &&
+					!fork_approval_entry.is_approved() &&
+					assigned_on_fork_block
+				{
+					let fork_block_entry = db.load_block_entry(fork_block_hash);
+					if let Ok(Some(fork_block_entry)) = fork_block_entry {
+						actions.push(Action::ScheduleWakeup {
+							block_hash: *fork_block_hash,
+							block_number: fork_block_entry.block_number(),
+							candidate_hash,
+							// Schedule the wakeup next tick, since the assignment must be a
+							// no-show, because there is no-wakeup scheduled.
+							tick: tick_now + 1,
+						})
+					} else {
+						gum::debug!(
+							target: LOG_TARGET,
+							?fork_block_entry,
+							?fork_block_hash,
+							"Failed to load block entry"
+						)
+					}
+				}
+			}
+		}
 		// We have no need to write the candidate entry if all of the following
 		// is true:
 		//
@@ -2896,6 +2943,7 @@ async fn process_wakeup<Context>(
 	relay_block: Hash,
 	candidate_hash: CandidateHash,
 	metrics: &Metrics,
+	wakeups: &Wakeups,
 ) -> SubsystemResult<Vec<Action>> {
 	let mut span = state
 		.spans
@@ -3064,6 +3112,7 @@ async fn process_wakeup<Context>(
 			candidate_hash,
 			candidate_entry,
 			ApprovalStateTransition::WakeupProcessed,
+			wakeups,
 		)
 		.await,
 	);
@@ -3294,6 +3343,7 @@ async fn issue_approval<Context>(
 	candidate_hash: CandidateHash,
 	delayed_approvals_timers: &mut DelayedApprovalTimer,
 	ApprovalVoteRequest { validator_index, block_hash }: ApprovalVoteRequest,
+	wakeups: &Wakeups,
 ) -> SubsystemResult<Vec<Action>> {
 	let mut issue_approval_span = state
 		.spans
@@ -3415,6 +3465,7 @@ async fn issue_approval<Context>(
 		candidate_hash,
 		candidate_entry,
 		ApprovalStateTransition::LocalApproval(validator_index as _),
+		wakeups,
 	)
 	.await;
 

polkadot/node/core/approval-voting/src/tests.rs

@@ -834,7 +834,6 @@ impl ChainBuilder {
 					cur_hash = cur_header.parent_hash;
 				}
 				ancestry.reverse();
-
 				import_block(
 					overseer,
 					ancestry.as_ref(),
@@ -1922,6 +1921,187 @@ fn subsystem_assignment_import_updates_candidate_entry_and_schedules_wakeup() {
 	});
 }
 
+#[test]
+fn subsystem_always_has_a_wakeup_when_pending() {
+	// Approvals sent after all assignments are no-show, the approval
+	// should be counted on the fork relay chain on the next tick.
+	test_approvals_on_fork_are_always_considered_after_no_show(
+		30,
+		vec![(29, false), (30, false), (31, true)],
+	);
+	// Approvals sent before fork no-shows, the approval
+	// should be counted on the fork relay chain when it no-shows.
+	test_approvals_on_fork_are_always_considered_after_no_show(
+		8, // a tick smaller than the no-show tick which is 30.
+		vec![(7, false), (8, false), (29, false), (30, true), (31, true)],
+	);
+}
+
+fn test_approvals_on_fork_are_always_considered_after_no_show(
+	tick_to_send_approval: Tick,
+	expected_approval_status: Vec<(Tick, bool)>,
+) {
+	let config = HarnessConfig::default();
+	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;
+
+		assert_matches!(
+			overseer_recv(&mut virtual_overseer).await,
+			AllMessages::ChainApi(ChainApiMessage::FinalizedBlockNumber(rx)) => {
+				rx.send(Ok(0)).unwrap();
+			}
+		);
+		let candidate_hash = Hash::repeat_byte(0x04);
+
+		let candidate_descriptor = make_candidate(ParaId::from(1_u32), &candidate_hash);
+		let candidate_hash = candidate_descriptor.hash();
+
+		let block_hash = Hash::repeat_byte(0x01);
+		let block_hash_fork = Hash::repeat_byte(0x02);
+
+		let candidate_index = 0;
+		let validator = ValidatorIndex(0);
+		let validators = vec![
+			Sr25519Keyring::Alice,
+			Sr25519Keyring::Bob,
+			Sr25519Keyring::Charlie,
+			Sr25519Keyring::Dave,
+			Sr25519Keyring::Eve,
+		];
+		// Add block hash 0x01 and for 0x02
+		ChainBuilder::new()
+			.add_block(
+				block_hash,
+				ChainBuilder::GENESIS_HASH,
+				1,
+				BlockConfig {
+					slot: Slot::from(1),
+					candidates: Some(vec![(
+						candidate_descriptor.clone(),
+						CoreIndex(0),
+						GroupIndex(0),
+					)]),
+					session_info: Some(SessionInfo {
+						validator_groups: IndexedVec::<GroupIndex, Vec<ValidatorIndex>>::from(
+							vec![
+								vec![ValidatorIndex(0), ValidatorIndex(1)],
+								vec![ValidatorIndex(2)],
+								vec![ValidatorIndex(3), ValidatorIndex(4)],
+							],
+						),
+						needed_approvals: 1,
+						..session_info(&validators)
+					}),
+					end_syncing: false,
+				},
+			)
+			.add_block(
+				block_hash_fork,
+				ChainBuilder::GENESIS_HASH,
+				1,
+				BlockConfig {
+					slot: Slot::from(1),
+					candidates: Some(vec![(candidate_descriptor, CoreIndex(0), GroupIndex(0))]),
+					session_info: Some(SessionInfo {
+						validator_groups: IndexedVec::<GroupIndex, Vec<ValidatorIndex>>::from(
+							vec![
+								vec![ValidatorIndex(0), ValidatorIndex(1)],
+								vec![ValidatorIndex(2)],
+								vec![ValidatorIndex(3), ValidatorIndex(4)],
+							],
+						),
+						needed_approvals: 1,
+						..session_info(&validators)
+					}),
+					end_syncing: false,
+				},
+			)
+			.build(&mut virtual_overseer)
+			.await;
+
+		// Send assignments for the same candidate on both forks
+		let rx = check_and_import_assignment(
+			&mut virtual_overseer,
+			block_hash,
+			candidate_index,
+			validator,
+		)
+		.await;
+		assert_eq!(rx.await, Ok(AssignmentCheckResult::Accepted));
+
+		let rx = check_and_import_assignment(
+			&mut virtual_overseer,
+			block_hash_fork,
+			candidate_index,
+			validator,
+		)
+		.await;
+
+		assert_eq!(rx.await, Ok(AssignmentCheckResult::Accepted));
+		// Wake on APPROVAL_DELAY first
+		assert!(clock.inner.lock().current_wakeup_is(2));
+		clock.inner.lock().set_tick(2);
+		futures_timer::Delay::new(Duration::from_millis(100)).await;
+
+		// Wake up on no-show
+		assert!(clock.inner.lock().current_wakeup_is(30));
+
+		for (tick, status) in expected_approval_status
+			.iter()
+			.filter(|(tick, _)| *tick < tick_to_send_approval)
+		{
+			// Wake up on no-show
+			clock.inner.lock().set_tick(*tick);
+			futures_timer::Delay::new(Duration::from_millis(100)).await;
+			let block_entry = store.load_block_entry(&block_hash).unwrap().unwrap();
+			let block_entry_fork = store.load_block_entry(&block_hash_fork).unwrap().unwrap();
+			assert!(!block_entry.is_fully_approved());
+			assert_eq!(block_entry_fork.is_fully_approved(), *status);
+		}
+
+		clock.inner.lock().set_tick(tick_to_send_approval);
+		futures_timer::Delay::new(Duration::from_millis(100)).await;
+
+		// Send the approval for candidate just in the context of 0x01 block.
+		let rx = check_and_import_approval(
+			&mut virtual_overseer,
+			block_hash,
+			candidate_index,
+			validator,
+			candidate_hash,
+			1,
+			false,
+			None,
+		)
+		.await;
+
+		assert_eq!(rx.await, Ok(ApprovalCheckResult::Accepted),);
+
+		// Check approval status for the fork_block is correctly transitioned.
+		for (tick, status) in expected_approval_status
+			.iter()
+			.filter(|(tick, _)| *tick >= tick_to_send_approval)
+		{
+			// Wake up on no-show
+			clock.inner.lock().set_tick(*tick);
+			futures_timer::Delay::new(Duration::from_millis(100)).await;
+			let block_entry = store.load_block_entry(&block_hash).unwrap().unwrap();
+			let block_entry_fork = store.load_block_entry(&block_hash_fork).unwrap().unwrap();
+			assert!(block_entry.is_fully_approved());
+			assert_eq!(block_entry_fork.is_fully_approved(), *status);
+		}
+
+		virtual_overseer
+	});
+}
+
 #[test]
 fn subsystem_process_wakeup_schedules_wakeup() {
 	test_harness(HarnessConfig::default(), |test_harness| async move {