#35003 validation: improve block data I/O error handling in P2P paths

full analysis

https://github.com/bitcoin/bitcoin/pull/35003 · furszy · +548/-7 in 10 files, 5 commits · labels: Validation

Goal

  • Prevent block storage I/O failures from leaving nodes silently stuck unable to advance the chain
  • Trigger graceful shutdown instead of ungraceful aborts when disk access fails during peer requests

This pull request prevents block I/O failures from being silently swallowed or causing hard aborts during P2P processing and chain activation. It modifies FlatFileSeq::Open to return nullptr instead of throwing when creating parent directories fails, replaces an assert in GETBLOCKTXN message processing with a fatal error shutdown, and adds comprehensive unit and functional tests simulating filesystem permission failures.

Problem: When the block storage directory becomes inaccessible (e.g., volume detach or permissions issue), FlatFileSeq::Open throws an exception that bypasses FatalError in ActivateBestChain and gets swallowed by the P2P catch-all, leaving the node alive but silently stuck. Additionally, GETDATA silently drops requests without disconnecting peers, and GETBLOCKTXN can trigger an ungraceful assertion crash.

Category: Validation (#16 of 48)

P3 · bug fix

  • P3 because it prevents a rare bug where an inaccessible disk leaves a node stuck instead of halting
  • A hung chain activation leaves the node alive but silently degraded and unable to process new blocks

P3 because it fixes an edge-case reliability bug where an inaccessible disk causes ActivateBestChain to get stuck rather than fatally shutting down. In the description, furszy notes that 'an error can leave the node alive but stuck, unable to process new blocks and advance the chain'. While the trigger condition (filesystem inaccessibility mid-run) is uncommon, preventing silent node degradation is worthwhile.

Membership: Alters FlatFileSeq error handling on block directory access and prevents ActivateBestChain from becoming stuck during block connection when disk access fails.

Factors: security/stability 2, bug 2, performance 0, user value 1, leverage 1

Category: P2P (#13 of 65)

P3 · stability

  • P3 because it replaces an assert crash with a controlled graceful shutdown on block read failure
  • Ensures peers are disconnected cleanly when disk reads fail instead of silently dropping requests

P3 because it replaces an assert abort with a controlled graceful shutdown on GETBLOCKTXN read failure and ensures GETDATA peers are disconnected when I/O fails. As commit 768014068b states, 'Replace assert with a fatal error so the node shuts down through the normal shutdown path instead', allowing cleaner teardown during disk faults.

Membership: Modifies net_processing message handling for GETBLOCKTXN to trigger graceful shutdown instead of aborting, and ensures GETDATA disconnects failing peers.

Factors: security/stability 2, bug 1, performance 0, user value 1, leverage 0

Reviewability: Stale: Author silent 70d

  • Author silent for 70 days following an approach objection and broader architectural questions

The author has been inactive for 70 days following an Approach NACK and a maintainer question on the overall error-handling direction.

Author status: silent since 2026-07-09 (70 days)

Open concerns:

  • josibake approach-NACKed folding filesystem_error into a nullptr return in FlatFileSeq::Open, arguing exceptions should be preserved and translated at the BlockManager boundary instead of expanding ambiguous nullable contracts.
  • maflcko noted the pull has outstanding architectural issues and asked whether the project wants to provide fallbacks for rare filesystem exceptions across all paths or use a unified strategy.

Resolved concerns:

  • Locking order issues with cs_main in blockmanager unit tests were resolved.
  • SimulateFileSystemError test utility was refined to handle root user and Windows limitations cleanly.

Agreement: Disputed

  • Approach objection to returning null pointers instead of preserving filesystem exceptions (josibake)
  • PR cannot be merged as-is without deciding a unified error-handling strategy across paths (maflcko)
  • Author defended consistency with existing callers but has not replied since July (furszy)

Disputed: josibake approach-NACKs returning nullptr instead of translating exceptions at BlockManager; author silent

Josibake registered an Approach NACK against returning nullptr from FlatFileSeq::Open to swallow filesystem exceptions. The author defended the change as a minimal consistency fix for existing callers, but discussion concluded with maintainers noting the PR cannot be merged as-is without broader consensus.

  • josibake: 'Approach NACK ... I am convinced removing exceptions from the low level file primitive and folding it into a nullptr is the wrong direction.'
  • furszy: 'Today, every filesystem error in FlatFileSeq::Open is handled through the same path except for this one case. Your proposal keeps that one case special ... That is just error-prone.'
  • maflcko: 'I don't think the pull will be merged as-is. Even if it was merged, there'd be a bunch of follow-ups, so it would be good to decide how to address them before merge.'

Review verdicts (DrahtBot): 1 (+4) -1

Files

537 lines under test/bench/ci.

  • test/functional/p2p_handle_io_errors.py +221/-0
  • src/test/validation_chainstate_tests.cpp +104/-0
  • src/test/flatfile_tests.cpp +77/-4
  • src/test/blockmanager_tests.cpp +57/-0
  • src/test/util/common.cpp +57/-0
  • src/test/util/common.h +15/-0
  • src/flatfile.cpp +10/-2
  • src/net_processing.cpp +5/-1
  • src/test/util/CMakeLists.txt +1/-0
  • test/functional/test_runner.py +1/-0

Card

PR #35003 ensures that block I/O errors occurring during P2P message handling or chain activation result in clean fatal shutdowns or peer disconnections instead of silent failures or assertion aborts. It specifically prevents FlatFileSeq::Open from throwing when parent directory creation fails, which previously bypassed validation fatal error handling and left nodes silently unable to advance the chain. Multiple reviewers supported the fix and its comprehensive test coverage, but josibake logged an Approach NACK against collapsing exceptions into nullable returns rather than translating them at the BlockManager boundary. The PR is currently stale, with the author silent for 70 days following maintainer calls for an architectural decision on exception handling.

Data

dossier JSON · extract JSON · model openrouter/google/gemini-3.8-flash, generated 2026-09-17T15:51, confidence high, input hash d87f00f68b685ca3