Repository: bitcoin/bitcoin (language: cpp)

Issue:
-prune doesn't work during import
It appears that `-prune` is ignored during import (`-reindex`, or `-loadblock`).


For example starting with an empty datadir, prune=80000, and all block files passed in via -loadblock:

```
...
2021-12-23T12:49:44Z [loadblk] [node/blockstorage.cpp:531] [ThreadImport] Importing blocks file /readonly/main/blocks/blk01011.dat...
2021-12-23T12:49:44Z [loadblk] [node/blockstorage.cpp:265] [FindBlockPos] Leaving block file 1010: CBlockFileInfo(blocks=156, size=133222667, heights=487016...487360, time=2017-09-26...2017-09-28)
2021-12-23T12:49:45Z [loadblk] [flatfile.cpp:69] [Allocate] Pre-allocating up to position 0x1000000 in blk01011.dat
2021-12-23T12:49:45Z [loadblk] [logging/timer.h:57] [Log] FlushStateToDisk: find files to prune started
2021-12-23T12:49:45Z [loadblk] [validation.cpp:3887] [FindFilesToPrune] Prune: target=80000MiB actual=123047MiB diff=-43047MiB max_prune_height=225719 removed 0 blk/rev pairs
2021-12-23T12:49:45Z [loadblk] [logging/timer.h:57] [Log] FlushStateToDisk: find files to prune completed (0.05ms)
...
```

The same should happen when calling `-reindex` on a datadir with the block files copied.

So prune only works with IBD from the network?
-prune doesn't work during import
It appears that `-prune` is ignored during import (`-reindex`, or `-loadblock`).


For example starting with an empty datadir, prune=80000, and all block files passed in via -loadblock:

```
...
2021-12-23T12:49:44Z [loadblk] [node/blockstorage.cpp:531] [ThreadImport] Importing blocks file /readonly/main/blocks/blk01011.dat...
2021-12-23T12:49:44Z [loadblk] [node/blockstorage.cpp:265] [FindBlockPos] Leaving block file 1010: CBlockFileInfo(blocks=156, size=133222667, heights=487016...487360, time=2017-09-26...2017-09-28)
2021-12-23T12:49:45Z [loadblk] [flatfile.cpp:69] [Allocate] Pre-allocating up to position 0x1000000 in blk01011.dat
2021-12-23T12:49:45Z [loadblk] [logging/timer.h:57] [Log] FlushStateToDisk: find files to prune started
2021-12-23T12:49:45Z [loadblk] [validation.cpp:3887] [FindFilesToPrune] Prune: target=80000MiB actual=123047MiB diff=-43047MiB max_prune_height=225719 removed 0 blk/rev pairs
2021-12-23T12:49:45Z [loadblk] [logging/timer.h:57] [Log] FlushStateToDisk: find files to prune completed (0.05ms)
...
```

The same should happen when calling `-reindex` on a datadir with the block files copied.

So prune only works with IBD from the network?


Discussion / hints:
As a workaround it is possible to start a node with the datadir on one hard drive and another one with the datadir on the other hard drive using `-connect=localhost` to import the blocks
@MarcoFalke I was just looking at this because I've been looking at issues related to re-indexing. Please re-open this issue if you agree with my thoughts below.

1. I think that `-prune=N` plus `-loadblock=path` could be made to work. That seems to make sense. An example use case is like what you were doing in your issue report above, where you have a clean set of block files and you want to create a pruned node from them, but you don't want to change the input set of block files.

1. I think that `-prune=N` plus `-reindex` makes potentially less sense together and should not be made to work.
    1. Enabling this combo could be dangerous by being destructive of the input blk files. If a problem occurs during re-indexing, then this combo may increase the chance of having to re-download lots of block data. If we leave them as a non-effective combo, then many re-indexing failures can be overcome by simply staring over with the original blk files still intact.
    1. A case where this combo may not make sense has to do with out of order blocks in blk files. The `-reindex` process can process blocks out of order. We wouldn't want to prune a blk file that contains a block that was encountered out of order because when an out of order block is processed during re-indexing, its location is saved for a later re-read from the blk file after its parent is processed. If we pruned the blk file containing it, then we'd lose it and all of its ancestor blocks. We'd have to re-download them because the re-index process would consider all of its ancestors as being encountered out of order and not connectable. Therefore, we would not prune those blk files and thus the `-prune=N` would be a potentially meaningless suggestion (depending on how out of order a block was in a set of input block files) rather than a limit set by the node operator that will be generally adhered too. A worst case would have the last/best block inserted after genesis in the first blk file (dumb, but possible). No pruning would be possible until after everything is re-indexed.
As a workaround it is possible to start a node with the datadir on one hard drive and another one with the datadir on the other hard drive using `-connect=localhost` to import the blocks
@MarcoFalke I was just looking at this because I've been looking at issues related to re-indexing. Please re-open this issue if you agree with my thoughts below.

1. I think that `-prune=N` plus `-loadblock=path` could be made to work. That seems to make sense. An example use case is like what you were doing in your issue report above, where you have a clean set of block files and you want to create a pruned node from them, but you don't want to change the input set of block files.

1. I think that `-prune=N` plus `-reindex` makes potentially less sense together and should not be made to work.
    1. Enabling this combo could be dangerous by being destructive of the input blk files. If a problem occurs during re-indexing, then this combo may increase the chance of having to re-download lots of block data. If we leave them as a non-effective combo, then many re-indexing failures can be overcome by simply staring over with the original blk files still intact.
    1. A case where this combo may not make sense has to do with out of order blocks in blk files. The `-reindex` process can process blocks out of order. We wouldn't want to prune a blk file that contains a block that was encountered out of order because when an out of order block is processed during re-indexing, its location is saved for a later re-read from the blk file after its parent is processed. If we pruned the blk file containing it, then we'd lose it and all of its ancestor blocks. We'd have to re-download them because the re-index process would consider all of its ancestors as being encountered out of order and not connectable. Therefore, we would not prune those blk files and thus the `-prune=N` would be a potentially meaningless suggestion (depending on how out of order a block was in a set of input block files) rather than a limit set by the node operator that will be generally adhered too. A worst case would have the last/best block inserted after genesis in the first blk file (dumb, but possible). No pruning would be possible until after everything is re-indexed.

Fix patch:
diff --git a/src/validation.cpp b/src/validation.cpp
index c839647b29533..3714335b39869 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4474,6 +4474,9 @@ void Chainstate::LoadExternalBlockFile(
                 // next block, but it's still possible to rewind to the start of the current block (without a disk read).
                 nRewind = nBlockPos + nSize;
                 blkdat.SkipTo(nRewind);
+
+                std::shared_ptr<CBlock> pblock{}; // needs to remain available after the cs_main lock is released to avoid duplicate reads from disk
+
                 {
                     LOCK(cs_main);
                     // detect out of order blocks, and store them for later
@@ -4491,7 +4494,7 @@ void Chainstate::LoadExternalBlockFile(
                     if (!pindex || (pindex->nStatus & BLOCK_HAVE_DATA) == 0) {
                         // This block can be processed immediately; rewind to its start, read and deserialize it.
                         blkdat.SetPos(nBlockPos);
-                        std::shared_ptr<CBlock> pblock{std::make_shared<CBlock>()};
+                        pblock = std::make_shared<CBlock>();
                         blkdat >> *pblock;
                         nRewind = blkdat.GetPos();
 
@@ -4515,6 +4518,21 @@ void Chainstate::LoadExternalBlockFile(
                     }
                 }
 
+                if (m_blockman.IsPruneMode() && !fReindex && pblock) {
+                    // must update the tip for pruning to work while importing with -loadblock.
+                    // this is a tradeoff to conserve disk space at the expense of time
+                    // spent updating the tip to be able to prune.
+                    // otherwise, ActivateBestChain won't be called by the import process
+                    // until after all of the block files are loaded. ActivateBestChain can be
+                    // called by concurrent network message processing. but, that is not
+                    // reliable for the purpose of pruning while importing.
+                    BlockValidationState state;
+                    if (!ActivateBestChain(state, pblock)) {
+                        LogPrint(BCLog::REINDEX, "failed to activate chain (%s)\n", state.ToString());
+                        break;
+                    }
+                }
+
                 NotifyHeaderTip(*this);
 
                 if (!blocks_with_unknown_parent) continue;


Test patch: