diff --git a/polkadot/node/core/approval-voting/src/lib.rs b/polkadot/node/core/approval-voting/src/lib.rs index 1a62c9ee55e6e..76b3d476e28f5 100644 --- a/polkadot/node/core/approval-voting/src/lib.rs +++ b/polkadot/node/core/approval-voting/src/lib.rs @@ -1285,10 +1285,10 @@ fn cores_to_candidate_indices( // Map from core index to candidate index. for claimed_core_index in core_indices.iter_ones() { - // Candidates are sorted by core index. - if let Ok(candidate_index) = block_entry + if let Some(candidate_index) = block_entry .candidates() - .binary_search_by_key(&(claimed_core_index as u32), |(core_index, _)| core_index.0) + .iter() + .position(|(core_index, _)| core_index.0 == claimed_core_index as u32) { candidate_indices.push(candidate_index as _); } diff --git a/polkadot/node/core/approval-voting/src/persisted_entries.rs b/polkadot/node/core/approval-voting/src/persisted_entries.rs index b924a1b52ccf4..6eeb99cb99ffa 100644 --- a/polkadot/node/core/approval-voting/src/persisted_entries.rs +++ b/polkadot/node/core/approval-voting/src/persisted_entries.rs @@ -454,7 +454,7 @@ pub struct BlockEntry { slot: Slot, relay_vrf_story: RelayVRFStory, // The candidates included as-of this block and the index of the core they are - // leaving. Sorted ascending by core index. + // leaving. candidates: Vec<(CoreIndex, CandidateHash)>, // A bitfield where the i'th bit corresponds to the i'th candidate in `candidates`. // The i'th bit is `true` iff the candidate has been approved in the context of this diff --git a/polkadot/node/core/approval-voting/src/tests.rs b/polkadot/node/core/approval-voting/src/tests.rs index a3013eab46dd6..1483af5658537 100644 --- a/polkadot/node/core/approval-voting/src/tests.rs +++ b/polkadot/node/core/approval-voting/src/tests.rs @@ -2479,6 +2479,173 @@ fn subsystem_import_checked_approval_sets_one_block_bit_at_a_time() { }); } +// See https://github.com/paritytech/polkadot-sdk/issues/3826 +#[test] +fn inclusion_events_can_be_unordered_by_core_index() { + let assignment_criteria = Box::new(MockAssignmentCriteria( + || { + let mut assignments = HashMap::new(); + for core in 0..3 { + let _ = assignments.insert( + CoreIndex(core), + approval_db::v2::OurAssignment { + cert: garbage_assignment_cert_v2( + AssignmentCertKindV2::RelayVRFModuloCompact { + core_bitfield: vec![CoreIndex(0), CoreIndex(1), CoreIndex(2)] + .try_into() + .unwrap(), + }, + ), + tranche: 0, + validator_index: ValidatorIndex(0), + triggered: false, + } + .into(), + ); + } + assignments + }, + |_| 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; + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::ChainApi(ChainApiMessage::FinalizedBlockNumber(rx)) => { + rx.send(Ok(0)).unwrap(); + } + ); + + let block_hash = Hash::repeat_byte(0x01); + + let candidate_receipt0 = { + let mut receipt = dummy_candidate_receipt(block_hash); + receipt.descriptor.para_id = ParaId::from(0_u32); + receipt + }; + let candidate_receipt1 = { + let mut receipt = dummy_candidate_receipt(block_hash); + receipt.descriptor.para_id = ParaId::from(1_u32); + receipt + }; + let candidate_receipt2 = { + let mut receipt = dummy_candidate_receipt(block_hash); + receipt.descriptor.para_id = ParaId::from(2_u32); + receipt + }; + let candidate_index0 = 0; + let candidate_index1 = 1; + let candidate_index2 = 2; + + let validator0 = ValidatorIndex(0); + let validator1 = ValidatorIndex(1); + let validator2 = ValidatorIndex(2); + let validator3 = ValidatorIndex(3); + + let validators = vec![ + Sr25519Keyring::Alice, + Sr25519Keyring::Bob, + Sr25519Keyring::Charlie, + Sr25519Keyring::Dave, + Sr25519Keyring::Eve, + ]; + let session_info = SessionInfo { + validator_groups: IndexedVec::>::from(vec![ + vec![validator0, validator1], + vec![validator2], + vec![validator3], + ]), + needed_approvals: 1, + zeroth_delay_tranche_width: 1, + relay_vrf_modulo_samples: 1, + n_delay_tranches: 1, + no_show_slots: 1, + ..session_info(&validators) + }; + + ChainBuilder::new() + .add_block( + block_hash, + ChainBuilder::GENESIS_HASH, + 1, + BlockConfig { + slot: Slot::from(0), + candidates: Some(vec![ + (candidate_receipt0.clone(), CoreIndex(2), GroupIndex(2)), + (candidate_receipt1.clone(), CoreIndex(1), GroupIndex(0)), + (candidate_receipt2.clone(), CoreIndex(0), GroupIndex(1)), + ]), + session_info: Some(session_info), + end_syncing: true, + }, + ) + .build(&mut virtual_overseer) + .await; + + assert_eq!(clock.inner.lock().next_wakeup().unwrap(), 2); + clock.inner.lock().wakeup_all(100); + + assert_eq!(clock.inner.lock().wakeups.len(), 0); + + futures_timer::Delay::new(Duration::from_millis(100)).await; + + // Assignment is distributed only once from `approval-voting` + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::ApprovalDistribution(ApprovalDistributionMessage::DistributeAssignment( + _, + c_indices, + )) => { + assert_eq!(c_indices, vec![candidate_index0, candidate_index1, candidate_index2].try_into().unwrap()); + } + ); + + // Candidate 0 + recover_available_data(&mut virtual_overseer).await; + fetch_validation_code(&mut virtual_overseer).await; + + // Candidate 1 + recover_available_data(&mut virtual_overseer).await; + fetch_validation_code(&mut virtual_overseer).await; + + // Candidate 2 + recover_available_data(&mut virtual_overseer).await; + fetch_validation_code(&mut virtual_overseer).await; + + // Check if assignment was triggered for candidate 0. + let candidate_entry = + store.load_candidate_entry(&candidate_receipt0.hash()).unwrap().unwrap(); + let our_assignment = + candidate_entry.approval_entry(&block_hash).unwrap().our_assignment().unwrap(); + assert!(our_assignment.triggered()); + + // Check if assignment was triggered for candidate 1. + let candidate_entry = + store.load_candidate_entry(&candidate_receipt1.hash()).unwrap().unwrap(); + let our_assignment = + candidate_entry.approval_entry(&block_hash).unwrap().our_assignment().unwrap(); + assert!(our_assignment.triggered()); + + // Check if assignment was triggered for candidate 2. + let candidate_entry = + store.load_candidate_entry(&candidate_receipt2.hash()).unwrap().unwrap(); + let our_assignment = + candidate_entry.approval_entry(&block_hash).unwrap().our_assignment().unwrap(); + assert!(our_assignment.triggered()); + + virtual_overseer + }); +} + fn approved_ancestor_test( skip_approval: impl Fn(BlockNumber) -> bool, approved_height: BlockNumber,