Remove the --unsafe-pruning CLI-argument (step 1) (#10995)

* sc-client-db: utils::open_database(...) — return OpenDbError so that the caller could tell the `OpenDbError::DoesNotExist` clearly * sc-client-db: utils::open_database(..) — accept the `create: bool` argument * sc-client-db: pruning — optional argument in the DatabaseSettings * sc-state-db: Split `Error<E>` into separate `Error<E>` and `StateDbError` * StateDb::open: choose the pruning-mode depending on the requested and stored values * sc-state-db: test for different combinations of stored and requested pruning-modes * CLI-argument: mark the unsafe-pruning as deprecated * Fix tests * tests: do not specify --pruning when running the substrate over the existing storage * fix types for benches * cargo fmt * Check whether the pruning-mode and sync-mode are compatible * cargo fmt * parity-db: 0.3.11 -> 0.3.12 * sc-state-db: MetaDb::set_meta — a better doc-test * cargo fmt * make MetaDb read-only again! * Remove the stray newline (and run the CI once again please) * Last nitpicks * A more comprehensive error message
paritytech · May 6, 2022 · 43d8c87 · 43d8c87
1 parent dedfef2
commit 43d8c87
Show file tree

Hide file tree

Showing 23 changed files with 546 additions and 338 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/bin/node/cli/benches/block_production.rs b/bin/node/cli/benches/block_production.rs
@@ -74,7 +74,7 @@ fn new_node(tokio_handle: Handle) -> node_cli::service::NewFullBase {
 		database: DatabaseSource::RocksDb { path: root.join("db"), cache_size: 128 },
 		state_cache_size: 67108864,
 		state_cache_child_ratio: None,
-		state_pruning: PruningMode::ArchiveAll,
+		state_pruning: Some(PruningMode::ArchiveAll),
 		keep_blocks: KeepBlocks::All,
 		chain_spec: spec,
 		wasm_method: WasmExecutionMethod::Compiled,

diff --git a/bin/node/cli/benches/transaction_pool.rs b/bin/node/cli/benches/transaction_pool.rs
@@ -65,7 +65,7 @@ fn new_node(tokio_handle: Handle) -> node_cli::service::NewFullBase {
 		database: DatabaseSource::RocksDb { path: root.join("db"), cache_size: 128 },
 		state_cache_size: 67108864,
 		state_cache_child_ratio: None,
-		state_pruning: PruningMode::ArchiveAll,
+		state_pruning: Some(PruningMode::ArchiveAll),
 		keep_blocks: KeepBlocks::All,
 		chain_spec: spec,
 		wasm_method: WasmExecutionMethod::Interpreted,

diff --git a/bin/node/cli/tests/benchmark_block_works.rs b/bin/node/cli/tests/benchmark_block_works.rs
@@ -37,7 +37,6 @@ async fn benchmark_block_works() {
 		.args(["benchmark", "block", "--dev"])
 		.arg("-d")
 		.arg(base_dir.path())
-		.args(["--pruning", "archive"])
 		.args(["--from", "1", "--to", "1"])
 		.args(["--repeat", "1"])
 		.args(["--execution", "wasm", "--wasm-execution", "compiled"])

diff --git a/bin/node/cli/tests/check_block_works.rs b/bin/node/cli/tests/check_block_works.rs
@@ -31,7 +31,7 @@ async fn check_block_works() {
 	common::run_node_for_a_while(base_path.path(), &["--dev", "--no-hardware-benchmarks"]).await;
 
 	let status = Command::new(cargo_bin("substrate"))
-		.args(&["check-block", "--dev", "--pruning", "archive", "-d"])
+		.args(&["check-block", "--dev", "-d"])
 		.arg(base_path.path())
 		.arg("1")
 		.status()

diff --git a/bin/node/cli/tests/export_import_flow.rs b/bin/node/cli/tests/export_import_flow.rs
@@ -79,9 +79,9 @@ impl<'a> ExportImportRevertExecutor<'a> {
 		// Adding "--binary" if need be.
 		let arguments: Vec<&str> = match format_opt {
 			FormatOpt::Binary => {
-				vec![&sub_command_str, "--dev", "--pruning", "archive", "--binary", "-d"]
+				vec![&sub_command_str, "--dev", "--binary", "-d"]
 			},
-			FormatOpt::Json => vec![&sub_command_str, "--dev", "--pruning", "archive", "-d"],
+			FormatOpt::Json => vec![&sub_command_str, "--dev", "-d"],
 		};
 
 		let tmp: TempDir;
@@ -161,7 +161,7 @@ impl<'a> ExportImportRevertExecutor<'a> {
 	/// Runs the `revert` command.
 	fn run_revert(&self) {
 		let output = Command::new(cargo_bin("substrate"))
-			.args(&["revert", "--dev", "--pruning", "archive", "-d"])
+			.args(&["revert", "--dev", "-d"])
 			.arg(&self.base_path.path())
 			.output()
 			.unwrap();

diff --git a/bin/node/cli/tests/inspect_works.rs b/bin/node/cli/tests/inspect_works.rs
@@ -31,7 +31,7 @@ async fn inspect_works() {
 	common::run_node_for_a_while(base_path.path(), &["--dev", "--no-hardware-benchmarks"]).await;
 
 	let status = Command::new(cargo_bin("substrate"))
-		.args(&["inspect", "--dev", "--pruning", "archive", "-d"])
+		.args(&["inspect", "--dev", "-d"])
 		.arg(base_path.path())
 		.args(&["block", "1"])
 		.status()

diff --git a/bin/node/testing/src/bench.rs b/bin/node/testing/src/bench.rs
@@ -389,7 +389,7 @@ impl BenchDb {
 		let db_config = sc_client_db::DatabaseSettings {
 			state_cache_size: 16 * 1024 * 1024,
 			state_cache_child_ratio: Some((0, 100)),
-			state_pruning: PruningMode::ArchiveAll,
+			state_pruning: Some(PruningMode::ArchiveAll),
 			source: database_type.into_settings(dir.into()),
 			keep_blocks: sc_client_db::KeepBlocks::All,
 		};

diff --git a/client/api/src/backend.rs b/client/api/src/backend.rs
@@ -546,6 +546,9 @@ pub trait Backend<Block: BlockT>: AuxStore + Send + Sync {
 	/// something that the import of a block would interfere with, e.g. importing
 	/// a new block or calculating the best head.
 	fn get_import_lock(&self) -> &RwLock<()>;
+
+	/// Tells whether the backend requires full-sync mode.
+	fn requires_full_sync(&self) -> bool;
 }
 
 /// Mark for all Backend implementations, that are making use of state data, stored locally.

diff --git a/client/api/src/client.rs b/client/api/src/client.rs
@@ -146,6 +146,9 @@ pub trait BlockBackend<Block: BlockT> {
 	fn has_indexed_transaction(&self, hash: &Block::Hash) -> sp_blockchain::Result<bool> {
 		Ok(self.indexed_transaction(hash)?.is_some())
 	}
+
+	/// Tells whether the current client configuration requires full-sync mode.
+	fn requires_full_sync(&self) -> bool;
 }
 
 /// Provide a list of potential uncle headers for a given block.

diff --git a/client/api/src/in_mem.rs b/client/api/src/in_mem.rs
@@ -795,6 +795,10 @@ where
 	fn get_import_lock(&self) -> &RwLock<()> {
 		&self.import_lock
 	}
+
+	fn requires_full_sync(&self) -> bool {
+		false
+	}
 }
 
 impl<Block: BlockT> backend::LocalBackend<Block> for Backend<Block> where Block::Hash: Ord {}

diff --git a/client/cli/src/config.rs b/client/cli/src/config.rs
@@ -251,9 +251,9 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
 	///
 	/// By default this is retrieved from `PruningMode` if it is available. Otherwise its
 	/// `PruningMode::default()`.
-	fn state_pruning(&self, unsafe_pruning: bool, role: &Role) -> Result<PruningMode> {
+	fn state_pruning(&self) -> Result<Option<PruningMode>> {
 		self.pruning_params()
-			.map(|x| x.state_pruning(unsafe_pruning, role))
+			.map(|x| x.state_pruning())
 			.unwrap_or_else(|| Ok(Default::default()))
 	}
 
@@ -494,8 +494,6 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
 		let telemetry_endpoints = self.telemetry_endpoints(&chain_spec)?;
 		let runtime_cache_size = self.runtime_cache_size()?;
 
-		let unsafe_pruning = self.import_params().map(|p| p.unsafe_pruning).unwrap_or(false);
-
 		Ok(Configuration {
 			impl_name: C::impl_name(),
 			impl_version: C::impl_version(),
@@ -516,7 +514,7 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
 			database: self.database_config(&config_dir, database_cache_size, database, &role)?,
 			state_cache_size: self.state_cache_size()?,
 			state_cache_child_ratio: self.state_cache_child_ratio()?,
-			state_pruning: self.state_pruning(unsafe_pruning, &role)?,
+			state_pruning: self.state_pruning()?,
 			keep_blocks: self.keep_blocks()?,
 			wasm_method: self.wasm_method()?,
 			wasm_runtime_overrides: self.wasm_runtime_overrides(),
@@ -643,6 +641,17 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
 			}
 		}
 
+		if self.import_params().map_or(false, |p| {
+			#[allow(deprecated)]
+			p.unsafe_pruning
+		}) {
+			// according to https://github.com/substrate/issues/8103;
+			warn!(
+				"WARNING: \"--unsafe-pruning\" CLI-flag is deprecated and has no effect. \
+				In future builds it will be removed, and providing this flag will lead to an error."
+			);
+		}
+
 		Ok(())
 	}
 }

diff --git a/client/cli/src/params/import_params.rs b/client/cli/src/params/import_params.rs
@@ -40,12 +40,16 @@ pub struct ImportParams {
 	#[clap(flatten)]
 	pub database_params: DatabaseParams,
 
-	/// Force start with unsafe pruning settings.
+	/// THIS IS A DEPRECATED CLI-ARGUMENT.
 	///
-	/// When running as a validator it is highly recommended to disable state
-	/// pruning (i.e. 'archive') which is the default. The node will refuse to
-	/// start as a validator if pruning is enabled unless this option is set.
+	/// It has been preserved in order to not break the compatibility with the existing scripts.
+	/// Enabling this option will lead to a runtime warning.
+	/// In future this option will be removed completely, thus specifying it will lead to a start
+	/// up error.
+	///
+	/// Details: <https://github.com/paritytech/substrate/issues/8103>
 	#[clap(long)]
+	#[deprecated = "According to https://github.com/paritytech/substrate/issues/8103"]
 	pub unsafe_pruning: bool,
 
 	/// Method for executing Wasm runtime code.

diff --git a/client/cli/src/params/pruning_params.rs b/client/cli/src/params/pruning_params.rs
@@ -18,7 +18,7 @@
 
 use crate::error;
 use clap::Args;
-use sc_service::{KeepBlocks, PruningMode, Role};
+use sc_service::{KeepBlocks, PruningMode};
 
 /// Parameters to define the pruning mode
 #[derive(Debug, Clone, PartialEq, Args)]
@@ -39,29 +39,17 @@ pub struct PruningParams {
 
 impl PruningParams {
 	/// Get the pruning value from the parameters
-	pub fn state_pruning(&self, unsafe_pruning: bool, role: &Role) -> error::Result<PruningMode> {
-		// by default we disable pruning if the node is an authority (i.e.
-		// `ArchiveAll`), otherwise we keep state for the last 256 blocks. if the
-		// node is an authority and pruning is enabled explicitly, then we error
-		// unless `unsafe_pruning` is set.
-		Ok(match &self.pruning {
-			Some(ref s) if s == "archive" => PruningMode::ArchiveAll,
-			None if role.is_authority() => PruningMode::ArchiveAll,
-			None => PruningMode::default(),
-			Some(s) => {
-				if role.is_authority() && !unsafe_pruning {
-					return Err(error::Error::Input(
-						"Validators should run with state pruning disabled (i.e. archive). \
-						You can ignore this check with `--unsafe-pruning`."
-							.to_string(),
-					))
-				}
-
-				PruningMode::keep_blocks(s.parse().map_err(|_| {
-					error::Error::Input("Invalid pruning mode specified".to_string())
-				})?)
-			},
-		})
+	pub fn state_pruning(&self) -> error::Result<Option<PruningMode>> {
+		self.pruning
+			.as_ref()
+			.map(|s| match s.as_str() {
+				"archive" => Ok(PruningMode::ArchiveAll),
+				bc => bc
+					.parse()
+					.map_err(|_| error::Error::Input("Invalid pruning mode specified".to_string()))
+					.map(PruningMode::keep_blocks),
+			})
+			.transpose()
 	}
 
 	/// Get the block pruning value from the parameters