Manual block file pruning. #7871

mrbandrews · 2016-04-13T14:29:08Z

Implements #7365.

Now there is auto-prune and manual-prune. The user enables manual pruning on the command line with prune=1 and then uses an RPC command to prune: "pruneblockchain X" to prune up to height X.

Updated the python test (pruning.py).

jonasschnelli · 2016-04-13T14:30:47Z

Nice!
Concept ACK.

instagibbs · 2016-04-13T20:13:07Z

What use cases are motivating this mode?

laanwj · 2016-04-15T06:28:17Z

Concept ACK

What use cases are motivating this mode?

This is explained in the original issue, #7365: if you have another application that needs to consume the block data before it's gone.

instagibbs · 2016-04-15T11:12:59Z

Should have read the issue, sorry. Concept ACK

sipa · 2016-06-02T14:30:26Z

Concept ACK. Needs rebase.

mrbandrews · 2016-06-08T18:26:04Z

Rebased.

rebroad · 2016-08-24T10:16:02Z

src/main.cpp

+/* This function is called from the RPC code for pruneblockchain */
+void PruneBlockFilesManual(int manualPruneHeight)
+{
+    // stuff the prune height into a global variable, and flush


I usually get told off for using global variables. Curious to know when they are permitted and when they are discouraged.

sipa · 2016-08-25T11:58:50Z

Needs rebase.

luke-jr · 2016-10-04T21:47:57Z

src/init.cpp

@@ -1335,7 +1338,7 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)

                // Check for changed -prune state.  What we are concerned about is a user who has pruned blocks
                // in the past, but is now trying to run unpruned.
-                if (fHavePruned && !fPruneMode) {
+                if (fHavePruned && pruneMode==PRUNE_NONE) {


Many places, you left (pruneMode) in a boolean context. I don't see a need to change to == NONE here.

luke-jr · 2016-10-04T21:48:29Z

src/main.cpp

@@ -71,13 +71,14 @@ bool fImporting = false;
 bool fReindex = false;
 bool fTxIndex = false;
 bool fHavePruned = false;
-bool fPruneMode = false;
+PruneMode pruneMode = PRUNE_NONE;


A bit ugly to have the enum and variable differ only by case.

I think it's fine.

luke-jr · 2016-10-04T21:50:32Z

src/main.cpp

 bool fIsBareMultisigStd = DEFAULT_PERMIT_BAREMULTISIG;
 bool fRequireStandard = true;
 bool fCheckBlockIndex = false;
 bool fCheckpointsEnabled = DEFAULT_CHECKPOINTS_ENABLED;
 size_t nCoinCacheUsage = 5000 * 300;
 uint64_t nPruneTarget = 0;
+unsigned int nManualPruneHeight = 0;


This appears to be used exclusively for passing a value from a caller to a callee, so it shouldn't be a global variable.

luke-jr · 2016-10-04T21:59:41Z

src/main.cpp

@@ -3842,10 +3843,44 @@ void UnlinkPrunedFiles(std::set<int>& setFilesToPrune)
    }
 }

+/* This function is called from the RPC code for pruneblockchain */
+void PruneBlockFilesManual(int manualPruneHeight)


int is the wrong type here. It is only guaranteed to hold up to 32768, which isn't very useful for block heights.

Lock heights will break before 29 bits are exceeded, so I suggest using either unsigned long or uint32_t

luke-jr · 2016-10-04T22:08:35Z

src/main.cpp

 {
    LOCK2(cs_main, cs_LastBlockFile);
+    const CChainParams& chainparams = Params();
+    uint64_t nPruneAfterHeight = chainparams.PruneAfterHeight();


We're trying to reduce calls to global Params() by passing things as arguments. This change has the opposite effect for no clear reason.

luke-jr · 2016-10-04T22:11:45Z

src/rpc/blockchain.cpp

+
+    int height = params[0].get_int();
+    if (height < 0) {
+        throw JSONRPCError(RPC_INTERNAL_ERROR, "Negative block height.");


RPC_INVALID_PARAMETER seems more appropriate here.

luke-jr · 2016-10-04T22:12:41Z

src/rpc/blockchain.cpp

+        throw JSONRPCError(RPC_INTERNAL_ERROR, "Negative block height.");
+    } else if ((unsigned int) chainActive.Height() < Params().PruneAfterHeight()) {
+        throw JSONRPCError(RPC_INTERNAL_ERROR, "Blockchain is too short for pruning.");
+    }


Should this also check if the requested height is too close to the tip? Or maybe just return the height it was able to successfully prune up to...

luke-jr · 2016-10-04T22:14:16Z

src/rpc/blockchain.cpp

+            "1. \"height\"       (int, required) The block height to prune up to.\n");
+
+    if (pruneMode != PRUNE_MANUAL) {
+        throw JSONRPCError(RPC_INTERNAL_ERROR, "Cannot prune via RPC unless in manual prune mode.");


RPC_METHOD_NOT_FOUND seems better for this (already used for wallet RPCs when the wallet is not enabled)

luke-jr · 2016-10-04T22:18:46Z

Oh, it might make sense to add "prunemode": "auto"/"manual" to getblockchaininfo also.

petertodd · 2016-10-13T23:28:31Z

Concept ACK

My OpenTimestamps Server could use this!

mrbandrews · 2016-10-26T18:33:54Z

Rebased and feedback addressed. I didn't make a couple of the suggested changes, though:

"int" for block height is used elsewhere in the code (chain.h/chain.cpp); perhaps a future PR could change the type used in each place.
Params() - this was just a code move, although I had left one unnecessary line of code in there, which I removed now.
heights close to tip - it will prune up to the limit and log.

laanwj · 2016-11-21T08:20:29Z

src/init.cpp

@@ -928,12 +928,15 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
        return InitError(_("Prune cannot be configured with a negative value."));
    }
    nPruneTarget = (uint64_t) nSignedPruneTarget;
-    if (nPruneTarget) {
+    if (nPruneTarget / 1024 / 1024 == 1) {  // manual pruning: -prune=1
+        LogPrintf("Manual block pruning enabled.  Use RPC call pruneblockchain=<height> to prune block and undo files.\n");


Nit: Use RPC call pruneblockchain(height) - otherwise the syntax can be easily confused with a command line option.

laanwj · 2016-11-21T08:21:25Z

src/main.h

@@ -191,8 +191,10 @@ static const uint64_t nMinDiskSpace = 52428800;
 /** Pruning-related variables and constants */
 /** True if any block files have ever been pruned. */
 extern bool fHavePruned;
-/** True if we're running in -prune mode. */
-extern bool fPruneMode;
+enum PruneMode {PRUNE_NONE = 0, PRUNE_AUTO, PRUNE_MANUAL };


Let's use C++11 scoped enums in new code

enum struct PruneMode { NONE=0, AUTO, MANUAL };

Then refer to PruneMode::NONE etc.

laanwj · 2016-11-21T08:25:42Z

src/init.cpp

@@ -928,12 +928,15 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
        return InitError(_("Prune cannot be configured with a negative value."));
    }
    nPruneTarget = (uint64_t) nSignedPruneTarget;
-    if (nPruneTarget) {
+    if (nPruneTarget / 1024 / 1024 == 1) {  // manual pruning: -prune=1


Can we please store and compare the option value before multiplication? Doing a division here, though it achieves the correct effect, seems circuitous and unclear.

laanwj · 2016-11-21T08:27:06Z

src/main.cpp

@@ -3317,7 +3320,7 @@ bool FindBlockPos(CValidationState &state, CDiskBlockPos &pos, unsigned int nAdd
        unsigned int nOldChunks = (pos.nPos + BLOCKFILE_CHUNK_SIZE - 1) / BLOCKFILE_CHUNK_SIZE;
        unsigned int nNewChunks = (vinfoBlockFile[nFile].nSize + BLOCKFILE_CHUNK_SIZE - 1) / BLOCKFILE_CHUNK_SIZE;
        if (nNewChunks > nOldChunks) {
-            if (fPruneMode)
+            if (pruneMode == PRUNE_AUTO)


Are you sure this only needs to be done in auto-pruning mode?

laanwj · 2016-11-21T08:29:01Z

src/wallet/rpcdump.cpp

@@ -115,7 +115,7 @@ UniValue importprivkey(const JSONRPCRequest& request)
    if (request.params.size() > 2)
        fRescan = request.params[2].get_bool();

-    if (fRescan && fPruneMode)
+    if (fRescan && pruneMode)


As you already have to change every line on which the variable occurs anyhow, I'd prefer explicitly comparing the enumeration against a value instead of using it like a boolean: e.g. pruneMode != PruneMode::NONE. This makes it clear to developers reading the code that this is an enumeration and not a boolean and can avoid them introducing silly bugs.

laanwj · 2016-11-21T08:31:56Z

src/main.cpp

+void FindFilesToPruneManual(std::set<int>& setFilesToPrune, int nManualPruneHeight)
+{
+    LOCK2(cs_main, cs_LastBlockFile);
+    if (chainActive.Tip() == NULL || pruneMode != PRUNE_MANUAL) {


Calling this function with pruneMode != PRUNE_MANUAL should be an assertion error, as it must be a bug in the code.

laanwj · 2016-11-21T08:36:19Z

src/main.h

@@ -273,7 +280,8 @@ CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams);
 *
 * @param[out]   setFilesToPrune   The set of file indices that can be unlinked will be returned
 */
-void FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPruneAfterHeight);
+void FindFilesToPruneAuto(std::set<int>& setFilesToPrune);
+void FindFilesToPruneManual(std::set<int>& setFilesToPrune, int nManualPruneHeight);


Aside: these functions are only used within main.cpp, why are we exporting them?
(thinking about it, let's just keep it for now, it seems common in main.cpp to export everything whether necessary or not, and changing will probably interfere with attempts of splitting up main such as #9183)

There are many functions in main that are not exported (see the whole namespace block for the handling of NodeState, for example).

laanwj · 2016-11-21T08:39:42Z

src/rpc/blockchain.cpp

+
+    if (pruneMode != PRUNE_MANUAL) {
+        throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Cannot prune via RPC unless in manual prune mode.");
+        return 0;


No need for a return if you have a throw

laanwj · 2016-11-21T08:48:40Z

src/rpc/blockchain.cpp

+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Negative block height.");
+    } else if ((unsigned int) chainActive.Height() < Params().PruneAfterHeight()) {
+        throw JSONRPCError(RPC_INTERNAL_ERROR, "Blockchain is too short for pruning.");
+    }


Do we need any foot-shooting checks: e.g. that the last 144 blocks are being retained to be robust against reorgs?

Edit: apparently the check for MIN_BLOCKS_TO_KEEP happens deeper in the pruning logic, and it continues in this case by pruning the allowed blocks only. Ok, makes sense I think, although a warning in the log may make it more transparent what happens.

mrbandrews · 2016-11-21T16:31:42Z

Feedback addressed. Re: the code in FindBlockPos (which reset fCheckForPruning only in auto-prune mode), yes that should only be auto-prune. I designed manual pruning to be a one-time act of pruning to the specified height. (In looking at this code, though, I noticed that nManualPruneHeight should probably be set=0 after we do the manual pruning, so I added that line of code, in a separate commit.)

So, there's a commit responding to laanwj's feedback, and another with that one line of code.

mrbandrews · 2016-11-28T16:06:18Z

There was a merge conflict (the declaration of FlushStateToDisk near the top of main.cpp conflicted with my adding an optional parameter), so I rebased and squashed everything. This should work now.

sipa · 2016-11-28T18:49:58Z

src/main.cpp

@@ -4140,7 +4175,7 @@ bool CVerifyDB::VerifyDB(const CChainParams& chainparams, CCoinsView *coinsview,
        uiInterface.ShowProgress(_("Verifying blocks..."), percentageDone);
        if (pindex->nHeight < chainActive.Height()-nCheckDepth)
            break;
-        if (fPruneMode && !(pindex->nStatus & BLOCK_HAVE_DATA)) {
+        if (pruneMode != PruneMode::NONE && !(pindex->nStatus & BLOCK_HAVE_DATA)) {


FindFilesToPruneAuto is only called when pruneMode == PruneMode::AUTO, so why do we need this test? Maybe turn it into an assert at the beginning of the function.

Also, if (pruneMode) is equivalent to if (pruneMode != PruneMode::NONE), since PruneMode::NONE is explicitly defined as 0.

(applies to many changed lines in this PR)

sipa · 2016-11-28T18:51:22Z

src/main.h

@@ -273,7 +280,8 @@ CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams);
 *
 * @param[out]   setFilesToPrune   The set of file indices that can be unlinked will be returned
 */
-void FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPruneAfterHeight);
+void FindFilesToPruneAuto(std::set<int>& setFilesToPrune);
+void FindFilesToPruneManual(std::set<int>& setFilesToPrune, int nManualPruneHeight);


There are many functions in main that are not exported (see the whole namespace block for the handling of NodeState, for example).

sipa · 2016-11-28T18:55:40Z

src/main.h

@@ -191,8 +191,10 @@ static const uint64_t nMinDiskSpace = 52428800;
 /** Pruning-related variables and constants */
 /** True if any block files have ever been pruned. */
 extern bool fHavePruned;
-/** True if we're running in -prune mode. */
-extern bool fPruneMode;
+enum struct PruneMode {NONE=0, AUTO, MANUAL};


Meta question: why do we need a tristate here? I think we could allow manual pruning even when in 'auto` mode. In that case, manual-only pruning could be requested by setting the limit very high.

gmaxwell · 2017-01-08T03:40:51Z

utACK. Great feature!

gmaxwell · 2017-01-08T03:52:25Z

One question: I view this feature as the perfect compliment to importmulti, that lets you be sure you've imported all your keys before you prune. But importmulti takes its scanning argument as a timestamp, while this takes it's argument as a height. Should we also support a timestamp option here that uses the same criteria as import multi?

Edit: I think I will fix this by adding a height based option to importmulti.

Extend pruneblockchain RPC to accept block timestamps as well as block indices.

ryanofsky · 2017-01-10T22:32:53Z

Rebased for named arguments.

@gmaxwell, I extended pruneblockchain in afffeea to be able to take a timestamp instead of a block index. The code change is pretty small. The test change is bigger but straightforward.

laanwj · 2017-01-11T13:16:20Z

utACK afffeea

afffeea fixup! Add pruneblockchain RPC to enable manual block file pruning. (Russell Yanofsky) 1fc4ec7 Add pruneblockchain RPC to enable manual block file pruning. (mrbandrews)

jonasschnelli · 2017-01-11T13:30:03Z

src/rpc/blockchain.cpp

+    else if (height > chainHeight)
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Blockchain is shorter than the attempted prune height.");
+    else if (height > chainHeight - MIN_BLOCKS_TO_KEEP)
+        LogPrint("rpc", "Attempt to prune blocks close to the tip.  Retaining the minimum number of blocks.");


one post merge nit:
We should probably report the pruned height in case the user given value was overrode.

Done in #9518

Change suggested by Jonas Schnelli <dev@jonasschnelli.ch> in bitcoin#7871 (comment)

afffeea fixup! Add pruneblockchain RPC to enable manual block file pruning. (Russell Yanofsky) 1fc4ec7 Add pruneblockchain RPC to enable manual block file pruning. (mrbandrews)

Change suggested by Jonas Schnelli <dev@jonasschnelli.ch> in bitcoin#7871 (comment)

afffeea fixup! Add pruneblockchain RPC to enable manual block file pruning. (Russell Yanofsky) 1fc4ec7 Add pruneblockchain RPC to enable manual block file pruning. (mrbandrews)

@codablock

…dablock committed on Jan 20, 2018 Use version 2 blocks for miner_tests … @codablock codablock committed on Jan 20, 2018 Merge bitcoin#7871: Manual block file pruning. … @laanwj @codablock laanwj authored and codablock committed on Jan 11, 2017 Merge bitcoin#9507: Fix use-after-free in CTxMemPool::removeConflicts() … @sipa @codablock sipa authored and codablock committed on Jan 11, 2017 Merge bitcoin#9297: Various RPC help outputs updated … @MarcoFalke @codablock MarcoFalke authored and codablock committed on Jan 12, 2017 Merge bitcoin#9416: travis: make distdir before make … @MarcoFalke @codablock MarcoFalke authored and codablock committed on Jan 12, 2017 Merge bitcoin#9520: Deprecate non-txindex getrawtransaction and bette… … @MarcoFalke @codablock MarcoFalke authored and codablock committed on Jan 12, 2017 Merge bitcoin#9518: Return height of last block pruned by pruneblockc… … @MarcoFalke @codablock MarcoFalke authored and codablock committed on Jan 12, 2017 Merge bitcoin#9472: Disentangle progress estimation from checkpoints … … @laanwj @codablock laanwj authored and codablock committed on Jan 12, 2017 Merge bitcoin#8883: Add all standard TXO types to bitcoin-tx … @laanwj @codablock laanwj authored and codablock committed on Jan 12, 2017 Merge bitcoin#9261: Add unstored orphans with rejected parents to rec… … @laanwj @codablock laanwj authored and codablock committed on Jan 12, 2017 Merge bitcoin#9468: [Depends] Dependency updates for 0.14.0 … @laanwj @codablock laanwj authored and codablock committed on Jan 12, 2017 Merge bitcoin#9222: Add 'subtractFeeFromAmount' option to 'fundrawtra… … @laanwj @codablock laanwj authored and codablock committed on Jan 12, 2017 Merge bitcoin#9490: Replace FindLatestBefore used by importmuti with … … @sipa @codablock sipa authored and codablock committed on Jan 13, 2017 Merge bitcoin#9469: [depends] Qt 5.7.1 … @laanwj @codablock laanwj authored and codablock committed on Jan 15, 2017 Merge bitcoin#9380: Separate different uses of minimum fees … @laanwj @codablock laanwj authored and codablock committed on Jan 16, 2017 Remove SegWit related code in dash-tx @codablock codablock committed on Sep 21, 2017 Merge bitcoin#9561: Wake message handling thread when we receive a ne… … @sipa @codablock sipa authored and codablock committed on Jan 17, 2017 Merge bitcoin#9508: Remove unused Python imports … @MarcoFalke @codablock MarcoFalke authored and codablock committed on Jan 18, 2017 Merge bitcoin#9512: Fix various things -fsanitize complains about

afffeea fixup! Add pruneblockchain RPC to enable manual block file pruning. (Russell Yanofsky) 1fc4ec7 Add pruneblockchain RPC to enable manual block file pruning. (mrbandrews)

This reverts commit 56dc1ce.

jonasschnelli added the Block storage label Apr 13, 2016

mrbandrews force-pushed the ba-manual6 branch from 383bee1 to 2695b56 Compare June 8, 2016 18:25

rebroad reviewed Aug 24, 2016
View reviewed changes

mrbandrews force-pushed the ba-manual6 branch from 2695b56 to 518a942 Compare September 7, 2016 13:59

luke-jr suggested changes Oct 4, 2016

View reviewed changes

sdaftuar mentioned this pull request Oct 13, 2016

Updatable minimum unpruned block height for use w/ external databases #8917

Closed

mrbandrews force-pushed the ba-manual6 branch from 518a942 to 98470f9 Compare October 26, 2016 18:29

laanwj reviewed Nov 21, 2016

View reviewed changes

mrbandrews force-pushed the ba-manual6 branch from 601aaa4 to 342262d Compare November 28, 2016 16:02

sipa reviewed Nov 28, 2016

View reviewed changes

ryanofsky force-pushed the ba-manual6 branch from dde3364 to 80aa3dd Compare January 6, 2017 16:28

gmaxwell mentioned this pull request Jan 8, 2017

Replace FindLatestBefore used by importmuti with FindEarliestAtLeast. #9490

Merged

mrbandrews and others added 2 commits January 10, 2017 08:14

Add pruneblockchain RPC to enable manual block file pruning.

1fc4ec7

fixup! Add pruneblockchain RPC to enable manual block file pruning.

afffeea

Extend pruneblockchain RPC to accept block timestamps as well as block indices.

ryanofsky force-pushed the ba-manual6 branch 2 times, most recently from 168651a to afffeea Compare January 10, 2017 22:24

laanwj merged commit afffeea into bitcoin:master Jan 11, 2017

jonasschnelli reviewed Jan 11, 2017

View reviewed changes

laanwj mentioned this pull request Jan 11, 2017

TODO for release notes 0.14.0 #8455

Closed

18 tasks

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 11, 2017

Return height of last block pruned by pruneblockchain RPC

918d1fb

Change suggested by Jonas Schnelli <dev@jonasschnelli.ch> in bitcoin#7871 (comment)

ryanofsky mentioned this pull request Jan 11, 2017

Return height of last block pruned by pruneblockchain RPC #9518

Merged

achow101 mentioned this pull request Jan 15, 2017

Manual pruning via RPC pruneblock #7365

Closed

sdaftuar mentioned this pull request Jan 25, 2017

qa: Increase a sync_blocks timeout in pruning.py #9628

Merged

lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Jan 4, 2019

Return height of last block pruned by pruneblockchain RPC

349d33c

Change suggested by Jonas Schnelli <dev@jonasschnelli.ch> in bitcoin#7871 (comment)

CryptoCentric added a commit to absolute-community/absolute that referenced this pull request Mar 5, 2019

Revert "Merge bitcoin#7871: Manual block file pruning."

e961199

This reverts commit 56dc1ce.

sipa mentioned this pull request Apr 4, 2020

Fixed too many spaces in the BlockChain Log #18525

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Manual block file pruning. #7871

Manual block file pruning. #7871

Uh oh!

Conversation

mrbandrews commented Apr 13, 2016

Uh oh!

jonasschnelli commented Apr 13, 2016

Uh oh!

instagibbs commented Apr 13, 2016

Uh oh!

laanwj commented Apr 15, 2016

Uh oh!

instagibbs commented Apr 15, 2016

Uh oh!

sipa commented Jun 2, 2016

Uh oh!

mrbandrews commented Jun 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Aug 25, 2016

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!

luke-jr commented Oct 4, 2016

Uh oh!

petertodd commented Oct 13, 2016

Uh oh!

mrbandrews commented Oct 26, 2016

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!

laanwj Nov 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrbandrews commented Nov 21, 2016

Uh oh!

mrbandrews commented Nov 28, 2016

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!

laanwj Nov 21, 2016 •

edited

Loading

gmaxwell commented Jan 8, 2017 •

edited

Loading