validation: Ignore eventual error message from flushing in AcceptBlock #35621

Shortcut: see #35570 (comment) and #29700 (review) .

Summary. At the end of ChainstateManager::AcceptBlock, the flushing of the chain state is initiated, but any potential failure from FlushStateToDisk is ignored. However, the error message might get propagated upwards. This change makes sure that both the error status and the error message are ignored, and adds an explanatory comment.

Related/background. The potential inconsistency between the return value and state returned from AcceptBlock was found during refactoring of the return of BlockValidationState in #35570. PR #29700 also touches this part by adding an explanatory comment, see #29700 (review) . Splitting this minor behavior change out of #35570 makes that PR a clean (no-bahavior-change) refactor (this option was mentioned from the start, and also proposed by reviewers; see: #35570 (comment) ).

Motivation

• Get rid of the potential inconsistency between the return value (true) and state returned (Error). This facilitates refactoring of the error returns in #35570.
• Make the ignoring clear and explicit, so there is no doubt about it being intentional or not.

Details. ChainstateManager::AcceptBlock is called for new block candidates. After checks and inclusion, it triggers flushing of the updated chain state to disk, by calling Chainstate::FlushStateToDisk. This method returns a bool and also takes & returns a BlockValidationState state. In case of error it returns false, and sets the state (to Error) and the reject reason string. At the call site, the return value is ignored, but for the state the state variable of AcceptBlock is provided, so an Error state may get propagated upwards in the call chain. A caller may react to this Error state.

Rationale for ignoring the flush error The validation code flushes internally in several places, and mostly doesn't treat flush failures as errors returned to callers. Disk errors should never be mistreated as block validation failure. Note that the fatal error notification inside FlushStateToDisk still fires, so the node will shut down on unrecoverable flush errors regardless.

Relevant use case. AcceptBlock is called from the following methods (excluding test code):

• ChainstateManager::ProcessNewBlock
• Twice from ChainstateManager::LoadExternalBlockFile

Of these, in two places the state returned is not used at all. Only at validation.cpp:5056 is the state used.

            BlockValidationState state;
            if (AcceptBlock(pblock, state, nullptr, true, dbp, nullptr, true)) {
                nLoaded++;
            }
            if (state.IsError()) {
                break;
            }

Here a flush error may trigger the break, causing an early exit from the loop over blocks in a block file. Post-change the returned state will not be Error in a disk error case, so the break will not be hit here. However:

• This branch is only called for blocks that are not known yet
• In case of disk error case the fatal error handler will cause the node to shut down in any case.

Based on the above, the change in error return behavior is acceptable.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35621.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #35646 (RFC: Separate out runtime errors from BlockValidationState using util::Expected by yuvicc)
• #29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Concept ACK

Code review ACK 256482ab566360bd9d9ae0cb9c16e1a85c2de5ac

I'm not sure about the approach here. Are there cases where it is legitimate that we use the passed out BlockValidationState from FlushStateToDisk? If not, could we just remove it as an argument there?

Are there cases where it is legitimate that we use the passed out BlockValidationState from FlushStateToDisk?

Valid question, but the answer is yes, there are. Here are some call chains where an error from FlushStateToDisk can make a difference:

• ActivateBestChain -> ActivateBestChainStep -> ConnectTip -> FlushStateToDisk
• ActivateBestChain -> ActivateBestChainStep -> DisconnectTip -> FlushStateToDisk
• ActivateBestChain -> FlushStateToDisk
• InvalidateBlock -> InvalidateBlock -> DisconnectTip -> FlushStateToDisk

(ActivateBestChain may be called by PreciousBlock, ProcessNewBlock, LoadExternalBlockFile, InvalidateBlock, ReconsiderBlock.)

ACK 256482ab566360bd9d9ae0cb9c16e1a85c2de5ac

Here a flush error may trigger the break, causing an early exit from the loop over blocks in a block file. Post-change the returned state will not be Error in a disk error case, so the break will not be hit here. However:

* This branch is only called for blocks that are not known yet (so not in case of reindex)

* In case of disk error case the fatal error handler will cause the node to shut down in any case.

I am not sure this is fully correct.

A -reindex discards everything, including the block tree, no block is known, including the genesis block. Moreover, the initload thread will attempt to activate the genesis block, which init.cpp blocks on:

src/init.cpp:2075:     * Wait for genesis block to be processed.

So what happens depends on whether the genesis block resulted in a flush error or not:

• (1) If the genesis block resulted in a flush error (maybe quota issues in the block dir?), then the program will fully deadlock, and would have to be killed with SIGKILL on current master.

The reason for the deadlock is the break you mention. When the break is hit, it will obviously skip activating the genesis block in initload.

• (2) If another block resulted in a flush error, the init.cpp main thread was already unlocked, and the fatal flush error would trigger the break and then the interrupt on current master.

Now with this pull request:

• (1) The deadlock is fixed, because the break is ignored, and the genesis block is activated.
• (2) As you say, the break will not be hit, but the interrupt will result in an equivalent early return (without log of the result)

So my recommendation would be to correct the pull description and drop the word not (before in case of reindex).

Moreover, I wonder what the point of the break is, when it is sometimes ignored anyway. Moreover, the break is completely untested code, before and after this pull request.

Idk, but given that reviewers will have to understand what the break means here and that it is possibly useless, it could make sense to just remove it here?

lgtm ACK 256482ab566360bd9d9ae0cb9c16e1a85c2de5ac

<!-- diff --git a/src/validation.cpp b/src/validation.cpp index 87cf646b8b..2ad3967e50 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4400,0 +4401 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, + @@ -5051,0 +5053,3 @@ void ChainstateManager::LoadExternalBlockFile( + pindex = m_blockman.LookupBlockIndex(hash); + LogDebug(BCLog::REINDEX, "Block Import: block %s at height %d\n", hash.ToString(), pindex->nHeight); + if(pindex->nHeight==1)FatalError(GetNotifications(), state, _("Corrupt AAAAAAAAAAAAAAAAAAA.")); @@ -5053 +5057 @@ void ChainstateManager::LoadExternalBlockFile( - break; + //break; @@ -5055 +5059 @@ void ChainstateManager::LoadExternalBlockFile( - } else if (hash != params.GetConsensus().hashGenesisBlock && pindex->nHeight % 1000 == 0) { + } else { diff --git a/test/functional/feature_reindex_readonly.py b/test/functional/feature_reindex_readonly.py index 889d0c2a0e..92281104de 100755 --- a/test/functional/feature_reindex_readonly.py +++ b/test/functional/feature_reindex_readonly.py @@ -2,6 +1,0 @@ -# Copyright (c) 2023-present The Bitcoin Core developers -# Distributed under the MIT software license, see the accompanying -# file COPYING or http://www.opensource.org/licenses/mit-license.php. -"""Test running bitcoind with -reindex from a read-only blockstore -- Start a node, generate blocks, then restart with -reindex after setting blk files to read-only -""" @@ -22 +16,3 @@ class BlockstoreReindexTest(BitcoinTestFramework): - def reindex_readonly(self): + def reindex(self): + self.generateblock(self.nodes[0], output=f"raw(55)", transactions=[]) + self.generateblock(self.nodes[0], output=f"raw(55)", transactions=[]) @@ -34,51 +30,4 @@ class BlockstoreReindexTest(BitcoinTestFramework): - self.log.debug("Make the first block file read-only") - filename = self.nodes[0].chain_path / "blocks" / "blk00000.dat" - filename.chmod(stat.S_IREAD) - - undo_immutable = lambda: None - # Linux - try: - subprocess.run(['chattr'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) - try: - subprocess.run(['chattr', '+i', filename], capture_output=True, check=True) - undo_immutable = lambda: subprocess.check_call(['chattr', '-i', filename]) - self.log.info("Made file immutable with chattr") - except subprocess.CalledProcessError as e: - self.log.warning(str(e)) - if e.stdout: - self.log.warning(f"stdout: {e.stdout}") - if e.stderr: - self.log.warning(f"stderr: {e.stderr}") - if os.getuid() == 0: - self.log.warning("Return early on Linux under root, because chattr failed.") - self.log.warning("This should only happen due to missing capabilities in a container.") - self.log.warning("Make sure to --cap-add LINUX_IMMUTABLE if you want to run this test.") - undo_immutable = False - except Exception: - # macOS, and *BSD - try: - subprocess.run(['chflags'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) - try: - subprocess.run(['chflags', 'uchg', filename], capture_output=True, check=True) - undo_immutable = lambda: subprocess.check_call(['chflags', 'nouchg', filename]) - self.log.info("Made file immutable with chflags") - except subprocess.CalledProcessError as e: - self.log.warning(str(e)) - if e.stdout: - self.log.warning(f"stdout: {e.stdout}") - if e.stderr: - self.log.warning(f"stderr: {e.stderr}") - if os.getuid() == 0: - self.log.warning("Return early on BSD under root, because chflags failed.") - undo_immutable = False - except Exception: - pass - - if undo_immutable: - self.log.debug("Attempt to restart and reindex the node with the unwritable block file") - with self.nodes[0].assert_debug_log(["Reindexing finished"], timeout=60): - self.start_node(0, extra_args=['-reindex', '-fastprune']) - assert_equal(block_count, self.nodes[0].getblockcount()) - undo_immutable() - - filename.chmod(0o777) + self.log.debug("Attempt to restart and reindex the node") + with self.nodes[0].assert_debug_log(["Reindexing finished"], timeout=60): + self.start_node(0, extra_args=['-reindex', '-fastprune', ]) + assert_equal(block_count, self.nodes[0].getblockcount()) @@ -87 +36 @@ class BlockstoreReindexTest(BitcoinTestFramework): - self.reindex_readonly() + self.reindex()

Idk, but given that reviewers will have to understand what the break means here and that it is possibly useless, it could make sense to just remove it here?

off-topic: [trigger-warning]: To expand on this, the commit message in commit 78100ea27d076e9c76f444364dfd8fbaf719fefc by Claude is ai slop, because it claims that Error refers to block validation errors, but a quick glance at src/consensus/validation.h reveals that runtime errors are tracked separate from validation errors.

The break in question can still be hit, as AcceptBlock can return false+Error, when WriteBlock fails (called before FlushStateToDisk). For this reason, I'm reluctant to remove the break in this PR. But i agree that it is untested.

For my statement on reindex: I may be wrong there. I based this on tests with starting bitcoind with -reindex, and my observation was that the if (!pindex || (pindex->nStatus & BLOCK_HAVE_DATA) == 0) condition was not satisfied. I had to change the code (force pindex to nullptr) to force it into the loop and observe what happens when FlushStateToDisk/AcceptBlock returns a forced Error. I'm not sure of the semantics of this if (my guess was that this is active when loading a block from an external file that is already present in the current chain).

As I have no strong grounds, I have removed the "(so not in case of reindex)" mention from the PR description.

Concept ACK

re #35621 (comment) and #35621 (comment)

I think it will be better if we accompany this with a test?

For my statement on reindex: I may be wrong there. I based this on tests with starting bitcoind with -reindex, and my observation was that the if (!pindex || (pindex->nStatus & BLOCK_HAVE_DATA) == 0) condition was not satisfied.

Are you sure? Locally I can trivially trigger this. Also, refer to this diff:

diff --git a/src/validation.cpp b/src/validation.cpp
index 87cf646b8b..31c93dbc43 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -5041,3 +5042,4 @@ void ChainstateManager::LoadExternalBlockFile(
                     if (!pindex || (pindex->nStatus & BLOCK_HAVE_DATA) == 0) {
-                        // This block can be processed immediately; rewind to its start, read and deserialize it.
+                        std::cout << "// This block can be processed immediately; rewind to its start, read and deserialize it." << std::endl;
+                        assert(false);
                         blkdat.SetPos(nBlockPos);

Which yields:

$ ./bld-cmake/bin/bitcoin-qt -datadir=/tmp/regtest -regtest -reindex 
// This block can be processed immediately; rewind to its start, read and deserialize it.
bitcoin-qt: validation.cpp:5044: void ChainstateManager::LoadExternalBlockFile(AutoFile &, FlatFilePos *, std::multimap<uint256, FlatFilePos> *): Assertion `false' failed.

Of course if there are no block files, then there is nothing to reindex. Did you make sure that block files exist, before you try to reindex?

passing a dummy state means the API does not express the real intent (@ismaelsadeeq)

On a related note, among the methods returning BlockValidationState there are some that may return validation errors (Invalid, but not Error), some that may return runtime errors (Error, but not Invalid), and some that may return either error as well. It would be nice if this was reflected/enforced in the types used as well. ValidationState is based on the three-state ModeState enum (Valid, Invalid, Error). Having structs for validation-only and runtime-error-only would be cleaner. This would also be handy with the util::Expected approach, suggested in #35570 (https://github.com/bitcoin/bitcoin/pull/35570#issuecomment-4872809113).

Are you sure? Locally I can trivially trigger this. Also, refer to this diff:

Retested, and you are right, I'm observing what you described, not my previous observation (probably I managed to end up with some invalid state, after repeatedly trying to force errors).

I also tried with a forced error from FlushStateToDisk (patch below), and the break was triggered. If I removed the break, the next break, after ActivateBestChain was hit, so the loop still exited after the first failing block.

<details> <summary>diff</summary>

diff --git a/src/validation.cpp b/src/validation.cpp
index 5341c604c4..7468e7d71a 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2719,6 +2719,8 @@ bool Chainstate::FlushStateToDisk(
 
     try {
     {
+        return FatalError(m_chainman.GetNotifications(), state, strprintf(_("FORCED ERROR")));
+
         bool fFlushForPrune = false;
 
         CoinsCacheSizeState cache_state = GetCoinsCacheSizeState();
@@ -5057,7 +5059,8 @@ void ChainstateManager::LoadExternalBlockFile(
                             nLoaded++;
                         }
                         if (state.IsError()) {
-                            break;
+                            std::cout << "BREAK1\n";
+                            //break;
                         }
                     } else if (hash != params.GetConsensus().hashGenesisBlock && pindex->nHeight % 1000 == 0) {
                         LogDebug(BCLog::REINDEX, "Block Import: already had block %s at height %d\n", hash.ToString(), pindex->nHeight);
@@ -5074,6 +5077,7 @@ void ChainstateManager::LoadExternalBlockFile(
                 if (hash == params.GetConsensus().hashGenesisBlock && WITH_LOCK(::cs_main, return ActiveHeight()) == -1) {
                     BlockValidationState state;
                     if (!ActiveChainstate().ActivateBestChain(state, nullptr)) {
+                        std::cout << "BREAK2\n";
                         break;
                     }
                 }
@@ -5088,6 +5092,7 @@ void ChainstateManager::LoadExternalBlockFile(
                     // reliable for the purpose of pruning while importing.
                     if (auto result{ActivateBestChains()}; !result) {
                         LogDebug(BCLog::REINDEX, "%s\n", util::ErrorString(result).original);
+                        std::cout << "BREAK3\n";
                         break;
                     }
                 }

</details>

re #35621 (comment)

I looked closely at the described deadlock at init's genesis wait, and it is still reachable after this fix lthough not through the flush error anymore.

The "import stops before genesis activation while init waits for genesis" deadlock doesn't need the state.IsError() break at all the time. During -reindex, it can be triggered by any shutdown request that lands after the main thread parks in the genesis wait and before the scan activates genesis: a Ctrl+C in that window, or a fatal error that results in an interrupt makes the scan return at first or next check. The window is normally milliseconds (genesis is the first entry of blk00000.dat), but it grows arbitrarily large if the file begins with blocks whose parents aren't known yet, since those pass through the out-of-order path without activating anything.

This occur because during -reindex, the block index and chainstate are wiped, and LoadGenesisBlock() at startup is skipped because it's gated on m_blockfiles_indexed:

https://github.com/bitcoin/bitcoin/blob/0a1bbec688b8c075ba45b6028b6f4fe3ef636be9/src/node/chainstate.cpp#L65

With no chain tip, the main thread parks here until the import thread activates genesis:

https://github.com/bitcoin/bitcoin/blob/0a1bbec688b8c075ba45b6028b6f4fe3ef636be9/src/init.cpp#L2085-L2090

A cv.wait(lock, pred) evaluates its predicate once on entry, and afterwards only when the cv is notified.

A shutdown request wakes nobody: the SIGINT/SIGTERM handler is (correctly) restricted to async-signal-safe operations it sets a flag and writes a pipe byte, and cannot notify a condition variable:

https://github.com/bitcoin/bitcoin/blob/0a1bbec688b8c075ba45b6028b6f4fe3ef636be9/src/util/signalinterrupt.cpp#L42-L61

The same applies to a fatal error AbortNode() just trips the same flag. The interrupted reindex then cancels the only wakeup. The import thread polls the flag and returns early, skipping both the LoadGenesisBlock() fallback and ActivateBestChains(): https://github.com/bitcoin/bitcoin/blob/0a1bbec688b8c075ba45b6028b6f4fe3ef636be9/src/node/blockstorage.cpp#L1298-L1308 So blockTip() the notification the main thread sleeps on never fires: https://github.com/bitcoin/bitcoin/blob/0a1bbec688b8c075ba45b6028b6f4fe3ef636be9/src/node/kernel_notifications.cpp#L51-L58 Interrupt() would notify the cv, but it only runs after AppInitMain returns, and the stuck thread is the main thread inside AppInitMain:

https://github.com/bitcoin/bitcoin/blob/0a1bbec688b8c075ba45b6028b6f4fe3ef636be9/src/init.cpp#L294-L295

ShutdownRequested() is true and the wait predicate would return true, but it is never re-evaluated. Further SIGINT/SIGTERM just re-set the flag. The node logs "Interrupt requested. Exit reindexing." and can then only be killed with SIGKILL.

The reindex loop's early return is the one import path that bypasses it. I have a deterministic reproducer (functional test https://github.com/ismaelsadeeq/bitcoin/commit/87a803f4e14527573fbc475409d087cd8c78bcc8), And a fix is attached that wakes the cv on the return of importBlocks so we can check for the ShutdownRequested output https://github.com/ismaelsadeeq/bitcoin/commit/2747f67d3d1aa81f5b85c9babeb62b328e0811f8.

Because of the short interval required to trigger this, I had to add lots of dummy block headers to the genesis block file to be able to reproduce deterministically.

<details> <summary>Before fix</summary>

 74b5b6ca377d209050f575786c38e9ead86ce49b39f9fc9e947ef04046fbe960, parent 0000000000000000000000000000000000000000000000000000000000000000 not known
2026-07-03T11:53:56.867815Z [initload] [node/blockstorage.cpp:1300] [ImportBlocks] Interrupt requested. Exit reindexing.
2026-07-03T11:53:56.871930Z [initload] [node/mempool_persist.cpp:77] [LoadMempool] Loading 0 mempool transactions from file...
2026-07-03T11:53:56.871935Z [initload] [node/mempool_persist.cpp:149] [LoadMempool] Imported mempool transactions from file: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
2026-07-03T11:53:56.871940Z [initload] [util/thread.cpp:21] [TraceThread] initload thread exit

❯ build/test/functional/feature_reindex_deadlock.py
2026-07-03T11:59:02.790731Z TestFramework (INFO): PRNG seed is: 6100894769303648655
2026-07-03T11:59:02.841112Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_myujz0uz
2026-07-03T11:59:03.309559Z TestFramework (INFO): Replace blk00000.dat with 2000000 genesis-free out-of-order entries
2026-07-03T11:59:03.524207Z TestFramework (INFO): Request shutdown while the reindex scan is before genesis activation
2026-07-03T11:59:04.080428Z TestFramework (INFO): Check that the node terminates
2026-07-03T12:00:04.087747Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
        self.wait_until(lambda: self.is_node_stopped(**kwargs), timeout=timeout)
'''
2026-07-03T12:00:04.138216Z TestFramework (ERROR): Unexpected exception:
Traceback (most recent call last):
  File "/home/ismaelsadeeq/bitcoin-dev/bitcoin-core/bitcoin/build/test/functional/feature_reindex_deadlock.py", line 78, in run_test
    node.wait_until_stopped()
  File "/home/ismaelsadeeq/bitcoin-dev/bitcoin-core/bitcoin/test/functional/test_framework/test_node.py", line 528, in wait_until_stopped
    self.wait_until(lambda: self.is_node_stopped(**kwargs), timeout=timeout)
  File "/home/ismaelsadeeq/bitcoin-dev/bitcoin-core/bitcoin/test/functional/test_framework/test_node.py", line 862, in wait_until
    return wait_until_helper_internal(test_function, timeout=timeout, timeout_factor=self.timeout_factor, check_interval=check_interval)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ismaelsadeeq/bitcoin-dev/bitcoin-core/bitcoin/test/functional/test_framework/util.py", line 450, in wait_until_helper_internal
    raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
AssertionError: Predicate ''''
        self.wait_until(lambda: self.is_node_stopped(**kwargs), timeout=timeout)
''' not true after 60 seconds

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/ismaelsadeeq/bitcoin-dev/bitcoin-core/bitcoin/test/functional/test_framework/test_framework.py", line 145, in main
    self.run_test()
  File "/home/ismaelsadeeq/bitcoin-dev/bitcoin-core/bitcoin/build/test/functional/feature_reindex_deadlock.py", line 81, in run_test
    raise AssertionError("node hung in init's genesis wait after shutdown was requested")
AssertionError: node hung in init's genesis wait after shutdown was requested
2026-07-03T12:00:04.189344Z TestFramework (INFO): Not stopping nodes as test failed. The dangling processes will be cleaned up later.
2026-07-03T12:00:04.189461Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_myujz0uz
2026-07-03T12:00:04.189495Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_myujz0uz/test_framework.log
2026-07-03T12:00:04.189575Z TestFramework (ERROR):
2026-07-03T12:00:04.189656Z TestFramework (ERROR): Hint: Call /home/ismaelsadeeq/bitcoin-dev/bitcoin-core/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_myujz0uz' to consolidate all logs
2026-07-03T12:00:04.189693Z TestFramework (ERROR):
2026-07-03T12:00:04.189716Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2026-07-03T12:00:04.189767Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2026-07-03T12:00:04.189806Z TestFramework (ERROR):

</details>

<details> <summary> After fix</summary>

❯ build/test/functional/feature_reindex_deadlock.py
2026-07-03T12:00:41.118370Z TestFramework (INFO): PRNG seed is: 8598874981728148931
2026-07-03T12:00:41.168752Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_qr6uqzpu
2026-07-03T12:00:41.637280Z TestFramework (INFO): Replace blk00000.dat with 2000000 genesis-free out-of-order entries
2026-07-03T12:00:41.867916Z TestFramework (INFO): Request shutdown while the reindex scan is before genesis activation
2026-07-03T12:00:42.429017Z TestFramework (INFO): Check that the node terminates
2026-07-03T12:00:42.429280Z TestFramework (INFO): Check that a restart resumes the reindex and recovers genesis
2026-07-03T12:00:48.519849Z TestFramework (INFO): Stopping nodes
2026-07-03T12:00:48.721112Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_qr6uqzpu on exit
2026-07-03T12:00:48.721150Z TestFramework (INFO): Tests successful

</details>

The window is normally milliseconds (genesis is the first entry of blk00000.dat), but it grows arbitrarily large if the file begins with blocks whose parents aren't known yet

Good catch, but I don't think having the genesis block not in the first spot in blk0.dat is possible, unless the file was maliciously crafted?

lgtm 2747f67d3d1aa81f5b85c9babeb62b328e0811f8 on the fix.

I also tried with a forced error from FlushStateToDisk (patch below), and the break was triggered. If I removed the break, the next break, after ActivateBestChain was hit, so the loop still exited after the first failing block.

Interesting. Though, I still don't see the point of the break on Error. I presume any runtime error in validation must pass through FatalError, which triggers a shutdown, which leads to an early return.

Having the break here seems like a belt-and-suspenders for a case where an error is set, but it wasn't set by FatalError, which is a bug. Looks like Failed to find position to write new block to disk is such a possible bug (and obviously completely untested).

My recommendation would still be to remove this break and fix all the bugs, but fine if you want to leave if for a later pull (I've already acked this one).

The later two breaks should likely be removed for the same reason.

ACK 256482ab566360bd9d9ae0cb9c16e1a85c2de5ac

lightly tested ACK 256482ab566360bd9d9ae0cb9c16e1a85c2de5ac

Reviewed the small change and skimmed the relevant discussion. I also checked it locally with a monkey-patched FlushStateToDisk failure: with the old call shape the forced error dirties the caller-visible BlockValidationState, while with this change AcceptBlock still returns true and the caller-visible state stays valid.

I did not go deep into the broader reindex/deadlock follow-ups, but this narrow fix looks correct to me. Good find, @optout21.

<details><summary>Test used to verify the fix</summary>

diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp
index 141c67da4a..8a8a6c7acb 100644
--- a/src/test/validation_chainstate_tests.cpp
+++ b/src/test/validation_chainstate_tests.cpp
@@ -25,11 +25,13 @@

 #include <boost/test/unit_test.hpp>

+#include <atomic>
 #include <memory>
 #include <optional>
 #include <vector>

 class CTxMemPool;
+extern std::atomic_bool g_force_flush_state_to_disk_failure_once;

 BOOST_FIXTURE_TEST_SUITE(validation_chainstate_tests, ChainTestingSetup)

@@ -98,6 +100,27 @@ BOOST_FIXTURE_TEST_CASE(connect_tip_does_not_cache_inputs_on_failed_connect, Tes
     BOOST_CHECK(!chainstate.CoinsTip().HaveCoinInCache(outpoint));    // input not cached
 }

+BOOST_FIXTURE_TEST_CASE(acceptblock_ignores_final_flush_error_state, TestChain100Setup)
+{
+    ChainstateManager& chainman{*Assert(m_node.chainman)};
+    m_node.notifications->m_shutdown_on_fatal_error = false;
+
+    const auto block{std::make_shared<const CBlock>(CreateBlock({}, CScript{} << OP_TRUE))};
+
+    LOCK(cs_main);
+    BlockValidationState state;
+    CBlockIndex* pindex{nullptr};
+    bool new_block{false};
+    BOOST_REQUIRE(CheckBlock(*block, state, Params().GetConsensus()));
+
+    g_force_flush_state_to_disk_failure_once = true;
+    const bool accepted{chainman.AcceptBlock(block, state, &pindex, /*fRequested=*/true, /*dbp=*/nullptr, &new_block, /*min_pow_checked=*/true)};
+
+    BOOST_CHECK(accepted);
+    BOOST_CHECK(state.IsValid()); // Fails without the fix
+    BOOST_CHECK(new_block);
+}
+
 //! Test UpdateTip behavior for both active and background chainstates.
 //!
 //! When run on the background chainstate, UpdateTip should do a subset
diff --git a/src/validation.cpp b/src/validation.cpp
index 87cf646b8b..b84318ddb6 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -66,6 +66,7 @@
 #include <validationinterface.h>

 #include <algorithm>
+#include <atomic>
 #include <cassert>
 #include <chrono>
 #include <deque>
@@ -78,6 +79,8 @@
 #include <utility>

 using kernel::CCoinsStats;
+
+std::atomic_bool g_force_flush_state_to_disk_failure_once{false};
 using kernel::ChainstateRole;
 using kernel::CoinStatsHashType;
 using kernel::ComputeUTXOStats;
@@ -2711,6 +2714,9 @@ bool Chainstate::FlushStateToDisk(
 {
     LOCK(cs_main);
     assert(this->CanFlushToDisk());
+    if (mode == FlushStateMode::NONE && g_force_flush_state_to_disk_failure_once.exchange(false)) {
+        return FatalError(m_chainman.GetNotifications(), state, _("Forced FlushStateToDisk test error"));
+    }
     std::set<int> setFilesToPrune;
     bool full_flush_completed = false;

</details>

the described deadlock at init's genesis wait, and it is still reachable

Great job digging up this issue -- fortunately it's a low-probability low-impact. Do you plan to create a new PR with it? I wouldn't include in this one.

rfm?