Fixes/improvements for disputes (#3753)

* More debugging output.

* Fix chain selection in case of disputes.

* Fix flaky test.

polkadot/node/core/dispute-coordinator/src/dummy.rs

@@ -233,13 +233,14 @@ async fn handle_incoming(
 		},
 		DisputeCoordinatorMessage::IssueLocalStatement(_, _, _, _) => {},
 		DisputeCoordinatorMessage::DetermineUndisputedChain {
-			base_number,
+			base: (base_number, base_hash),
 			block_descriptions,
 			tx,
 		} => {
 			let undisputed_chain = block_descriptions
 				.last()
-				.map(|e| (base_number + block_descriptions.len() as BlockNumber, e.block_hash));
+				.map(|e| (base_number + block_descriptions.len() as BlockNumber, e.block_hash))
+				.unwrap_or((base_number, base_hash));
 
 			let _ = tx.send(undisputed_chain);
 		},

polkadot/node/core/dispute-coordinator/src/real/mod.rs

@@ -582,12 +582,12 @@ async fn handle_incoming(
 			.await?;
 		},
 		DisputeCoordinatorMessage::DetermineUndisputedChain {
-			base_number,
+			base: (base_number, base_hash),
 			block_descriptions,
 			tx,
 		} => {
 			let undisputed_chain =
-				determine_undisputed_chain(overlay_db, base_number, block_descriptions)?;
+				determine_undisputed_chain(overlay_db, base_number, base_hash, block_descriptions)?;
 
 			let _ = tx.send(undisputed_chain);
 		},
@@ -882,7 +882,7 @@ async fn issue_local_statement(
 
 	// Do import
 	if !statements.is_empty() {
-		let (pending_confirmation, _rx) = oneshot::channel();
+		let (pending_confirmation, rx) = oneshot::channel();
 		handle_import_statements(
 			ctx,
 			overlay_db,
@@ -895,6 +895,32 @@ async fn issue_local_statement(
 			pending_confirmation,
 		)
 		.await?;
+		match rx.await {
+			Err(_) => {
+				tracing::error!(
+					target: LOG_TARGET,
+					?candidate_hash,
+					?session,
+					"pending confirmation receiver got dropped by `handle_import_statements` for our own votes!"
+				);
+			},
+			Ok(ImportStatementsResult::InvalidImport) => {
+				tracing::error!(
+					target: LOG_TARGET,
+					?candidate_hash,
+					?session,
+					"handle_import_statements` considers our own votes invalid!"
+				);
+			},
+			Ok(ImportStatementsResult::ValidImport) => {
+				tracing::trace!(
+					target: LOG_TARGET,
+					?candidate_hash,
+					?session,
+					"handle_import_statements` successfully imported our vote!"
+				);
+			},
+		}
 	}
 
 	Ok(())
@@ -970,11 +996,13 @@ fn make_dispute_message(
 fn determine_undisputed_chain(
 	overlay_db: &mut OverlayedBackend<'_, impl Backend>,
 	base_number: BlockNumber,
+	base_hash: Hash,
 	block_descriptions: Vec<BlockDescription>,
-) -> Result<Option<(BlockNumber, Hash)>, Error> {
+) -> Result<(BlockNumber, Hash), Error> {
 	let last = block_descriptions
 		.last()
-		.map(|e| (base_number + block_descriptions.len() as BlockNumber, e.block_hash));
+		.map(|e| (base_number + block_descriptions.len() as BlockNumber, e.block_hash))
+		.unwrap_or((base_number, base_hash));
 
 	// Fast path for no disputes.
 	let recent_disputes = match overlay_db.load_recent_disputes()? {
@@ -992,12 +1020,9 @@ fn determine_undisputed_chain(
 	for (i, BlockDescription { session, candidates, .. }) in block_descriptions.iter().enumerate() {
 		if candidates.iter().any(|c| is_possibly_invalid(*session, *c)) {
 			if i == 0 {
-				return Ok(None)
+				return Ok((base_number, base_hash))
 			} else {
-				return Ok(Some((
-					base_number + i as BlockNumber,
-					block_descriptions[i - 1].block_hash,
-				)))
+				return Ok((base_number + i as BlockNumber, block_descriptions[i - 1].block_hash))
 			}
 		}
 	}

polkadot/node/core/dispute-coordinator/src/real/tests.rs

@@ -645,13 +645,14 @@ fn finality_votes_ignore_disputed_candidates() {
 			{
 				let (tx, rx) = oneshot::channel();
 
+				let base_block = Hash::repeat_byte(0x0f);
 				let block_hash_a = Hash::repeat_byte(0x0a);
 				let block_hash_b = Hash::repeat_byte(0x0b);
 
 				virtual_overseer
 					.send(FromOverseer::Communication {
 						msg: DisputeCoordinatorMessage::DetermineUndisputedChain {
-							base_number: 10,
+							base: (10, base_block),
 							block_descriptions: vec![BlockDescription {
 								block_hash: block_hash_a,
 								session,
@@ -662,13 +663,13 @@ fn finality_votes_ignore_disputed_candidates() {
 					})
 					.await;
 
-				assert!(rx.await.unwrap().is_none());
+				assert_eq!(rx.await.unwrap(), (10, base_block));
 
 				let (tx, rx) = oneshot::channel();
 				virtual_overseer
 					.send(FromOverseer::Communication {
 						msg: DisputeCoordinatorMessage::DetermineUndisputedChain {
-							base_number: 10,
+							base: (10, base_block),
 							block_descriptions: vec![
 								BlockDescription {
 									block_hash: block_hash_a,
@@ -686,7 +687,7 @@ fn finality_votes_ignore_disputed_candidates() {
 					})
 					.await;
 
-				assert_eq!(rx.await.unwrap(), Some((11, block_hash_a)));
+				assert_eq!(rx.await.unwrap(), (11, block_hash_a));
 			}
 
 			virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await;
@@ -777,13 +778,14 @@ fn supermajority_valid_dispute_may_be_finalized() {
 			{
 				let (tx, rx) = oneshot::channel();
 
+				let base_hash = Hash::repeat_byte(0x0f);
 				let block_hash_a = Hash::repeat_byte(0x0a);
 				let block_hash_b = Hash::repeat_byte(0x0b);
 
 				virtual_overseer
 					.send(FromOverseer::Communication {
 						msg: DisputeCoordinatorMessage::DetermineUndisputedChain {
-							base_number: 10,
+							base: (10, base_hash),
 							block_descriptions: vec![BlockDescription {
 								block_hash: block_hash_a,
 								session,
@@ -794,13 +796,13 @@ fn supermajority_valid_dispute_may_be_finalized() {
 					})
 					.await;
 
-				assert_eq!(rx.await.unwrap(), Some((11, block_hash_a)));
+				assert_eq!(rx.await.unwrap(), (11, block_hash_a));
 
 				let (tx, rx) = oneshot::channel();
 				virtual_overseer
 					.send(FromOverseer::Communication {
 						msg: DisputeCoordinatorMessage::DetermineUndisputedChain {
-							base_number: 10,
+							base: (10, base_hash),
 							block_descriptions: vec![
 								BlockDescription {
 									block_hash: block_hash_a,
@@ -818,7 +820,7 @@ fn supermajority_valid_dispute_may_be_finalized() {
 					})
 					.await;
 
-				assert_eq!(rx.await.unwrap(), Some((12, block_hash_b)));
+				assert_eq!(rx.await.unwrap(), (12, block_hash_b));
 			}
 
 			virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await;

polkadot/node/network/availability-recovery/src/lib.rs

@@ -93,7 +93,7 @@ const COST_INVALID_REQUEST: Rep = Rep::CostMajor("Peer sent unparsable request")
 #[cfg(not(test))]
 const TIMEOUT_START_NEW_REQUESTS: Duration = CHUNK_REQUEST_TIMEOUT;
 #[cfg(test)]
-const TIMEOUT_START_NEW_REQUESTS: Duration = Duration::from_millis(4);
+const TIMEOUT_START_NEW_REQUESTS: Duration = Duration::from_millis(10);
 
 /// The Availability Recovery Subsystem.
 pub struct AvailabilityRecoverySubsystem {

polkadot/node/service/src/relay_chain_selection.rs

@@ -434,7 +434,7 @@ where
 			overseer
 				.send_msg(
 					DisputeCoordinatorMessage::DetermineUndisputedChain {
-						base_number: target_number,
+						base: (target_number, target_hash),
 						block_descriptions: subchain_block_descriptions,
 						tx,
 					},
@@ -444,8 +444,7 @@ where
 			let (subchain_number, subchain_head) = rx
 				.await
 				.map_err(Error::OverseerDisconnected)
-				.map_err(|e| ConsensusError::Other(Box::new(e)))?
-				.unwrap_or_else(|| (subchain_number, subchain_head));
+				.map_err(|e| ConsensusError::Other(Box::new(e)))?;
 
 			// The the total lag accounting for disputes.
 			let lag_disputes = initial_leaf_number.saturating_sub(subchain_number);

polkadot/node/service/src/tests.rs

@@ -383,18 +383,20 @@ async fn test_skeleton(
 	);
 
 	tracing::trace!("determine undisputed chain response: {:?}", undisputed_chain);
+
+	let target_block_number = chain.number(target_block_hash).unwrap().unwrap();
 	assert_matches!(
 		overseer_recv(
 			virtual_overseer
 		).await,
 		AllMessages::DisputeCoordinator(
 			DisputeCoordinatorMessage::DetermineUndisputedChain {
-				base_number: _,
+				base: _,
 				block_descriptions: _,
 				tx,
 			}
 		) => {
-			tx.send(undisputed_chain).unwrap();
+			tx.send(undisputed_chain.unwrap_or((target_block_number, target_block_hash))).unwrap();
 	});
 }
 

polkadot/node/subsystem-types/src/messages.rs

@@ -189,7 +189,7 @@ impl BoundToRelayParent for CollatorProtocolMessage {
 /// Messages received by the dispute coordinator subsystem.
 #[derive(Debug)]
 pub enum DisputeCoordinatorMessage {
-	/// Import a statement by a validator about a candidate.
+	/// Import statements by validators about a candidate.
 	///
 	/// The subsystem will silently discard ancient statements or sets of only dispute-specific statements for
 	/// candidates that are previously unknown to the subsystem. The former is simply because ancient
@@ -251,12 +251,12 @@ pub enum DisputeCoordinatorMessage {
 	/// is typically the number of the last finalized block but may be slightly higher. This block
 	/// is inevitably going to be finalized so it is not accounted for by this function.
 	DetermineUndisputedChain {
-		/// The number of the lowest possible block to vote on.
-		base_number: BlockNumber,
+		/// The lowest possible block to vote on.
+		base: (BlockNumber, Hash),
 		/// Descriptions of all the blocks counting upwards from the block after the base number
 		block_descriptions: Vec<BlockDescription>,
-		/// A response channel - `None` to vote on base, `Some` to vote higher.
-		tx: oneshot::Sender<Option<(BlockNumber, Hash)>>,
+		/// The block to vote on, might be base in case there is no better.
+		tx: oneshot::Sender<(BlockNumber, Hash)>,
 	},
 }