validation: stop writes after flush failure #35714

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.

<!--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/35714.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy 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:

• #35731 (Indexes: Harden the flush-error notification invariant by arejula27)
• #35307 (blockstorage: keep snapshot base in normal blockfile range by shuv-amp)
• #30342 (kernel, logging: Pass Logger instances to kernel objects 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

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.

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

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.

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

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.

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