validation: return block-file flush errors #35714

l0rinc commented at 7:37 PM on July 13, 2026: contributor

Problem: When flushing block or undo data fails, FlushStateToDisk() still writes block-index metadata and coins data, then advances m_last_flushed_block. With a separate -blocksdir, persistent failures there can leave metadata referring to block data that is not durable even when chainstate writes succeed.

Fix: Return the flush error before those writes, leaving the last-flushed marker and on-disk coins tip unchanged in the failing call. Later flushes can retry and write if they succeed, including during shutdown. ConnectTip() and DisconnectTip() propagate the error before updating the chain tip, after applying the coins changes in memory.

History:

#27866 raised whether saving other state after a flush failure was preferable. This change preserves later retries while skipping writes in the failing call.
#34897 introduced m_last_flushed_block to prevent persistent indexes from advancing past flushed chainstate. This fix keeps it at the last successful flush.

DrahtBot added the label Validation on Jul 13, 2026

DrahtBot commented at 7:37 PM on July 13, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/35714
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/35714.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/35714#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	optout21, arejula27, pzafonte
Concept ACK	mzumsande

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36074 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36074.svg"> (scripted-diff: [test] Add util/check.h includes for assertions by maflcko)
#35731 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35731.svg"> (Indexes: Harden the flush-error notification invariant by arejula27)
#35307 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35307.svg"> (blockstorage: keep snapshot base in normal blockfile range by shuv-amp)

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.

arejula27 commented at 8:13 PM on July 13, 2026: contributor

Concept ACK

I still have to dig into the code and the implications of this change (just did a quick review), but in the meantime: would it be worth adding a test case for the undo-file failure path as well? both the commit message and the error string mention block or undo files, but the test only injects a failure on blk*.dat.

in src/test/chainstate_write_tests.cpp:124 in e1a337ee96 outdated

 119 | +    const auto* old_flushed{WITH_LOCK(::cs_main, return chainstate.GetLastFlushedBlock())};
 120 | +
 121 | +    mineBlocks(1);
 122 | +    const auto* new_tip{WITH_LOCK(::cs_main, return chainstate.m_chain.Tip())};
 123 | +    BOOST_REQUIRE_NE(old_flushed, new_tip);
 124 | +    inject_file_open_failure(WITH_LOCK(::cs_main, return chainstate.m_blockman.GetBlockPosFilename(new_tip->GetBlockPos())));

l0rinc commented at 7:28 PM on July 14, 2026:

would it be worth adding a test case for the undo-file failure path as well

Good question, thanks - FlushBlockFile() returns false for either block or undo flush failures, see: https://github.com/bitcoin/bitcoin/blob/e1a337ee961db4308697de7cddcf4aad9684c7f1/src/node/blockstorage.cpp#L774-L786 Since this PR only changes how that shared result is handled, testing both failure sources separately would fit better in a lower-level BlockManager test.

arejula27 commented at 5:19 PM on July 15, 2026: contributor

ACK e1a337ee961db4308697de7cddcf4aad9684c7f1 This is a small change, closing a gap discussed at #34897 I built it and tested it locally. I tryied to play around it and I did not find anything wrong.

One small, non-blocking point: the comment // FlushChainstateBlockFile() already emitted the flush-error notification. describes an invariant, but nothing checks it . It is true today only because every false path inside FlushBlockFile()/FlushUndoFile() calls flushError() first. Would be correct adding an assume to verify this behaviour is keep it in the future?:

if (!m_blockman.FlushChainstateBlockFile(m_chain.Height())) {
    // FlushChainstateBlockFile() already emitted the flush-error notification.
    Assume(m_chainman.m_interrupt);
    return state.Error("Failed to flush block or undo file");
}

Also, I want to ask about something I found while testing, not sure if it is related to this PR. FlushStateToDisk() does not check if shutdown was already requested before it starts. I wrote a test where a first flush fails and requests shutdown, then I fix the file and call FlushStateToDisk() again: it succeeds and m_last_flushed_block moves forward, even though shutdown was already requested before this second call. Is this expected? I think it may be needed for the final flush in Shutdown() (init.cpp), but I am not 100% sure, so I wanted to ask before assuming it is fine

arejula27 commented at 5:36 PM on July 15, 2026: contributor

I opened a follow-up PR #35731 i did while reviewing the PR with the intention to group this notify + state.Error() behaviour into one helper, (making the invariant of the comment harder) and add more low-level tests like the ones you mentioned above.

in src/test/chainstate_write_tests.cpp:128 in 0f04fbee2f

 123 | +    BOOST_REQUIRE_NE(old_flushed, new_tip);
 124 | +    inject_file_open_failure(WITH_LOCK(::cs_main, return chainstate.m_blockman.GetBlockPosFilename(new_tip->GetBlockPos())));
 125 | +
 126 | +    const bool flushed{chainstate.FlushStateToDisk(state, FlushStateMode::FORCE_FLUSH)};
 127 | +    BOOST_CHECK_EQUAL(m_node.exit_status.load(), EXIT_FAILURE);
 128 | +    BOOST_CHECK(flushed && state.IsValid()); // TODO: Return the flush error

maflcko commented at 11:36 AM on July 16, 2026:

nit in 0f04fbee2f123c37c4b6fbff7dc10f61dd0b5924: The && make this a bit harder to read and violate https://github.com/bitcoin/bitcoin/pull/35729

l0rinc commented at 5:01 PM on July 16, 2026:

flushed is tied to the state being invalid, it makes more sense to test them in groups, see #35729#pullrequestreview-4715961548

If you insist I can remove the state validation here.

optout21 commented at 10:28 AM on August 4, 2026:

0f04fbe test: characterize writes after flush failure:

Why not simply BOOST_CHECK(flushed); BOOST_CHECK(state.IsValid()); ?

l0rinc commented at 8:59 PM on August 7, 2026:

Yes, I could also do that, but that would assume that I find it likely that they will fail independently, and since I don't, I consider it just noise.

maflcko commented at 8:32 AM on August 18, 2026:

not important, but it looks like #35729 was merged, so it could make sense to follow the style used in src/test/coinscachepair_tests.cpp. Or, if you really want to group them, wrap them in another pair of (). I just mention it to avoid a silent merge conflict with #35713 and avoid having to touch this line again after merge.

l0rinc commented at 5:19 PM on August 18, 2026:

I gave this exact line as a concrete example for why we shouldn't just blindly split these conditions #35729#pullrequestreview-4715961548 Why not fix #35713 instead?

optout21 commented at 4:43 AM on August 19, 2026:

I see your argument about how unrolling is sub-optimal, but it's a weak one for me. If you insist, the proper solution may be to create a new macro with 2 (or N) conditions, BOOST_CHECK_ALL, that evaluates the conditions, and if one fails, prints the values of all. After moving away from BOOST it would be easy to add such a macro.

mzumsande commented at 2:52 PM on July 16, 2026: contributor

It would be good to discuss the real-world consequences of this change a bit more:

As far as I understand one reason for the status quo is that even if an error would occur, and the data is not durable, there is a good chance of the fsync being done by the OS eventually if the failure is temporary - data is not necessarily lost, so we might as well flush the other state to avoid having to re-do the syncing. If the failure was permanent, the other write operations (chainstate etc.) would likely fail as well, so nothing gets out of sync in that scenario either.

However, one point for this change is that block data is often stored separately (-blocksdir option). In this case, it is quite possible that actually corruption occurs if the blocksdir fsync fails permanently, but flushes of the chainstate etc. all succeed, and BLOCK_HAVE_DATA is set in the blocktree db, while the actual block data is missing.

So I am Concept ACK

in src/test/chainstate_write_tests.cpp:126 in 0f04fbee2f

 121 | +    mineBlocks(1);
 122 | +    const auto* new_tip{WITH_LOCK(::cs_main, return chainstate.m_chain.Tip())};
 123 | +    BOOST_REQUIRE_NE(old_flushed, new_tip);
 124 | +    inject_file_open_failure(WITH_LOCK(::cs_main, return chainstate.m_blockman.GetBlockPosFilename(new_tip->GetBlockPos())));
 125 | +
 126 | +    const bool flushed{chainstate.FlushStateToDisk(state, FlushStateMode::FORCE_FLUSH)};

optout21 commented at 10:24 AM on August 4, 2026:

0f04fbe test: characterize writes after flush failure:

Unit test observation: Running all 781 unit tests, they all pass, but a single output line is printed on the output: Error: A fatal internal error occurred, see debug.log for details: Flushing block file to disk failed. This is likely the result of an I/O error. This is noted as this is the only stdout printout from the whole unit test suite. It is emitted from the new unit test (chainstate_flush_failure_boundary -> 2ns call to FlushStateToDisk -> FlushChainstateBlockFile -> FlushBlockFile.) Any chance to suppress this?

l0rinc commented at 8:59 PM on August 7, 2026:

Good point, added ASSERT_DEBUG_LOG, which suppresses the expected output and verifies the lower-level error message. It fixes it for me, can you please check it? Added you as coauthor.

optout21 commented at 4:55 AM on August 19, 2026:

Verified, no more output produced, thanks!

in src/validation.cpp:2796 in e1a337ee96

2791 | @@ -2792,10 +2792,9 @@ bool Chainstate::FlushStateToDisk(
2792 |                  LOG_TIME_MILLIS_WITH_CATEGORY("write block and undo data to disk", BCLog::BENCH);
2793 |  
2794 |                  // First make sure all block and undo data is flushed to disk.
2795 | -                // TODO: Handle return error, or add detailed comment why it is
2796 | -                // safe to not return an error upon failure.
2797 |                  if (!m_blockman.FlushChainstateBlockFile(m_chain.Height())) {
2798 | -                    LogWarning("%s: Failed to flush block file.\n", __func__);
2799 | +                    // FlushChainstateBlockFile() already emitted the flush-error notification.

optout21 commented at 10:36 AM on August 4, 2026:

e1a337e validation: stop writes after flush failure:

Why is the LogWarning removed? I could not convince myself that the fatalError()/state.Error() calls include the same logging. Is it possible to preserve the log line?

At first I though that the added comments refers to the reason of removal of LogWarning, but then I realized it explains the usage of state.Error() instead of FatalError().

Regarding the comment, FlushChainstateBlockFile indeed calls KernelNotifications::flushError whenever it returns false, but this is handled across two levels (FlushUndoFile), which is a bit too brittle.

l0rinc commented at 7:01 PM on August 8, 2026:

The lower layer already reports the specific failure through flushError() before returning false, so restoring this generic warning would duplicate the error. I clarified the comment to make that explicit, let me know if it helps.

3097:2026-08-09T00:59:53.599616Z (mocktime: 2020-08-31T15:34:13Z) [test] [noui.cpp:66] [noui_ThreadSafeMessageBoxRedirect] A fatal internal error occurred, see debug.log for details: Flushing block file to disk failed. This is likely the result of an I/O error.

optout21 commented at 4:46 AM on August 19, 2026:

Very good, thanks.

in src/validation.cpp:2797 in e1a337ee96 outdated

2791 | @@ -2792,10 +2792,9 @@ bool Chainstate::FlushStateToDisk(
2792 |                  LOG_TIME_MILLIS_WITH_CATEGORY("write block and undo data to disk", BCLog::BENCH);
2793 |  
2794 |                  // First make sure all block and undo data is flushed to disk.
2795 | -                // TODO: Handle return error, or add detailed comment why it is
2796 | -                // safe to not return an error upon failure.
2797 |                  if (!m_blockman.FlushChainstateBlockFile(m_chain.Height())) {
2798 | -                    LogWarning("%s: Failed to flush block file.\n", __func__);
2799 | +                    // FlushChainstateBlockFile() already emitted the flush-error notification.
2800 | +                    return state.Error("Failed to flush block or undo file");

optout21 commented at 10:55 AM on August 4, 2026:

e1a337e validation: stop writes after flush failure:

With this early exit, the update of m_next_write below is also skipped. It looks like that may cause some unwanted side-effects, so maybe it should be kept (e.g. by storing the return value, skipping the steps to be skipped, and keeping the if (should_write || m_next_write == ... block).

l0rinc commented at 7:02 PM on August 8, 2026:

I don't think m_next_write should advance after a failed write: the timer update was deliberately moved after the chainstate write in #30611. The lower layer has already requested shutdown, and the final FORCE_FLUSH during shutdown does not depend on the periodic deadline.

optout21 commented at 4:33 AM on August 19, 2026:

Thanks, it makes sense.

optout21 commented at 11:04 AM on August 4, 2026: contributor

Concept ACK (e1a337ee961db4308697de7cddcf4aad9684c7f1)

The change is within FlushStateToDisk, and changes how an error from FlushChainstateBlockFile is handled -- ignored previously, returned upwards with the change.

Build & unit tests verfied locally.

The effectiveness of the new test (i.e., it catches the error) is checked by manually reverting the change.

Analyis of call pattern of FlushChainstateBlockFile: directly called in only one place, the one affected in this change. Indirectly it is used in several usecases:

The chain tip advances (connect/disconnect, including during IBD/reorgs),
A new block is accepted to disk,
The mempool cache is trimmed after accepting transactions/packages,
A user manually prunes via RPC,
The coins cache is resized,
An assumeutxo snapshot finishes background validation,
The node shuts down or an RPC/kernel-API caller explicitly forces a flush.

It's hard to conceptually assess the high-level effects in all the use cases.

Reviewing the steps excluded with the early exit, the update of the m_next_write seems uneqivocal, it may have unintended consequences (left comment).

LGTM, left some comments.

l0rinc force-pushed on Aug 8, 2026

DrahtBot added the label CI failed on Aug 8, 2026

DrahtBot removed the label CI failed on Aug 8, 2026

l0rinc force-pushed on Aug 9, 2026

l0rinc commented at 1:14 AM on August 9, 2026: contributor

Thanks for the review, took me a bit longer to react, I'm juggling a few open ones...

optout21 commented at 11:41 AM on September 14, 2026: contributor

ACK c42d7fe1cf2aee821b4f9931ef94f16e09de8459

Concluding my previous review. Reviewed, built and unit-tested locally. Also verified that without the behavior change, the new test fails.

DrahtBot requested review from mzumsande on Sep 14, 2026

l0rinc force-pushed on Oct 1, 2026

l0rinc renamed this:
~~validation: stop writes after flush failure~~
validation: return block-file flush errors
on Oct 4, 2026

pzafonte commented at 3:06 PM on October 6, 2026: contributor

tACK 90fd0f15b6 I'm looking into pruning for the kernel (#35627), which would go through this flush path.

DrahtBot requested review from optout21 on Oct 6, 2026

test: characterize writes after flush failure

Record the flush result, last-flushed marker, block-file metadata and on-disk coins tip after a block-file flush failure.
The failing flush still writes the new coins tip before the fix.

Co-authored-by: optout <13562139+optout21@users.noreply.github.com>

07d23eb755

validation: return block-file flush errors

Return before writing block-index metadata or coins data when the block or undo file flush fails in FlushStateToDisk().
Keep the last-flushed marker and on-disk coins tip at the previous successful flush.
The lower layer already reports the specific error through flushError().

This stops subsequent writes in the failing invocation. Later flushes can retry and write if they succeed, including during shutdown.
ConnectTip() and DisconnectTip() propagate the error before updating the chain tip, after applying the block's coins changes in memory.

12f691cf28

l0rinc force-pushed on Oct 7, 2026

l0rinc commented at 6:13 AM on October 7, 2026: contributor

Added missed coverage found by AI review.

optout21 commented at 6:35 AM on October 7, 2026: contributor

reACK 12f691cf286ea9a020583d267b94efca102a4056

arejula27 commented at 6:58 PM on October 9, 2026: contributor

reACK 12f691cf286ea9a020583d267b94efca102a4056

pzafonte commented at 8:27 PM on October 9, 2026: contributor

reACK 12f691cf286ea9a020583d267b94efca102a4056