diff --git a/polkadot/node/core/approval-voting/src/lib.rs b/polkadot/node/core/approval-voting/src/lib.rs index 7ecc2b2595bce78c3c65569369a932c847552b1c..b5ed92fa39c873c0a1e5f40c52705a5803971b60 100644 --- a/polkadot/node/core/approval-voting/src/lib.rs +++ b/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; diff --git a/polkadot/node/core/approval-voting/src/tests.rs b/polkadot/node/core/approval-voting/src/tests.rs index f7bbbca4b8a1c0c5609898d714ad045dfd33f3ef..312d805bbefb7b2b2605093fa2cadde0a1f10511 100644 --- a/polkadot/node/core/approval-voting/src/tests.rs +++ b/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 {