Add -pruneduringinit option to temporarily use another prune target during IBD #31845
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31845. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
64e8e1a to
7a416d6
Compare
7a416d6 to
d612e26
Compare
There was a problem hiding this comment.
Before merging I would like to run an IBD, a reindex and an assumeUTXO on the branch.
My main concern currently (or rather the part I don't yet fully understand) is that if "during IBD there shouldn't be reorgs in the first place" (given a reliable block-header-first sync), why is this even configurable, why not always skip storage of blocks and undos completely during IBD (or at least reduce it to 6 blocks or something trivial) when prune is enabled (e.g. only enable it after the last 100 blocks or after assumevalid or something similar)?
Also note that #31144 is meant to optimize block writing speed considerably.
| @@ -508,6 +508,9 @@ void SetupServerArgs(ArgsManager& argsman) | |||
| argsman.AddArg("-prune=<n>", strprintf("Reduce storage requirements by enabling pruning (deleting) of old blocks. This allows the pruneblockchain RPC to be called to delete specific blocks and enables automatic pruning of old blocks if a target size in MiB is provided. This mode is incompatible with -txindex. " | |||
| "Warning: Reverting this setting requires re-downloading the entire blockchain. " | |||
| "(default: 0 = disable pruning blocks, 1 = allow manual pruning via RPC, >=%u = automatically prune block files to stay under the specified target size in MiB)", MIN_DISK_SPACE_FOR_BLOCK_FILES / 1024 / 1024), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); | |||
| argsman.AddArg("-pruneduringinit", "Temporarily adjusts the -prune setting until initial sync completes." | |||
There was a problem hiding this comment.
is this compatible with -fastprune? If so, we should cover the new feature with tests, separating IBD pruning from up-to-date pruning (maybe in feature_index_prune.py)
| @@ -24,6 +24,7 @@ struct BlockManagerOpts { | |||
| const CChainParams& chainparams; | |||
| bool use_xor{DEFAULT_XOR_BLOCKSDIR}; | |||
| uint64_t prune_target{0}; | |||
| int64_t prune_target_during_init{-1}; | |||
There was a problem hiding this comment.
how many block do we absolutely need (for reorgs, I guess) during IBD?
There was a problem hiding this comment.
idk, not sure why it matters
There was a problem hiding this comment.
To see how low we can go safely to speed up IBD
There was a problem hiding this comment.
That's backward. We need to go higher to speed up IBD. Lower slows it down.
There was a problem hiding this comment.
I meant that if we could avoid writing blocks (and especially undos, which likely don't serve any purpose during IBD as far as I can tell), we could speed up IBD measurably for pruned nodes.
If the reason for writing blocks is to avoid flushing to LevelDB, there may be better ways (as mentioned, #31645 + sorting speeds up dumping from e.g. 30 minutes to 7).
d612e26 to
2dfdff5
Compare
2dfdff5 to
d4a3abf
Compare
|
Can't we just do an IBD without pruning, then shutdown and restart with a prune height set? Or just call |
There was a problem hiding this comment.
Can't we just do an IBD without pruning
Disabling pruning is only possible on devices with sufficient disk space, but I agree that a simple restart seems like an easy enough workaround if benchmarks indicate that this a worthwhile performance improvement:
bitcoind --prune=4000 --stopatheight={current_tip} && bitcoind --prune=300
Approach NACK, I don't think this needs a separate startup option. If benchmarks agree, I think a PR that improves the --prune documentation would be worthwhile.
While I think I agree that this does not need a separate startup option, the current flushing behaviour does indeed feel backwards. That should just be independently addressed though. I also echo l0rincs intuition from before, why are we writing to disk at all? At least to the assume valid height, I can't think of any good reason to do so, and even beyond a target height calculated on the fly from the prune target and the best header, still seems acceptable. Maybe @sipa can help us understand the threshold tradeoffs here? |
Low prune sizes can force flushing long before dbcache is full, making a significant impact on real-world IBD time.
This new option allows a higher (or lower) prune size during IBD to mitigate this. When assumeutxo is active, the entire additional budget is allocated to the background chainstate.