#35714 validation: stop writes after flush failure
https://github.com/bitcoin/bitcoin/pull/35714 · · +28/-3 in 2 files, 2 commits · labels: Validation
Goal
- Prevent database corruption by stopping metadata flushes immediately if block file writes fail
- Avoid falsely marking block data as available when underlying block or undo writes did not persist
This pull request stops `FlushStateToDisk()` from writing block-index metadata and coins data when `FlushChainstateBlockFile()` fails. By returning an error immediately, it prevents `m_last_flushed_block` from advancing past non-durable block or undo data and adds a unit test for this failure path.
Problem: When writing block or undo files to disk fails during a chainstate flush, the node previously continued to write coins cache and block-index metadata, advancing the last-flushed marker. If block storage and chainstate reside on different filesystems or block writing fails permanently, this can leave block-index metadata pointing to missing or incomplete block data.
Category: Validation (#9 of 48)
P2 · bug fix
- P2 because it prevents chainstate and block index desynchronization during disk I/O errors
- Avoids recording durable block status when block files fail to fsync or write completely
Chainstate flush correctness and crash safety are central validation responsibilities. Continuing to commit block metadata and coins when underlying block or undo data failed to flush creates inconsistent state on disk, which is particularly hazardous when block files reside on a distinct mount point via `-blocksdir`.
Membership: Directly alters Chainstate::FlushStateToDisk in src/validation.cpp to handle block file flush failure before committing coins and block index metadata.
Factors: security/stability 2, bug 2, performance 0, user value 0, leverage 1
Reviewability: Ready
- Ready to review: small self-contained change with passing CI and resolved reviewer questions
The patch is small, cleanly rebased, passes CI, and includes unit tests for the error boundary.
Author status: Active and addressed review comments
Open concerns:
- Test formatting nit regarding compound assertions under BOOST_CHECK versus split assertions
Resolved concerns:
- Whether removing LogWarning lost logging coverage; resolved by noting lower-level FlushBlockFile already calls flushError
- Whether skipping the m_next_write timer update had side effects; resolved by showing periodic timer should not advance after a failed flush before shutdown
- Suppression of expected fatal log messages during unit tests; resolved by adding ASSERT_DEBUG_LOG
Agreement: Strong
- Strong support for halting writes immediately on flush failures
- Concept approval highlighting corruption risks with separate -blocksdir storage (mzumsande)
- Verified by testing failure handling and unit test coverage (optout21)
- Confirmed the logic gap is closed and opened a follow-up refactor (arejula27)
Strong consensus with multiple ACKs (arejula27, optout21, mzumsande); all reviewer questions were answered.
Reviewers tested the patch, confirmed the problem and corruption risks, and resolved all questions on failure semantics and test output.
- mzumsande gave Concept ACK explaining that if blocksdir fsync fails permanently while chainstate succeeds, corruption occurs with BLOCK_HAVE_DATA set
- arejula27 ACKed the approach and code
- optout21 thoroughly reviewed call sites, tested failure injection, and gave full ACK
Objections:
| Reviewer | Kind | Harm | Status | Blocking | Author replied | Quote |
|---|---|---|---|---|---|---|
| optout21 | correctness | Skipping the update of m_next_write on early return could cause unwanted side effects | resolved | no | yes | 2026-08-04: 'With this early exit, the update of m_next_write below is also skipped. It looks like that may cause some unwanted side-effects' Settled: 2026-08-19: 'Thanks, it makes sense.' after author explained m_next_write should not advance after a failed write before shutdown |
Support:
- optout21: Verified that without the behavior change the new test fails, and verified all call patterns
- arejula27: Tested locally, noted it closes a gap discussed in #34897
- mzumsande: Noted it prevents corruption when -blocksdir storage fails while chainstate write succeeds
Participants: arejula27 (support), maflcko (neutral), mzumsande (support), optout21 (objection)
State derived from the lists: substantive support, no open objection (optout21, arejula27, mzumsande)
Review verdicts (DrahtBot): 1 (+1)
Dependencies
Enables:
Files
26 lines under test/bench/ci.
- src/test/chainstate_write_tests.cpp +26/-0
- src/validation.cpp +2/-3
Card
This PR stops FlushStateToDisk from writing block-index metadata and coins data if FlushChainstateBlockFile fails, resolving a deferred TODO from PR 27866. Without this check, a failed block or undo flush can still advance the flushed block pointer and record block-index data, risking inconsistent disk state or corruption especially when using -blocksdir. The PR includes an error-injection unit test verifying that the last-flushed pointer does not advance on failure. It has strong support and ACKs from multiple reviewers with no open objections.