Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Manual block file pruning. #7871

Merged
merged 2 commits into from Jan 11, 2017
Merged

Manual block file pruning. #7871

merged 2 commits into from Jan 11, 2017

Conversation

@mrbandrews
Copy link
Contributor

@mrbandrews mrbandrews commented Apr 13, 2016

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
Copy link
Member

@jonasschnelli jonasschnelli commented Apr 13, 2016

Nice!
Concept ACK.

@instagibbs
Copy link
Member

@instagibbs instagibbs commented Apr 13, 2016

What use cases are motivating this mode?

@laanwj
Copy link
Member

@laanwj laanwj commented Apr 15, 2016

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
Copy link
Member

@instagibbs instagibbs commented Apr 15, 2016

Should have read the issue, sorry. Concept ACK

@sipa
Copy link
Member

@sipa sipa commented Jun 2, 2016

Concept ACK. Needs rebase.

@mrbandrews mrbandrews force-pushed the mrbandrews:ba-manual6 branch Jun 8, 2016
@mrbandrews
Copy link
Contributor Author

@mrbandrews mrbandrews commented Jun 8, 2016

Rebased.

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

This comment has been minimized.

@rebroad

rebroad Aug 24, 2016
Contributor

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

@sipa
Copy link
Member

@sipa sipa commented Aug 25, 2016

Needs rebase.

@mrbandrews mrbandrews force-pushed the mrbandrews:ba-manual6 branch Sep 7, 2016
@@ -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) {

This comment has been minimized.

@luke-jr

luke-jr Oct 4, 2016
Member

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

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

This comment has been minimized.

@luke-jr

luke-jr Oct 4, 2016
Member

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

This comment has been minimized.

@laanwj

laanwj Nov 21, 2016
Member

I think it's fine.

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 comment has been minimized.

@luke-jr

luke-jr Oct 4, 2016
Member

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

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

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

This comment has been minimized.

@luke-jr

luke-jr Oct 4, 2016
Member

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

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

This comment has been minimized.

@luke-jr

luke-jr Oct 4, 2016
Member

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


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

This comment has been minimized.

@luke-jr

luke-jr Oct 4, 2016
Member

RPC_INVALID_PARAMETER seems more appropriate here.

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.");
}

This comment has been minimized.

@luke-jr

luke-jr Oct 4, 2016
Member

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...

"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.");

This comment has been minimized.

@luke-jr

luke-jr Oct 4, 2016
Member

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

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Oct 4, 2016

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

@petertodd
Copy link
Contributor

@petertodd petertodd commented Oct 13, 2016

Concept ACK

My OpenTimestamps Server could use this!

@mrbandrews mrbandrews force-pushed the mrbandrews:ba-manual6 branch Oct 26, 2016
@mrbandrews
Copy link
Contributor Author

@mrbandrews mrbandrews commented Oct 26, 2016

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

  1. "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.
  2. Params() - this was just a code move, although I had left one unnecessary line of code in there, which I removed now.
  3. heights close to tip - it will prune up to the limit and log.

@@ -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");

This comment has been minimized.

@laanwj

laanwj Nov 21, 2016
Member

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

@@ -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 };

This comment has been minimized.

@laanwj

laanwj Nov 21, 2016
Member

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

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

Then refer to PruneMode::NONE etc.

@@ -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

This comment has been minimized.

@laanwj

laanwj Nov 21, 2016
Member

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.

@@ -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)

This comment has been minimized.

@laanwj

laanwj Nov 21, 2016
Member

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

@@ -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)

This comment has been minimized.

@laanwj

laanwj Nov 21, 2016
Member

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.

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

This comment has been minimized.

@laanwj

laanwj Nov 21, 2016
Member

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

@@ -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);

This comment has been minimized.

@laanwj

laanwj Nov 21, 2016
Member

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)

This comment has been minimized.

@sipa

sipa Nov 28, 2016
Member

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


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

This comment has been minimized.

@laanwj

laanwj Nov 21, 2016
Member

No need for a return if you have a throw

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.");
}

This comment has been minimized.

@laanwj

laanwj Nov 21, 2016
Member

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
Copy link
Contributor Author

@mrbandrews mrbandrews commented Nov 21, 2016

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 mrbandrews force-pushed the mrbandrews:ba-manual6 branch Nov 28, 2016
@mrbandrews
Copy link
Contributor Author

@mrbandrews mrbandrews commented Nov 28, 2016

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.

@@ -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)) {

This comment has been minimized.

@sipa

sipa Nov 28, 2016
Member

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.

This comment has been minimized.

@sipa

sipa Nov 28, 2016
Member

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)

@@ -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);

This comment has been minimized.

@sipa

sipa Nov 28, 2016
Member

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

@@ -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};

This comment has been minimized.

@sipa

sipa Nov 28, 2016
Member

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.

@ryanofsky ryanofsky force-pushed the mrbandrews:ba-manual6 branch from dde3364 to 80aa3dd Jan 6, 2017
@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Jan 8, 2017

utACK. Great feature!

@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Jan 8, 2017

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.

mrbandrews and others added 2 commits Nov 29, 2016
@ryanofsky ryanofsky force-pushed the mrbandrews:ba-manual6 branch 2 times, most recently from 168651a to afffeea Jan 10, 2017
@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Jan 10, 2017

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
Copy link
Member

@laanwj laanwj commented Jan 11, 2017

utACK afffeea

@laanwj laanwj merged commit afffeea into bitcoin:master Jan 11, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Jan 11, 2017
afffeea fixup! Add pruneblockchain RPC to enable manual block file pruning. (Russell Yanofsky)
1fc4ec7 Add pruneblockchain RPC to enable manual block file pruning. (mrbandrews)
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.");

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 11, 2017
Member

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

This comment has been minimized.

@ryanofsky

ryanofsky Jan 11, 2017
Contributor

Done in #9518

@laanwj laanwj mentioned this pull request Jan 11, 2017
16 of 18 tasks
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 11, 2017
Change suggested by Jonas Schnelli <dev@jonasschnelli.ch> in
bitcoin#7871 (comment)
codablock added a commit to codablock/dash that referenced this pull request Jan 21, 2018
afffeea fixup! Add pruneblockchain RPC to enable manual block file pruning. (Russell Yanofsky)
1fc4ec7 Add pruneblockchain RPC to enable manual block file pruning. (mrbandrews)
lateminer added a commit to lateminer/bitcoin that referenced this pull request Jan 4, 2019
Change suggested by Jonas Schnelli <dev@jonasschnelli.ch> in
bitcoin#7871 (comment)
andvgal added a commit to energicryptocurrency/energi that referenced this pull request Jan 6, 2019
afffeea fixup! Add pruneblockchain RPC to enable manual block file pruning. (Russell Yanofsky)
1fc4ec7 Add pruneblockchain RPC to enable manual block file pruning. (mrbandrews)
CryptoCentric added a commit to absolute-community/absolute that referenced this pull request Feb 27, 2019
afffeea fixup! Add pruneblockchain RPC to enable manual block file pruning. (Russell Yanofsky)
1fc4ec7 Add pruneblockchain RPC to enable manual block file pruning. (mrbandrews)
CryptoCentric added a commit to absolute-community/absolute that referenced this pull request Feb 27, 2019
…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
CryptoCentric added a commit to absolute-community/absolute that referenced this pull request Mar 5, 2019
afffeea fixup! Add pruneblockchain RPC to enable manual block file pruning. (Russell Yanofsky)
1fc4ec7 Add pruneblockchain RPC to enable manual block file pruning. (mrbandrews)
CryptoCentric added a commit to absolute-community/absolute that referenced this pull request Mar 5, 2019
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet