Add chain data pruning experimental feature by wcgcyx · Pull Request #4686 · besu-eth/besu

wcgcyx · 2022-11-16T04:21:20Z

PR description

This PR introduces a new feature that allows besu to prune old chain data. Three experimental CLI options are introduced as well: --Xchain-pruning-enabled to turn on/off pruning, --Xchain-pruning-blocks-retained to specify the number of recent blocks to retain and --Xchain-pruning-frequency to specify how often to perform chain data pruning.

Fixed Issue(s)

Linked to #4476

Documentation

I thought about documentation and added the doc-change-required label to this PR if
updates are required.

Changelog

I thought about the changelog and included a changelog update if required.

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com>

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com>

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

jframe · 2022-11-23T06:34:11Z

    // TODO: post-merge cleanup, this should be unnecessary after merge
-    if (!mergeCoordinator.latestValidAncestorDescendsFromTerminal(newHead.get())) {
+    if (!mergeCoordinator.latestValidAncestorDescendsFromTerminal(newHead.get())
+        && !ChainDataPruner.isPruningEnabled()) {


Would be nicer to drive this based on the chain data pruning enabled flag rather than setting and using a static global field

Yea, I tried that a bit but it seems like that would involve bringing besu command into the protocol context. I consider it to be a temporary solution until #4703 is merged. Then, we could just do a cleanup to get rid of this global field completely.

We shouldn't need to bring the entire Besu command into the protocol context. At most, the chain pruning config values would need to be passed through. A compromise could be to make the ChainPruner a singleton instead.

Actually, It seems cleaner to add a flag in merge context instead, following what @gfukushima did in #4735

updated to have a temporary flag in protocol context instead. Global variable gets removed.

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com>

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com>

jframe · 2022-12-05T06:40:20Z

+        .isChainDataPruningEnabled(unstableChainDataPruningOptions.getChainDataPruningEnabled())
+        .chainDataPruningBlocksRetained(
+            unstableChainDataPruningOptions.getChainDataPruningBlocksRetained())
+        .chainDataPruningFrequency(
+            unstableChainDataPruningOptions.getChainDataPruningBlocksFrequency());


Can domain object be created instead so that 3 values don't need to passed through

jframe · 2022-12-05T06:44:24Z

+    this.blocksToRetain = blocksToRetain;
+    this.pruningFrequency = pruningFrequency;
+    this.pruningExecutor =
+        new ThreadPoolExecutor(


Can this instead use the MonitoredExecutors.newFixedThreadPool so we have monitoring

Updated to use monitored executors.

jframe · 2022-12-05T06:44:48Z

+    this.blocksToRetain = blocksToRetain;
+    this.pruningFrequency = pruningFrequency;
+    this.pruningExecutor =
+        new ThreadPoolExecutor(


Rather the executor was passed in so that this isn't created in the constructor

jframe · 2022-12-05T06:46:43Z

+          if (blockNumber < pruningMark) {
+            // Ignore and warn if block number < pruning mark, this normally indicates the
+            // blocksToRetain
+            // is too small.


nit: comment could be moved to previous line // is too small.

Updated, I've removed unnecessary comments. This comment goes into the logging to show the user that the blocks to retain are too small (probably misconfiguration).

jframe · 2022-12-05T07:04:14Z

+
+  // TODO: cleanup - isChainPruningEnabled will not be required after
+  // https://github.com/hyperledger/besu/pull/4703 is merged.
+  public void setIsChainPruningEnabled(final boolean isChainPruningEnabled) {


Don't think this is a good place for the chain pruning enabled flag, it is very high level. Maybe part of the Merge context would be ok, as we only need this check for the Merge RPCs.

Updated. Ended up placing a flag in merge context since it involves very little change.

Co-authored-by: Simon Dudley <simon.l.dudley@hotmail.com> Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com>

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

siladu · 2022-12-08T07:28:03Z

+              + " which normally indicates the blocksToRetain is too small");
+      return;
+    }
+    final KeyValueStorageTransaction tx1 = prunerStorage.startTransaction();


nit: do we need tx1 and tx2, any reason the same variable can't be reused?

If we do need two, then I think a more descriptive name would help readability

I was trying to use one variable actually but tx must be final in the lambda expression so couldn't be reused. Is there a way to get around that?

Gotcha, you could probably work around it with a class instead of lambda but it's not worth it for this. Renaming would be an improvement IMO but I wouldn't block the PR on it :)

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

siladu

Approved! Just the tx1, tx2 renaming suggestion but not a blocker for me

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com>

jframe · 2022-12-09T06:28:15Z

 - Increase the speed of modexp gas execution and execution. [#4780](https://github.com/hyperledger/besu/pull/4780)
 - Added experimental CLI options `--Xeth-capability-max` and `--Xeth-capability-min` to specify a range of capabilities to be supported by the Eth protocol. [#4752](https://github.com/hyperledger/besu/pull/4752)
 - Set the default curve in the EVMTool, like is done in production operations [#4790](https://github.com/hyperledger/besu/pull/4790)
+- Add chain data pruning feature with three experimental CLI options: `--Xchain-data-pruning-enabled`, `--Xchain-data-pruning-blocks-retained` and `--Xchain-data-pruning-frequency` [#4686](https://github.com/hyperledger/besu/pull/4686)


nit: remove data from description based previous naming conversation

jframe · 2022-12-09T06:28:31Z

+
+import picocli.CommandLine;
+
+public class ChainDataPruningOptions implements CLIOptions<ChainPrunerConfiguration> {


rename to ChainPruningOPtions

jframe · 2022-12-09T06:33:14Z

    validateContext(protocolContext);

+    if (chainPrunerConfiguration.getChainPruningEnabled()) {
+      protocolContext.getConsensusContext(MergeContext.class).setIsChainPruningEnabled(true);


If we are not post-merge e.g. using QBFT then the merge context will not be available. You can use the ProtocolContext.safeConsensusContext to handle this case.

jframe · 2022-12-09T06:38:34Z

  implementation 'org.apache.tuweni:tuweni-rlp'
  implementation 'org.hyperledger.besu:bls12-381'
  implementation 'org.immutables:value-annotations'
+  implementation 'org.awaitility:awaitility'


This should only for testImplementation not implementation it's only needed for tests

yep, that's true, updated.

jframe · 2022-12-09T07:01:17Z

+          final KeyValueStorageTransaction pruningTransaction = prunerStorage.startTransaction();
+          long currentPruningMark = storedPruningMark;
+          final long newPruningMark = blockNumber - blocksToRetain;
+          if (event.isNewCanonicalHead()


I think this check for pruning frequency could be done before submitting it to the executor so that less work is done in case nothing needs to happen.

I need to think about this one.

jframe · 2022-12-09T07:11:57Z

+    this.chainPruningBlocksRetained = chainPruningBlocksRetained;
+    this.chainPruningBlocksFrequency = chainPruningBlocksFrequency;
+  }
+


nit: chainPruning prefix could removed from the getters and fields are this is all on the ChainPrunerConfiguration class

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

* Add chain pruner * Increase minimum blocks to retain * Skip ancestor check in pruning mode * Separate class for pruning storage * Move pruning to separate thread * Limit total pruning threads Signed-off-by: wcgcyx <wcgcyx@gmail.com> Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com> Co-authored-by: Simon Dudley <simon.l.dudley@hotmail.com> Co-authored-by: Jason Frame <jason.frame@consensys.net> Signed-off-by: Karim TAAM <karim.t2am@gmail.com>

* Add chain pruner * Increase minimum blocks to retain * Skip ancestor check in pruning mode * Separate class for pruning storage * Move pruning to separate thread * Limit total pruning threads Signed-off-by: wcgcyx <wcgcyx@gmail.com> Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com> Co-authored-by: Simon Dudley <simon.l.dudley@hotmail.com> Co-authored-by: Jason Frame <jason.frame@consensys.net> Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

* Add chain pruner * Increase minimum blocks to retain * Skip ancestor check in pruning mode * Separate class for pruning storage * Move pruning to separate thread * Limit total pruning threads Signed-off-by: wcgcyx <wcgcyx@gmail.com> Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com> Co-authored-by: Simon Dudley <simon.l.dudley@hotmail.com> Co-authored-by: Jason Frame <jason.frame@consensys.net> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

* Add chain pruner * Increase minimum blocks to retain * Skip ancestor check in pruning mode * Separate class for pruning storage * Move pruning to separate thread * Limit total pruning threads Signed-off-by: wcgcyx <wcgcyx@gmail.com> Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com> Co-authored-by: Simon Dudley <simon.l.dudley@hotmail.com> Co-authored-by: Jason Frame <jason.frame@consensys.net>

wcgcyx force-pushed the 4476-chain-data-pruning branch from 04f5c3b to fff4909 Compare November 16, 2022 08:38

wcgcyx added 6 commits November 18, 2022 15:00

Add chain pruner

fce33c1

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Add tests and options

be8f081

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Add pruning frequency

af59579

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Fix unit tests

16de0e9

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Fix chain pruning tests

3e67042

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Increase minimum blocks to retain

0d433a3

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

wcgcyx force-pushed the 4476-chain-data-pruning branch from 1fd4fa6 to 0d433a3 Compare November 18, 2022 05:00

wcgcyx added 4 commits November 21, 2022 16:39

Skip ancestor check in pruning mode

0e545c5

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Merge branch 'main' into 4476-chain-data-pruning

4e0e627

Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com>

Insert changelog entry & fix build issue

9313b02

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Fix unit test

3e3596b

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

wcgcyx marked this pull request as ready for review November 21, 2022 08:47

Merge branch 'main' into 4476-chain-data-pruning

d0807ff

Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com>

usmansaleem reviewed Nov 21, 2022

View reviewed changes

Comment thread CHANGELOG.md Outdated

usmansaleem reviewed Nov 21, 2022

View reviewed changes

Comment thread besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java Outdated

usmansaleem reviewed Nov 21, 2022

View reviewed changes

Comment thread besu/src/main/java/org/hyperledger/besu/cli/options/unstable/ChainDataPruningOptions.java Outdated

usmansaleem reviewed Nov 21, 2022

View reviewed changes

Comment thread besu/src/main/java/org/hyperledger/besu/controller/BesuControllerBuilder.java Outdated

usmansaleem reviewed Nov 21, 2022

View reviewed changes

Comment thread ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/ChainDataPruner.java Outdated

usmansaleem reviewed Nov 21, 2022

View reviewed changes

Comment thread .../src/main/java/org/hyperledger/besu/ethereum/storage/keyvalue/KeyValueSegmentIdentifier.java

Fix changelog & address reviews

4c1d792

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

wcgcyx requested a review from usmansaleem November 22, 2022 03:54

jframe reviewed Nov 23, 2022

View reviewed changes

wcgcyx added 3 commits November 24, 2022 13:03

Fix minor issues & code cleanup

95e86a5

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Separate class for pruning storage

a63542a

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Merge branch 'main' into 4476-chain-data-pruning

580942a

Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com>

siladu reviewed Nov 25, 2022

View reviewed changes

Comment thread besu/src/main/java/org/hyperledger/besu/cli/options/unstable/ChainDataPruningOptions.java Outdated

siladu reviewed Nov 25, 2022

View reviewed changes

Comment thread besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java

wcgcyx added 2 commits November 25, 2022 15:48

Rename options

328005b

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Merge branch 'main' into 4476-chain-data-pruning

c34154f

Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com>

jframe reviewed Dec 5, 2022

View reviewed changes

wcgcyx and others added 9 commits December 6, 2022 12:42

Update besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java

cc4648e

Co-authored-by: Simon Dudley <simon.l.dudley@hotmail.com> Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com>

Spotless & Remove unnecessary comments

8315393

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

More clear variable

5992b66

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Clear logging

5d4da4e

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Fix bug & use monitored executors

118178e

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

New domain object

91102a7

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Fix tests

962adb6

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Move flag to merge context

1368c55

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Merge branch 'main' into 4476-chain-data-pruning

d41661b

wcgcyx requested review from jframe and siladu December 8, 2022 01:38

siladu reviewed Dec 8, 2022

View reviewed changes

Renaming

2a439f1

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

wcgcyx force-pushed the 4476-chain-data-pruning branch from 88d6743 to 2a439f1 Compare December 9, 2022 02:44

siladu approved these changes Dec 9, 2022

View reviewed changes

wcgcyx added 2 commits December 9, 2022 15:37

Rename tx

d29a286

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

Merge branch 'main' into 4476-chain-data-pruning

e2c088c

Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com>

jframe reviewed Dec 9, 2022

View reviewed changes

Fix issues

25d514d

Signed-off-by: wcgcyx <wcgcyx@gmail.com>

jframe approved these changes Dec 12, 2022

View reviewed changes

Merge branch 'main' into 4476-chain-data-pruning

571a022

macfarla merged commit 97588ae into besu-eth:main Dec 12, 2022

garyschulte mentioned this pull request Feb 24, 2023

propose Simon Dudley protocolguild/documentation#48

Merged

non-fungible-nelson mentioned this pull request Aug 21, 2023

Bonsai & Blockchain Storage Improvements #5387

Closed

7 tasks

garyschulte mentioned this pull request Aug 31, 2023

Rocksdb Error when Downgrading from Besu 23.7.1 to 22.10.2 #5808

Closed


		import picocli.CommandLine;

		public class ChainDataPruningOptions implements CLIOptions<ChainPrunerConfiguration> {

Uh oh!

Conversation

wcgcyx commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR description

Fixed Issue(s)

Documentation

Changelog

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

siladu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

wcgcyx commented Nov 16, 2022 •

edited

Loading