Address review feedback

- Filter mining status worker_stats to currently connected workers
  (StratumStats retains disconnected workers for the node lifetime)
- Return an internal error from get_mining_status when no stats
  provider is wired, instead of masking it as disabled
- Restore the public node_apis signature; add
  node_apis_with_mining_stats for provider wiring
- Document is_running as set-once at startup
- Trim get_mining_status doc comment to match neighboring methods

Author: iho

api/src/handlers.rs

@@ -62,6 +62,39 @@ pub fn node_apis<B, P>(
 	tls_config: Option<TLSConfig>,
 	api_chan: (mpsc::Sender<()>, mpsc::Receiver<()>),
 	stop_state: Arc<StopState>,
+) -> Result<(), Error>
+where
+	B: BlockChain + 'static,
+	P: PoolAdapter + 'static,
+{
+	node_apis_with_mining_stats(
+		addr,
+		chain,
+		tx_pool,
+		peers,
+		sync_state,
+		api_secret,
+		foreign_api_secret,
+		tls_config,
+		api_chan,
+		stop_state,
+		None,
+	)
+}
+
+/// Same as [`node_apis`] but additionally wires a provider of live stratum
+/// mining stats into the Owner API `get_mining_status` method.
+pub fn node_apis_with_mining_stats<B, P>(
+	addr: &str,
+	chain: Arc<Chain>,
+	tx_pool: Arc<RwLock<pool::TransactionPool<B, P>>>,
+	peers: Arc<p2p::Peers>,
+	sync_state: Arc<SyncState>,
+	api_secret: Option<String>,
+	foreign_api_secret: Option<String>,
+	tls_config: Option<TLSConfig>,
+	api_chan: (mpsc::Sender<()>, mpsc::Receiver<()>),
+	stop_state: Arc<StopState>,
 	mining_stats: Option<MiningStatsProvider>,
 ) -> Result<(), Error>
 where

api/src/lib.rs

@@ -47,7 +47,7 @@ pub use crate::auth::{
 };
 pub use crate::foreign::Foreign;
 pub use crate::foreign_rpc::ForeignRpc;
-pub use crate::handlers::node_apis;
+pub use crate::handlers::{node_apis, node_apis_with_mining_stats};
 pub use crate::owner::{MiningStatsProvider, Owner};
 pub use crate::owner_rpc::OwnerRpc;
 pub use crate::rest::*;

api/src/owner.rs

@@ -99,22 +99,20 @@ impl Owner {
 		status_handler.get_status()
 	}
 
-	/// Returns current stratum mining statistics (enabled/running, workers, difficulty, etc.).
-	/// Useful when running headless without the TUI mining tab.
-	///
-	/// When stratum is disabled or no stats provider is wired up, returns a default
-	/// (disabled) [`MiningStatus`](types/struct.MiningStatus.html).
+	/// Returns current stratum mining statistics, mirroring the TUI mining tab.
 	///
 	/// # Returns
 	/// * Result Containing:
 	/// * A [`MiningStatus`](types/struct.MiningStatus.html)
-	/// * or [`Error`](struct.Error.html) if an error is encountered.
+	/// * or [`Error`](struct.Error.html) if no mining stats provider is configured.
 	///
 
 	pub fn get_mining_status(&self) -> Result<MiningStatus, Error> {
 		match &self.mining_stats {
 			Some(provider) => Ok(provider()),
-			None => Ok(MiningStatus::default()),
+			None => Err(Error::Internal(
+				"no mining stats provider configured".to_string(),
+			)),
 		}
 	}
 
@@ -249,10 +247,9 @@ mod test {
 	use std::sync::Arc;
 
 	#[test]
-	fn get_mining_status_without_provider_returns_default() {
+	fn get_mining_status_without_provider_returns_error() {
 		let owner = Owner::new(Weak::new(), Weak::new(), Weak::new());
-		let status = owner.get_mining_status().unwrap();
-		assert_eq!(status, MiningStatus::default());
+		assert!(owner.get_mining_status().is_err());
 	}
 
 	#[test]

api/src/types.rs

@@ -133,7 +133,8 @@ pub struct WorkerInfo {
 pub struct MiningStatus {
 	/// Whether the stratum server is enabled in config
 	pub is_enabled: bool,
-	/// Whether the stratum server is currently running
+	/// Whether the stratum server has been started. Set once at startup and
+	/// not cleared on listener failure or shutdown.
 	pub is_running: bool,
 	/// Number of currently connected workers
 	pub num_workers: usize,
@@ -149,7 +150,7 @@ pub struct MiningStatus {
 	pub network_hashrate: f64,
 	/// Minimum share difficulty requested from miners
 	pub minimum_share_difficulty: u64,
-	/// Per-worker status
+	/// Per-worker status for currently connected workers
 	pub worker_stats: Vec<WorkerInfo>,
 }
 

doc/api/api.md

@@ -30,7 +30,7 @@ Or from the CLI:
 grin client miningstatus
 ```
 
-The response includes whether stratum is enabled/running, connected worker count, current block height and network difficulty, blocks found, network hashrate estimate, and per-worker share stats.
+The response includes whether stratum is enabled/running, connected worker count, current block height and network difficulty, blocks found, network hashrate estimate, and per-worker share stats for currently connected workers.
 
 ## Node API v1
 

servers/src/common/stats.rs

@@ -327,7 +327,14 @@ impl From<&StratumStats> for MiningStatus {
 			blocks_found: stats.blocks_found,
 			network_hashrate: stats.network_hashrate,
 			minimum_share_difficulty: stats.minimum_share_difficulty,
-			worker_stats: stats.worker_stats.iter().map(WorkerInfo::from).collect(),
+			// worker_stats retains every worker that ever connected; only expose
+			// currently connected ones so the response does not grow unbounded.
+			worker_stats: stats
+				.worker_stats
+				.iter()
+				.filter(|w| w.is_connected)
+				.map(WorkerInfo::from)
+				.collect(),
 		}
 	}
 }
@@ -350,6 +357,10 @@ mod test {
 		worker.num_stale = 2;
 		worker.num_blocks_found = 1;
 
+		let mut disconnected = WorkerStats::default();
+		disconnected.id = "gone".into();
+		disconnected.is_connected = false;
+
 		let stats = StratumStats {
 			is_enabled: true,
 			is_running: true,
@@ -360,7 +371,7 @@ mod test {
 			blocks_found: 4,
 			network_hashrate: 12.5,
 			minimum_share_difficulty: 3,
-			worker_stats: vec![worker],
+			worker_stats: vec![worker, disconnected],
 		};
 
 		let mining = MiningStatus::from(&stats);

servers/src/grin/server.rs

@@ -316,7 +316,7 @@ impl Server {
 			api::types::MiningStatus::from(&*stats)
 		});
 
-		api::node_apis(
+		api::node_apis_with_mining_stats(
 			&config.api_http_addr,
 			shared_chain.clone(),
 			tx_pool.clone(),

src/bin/cmd/client.rs

@@ -178,9 +178,6 @@ impl HTTPNodeClient {
 				if !status.worker_stats.is_empty() {
 					writeln!(e, "Workers:").unwrap();
 					for worker in status.worker_stats {
-						if !worker.is_connected {
-							continue;
-						}
 						writeln!(
 							e,
 							"  id={} accepted={} rejected={} stale={} blocks={} difficulty={}",