Remove the `--unsafe-pruning` CLI-argument (step 1) by RGafiyatullin · Pull Request #10995 · paritytech/substrate

RGafiyatullin · 2022-03-08T15:23:42Z

This PR addresses the #8103.

It does not remove CLI-argument completely in order not to break the existing scripts that may use this argument.
Instead it emits a warning when the argument is used. Other than that the argument will take no effect on the execution.

NB: It will be necessary to remove any notion of unsafe-pruning in further releases.

cumulus companion: paritytech/cumulus#1162
polkadot companion: paritytech/polkadot#5446

davxy

LGTM

wigy-opensource-developer

Besides the one annoying rustdoc lint it looks fine. The issue #8103 stays open until step 2, I guess.

bkchr

This doesn't include the required changes regarding continue working as archive node for validators, but new validator nodes should start as pruned node. So, that we don't change any behavior. (The same applies, if you already used unsafe pruning you just continue working as pruned node)

Read the original issue 😊

client/db/src/blockchain_db.rs

client/service/src/builder.rs

RGafiyatullin · 2022-04-04T18:13:33Z

The PR summary:

the CLI argument --pruning archive|<block-num> is exposed to the application as Option<PruningMode> (as opposed to previously used PruningMode);
the sc-client-database::Backend and sc-state-db::StateDb are aware when they are initialised for the first time:
- an explicit argument should_init: bool is passed to the constructors;
- the latter argument's value is decided upon the attempt to open the underlying dyn Database;
- for the test-database DatabaseSource::Custom the flag should_init is to be passed explicitely;
the StateDb stores the value of the PruningMode that was last to be successfully used;
the StateDb instead of the pruning_mode: PruningMode argument accepts requested_pruning_mode: Option<PruningMode>
- that argument's value reflects the --pruning CLI-argument closely;
- the actual pruning-mode that would be used is decided like that

// client/state-db/src/lib.rs: StateDb::open(...)
let (should_update_meta, selected_mode) = match (should_init, stored_mode, requested_mode) ...

client/db/Cargo.toml

client/db/src/state_db_meta.rs

client/db/src/lib.rs

client/cli/src/config.rs

client/cli/src/params/pruning_params.rs

client/state-db/src/lib.rs

…rate-8103

…-make-metadb-readonly-again make MetaDb read-only again!

bkchr

Ty!

Looks good besides some last nitpicks.

bkchr · 2022-05-05T10:18:11Z

client/service/src/builder.rs

+	if client.requires_full_sync() &&
+		!matches!(config.network.sync_mode, sc_network::config::SyncMode::Full)
+	{
+		return Err("The backend settings require the full-sync mode".into())


Suggested change

if client.requires_full_sync() &&

!matches!(config.network.sync_mode, sc_network::config::SyncMode::Full)

{

return Err("The backend settings require the full-sync mode".into())

if client.requires_full_sync() {

match config.network.sync_mode {

SyncMode::Fast { .. } => return Err("Fast sync doesn't work for archive nodes".into()),

SyncMode::Warp => return Err("Warp sync doesn't work for archive nodes".into()),

SyncMode::Full => {},

};

I think these error messages are little bit more clear.

Those wordings are definitely more clear, but they just happen to tell the truth with the current implementation of requires_full_sync(&self) -> bool.

Perhaps the "question" to the client should be formulated as is_archive_node(&self) -> bool in that case...

(I couldn't expose the exact PruningMode via any of the sc-client-api interfaces because I didn't see it fit to add another dependency to that crate: either sc-state-db or sc-client-db).

Those wordings are definitely more clear, but they just happen to tell the truth with the current implementation of requires_full_sync(&self) -> bool.

Perhaps the "question" to the client should be formulated as is_archive_node(&self) -> bool in that case...

Yes that is being correct. I mean we could change the error message to not contain for archive nodes and replace that with something like "when the backed requires full sync mode".

Or we expose the pruning mode to make it work like before.

client/state-db/src/lib.rs

…rate-8103

RGafiyatullin · 2022-05-06T10:07:35Z

bot merge

* Add BlockBackend::requires_full_sync() implementation * do not specify --pruning explicitly * update lockfile for {"substrate"} * Please re-run the CI * Please re-run the CI Co-authored-by: Roman Gafiyatullin <[email protected]> Co-authored-by: parity-processbot <>

* Substrate API change: paritytech/substrate#8103 * fix fallout of paritytech/polkadot#5454 * update lockfile for {"polkadot"} Co-authored-by: Bernhard Schuster <[email protected]> Co-authored-by: parity-processbot <>

* 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

* Substrate API change: paritytech/substrate#8103 * fix fallout of paritytech/polkadot#5454 * update lockfile for {"polkadot"} Co-authored-by: Bernhard Schuster <[email protected]> Co-authored-by: parity-processbot <>

* 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

RGafiyatullin added A0-please_review Pull request needs code review. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Mar 8, 2022

RGafiyatullin requested review from andresilva, bkchr, davxy and wigy-opensource-developer March 8, 2022 15:23

RGafiyatullin marked this pull request as ready for review March 8, 2022 20:47

davxy approved these changes Mar 9, 2022

View reviewed changes

wigy-opensource-developer previously approved these changes Mar 9, 2022

View reviewed changes

bkchr suggested changes Mar 9, 2022

View reviewed changes

RGafiyatullin marked this pull request as draft March 28, 2022 12:23

bkchr reviewed Mar 29, 2022

View reviewed changes

client/db/src/blockchain_db.rs Outdated Show resolved Hide resolved

RGafiyatullin force-pushed the rgafiyatullin-substrate-8103 branch from 8bbb0fa to 6435c39 Compare April 3, 2022 20:31

RGafiyatullin commented Apr 4, 2022

View reviewed changes

client/service/src/builder.rs Outdated Show resolved Hide resolved

RGafiyatullin requested review from bkchr, davxy and wigy-opensource-developer April 4, 2022 18:27

RGafiyatullin marked this pull request as ready for review April 4, 2022 21:42

This comment was marked as resolved.

Sign in to view

RGafiyatullin added the D2-breaksapi label Apr 5, 2022

RGafiyatullin mentioned this pull request Apr 11, 2022

A companion PR to the https://github.com/paritytech/substrate/pull/10995 paritytech/cumulus#1162

Merged

bkchr reviewed Apr 25, 2022

View reviewed changes

the-right-joyce removed the C1-low PR touches the given topic and has a low impact on builders. label Apr 26, 2022

RGafiyatullin force-pushed the rgafiyatullin-substrate-8103 branch from 46f8fa1 to 126dbac Compare April 26, 2022 13:57

RGafiyatullin added the C1-low PR touches the given topic and has a low impact on builders. label Apr 26, 2022

RGafiyatullin mentioned this pull request May 4, 2022

A companion PR to the https://github.com/paritytech/substrate/pull/10995 paritytech/polkadot#5446

Merged

Merge remote-tracking branch 'origin/master' into rgafiyatullin-subst…

443c10f

…rate-8103

RGafiyatullin force-pushed the rgafiyatullin-substrate-8103 branch from b3a885a to 443c10f Compare May 4, 2022 12:25

parity-db: 0.3.11 -> 0.3.12

3dfe1b9

RGafiyatullin requested a review from bkchr May 5, 2022 08:59

RGafiyatullin and others added 4 commits May 5, 2022 12:05

sc-state-db: MetaDb::set_meta — a better doc-test

44c5792

cargo fmt

847d97f

make MetaDb read-only again!

82320a2

Merge pull request #1 from RGafiyatullin/rgafiyatullin-substrate-8103…

e90167e

…-make-metadb-readonly-again make MetaDb read-only again!

arkpar approved these changes May 5, 2022

View reviewed changes

bkchr approved these changes May 5, 2022

View reviewed changes

Remove the stray newline (and run the CI once again please)

df30449

RGafiyatullin force-pushed the rgafiyatullin-substrate-8103 branch from 008e97b to df30449 Compare May 5, 2022 11:08

RGafiyatullin added 2 commits May 5, 2022 14:22

Last nitpicks

fd1623d

A more comprehensive error message

b6fe998

RGafiyatullin force-pushed the rgafiyatullin-substrate-8103 branch from c402bfb to 8425aac Compare May 5, 2022 15:33

Merge remote-tracking branch 'origin/master' into rgafiyatullin-subst…

8e0bc33

…rate-8103

RGafiyatullin force-pushed the rgafiyatullin-substrate-8103 branch from 8425aac to 8e0bc33 Compare May 6, 2022 09:23

paritytech-processbot bot merged commit 43d8c87 into paritytech:master May 6, 2022

nazar-pc mentioned this pull request May 19, 2022

--validator doesn't imply --pruning archive anymore #11475

Closed

2 tasks

github-actions bot mentioned this pull request May 31, 2022

Update substrate/polkadot/cumulus from v0.9.20 to v0.9.22 moonbeam-foundation/moonbeam#1557

Closed

github-actions bot mentioned this pull request Jun 22, 2022

Update substrate/polkadot/cumulus from v0.9.20 to v0.9.23 moonbeam-foundation/moonbeam#1623

Closed

Dengjianping mentioned this pull request Jul 7, 2022

Bump deps to v0.9.22 Manta-Network/Manta#571

Merged

10 tasks

-	if client.requires_full_sync() &&
-		!matches!(config.network.sync_mode, sc_network::config::SyncMode::Full)
-	{
-		return Err("The backend settings require the full-sync mode".into())
+	if client.requires_full_sync() {
+		match config.network.sync_mode {
+			SyncMode::Fast { .. } => return Err("Fast sync doesn't work for archive nodes".into()),
+			SyncMode::Warp => return Err("Warp sync doesn't work for archive nodes".into()),
+			SyncMode::Full => {},
+		};

Conversation

RGafiyatullin commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davxy left a comment

Choose a reason for hiding this comment

Uh oh!

wigy-opensource-developer left a comment

Choose a reason for hiding this comment

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RGafiyatullin commented Apr 4, 2022

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

bkchr May 5, 2022

Choose a reason for hiding this comment

Uh oh!

RGafiyatullin May 5, 2022

Choose a reason for hiding this comment

Uh oh!

bkchr May 5, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RGafiyatullin commented May 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

RGafiyatullin commented Mar 8, 2022 •

edited

Loading