diff --git a/polkadot/node/core/dispute-coordinator/src/import.rs b/polkadot/node/core/dispute-coordinator/src/import.rs index 278561d5d00c..d3a4625f0d24 100644 --- a/polkadot/node/core/dispute-coordinator/src/import.rs +++ b/polkadot/node/core/dispute-coordinator/src/import.rs @@ -52,7 +52,8 @@ pub struct CandidateEnvironment<'a> { executor_params: &'a ExecutorParams, /// Validator indices controlled by this node. controlled_indices: HashSet, - /// Indices of disabled validators at the `relay_parent`. + /// Indices of on-chain disabled validators at the `relay_parent` combined + /// with the off-chain state. disabled_indices: HashSet, } @@ -67,16 +68,15 @@ impl<'a> CandidateEnvironment<'a> { runtime_info: &'a mut RuntimeInfo, session_index: SessionIndex, relay_parent: Hash, + disabled_offchain: impl IntoIterator, ) -> Option> { - let disabled_indices = runtime_info + let disabled_onchain = runtime_info .get_disabled_validators(ctx.sender(), relay_parent) .await .unwrap_or_else(|err| { gum::info!(target: LOG_TARGET, ?err, "Failed to get disabled validators"); Vec::new() - }) - .into_iter() - .collect(); + }); let (session, executor_params) = match runtime_info .get_session_info_by_index(ctx.sender(), relay_parent, session_index) @@ -87,6 +87,24 @@ impl<'a> CandidateEnvironment<'a> { Err(_) => return None, }; + let n_validators = session.validators.len(); + let byzantine_threshold = polkadot_primitives::byzantine_threshold(n_validators); + // combine on-chain with off-chain disabled validators + // process disabled validators in the following order: + // - on-chain disabled validators + // - prioritized order of off-chain disabled validators + // deduplicate the list and take at most `byzantine_threshold` validators + let disabled_indices = { + let mut d: HashSet = HashSet::new(); + for v in disabled_onchain.into_iter().chain(disabled_offchain.into_iter()) { + if d.len() == byzantine_threshold { + break + } + d.insert(v); + } + d + }; + let controlled_indices = find_controlled_validator_indices(keystore, &session.validators); Some(Self { session_index, session, executor_params, controlled_indices, disabled_indices }) } @@ -116,7 +134,7 @@ impl<'a> CandidateEnvironment<'a> { &self.controlled_indices } - /// Indices of disabled validators at the `relay_parent`. + /// Indices of off-chain and on-chain disabled validators. pub fn disabled_indices(&'a self) -> &'a HashSet { &self.disabled_indices } @@ -237,13 +255,19 @@ impl CandidateVoteState { let supermajority_threshold = polkadot_primitives::supermajority_threshold(n_validators); - // We have a dispute, if we have votes on both sides: - let is_disputed = !votes.invalid.is_empty() && !votes.valid.raw().is_empty(); + // We have a dispute, if we have votes on both sides, with at least one invalid vote + // from non-disabled validator or with votes on both sides and confirmed. + let has_non_disabled_invalid_votes = + votes.invalid.keys().any(|i| !env.disabled_indices().contains(i)); + let byzantine_threshold = polkadot_primitives::byzantine_threshold(n_validators); + let votes_on_both_sides = !votes.valid.raw().is_empty() && !votes.invalid.is_empty(); + let is_confirmed = + votes_on_both_sides && (votes.voted_indices().len() > byzantine_threshold); + let is_disputed = + is_confirmed || (has_non_disabled_invalid_votes && !votes.valid.raw().is_empty()); let (dispute_status, byzantine_threshold_against) = if is_disputed { let mut status = DisputeStatus::active(); - let byzantine_threshold = polkadot_primitives::byzantine_threshold(n_validators); - let is_confirmed = votes.voted_indices().len() > byzantine_threshold; if is_confirmed { status = status.confirm(); }; diff --git a/polkadot/node/core/dispute-coordinator/src/initialized.rs b/polkadot/node/core/dispute-coordinator/src/initialized.rs index a1bcc1f01707..54e0410268f1 100644 --- a/polkadot/node/core/dispute-coordinator/src/initialized.rs +++ b/polkadot/node/core/dispute-coordinator/src/initialized.rs @@ -17,7 +17,7 @@ //! Dispute coordinator subsystem in initialized state (after first active leaf is received). use std::{ - collections::{BTreeMap, HashSet, VecDeque}, + collections::{BTreeMap, VecDeque}, sync::Arc, }; @@ -970,6 +970,7 @@ impl Initialized { &mut self.runtime_info, session, relay_parent, + self.offchain_disabled_validators.iter(session), ) .await { @@ -1099,36 +1100,14 @@ impl Initialized { let new_state = import_result.new_state(); - let byzantine_threshold = polkadot_primitives::byzantine_threshold(n_validators); - // combine on-chain with off-chain disabled validators - // process disabled validators in the following order: - // - on-chain disabled validators - // - prioritized order of off-chain disabled validators - // deduplicate the list and take at most `byzantine_threshold` validators - let disabled_validators = { - let mut d: HashSet = HashSet::new(); - for v in env - .disabled_indices() - .iter() - .cloned() - .chain(self.offchain_disabled_validators.iter(session)) - { - if d.len() == byzantine_threshold { - break - } - d.insert(v); - } - d - }; - let is_included = self.scraper.is_candidate_included(&candidate_hash); let is_backed = self.scraper.is_candidate_backed(&candidate_hash); let own_vote_missing = new_state.own_vote_missing(); let is_disputed = new_state.is_disputed(); let is_confirmed = new_state.is_confirmed(); - let potential_spam = is_potential_spam(&self.scraper, &new_state, &candidate_hash, |v| { - disabled_validators.contains(v) - }); + let is_disabled = |v: &ValidatorIndex| env.disabled_indices().contains(v); + let potential_spam = + is_potential_spam(&self.scraper, &new_state, &candidate_hash, is_disabled); let allow_participation = !potential_spam; gum::trace!( @@ -1139,7 +1118,7 @@ impl Initialized { ?candidate_hash, confirmed = ?new_state.is_confirmed(), has_invalid_voters = ?!import_result.new_invalid_voters().is_empty(), - n_disabled_validators = ?disabled_validators.len(), + n_disabled_validators = ?env.disabled_indices().len(), "Is spam?" ); @@ -1439,6 +1418,7 @@ impl Initialized { &mut self.runtime_info, session, candidate_receipt.descriptor.relay_parent, + self.offchain_disabled_validators.iter(session), ) .await { diff --git a/polkadot/node/core/dispute-coordinator/src/lib.rs b/polkadot/node/core/dispute-coordinator/src/lib.rs index c3038fc0953c..4b511e7430af 100644 --- a/polkadot/node/core/dispute-coordinator/src/lib.rs +++ b/polkadot/node/core/dispute-coordinator/src/lib.rs @@ -341,6 +341,8 @@ impl DisputeCoordinatorSubsystem { runtime_info, highest_session, leaf_hash, + // on startup we don't have any off-chain disabled state + std::iter::empty(), ) .await { @@ -370,10 +372,9 @@ impl DisputeCoordinatorSubsystem { }, }; let vote_state = CandidateVoteState::new(votes, &env, now); - let onchain_disabled = env.disabled_indices(); - let potential_spam = is_potential_spam(&scraper, &vote_state, candidate_hash, |v| { - onchain_disabled.contains(v) - }); + let is_disabled = |v: &ValidatorIndex| env.disabled_indices().contains(v); + let potential_spam = + is_potential_spam(&scraper, &vote_state, candidate_hash, is_disabled); let is_included = scraper.is_candidate_included(&vote_state.votes().candidate_receipt.hash()); diff --git a/polkadot/node/core/dispute-coordinator/src/tests.rs b/polkadot/node/core/dispute-coordinator/src/tests.rs index af384256c4f7..0360e357bee4 100644 --- a/polkadot/node/core/dispute-coordinator/src/tests.rs +++ b/polkadot/node/core/dispute-coordinator/src/tests.rs @@ -2645,14 +2645,23 @@ fn participation_with_onchain_disabling_unconfirmed() { .await; handle_disabled_validators_queries(&mut virtual_overseer, vec![disabled_index]).await; - handle_approval_vote_request(&mut virtual_overseer, &candidate_hash, HashMap::new()) - .await; - assert_eq!(confirmation_rx.await, Ok(ImportStatementsResult::ValidImport)); // we should not participate due to disabled indices on chain assert!(virtual_overseer.recv().timeout(TEST_TIMEOUT).await.is_none()); + { + // make sure the dispute is not active + let (tx, rx) = oneshot::channel(); + virtual_overseer + .send(FromOrchestra::Communication { + msg: DisputeCoordinatorMessage::ActiveDisputes(tx), + }) + .await; + + assert_eq!(rx.await.unwrap().len(), 0); + } + // Scenario 2: unconfirmed dispute with non-disabled validator against. // Expectation: even if the dispute is unconfirmed, we should participate // once we receive an invalid vote from a non-disabled validator. @@ -2679,6 +2688,9 @@ fn participation_with_onchain_disabling_unconfirmed() { }) .await; + handle_approval_vote_request(&mut virtual_overseer, &candidate_hash, HashMap::new()) + .await; + assert_eq!(confirmation_rx.await, Ok(ImportStatementsResult::ValidImport)); participation_with_distribution( @@ -2710,7 +2722,7 @@ fn participation_with_onchain_disabling_unconfirmed() { .await; let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone(); - assert_eq!(votes.valid.raw().len(), 2); // 3+1 => we have participated + assert_eq!(votes.valid.raw().len(), 2); // 1+1 => we have participated assert_eq!(votes.invalid.len(), 2); } @@ -2832,6 +2844,7 @@ fn participation_with_onchain_disabling_confirmed() { #[test] fn participation_with_offchain_disabling() { + sp_tracing::init_for_tests(); test_harness(|mut test_state, mut virtual_overseer| { Box::pin(async move { let session = 1; @@ -2968,17 +2981,23 @@ fn participation_with_offchain_disabling() { // let's disable validators 3, 4 on chain, but this should not affect this import let disabled_validators = vec![ValidatorIndex(3), ValidatorIndex(4)]; handle_disabled_validators_queries(&mut virtual_overseer, disabled_validators).await; - handle_approval_vote_request( - &mut virtual_overseer, - &another_candidate_hash, - HashMap::new(), - ) - .await; assert_eq!(confirmation_rx.await, Ok(ImportStatementsResult::ValidImport)); // we should not participate since due to offchain disabling assert!(virtual_overseer.recv().timeout(TEST_TIMEOUT).await.is_none()); + { + // make sure the new dispute is not active + let (tx, rx) = oneshot::channel(); + virtual_overseer + .send(FromOrchestra::Communication { + msg: DisputeCoordinatorMessage::ActiveDisputes(tx), + }) + .await; + + assert_eq!(rx.await.unwrap().len(), 1); + } + // now import enough votes for dispute confirmation // even though all of these votes are from (on chain) disabled validators let mut statements = Vec::new(); @@ -3007,6 +3026,12 @@ fn participation_with_offchain_disabling() { }) .await; + handle_approval_vote_request( + &mut virtual_overseer, + &another_candidate_hash, + HashMap::new(), + ) + .await; assert_eq!(confirmation_rx.await, Ok(ImportStatementsResult::ValidImport)); participation_with_distribution( diff --git a/prdoc/pr_3385.prdoc b/prdoc/pr_3385.prdoc new file mode 100644 index 000000000000..b6a03b3872f4 --- /dev/null +++ b/prdoc/pr_3385.prdoc @@ -0,0 +1,11 @@ +title: Do not stall finality on spam disputes + +doc: + - audience: Node Operator + description: | + This PR fixes the issue that periodically caused + finality stalls on Kusama due to disputes happening + there in combination with disputes spam protection mechanism. + See: https://github.com/paritytech/polkadot-sdk/issues/3345 + +crates: [ ]