From 35c71bf315a82eb05f3605c2f63f5eb9ab645d98 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Thu, 10 Dec 2020 16:57:36 +0100 Subject: [PATCH] addition error definitions (#2107) * remove low information density error doc comments * another round of error dancing * fix compilation * remove stale `None` argument * adjust test, minor slip in command * only add AvailabilityError for full node features * another None where none shuld be --- polkadot/Cargo.lock | 1 + polkadot/cli/src/command.rs | 65 ++++++++++++++----- polkadot/node/core/av-store/src/lib.rs | 19 ++++-- .../node/core/bitfield-signing/src/lib.rs | 15 ++--- polkadot/node/service/Cargo.toml | 1 + polkadot/node/service/src/lib.rs | 46 +++++++++++-- polkadot/node/subsystem/src/lib.rs | 6 +- polkadot/node/test/service/src/lib.rs | 7 +- .../adder/collator/src/main.rs | 10 +-- 9 files changed, 124 insertions(+), 46 deletions(-) diff --git a/polkadot/Cargo.lock b/polkadot/Cargo.lock index 91f480a07f..9bbe334f7e 100644 --- a/polkadot/Cargo.lock +++ b/polkadot/Cargo.lock @@ -5564,6 +5564,7 @@ dependencies = [ "sp-transaction-pool", "sp-trie", "substrate-prometheus-endpoint", + "thiserror", "tracing", "tracing-futures", "westend-runtime", diff --git a/polkadot/cli/src/command.rs b/polkadot/cli/src/command.rs index 85341278fb..d09de02a12 100644 --- a/polkadot/cli/src/command.rs +++ b/polkadot/cli/src/command.rs @@ -16,9 +16,32 @@ use log::info; use service::{IdentifyVariant, self}; -use sc_cli::{SubstrateCli, Result, RuntimeVersion, Role}; +use sc_cli::{SubstrateCli, RuntimeVersion, Role}; use crate::cli::{Cli, Subcommand}; +#[derive(thiserror::Error, Debug)] +pub enum Error { + #[error(transparent)] + PolkadotService(#[from] service::Error), + + #[error(transparent)] + SubstrateCli(#[from] sc_cli::Error), + + #[error(transparent)] + SubstrateService(#[from] sc_service::Error), + + #[error("Other: {0}")] + Other(String), +} + +impl std::convert::From for Error { + fn from(s: String) -> Self { + Self::Other(s) + } +} + +type Result = std::result::Result; + fn get_exec_name() -> Option { std::env::current_exe() .ok() @@ -139,22 +162,27 @@ pub fn run() -> Result<()> { info!("----------------------------"); } - runner.run_node_until_exit(|config| async move { + + Ok(runner.run_node_until_exit(move |config| async move { let role = config.role.clone(); - match role { - Role::Light => service::build_light(config).map(|(task_manager, _)| task_manager), + let task_manager = match role { + Role::Light => service::build_light(config).map(|(task_manager, _)| task_manager) + .map_err(|e| sc_service::Error::Other(e.to_string()) ), _ => service::build_full( config, service::IsCollator::No, grandpa_pause, - ).map(|full| full.task_manager), - } - }) + ).map(|full| full.task_manager) + .map_err(|e| sc_service::Error::Other(e.to_string()) ) + }; + task_manager + }).map_err(|e| -> sc_cli::Error { e.into() })?) + }, Some(Subcommand::BuildSpec(cmd)) => { let runner = cli.create_runner(cmd)?; - runner.sync_run(|config| cmd.run(config.chain_spec, config.network)) + Ok(runner.sync_run(|config| cmd.run(config.chain_spec, config.network))?) }, Some(Subcommand::CheckBlock(cmd)) => { let runner = cli.create_runner(cmd)?; @@ -163,7 +191,8 @@ pub fn run() -> Result<()> { set_default_ss58_version(chain_spec); runner.async_run(|mut config| { - let (client, _, import_queue, task_manager) = service::new_chain_ops(&mut config)?; + let (client, _, import_queue, task_manager) = service::new_chain_ops(&mut config) + .map_err(|e| sc_service::Error::Other(e.to_string()))?; Ok((cmd.run(client, import_queue), task_manager)) }) }, @@ -174,7 +203,8 @@ pub fn run() -> Result<()> { set_default_ss58_version(chain_spec); runner.async_run(|mut config| { - let (client, _, _, task_manager) = service::new_chain_ops(&mut config)?; + let (client, _, _, task_manager) = service::new_chain_ops(&mut config) + .map_err(|e| sc_service::Error::Other(e.to_string()))?; Ok((cmd.run(client, config.database), task_manager)) }) }, @@ -185,7 +215,8 @@ pub fn run() -> Result<()> { set_default_ss58_version(chain_spec); runner.async_run(|mut config| { - let (client, _, _, task_manager) = service::new_chain_ops(&mut config)?; + let (client, _, _, task_manager) = service::new_chain_ops(&mut config) + .map_err(|e| sc_service::Error::Other(e.to_string()))?; Ok((cmd.run(client, config.chain_spec), task_manager)) }) }, @@ -196,13 +227,15 @@ pub fn run() -> Result<()> { set_default_ss58_version(chain_spec); runner.async_run(|mut config| { - let (client, _, import_queue, task_manager) = service::new_chain_ops(&mut config)?; + let (client, _, import_queue, task_manager) = service::new_chain_ops(&mut config) + .map_err(|e| sc_service::Error::Other(e.to_string()))?; Ok((cmd.run(client, import_queue), task_manager)) }) }, Some(Subcommand::PurgeChain(cmd)) => { let runner = cli.create_runner(cmd)?; - runner.sync_run(|config| cmd.run(config.database)) + Ok(runner.sync_run(|config| cmd.run(config.database)) + .map_err(|e| sc_service::Error::Other(e.to_string()))?) }, Some(Subcommand::Revert(cmd)) => { let runner = cli.create_runner(cmd)?; @@ -211,7 +244,8 @@ pub fn run() -> Result<()> { set_default_ss58_version(chain_spec); runner.async_run(|mut config| { - let (client, backend, _, task_manager) = service::new_chain_ops(&mut config)?; + let (client, backend, _, task_manager) = service::new_chain_ops(&mut config) + .map_err(|e| sc_service::Error::Other(e.to_string()))?; Ok((cmd.run(client, backend), task_manager)) }) }, @@ -237,5 +271,6 @@ pub fn run() -> Result<()> { }) }, Some(Subcommand::Key(cmd)) => cmd.run(), - } + }?; + Ok(()) } diff --git a/polkadot/node/core/av-store/src/lib.rs b/polkadot/node/core/av-store/src/lib.rs index 72c1d9cb4c..795d89d9ee 100644 --- a/polkadot/node/core/av-store/src/lib.rs +++ b/polkadot/node/core/av-store/src/lib.rs @@ -44,7 +44,6 @@ use polkadot_node_subsystem_util::metrics::{self, prometheus}; use polkadot_subsystem::messages::{ AllMessages, AvailabilityStoreMessage, ChainApiMessage, RuntimeApiMessage, RuntimeApiRequest, }; -use thiserror::Error; const LOG_TARGET: &str = "availability"; @@ -54,22 +53,32 @@ mod columns { pub const NUM_COLUMNS: u32 = 2; } -#[derive(Debug, Error)] -enum Error { +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] +pub enum Error { #[error(transparent)] RuntimeApi(#[from] RuntimeApiError), + #[error(transparent)] ChainApi(#[from] ChainApiError), + #[error(transparent)] Erasure(#[from] erasure::Error), + #[error(transparent)] Io(#[from] io::Error), + #[error(transparent)] Oneshot(#[from] oneshot::Canceled), + #[error(transparent)] Subsystem(#[from] SubsystemError), + #[error(transparent)] Time(#[from] SystemTimeError), + + #[error("Custom databases are not supported")] + CustomDatabase, } impl Error { @@ -418,10 +427,10 @@ pub struct Config { } impl std::convert::TryFrom for Config { - type Error = &'static str; + type Error = Error; fn try_from(config: sc_service::config::DatabaseConfig) -> Result { - let path = config.path().ok_or("custom databases are not supported")?; + let path = config.path().ok_or(Error::CustomDatabase)?; Ok(Self { // substrate cache size is improper here; just use the default diff --git a/polkadot/node/core/bitfield-signing/src/lib.rs b/polkadot/node/core/bitfield-signing/src/lib.rs index 7937d908ef..92478afb13 100644 --- a/polkadot/node/core/bitfield-signing/src/lib.rs +++ b/polkadot/node/core/bitfield-signing/src/lib.rs @@ -35,7 +35,6 @@ use polkadot_node_subsystem_util::{ use polkadot_primitives::v1::{AvailabilityBitfield, CoreState, Hash, ValidatorIndex}; use std::{pin::Pin, time::Duration, iter::FromIterator}; use wasm_timer::{Delay, Instant}; -use thiserror::Error; /// Delay between starting a bitfield signing job and its attempting to create a bitfield. const JOB_DELAY: Duration = Duration::from_millis(1500); @@ -45,24 +44,24 @@ const LOG_TARGET: &str = "bitfield_signing"; pub struct BitfieldSigningJob; /// Errors we may encounter in the course of executing the `BitfieldSigningSubsystem`. -#[derive(Debug, Error)] +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] pub enum Error { - /// error propagated from the utility subsystem #[error(transparent)] Util(#[from] util::Error), - /// io error + #[error(transparent)] Io(#[from] std::io::Error), - /// a one shot channel was canceled + #[error(transparent)] Oneshot(#[from] oneshot::Canceled), - /// a mspc channel failed to send + #[error(transparent)] MpscSend(#[from] mpsc::SendError), - /// the runtime API failed to return what we wanted + #[error(transparent)] Runtime(#[from] RuntimeApiError), - /// the keystore failed to process signing request + #[error("Keystore failed: {0:?}")] Keystore(KeystoreError), } diff --git a/polkadot/node/service/Cargo.toml b/polkadot/node/service/Cargo.toml index 541f4ab391..0cb3fdc670 100644 --- a/polkadot/node/service/Cargo.toml +++ b/polkadot/node/service/Cargo.toml @@ -57,6 +57,7 @@ hex-literal = "0.3.1" tracing = "0.1.22" tracing-futures = "0.2.4" serde = { version = "1.0.118", features = ["derive"] } +thiserror = "1.0.21" # Polkadot polkadot-node-core-proposer = { path = "../core/proposer" } diff --git a/polkadot/node/service/src/lib.rs b/polkadot/node/service/src/lib.rs index 43f43657b8..0bb1769d9a 100644 --- a/polkadot/node/service/src/lib.rs +++ b/polkadot/node/service/src/lib.rs @@ -27,9 +27,9 @@ use grandpa::{self, FinalityProofProvider as GrandpaFinalityProofProvider}; use { std::convert::TryInto, std::time::Duration, - tracing::info, polkadot_node_core_av_store::Config as AvailabilityConfig, + polkadot_node_core_av_store::Error as AvailabilityError, polkadot_node_core_proposer::ProposerFactory, polkadot_overseer::{AllSubsystems, BlockInfo, Overseer, OverseerHandler}, polkadot_primitives::v1::ParachainHost, @@ -56,7 +56,7 @@ pub use sc_client_api::{Backend, ExecutionStrategy, CallExecutor}; pub use sc_consensus::LongestChain; pub use sc_executor::NativeExecutionDispatch; pub use service::{ - Role, PruningMode, TransactionPoolOptions, Error, RuntimeGenesis, + Role, PruningMode, TransactionPoolOptions, Error as SubstrateServiceError, RuntimeGenesis, TFullClient, TLightClient, TFullBackend, TLightBackend, TFullCallExecutor, TLightCallExecutor, Configuration, ChainSpec, TaskManager, }; @@ -97,6 +97,38 @@ native_executor_instance!( frame_benchmarking::benchmarking::HostFunctions, ); + +#[derive(thiserror::Error, Debug)] +pub enum Error { + #[error(transparent)] + Io(#[from] std::io::Error), + + #[error(transparent)] + AddrFormatInvalid(#[from] std::net::AddrParseError), + + #[error(transparent)] + Sub(#[from] SubstrateServiceError), + + #[error(transparent)] + Blockchain(#[from] sp_blockchain::Error), + + #[error(transparent)] + Consensus(#[from] consensus_common::Error), + + #[error("Failed to create an overseer")] + Overseer(#[from] polkadot_overseer::SubsystemError), + + #[error(transparent)] + Prometheus(#[from] prometheus_endpoint::PrometheusError), + + #[cfg(feature = "full-node")] + #[error(transparent)] + Availability(#[from] AvailabilityError), + + #[error("Authorities require the real overseer implementation")] + AuthoritiesRequireRealOverseer, +} + /// Can be called for a `Configuration` to check if it is a configuration for the `Kusama` network. pub trait IdentifyVariant { /// Returns if this is a configuration for the `Kusama` network. @@ -304,7 +336,7 @@ where AllSubsystems::<()>::dummy(), registry, spawner, - ).map_err(|e| Error::Other(format!("Failed to create an Overseer: {:?}", e))) + ).map_err(|e| e.into()) } #[cfg(all(feature = "full-node", feature = "real-overseer"))] @@ -418,7 +450,7 @@ where all_subsystems, registry, spawner, - ).map_err(|e| Error::Other(format!("Failed to create an Overseer: {:?}", e))) + ).map_err(|e| e.into()) } #[cfg(feature = "full-node")] @@ -529,7 +561,7 @@ pub fn new_full( let telemetry_connection_sinks = service::TelemetryConnectionSinks::default(); - let availability_config = config.database.clone().try_into(); + let availability_config = config.database.clone().try_into().map_err(Error::Availability)?; let rpc_handlers = service::spawn_tasks(service::SpawnTasksParams { config, @@ -605,7 +637,7 @@ pub fn new_full( leaves, keystore_container.sync_keystore(), overseer_client.clone(), - availability_config?, + availability_config, network.clone(), authority_discovery_service, prometheus_registry.as_ref(), @@ -644,7 +676,7 @@ pub fn new_full( task_manager.spawn_handle(), client.clone(), transaction_pool, - overseer_handler.as_ref().ok_or("authorities require real overseer handlers")?.clone(), + overseer_handler.as_ref().ok_or_else(|| Error::AuthoritiesRequireRealOverseer)?.clone(), prometheus_registry.as_ref(), ); diff --git a/polkadot/node/subsystem/src/lib.rs b/polkadot/node/subsystem/src/lib.rs index 143c35d2c0..1d9d7b7352 100644 --- a/polkadot/node/subsystem/src/lib.rs +++ b/polkadot/node/subsystem/src/lib.rs @@ -77,7 +77,7 @@ impl PartialEq for ActiveLeavesUpdate { /// /// Instead, it means equality when `activated` and `deactivated` are considered as sets. fn eq(&self, other: &Self) -> bool { - self.activated.len() == other.activated.len() && self.deactivated.len() == other.deactivated.len() + self.activated.len() == other.activated.len() && self.deactivated.len() == other.deactivated.len() && self.activated.iter().all(|a| other.activated.contains(a)) && self.deactivated.iter().all(|a| other.deactivated.contains(a)) } @@ -151,13 +151,13 @@ pub enum SubsystemError { /// An additional anotation tag for the origin of `source`. origin: &'static str, /// The wrapped error. Marked as source for tracking the error chain. - #[source] source: Box + #[source] source: Box }, } impl SubsystemError { /// Adds a `str` as `origin` to the given error `err`. - pub fn with_origin(origin: &'static str, err: E) -> Self { + pub fn with_origin(origin: &'static str, err: E) -> Self { Self::FromOrigin { origin, source: Box::new(err) } } } diff --git a/polkadot/node/test/service/src/lib.rs b/polkadot/node/test/service/src/lib.rs index 4a4677a0ef..7a37e3db31 100644 --- a/polkadot/node/test/service/src/lib.rs +++ b/polkadot/node/test/service/src/lib.rs @@ -28,7 +28,7 @@ use polkadot_primitives::v1::{ }; use polkadot_runtime_common::BlockHashCount; use polkadot_service::{ - NewFull, FullClient, ClientHandle, ExecuteWithClient, IsCollator, + Error, NewFull, FullClient, ClientHandle, ExecuteWithClient, IsCollator, }; use polkadot_node_subsystem::messages::{CollatorProtocolMessage, CollationGenerationMessage}; use polkadot_test_runtime::{ @@ -45,7 +45,6 @@ use sc_network::{ }; use service::{ config::{DatabaseConfig, KeystoreConfig, MultiaddrWithPeerId, WasmExecutionMethod}, - error::Error as ServiceError, RpcHandlers, TaskExecutor, TaskManager, }; use service::{BasePath, Configuration, Role}; @@ -76,14 +75,14 @@ pub fn new_full( is_collator: IsCollator, ) -> Result< NewFull>, - ServiceError, + Error, > { polkadot_service::new_full::( config, is_collator, None, polkadot_parachain::wasm_executor::IsolationStrategy::InProcess, - ).map_err(Into::into) + ) } /// A wrapper for the test client that implements `ClientHandle`. diff --git a/polkadot/parachain/test-parachains/adder/collator/src/main.rs b/polkadot/parachain/test-parachains/adder/collator/src/main.rs index 03b63989cc..49ffc379eb 100644 --- a/polkadot/parachain/test-parachains/adder/collator/src/main.rs +++ b/polkadot/parachain/test-parachains/adder/collator/src/main.rs @@ -19,7 +19,7 @@ use polkadot_node_primitives::CollationGenerationConfig; use polkadot_node_subsystem::messages::{CollationGenerationMessage, CollatorProtocolMessage}; use polkadot_primitives::v1::Id as ParaId; -use sc_cli::{Result, Role, SubstrateCli}; +use sc_cli::{Result, Error as SubstrateCliError, Role, SubstrateCli}; use sp_core::hexdisplay::HexDisplay; use test_parachain_adder_collator::Collator; @@ -46,7 +46,8 @@ fn main() -> Result<()> { Ok(()) } None => { - let runner = cli.create_runner(&cli.run.base)?; + let runner = cli.create_runner(&cli.run.base) + .map_err(|e| SubstrateCliError::Application(Box::new(e) as Box::<(dyn 'static + Send + Sync + std::error::Error)>))?; runner.run_node_until_exit(|config| async move { let role = config.role.clone(); @@ -60,7 +61,7 @@ fn main() -> Result<()> { config, polkadot_service::IsCollator::Yes(collator.collator_id()), None, - )?; + ).map_err(|e| e.to_string())?; let mut overseer_handler = full_node .overseer_handler .expect("Overseer handler should be initialized for collators"); @@ -94,5 +95,6 @@ fn main() -> Result<()> { } }) } - } + }?; + Ok(()) }