Staking ledger bonding fixes (#3639)

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<ValidatorIndex>,
-	/// 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<ValidatorIndex>,
 }
 
@@ -67,16 +68,15 @@ impl<'a> CandidateEnvironment<'a> {
 		runtime_info: &'a mut RuntimeInfo,
 		session_index: SessionIndex,
 		relay_parent: Hash,
+		disabled_offchain: impl IntoIterator<Item = ValidatorIndex>,
 	) -> Option<CandidateEnvironment<'a>> {
-		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<ValidatorIndex> = 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<ValidatorIndex> {
 		&self.disabled_indices
 	}
@@ -237,13 +255,19 @@ impl CandidateVoteState<CandidateVotes> {
 
 		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();
 			};

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<ValidatorIndex> = 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
 		{

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());
 

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(

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: [ ]

prdoc/pr_3639.prdoc

@@ -0,0 +1,19 @@
+title: Prevents staking controllers from becoming stashes of different ledgers; Ensures that no ledger in bad state is mutated.
+
+doc:
+  - audience: Runtime User
+    description: |
+      This PR introduces a fix to the staking logic which prevents an existing controller from bonding as a stash of another ledger, which
+      lead to staking ledger inconsistencies down the line. In addition, it adds a few (temporary) gates to prevent ledgers that are already
+      in a bad state from mutating its state.
+
+      In summary:
+      * Checks if stash is already a controller when calling `Call::bond` and fails if that's the case;
+      * Ensures that all fetching ledgers from storage are done through the `StakingLedger` API;
+      * Ensures that a `Error::BadState` is returned if the ledger bonding is in a bad state. This prevents bad ledgers from mutating (e.g.
+        `bond_extra`, `set_controller`, etc) its state and avoid further data inconsistencies.
+      * Prevents stashes which are controllers or another ledger from calling `set_controller`, since that may lead to a bad state.
+      * Adds further try-state runtime checks that check if there are ledgers in a bad state based on their bonded metadata.
+
+crates:
+- name: pallet-staking