validation: stop writes after flush failure #35714

pull l0rinc wants to merge 2 commits into bitcoin:master from l0rinc:l0rinc/flush-state-failure-boundary changing 2 files +26 −3

l0rinc commented at 7:37 PM on July 13, 2026: contributor
Problem: When FlushChainstateBlockFile() fails, shutdown is requested, but FlushStateToDisk() still writes block-index metadata and coins data, then advances m_last_flushed_block. Those writes can record a block even though its block or undo data may not be durable.

Fix: Return the flush error before those writes, leaving m_last_flushed_block at the last successfully flushed block.

History:
- #27866 proposed returning here to avoid writing metadata for block data that failed to flush, but review requested a test and caller audit before changing behavior, so it was deferred as the current TODO; this PR now resolves it.
- #34897 later introduced m_last_flushed_block to prevent persistent indexes from advancing past flushed chainstate; this fix keeps it at the last successful flush.
  </details>
test: characterize writes after flush failure 0f04fbee2f

validation: stop writes after flush failure

When a block-file or undo-file flush fails, shutdown is requested, but `FlushStateToDisk()` still writes block-index metadata and coins data, then advances `m_last_flushed_block`.
Return the error before those writes, keeping `m_last_flushed_block` at the last successful flush.

e1a337ee96

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.



Code Coverage & Benchmarks

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



Reviews

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

Type Reviewers

ACK arejula27

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:
- #35307 (blockstorage: keep snapshot base in normal blockfile range by shuv-amp)
- #30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
- #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.
arejula27 commented at 8:13 PM on July 13, 2026: none

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
```
 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: none
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: none

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.

Type	Reviewers
ACK	arejula27
Concept ACK	mzumsande

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.

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

Contributors

Labels

Linked (view graph)

#27866 blockstorage: Return on fatal flush errors #34897 indexes: Don't commit ahead of the flushed chainstate #35731 Indexes: Harden the flush-error notification invariant

validation: stop writes after flush failure #35714

Code Coverage & Benchmarks

Reviews

Conflicts