Merge bitcoin/bitcoin#27039: blockstorage: do not flush block to disk if it is already there

dfcef536d0 blockstorage: do not flush block to disk if it is already there (Matthew Zipkin)

Pull request description:

  Closes https://github.com/bitcoin/bitcoin/issues/2039

  When reindexing from flat-file block storage there is no need to write anything back to disk, since the block data is already there. This PR skips flushing to disk those blocks that already have a known position in the datastore. Skipping this means that users can write-protect the `blk` files on disk which may be useful for security or even safely sharing that data between multiple bitcoind instances.

  `FindBlockPos()` may also flush the undo data file, but again this is skipped if the corresponding block position is known, like during the initial stage of a reindex when block data is being indexed. Once the block index is complete the validation mechanism will call `ConnectBlock()` which will save undo data at that time.

  The call stack looks like this:

  ```
  init()
  ThreadImport() <-- process fReindex flag
  LoadExternalBlockFile()
  AcceptBlock()
  SaveBlockToDisk()
  FindBlockPos()
  FlushBlockFile() <-- unnecessary if block is already on disk
  ```

  A larger refactor of this part of the code was started by mzumsande here:  https://github.com/mzumsande/bitcoin/tree/202207_refactor_findblockpos including this fix, reviewers can let me know if the changes should be combined.

ACKs for top commit:
  sipa:
    utACK dfcef536d0
  mzumsande:
    re-ACK dfcef536d0
  achow101:
    ACK dfcef536d0
  furszy:
    Rebase diff ACK dfcef53.

Tree-SHA512: 385c5ac1288b325135398d0ddd3ab788fa98cc0ca19bd2474c74039f2ce70d5088c1d1c9d4dd10aefcbd4c757767ec5805d07ba8cee9289a66f96e6f9eaa5279

src/node/blockstorage.cpp

@@ -906,19 +906,19 @@ bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigne
         if (!fKnown) {
             LogPrint(BCLog::BLOCKSTORAGE, "Leaving block file %i: %s (onto %i) (height %i)\n",
                 last_blockfile, m_blockfile_info[last_blockfile].ToString(), nFile, nHeight);
-        }
 
-        // Do not propagate the return code. The flush concerns a previous block
-        // and undo file that has already been written to. If a flush fails
-        // here, and we crash, there is no expected additional block data
-        // inconsistency arising from the flush failure here. However, the undo
-        // data may be inconsistent after a crash if the flush is called during
-        // a reindex. A flush error might also leave some of the data files
-        // untrimmed.
-        if (!FlushBlockFile(last_blockfile, !fKnown, finalize_undo)) {
-            LogPrintLevel(BCLog::BLOCKSTORAGE, BCLog::Level::Warning,
-                          "Failed to flush previous block file %05i (finalize=%i, finalize_undo=%i) before opening new block file %05i\n",
-                          last_blockfile, !fKnown, finalize_undo, nFile);
+            // Do not propagate the return code. The flush concerns a previous block
+            // and undo file that has already been written to. If a flush fails
+            // here, and we crash, there is no expected additional block data
+            // inconsistency arising from the flush failure here. However, the undo
+            // data may be inconsistent after a crash if the flush is called during
+            // a reindex. A flush error might also leave some of the data files
+            // untrimmed.
+            if (!FlushBlockFile(last_blockfile, !fKnown, finalize_undo)) {
+                LogPrintLevel(BCLog::BLOCKSTORAGE, BCLog::Level::Warning,
+                              "Failed to flush previous block file %05i (finalize=%i, finalize_undo=%i) before opening new block file %05i\n",
+                              last_blockfile, !fKnown, finalize_undo, nFile);
+            }
         }
         // No undo data yet in the new file, so reset our undo-height tracking.
         m_blockfile_cursors[chain_type] = BlockfileCursor{nFile};

test/functional/feature_reindex_readonly.py

@@ -24,6 +24,7 @@ class BlockstoreReindexTest(BitcoinTestFramework):
         opreturn = "6a"
         nulldata = fastprune_blockfile_size * "ff"
         self.generateblock(self.nodes[0], output=f"raw({opreturn}{nulldata})", transactions=[])
+        block_count = self.nodes[0].getblockcount()
         self.stop_node(0)
 
         assert (self.nodes[0].chain_path / "blocks" / "blk00000.dat").exists()
@@ -73,10 +74,10 @@ class BlockstoreReindexTest(BitcoinTestFramework):
                 pass
 
         if undo_immutable:
-            self.log.info("Attempt to restart and reindex the node with the unwritable block file")
-            with self.nodes[0].assert_debug_log(expected_msgs=['FlushStateToDisk', 'failed to open file'], unexpected_msgs=[]):
-                self.nodes[0].assert_start_raises_init_error(extra_args=['-reindex', '-fastprune'],
-                    expected_msg="Error: A fatal internal error occurred, see debug.log for details")
+            self.log.debug("Attempt to restart and reindex the node with the unwritable block file")
+            with self.nodes[0].wait_for_debug_log([b"Reindexing finished"]):
+                self.start_node(0, extra_args=['-reindex', '-fastprune'])
+            assert block_count == self.nodes[0].getblockcount()
             undo_immutable()
 
         filename.chmod(0o777)