From 3fc5b826534438313d03f6e0404db44099be6d9d Mon Sep 17 00:00:00 2001
From: ordian <write@reusable.software>
Date: Tue, 26 Mar 2024 23:59:47 +0100
Subject: [PATCH] fix regression in approval-voting introduced in #3747 (#3831)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Fixes #3826.
The docs on the `candidates` field of `BlockEntry` were incorrectly
stating that they are sorted by core index. The (incorrect) optimization
was introduced in #3747 based on this assumption. The actual ordering is
based on `CandidateIncluded` events ordering in the runtime. We revert
this optimization here.
- [x] verify the underlying issue
- [x] add a regression test
---------
Co-authored-by: Bastian Köcher <git@kchr.de>
---
polkadot/node/core/approval-voting/src/lib.rs | 6 +-
.../approval-voting/src/persisted_entries.rs | 2 +-
.../node/core/approval-voting/src/tests.rs | 167 ++++++++++++++++++
3 files changed, 171 insertions(+), 4 deletions(-)
diff --git a/polkadot/node/core/approval-voting/src/lib.rs b/polkadot/node/core/approval-voting/src/lib.rs
index 1a62c9ee55e..76b3d476e28 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 b924a1b52cc..6eeb99cb99f 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 a3013eab46d..1483af56585 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::<GroupIndex, Vec<ValidatorIndex>>::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,
--
GitLab