Add -pruneduringinit option to temporarily use another prune target during IBD #31845

In many places in the codebase we use bit shifts for Kib/MiB/GiB, consider using them here as well:

    const uint64_t nPruneTarget{uint64_t(nPruneArg) << 20};

It's clearer using * 1024 IMO

I know the extra space was there before, but it might be time to fix it now that we're touching it:

        return util::Error{strprintf(_("%s configured below the minimum of %d MiB. Please use a higher number."), opt_name, MIN_DISK_SPACE_FOR_BLOCK_FILES / 1024 / 1024)};

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)

how many block do we absolutely need (for reorgs, I guess) during IBD?

idk, not sure why it matters

To see how low we can go safely to speed up IBD

That's backward. We need to go higher to speed up IBD. Lower slows it down.

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

can we clarify what disabling this feature means exactly? Can we rather add a default value for minimal blocks kept during IBD instead of disabling it by default?

It's to disable pruning during IBD.

I'm not seeing a use case to set this lower than -prune (we already prune extra during IBD when we prune), only higher (to avoid flushing).

Could you please rebase the build to make sure the latest tests are passing? It's ~7 months old and the build system is complaining when switching branches.

Merge it into master for testing. Rebasing for no reason is a bad practice.

It's a new PR, why base it on an old state? I ran the test locally and they're passing, but rebasing the PR assures that CI also runs against the latest changes.

Does unused mean that the optional is never empty because we've just checked IsArgSet? Given that it's an optional, we could include it directly in the condition instead:

    if (auto prune_during_init{args.GetIntArg("-pruneduringinit")}) {
        const auto pdi_parsed{ParsePruneOption(*prune_during_init, "-pruneduringinit")};

to avoid non-standard abbreviations such as pdi - and given that above we call the parsed prune option prune_parsed -, we could rather name this as prune_init_parsed.

MIN_DISK_SPACE_FOR_BLOCK_FILES docs also mention block and undo data - should we add orphan blocks as well in the description here?

There's no such thing as orphan blocks. Stale blocks are implicitly included in the more general "blocks".

Anyway, this is just a move.

Looks like we should update MIN_DISK_SPACE_FOR_BLOCK_FILES in that case in a separate PR.

To obviate that prune_parsed has a very narrow scope here we could encapsulate it into an if/else:

    if (auto prune_parsed{ParsePruneOption(nPruneArg, "Prune")}) {
        opts.prune_target = *prune_parsed;
    } else {
        return util::Error{ErrorString(prune_parsed)};
    }

Previously the target calculations were trivial, now it might be ripe for an extraction to unburden FindFilesToPrune (making this sub-feature unit-testable as well):

uint64_t BlockManager::CalculateTarget(const ChainstateRole role, ChainstateManager& chainman) const
{
    uint64_t target{GetPruneTarget()};
    uint64_t target_boost{0};
    if (m_opts.prune_target_during_init > -1 && chainman.IsInitialBlockDownload()) {
        if (uint64_t(m_opts.prune_target_during_init) <= target) {
            target = m_opts.prune_target_during_init;
        } else if (role != ChainstateRole::ASSUMEDVALID) {
            // Only the background/normal gets the benefit
            // NOTE: This assumes only one such chainstate exists
            target_boost = m_opts.prune_target_during_init - target;
        }
    }
    // Distribute our -prune budget over all chainstates.
    const auto number_of_chainstates{chainman.GetAll().size()};
    target = (target / number_of_chainstates) + target_boost;
    return std::max(MIN_DISK_SPACE_FOR_BLOCK_FILES, target);
}

with

const uint64_t target{CalculateTarget(chain.GetRole(), chainman)};

Do we really need the same standards for IBD block retention and normal flow? The comment states 288 blocks (and only 1 mb per block for some pre-SegWit reason) which seems absurd during IBD, doesn't it?

Outside the scope of this PR

How so? This is basically my main concern: why are we storing blocks during IBD at all, when pruned? Or if they're needed for some reason, do we really need to store 288 blocks throughout the IBD as well?

I think the scope of this PR is to reduce chainstate writes, which occur when we need to prune blocks. Pruning blocks means we can't reindex them and would have to redownload, so we write the chainstate then to be safe.

If we were to not store blocks, we would have to write the chainstate after indexing every block to avoid potentially redownloading them.

Thanks for the details, @andrewtoth. So basically having a low pruning target during IBD (preferably 0) with high dbcache (preferably max) would be the fastest possible option (especially after #31645), right? @luke-jr, it would be great if we could lower the IBD block writing threshold further (especially since the mentioned 288-block limit already seems incorrect - I'll run an IBD to measure it exactly).