From 9514caee8875b8476dcd6f70c51d8e3e607b2089 Mon Sep 17 00:00:00 2001
From: Robert Klotzner <eskimor@users.noreply.github.com>
Date: Mon, 10 Jan 2022 12:28:20 +0100
Subject: [PATCH] Small mostly doc improvements (#4661)

* Better docs on non initialized state.

* Document better what is happening.

* More precise errors.

* cargo fmt
---
 polkadot/node/core/chain-selection/src/lib.rs |  1 +
 .../core/dispute-coordinator/src/real/mod.rs  | 12 +++++-----
 .../node/service/src/relay_chain_selection.rs | 22 +++++++++++++------
 polkadot/node/subsystem-types/src/messages.rs |  3 +++
 4 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/polkadot/node/core/chain-selection/src/lib.rs b/polkadot/node/core/chain-selection/src/lib.rs
index 925d647c49..ee968496da 100644
--- a/polkadot/node/core/chain-selection/src/lib.rs
+++ b/polkadot/node/core/chain-selection/src/lib.rs
@@ -352,6 +352,7 @@ async fn run<Context, B>(
 		match res {
 			Err(e) => {
 				e.trace();
+				// All errors right now are considered fatal:
 				break
 			},
 			Ok(()) => {
diff --git a/polkadot/node/core/dispute-coordinator/src/real/mod.rs b/polkadot/node/core/dispute-coordinator/src/real/mod.rs
index 345735ed39..5446d91ddc 100644
--- a/polkadot/node/core/dispute-coordinator/src/real/mod.rs
+++ b/polkadot/node/core/dispute-coordinator/src/real/mod.rs
@@ -390,11 +390,13 @@ where
 			},
 			FromOverseer::Signal(OverseerSignal::BlockFinalized(_, _)) => {},
 			FromOverseer::Communication { msg } =>
-			// Note: It seems we really should not receive any messages before the first
-			// `ActiveLeavesUpdate`, if that proves wrong over time and we do receive
-			// messages before the first `ActiveLeavesUpdate` that should not be dropped,
-			// this can easily be fixed by collecting those messages and passing them on to
-			// `Initialized::new()`.
+			// NOTE: We could technically actually handle a couple of message types, even if
+			// not initialized (e.g. all requests that only query the database). The problem
+			// is, we would deliver potentially outdated information, especially in the event
+			// of bugs where initialization fails for a while (e.g. `SessionInfo`s are not
+			// available). So instead of telling subsystems, everything is fine, because of an
+			// hour old database state, we should rather cancel contained oneshots and delay
+			// finality until we are fully functional.
 				tracing::warn!(
 					target: LOG_TARGET,
 					?msg,
diff --git a/polkadot/node/service/src/relay_chain_selection.rs b/polkadot/node/service/src/relay_chain_selection.rs
index e0bdda348d..317cdb03f3 100644
--- a/polkadot/node/service/src/relay_chain_selection.rs
+++ b/polkadot/node/service/src/relay_chain_selection.rs
@@ -298,9 +298,17 @@ where
 
 #[derive(thiserror::Error, Debug)]
 enum Error {
-	// A request to the subsystem was canceled.
-	#[error("Overseer is disconnected from Chain Selection")]
-	OverseerDisconnected(oneshot::Canceled),
+	// Oneshot for requesting leaves from chain selection got canceled - check errors in that
+	// subsystem.
+	#[error("Request for leaves from chain selection got canceled")]
+	LeavesCanceled(oneshot::Canceled),
+	#[error("Request for leaves from chain selection got canceled")]
+	BestLeafContainingCanceled(oneshot::Canceled),
+	// Requesting recent disputes oneshot got canceled.
+	#[error("Request for determining the undisputed chain from DisputeCoordinator got canceled")]
+	DetermineUndisputedChainCanceled(oneshot::Canceled),
+	#[error("Request approved ancestor from approval voting got canceled")]
+	ApprovedAncestorCanceled(oneshot::Canceled),
 	/// Chain selection returned empty leaves.
 	#[error("ChainSelection returned no leaves")]
 	EmptyLeaves,
@@ -338,7 +346,7 @@ where
 
 		let leaves = rx
 			.await
-			.map_err(Error::OverseerDisconnected)
+			.map_err(Error::LeavesCanceled)
 			.map_err(|e| ConsensusError::Other(Box::new(e)))?;
 
 		tracing::trace!(target: LOG_TARGET, ?leaves, "Chain selection leaves");
@@ -392,7 +400,7 @@ where
 
 			let best = rx
 				.await
-				.map_err(Error::OverseerDisconnected)
+				.map_err(Error::BestLeafContainingCanceled)
 				.map_err(|e| ConsensusError::Other(Box::new(e)))?;
 
 			tracing::trace!(target: LOG_TARGET, ?best, "Best leaf containing");
@@ -467,7 +475,7 @@ where
 
 			match rx
 				.await
-				.map_err(Error::OverseerDisconnected)
+				.map_err(Error::ApprovedAncestorCanceled)
 				.map_err(|e| ConsensusError::Other(Box::new(e)))?
 			{
 				// No approved ancestors means target hash is maximal vote.
@@ -514,7 +522,7 @@ where
 				.await;
 			let (subchain_number, subchain_head) = rx
 				.await
-				.map_err(Error::OverseerDisconnected)
+				.map_err(Error::DetermineUndisputedChainCanceled)
 				.map_err(|e| ConsensusError::Other(Box::new(e)))?;
 
 			// The the total lag accounting for disputes.
diff --git a/polkadot/node/subsystem-types/src/messages.rs b/polkadot/node/subsystem-types/src/messages.rs
index 3cec45d03d..56c5f93d37 100644
--- a/polkadot/node/subsystem-types/src/messages.rs
+++ b/polkadot/node/subsystem-types/src/messages.rs
@@ -228,6 +228,9 @@ impl BoundToRelayParent for CollatorProtocolMessage {
 }
 
 /// Messages received by the dispute coordinator subsystem.
+///
+/// NOTE: Any response oneshots might get cancelled if the `DisputeCoordinator` was not yet
+/// properly initialized for some reason.
 #[derive(Debug)]
 pub enum DisputeCoordinatorMessage {
 	/// Import statements by validators about a candidate.