Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Remove discarded blocks and states from database by default #11983

Conversation

hzy1919
Copy link
Contributor

@hzy1919 hzy1919 commented Aug 5, 2022

Fixes #11912

1.Add state_pruning's sub-param archive-canonical in sc-cli.
2.Change blocks_pruning to be like state_pruning : use the string archive for BlocksPruning::KeepAll and archive-canonical(and default) for BlocksPruning::KeepFinalized and then a number of BlocksPruning::Some(x).

polkadot companion: paritytech/polkadot#5992
cumulus companion: paritytech/cumulus#1613

Closes: #12094

@hzy1919 hzy1919 changed the title Remove discarded blocks and states from database Remove discarded blocks and states from database by default Aug 8, 2022
@hzy1919 hzy1919 marked this pull request as ready for review August 8, 2022 09:21
@hzy1919 hzy1919 marked this pull request as draft August 9, 2022 11:43
@@ -46,6 +46,7 @@ impl PruningParams {
.as_ref()
.map(|s| match s.as_str() {
"archive" => Ok(PruningMode::ArchiveAll),
"canonical" => Ok(PruningMode::ArchiveCanonical),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"canonical" is more of an internal concept, and not a user friendly term. Let's call it "final" or "finalized"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok , Let's set it "final".

@arkpar
Copy link
Member

arkpar commented Aug 9, 2022

There's no need to change the defult pruning mode. It should remain at 256 block history.

@arkpar
Copy link
Member

arkpar commented Aug 9, 2022

Regarding #11912, adding a CLI for ArchiveCanonical only fixes it half way. The major piece of work required here is to remove block bodies, and not just state. This part is a bit more involved.

@arkpar arkpar requested a review from bkchr August 9, 2022 17:49
@hzy1919
Copy link
Contributor Author

hzy1919 commented Aug 10, 2022

Regarding #11912, adding a CLI for ArchiveCanonical only fixes it half way. The major piece of work required here is to remove block bodies, and not just state. This part is a bit more involved.

Well , I'll rethink where to start...

@stale
Copy link

stale bot commented Sep 9, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 9, 2022
… Corresponds to `blocks_pruning 0` in CLI .

2.Change value `All` to `AllFinalized` in `enum BlocksPruning` and make it to keep full finalized block history.
@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 9, 2022
@hzy1919 hzy1919 marked this pull request as ready for review September 9, 2022 07:09
client/db/src/lib.rs Outdated Show resolved Hide resolved
client/db/src/lib.rs Outdated Show resolved Hide resolved
client/db/src/lib.rs Outdated Show resolved Hide resolved
@arkpar
Copy link
Member

arkpar commented Sep 9, 2022

Looks good with some minor comments. One addition that's also required here, is that prune_block function also deletes justification data.

		utils::remove_from_db(
			transaction,
			&*self.storage.db,
			columns::KEY_LOOKUP,
			columns::JUSTIFICATIONS,
			id,
		)?;

@hzy1919
Copy link
Contributor Author

hzy1919 commented Sep 10, 2022

Looks good with some minor comments. One addition that's also required here, is that prune_block function also deletes justification data.

Done.

Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Just got some question suggestion for the Cli and maybe using different names, but did not really have any good ideas.

@@ -32,7 +32,7 @@ pub struct PruningParams {
pub state_pruning: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc for this param should be updated with the new final option.

@@ -46,6 +46,7 @@ impl PruningParams {
.as_ref()
.map(|s| match s.as_str() {
"archive" => Ok(PruningMode::ArchiveAll),
"final" => Ok(PruningMode::ArchiveCanonical),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the doc there is also a "archive-canonical" mention that did not look linked, I would maybe use it instead of "final".
But tbh I did not find a good name. Since it is the new default it should be shorter or more attractive than 'archive', but 'final' is not really saying much to me.
If anybody got other ideas?
(ideally former 'archive' becomes 'archive-all' and 'final' wil be 'archive', but it breaks compatibility)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it should be called archive-canonical

@@ -32,7 +32,7 @@ pub struct PruningParams {
pub state_pruning: Option<String>,
/// Specify the number of finalized blocks to keep in the database.
///
/// Default is to keep all blocks.
/// Default is to keep all of finalized blocks.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we should add that 0 keep all blocks.
Generally this is not very intuitive.
Maybe we should do something like state_pruning: use the same string: 'archive' for 'keep_all' and 'final' fo 'keep_finalized'. (subject to change of name too).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm,,It's really not very intuitive that 0 keep all blocks. IMO it's a compromise approach now.
I agree that change blocks_pruning to be like state_pruning.

columns::KEY_LOOKUP,
columns::JUSTIFICATIONS,
id,
)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be worth mentioning in release note that justification are now pruned with block (additionally to the new parameters).
But looks like a good thing to me 👍

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looking good. However, there is no test.

client/db/src/lib.rs Outdated Show resolved Hide resolved
@@ -46,6 +46,7 @@ impl PruningParams {
.as_ref()
.map(|s| match s.as_str() {
"archive" => Ok(PruningMode::ArchiveAll),
"final" => Ok(PruningMode::ArchiveCanonical),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it should be called archive-canonical

@@ -57,8 +58,9 @@ impl PruningParams {
/// Get the block pruning value from the parameters
pub fn blocks_pruning(&self) -> error::Result<BlocksPruning> {
Ok(match self.blocks_pruning {
Some(0) => BlocksPruning::KeepAll,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay that is relative random. That this switches to KeepAll when you give it 0.

Maybe we should not expose KeepAll at all. Or we really need to introduce some new cli flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about that change blocks_pruning to be like state_pruning : use the string keep-all for BlocksPruning::KeepAll and keep-finalized(and default) for BlocksPruning::KeepFinalized ?
And IMO ,It may help to keep the style consistent with state_pruning and be better to be understood .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using same naming would be good IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using same naming would be good IMO.

That also be OK .

Let's change blocks_pruning to be like state_pruning : use the string archive for BlocksPruning::KeepAll and archive-canonical(and default) for BlocksPruning::KeepFinalized ? @bkchr @arkpar

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then a number of BlocksPruning::Some(x)?

Sounds good.

@hzy1919
Copy link
Contributor Author

hzy1919 commented Sep 14, 2022

Looks good. Could also use a companion PR to the polkadot repo. See here: https://github.com/paritytech/substrate/blob/master/docs/CONTRIBUTING.adoc#updating-polkadot-as-well

yeah, I added before. :D

client/cli/src/params/pruning_params.rs Outdated Show resolved Hide resolved
///
/// NOTE: only finalized blocks are subject for removal!
#[clap(alias = "keep-blocks", long, value_name = "COUNT")]
pub blocks_pruning: Option<u32>,
pub blocks_pruning: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to document the options available.

client/cli/src/params/pruning_params.rs Outdated Show resolved Hide resolved
/// Same function as `new_test_with_tx_storage`,just change the type of `blocks_pruning` to
/// `BlocksPruning`.
#[cfg(any(test, feature = "test-helpers"))]
pub fn new_test_with_tx_storage_2(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not just change new_test_with_tx_storage to take BlocksPruning by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm,,I think that will work too.

@bkchr
Copy link
Member

bkchr commented Sep 26, 2022

Ty @hzy1919

@bkchr
Copy link
Member

bkchr commented Sep 26, 2022

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/cumulus#1613

@bkchr
Copy link
Member

bkchr commented Sep 26, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit a0ec652 into paritytech:master Sep 26, 2022
@hzy1919
Copy link
Contributor Author

hzy1919 commented Sep 27, 2022

Ty @hzy1919

You're welcome. @bkchr

ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…ch#11983)

* 1.Add pruning param "canonical" in sc-cli.
2.Make PruningMode's default value to ArchiveCanonical.

* Update tests in sc-state-db.

* Update tests in sc-state-db.

* 1.Add a new value `AllWithNonFinalized` in `enum BlocksPruning` which Corresponds to `blocks_pruning 0` in CLI .
2.Change value `All` to `AllFinalized` in `enum BlocksPruning` and make it to keep full finalized block history.

* Make some corresponding adjustments based on the content in the conversation.

* Update client/db/src/lib.rs

Co-authored-by: Bastian Köcher <[email protected]>

* Apply suggestions from code review.

* 1.Change `blocks_pruning` to be like `state_pruning` .

* Fmt and add some doc.

* Update client/cli/src/params/pruning_params.rs

Co-authored-by: Bastian Köcher <[email protected]>

* Update client/cli/src/params/pruning_params.rs

Co-authored-by: Bastian Köcher <[email protected]>

* Update doc.

* Change `new_test_with_tx_storage` to take `BlocksPruning`.

* Fmt

Co-authored-by: Bastian Köcher <[email protected]>
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-39/2277/1

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
5 participants