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

Reviewability

Reviews

Agreement

Size

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

Waiting on author

1 (+4) -1

Blocked

15 + 533 tests

Prevent nodes from hanging or crashing when block storage encounters filesystem errors
Ensure fatal storage failures trigger a clean shutdown rather than silently stalling block relay

Reviewing now is worthwhile as there are no pending changes invalidating review

ACK: l0rinc
Stale ACK: frankomosh, sedited, rkrux, w0xlt
Concept ACK: pinheadmz
Approach NACK: josibake

Unaddressed objection: getrawtransaction under txindex masks storage errors as missing txs (w0xlt)
Unaddressed objection: introduces index regressions and leaves error handling inconsistent (maflcko)
Contested objection: returning nullptr for low-level file errors loses failure context (josibake)
Support for improved test coverage and preventing silent hangs during I/O failures (frankomosh, l0rinc)

15 lines added or modified outside tests
533 lines added or modified in tests
7 lines removed in total
10 files, 5 commits

P2P#35 of 71
P3 · bug fix
P3 because it fixes edge-case message processing failures during filesystem errors Replaces an ungraceful crash during block transaction requests with a controlled shutdown Addresses silent request drops when relaying blocks to peers under I/O errors Ranking pass: Stability under I/O failure belongs at the top of P3 with the other correctness fixes. Ranking notes

Validation#9 of 53

P2 · bug fix

P2 because it stops block I/O errors from bypassing fatal error handling during chain activation
Prevents nodes from becoming silently stuck when the block storage directory is inaccessible
Provides durable crash safety and reliability improvements for chainstate storage invariants
Ranking pass: Moved up: silently stuck node on block-storage I/O errors is a reliability bug, ranked with the other storage-error fixes

Ranking notes

Reviewability: ReadyStalePaused

Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets

Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.

Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

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.

Reviewability

Paused · Waiting on author

Open objections from maflcko, w0xlt with no author reply for 23 days; review would repeat what has been asked.

Waiting on (from the thread read):

author: response to index regression objections from maflcko and w0xlt 2026-09-23
decision: architectural resolution on whether to translate exceptions at block storage or fold into nullptr 2026-07-09
author: respond to w0xlt's objection regarding getrawtransaction -txindex regression 2026-09-23
author: respond to maflcko's objection regarding index regression and disk space error consistency 2026-09-23
decision: agree on overall error handling approach across exceptions vs nullptr return vs abort 2026-10-09

Agreement

Blocked

objection that needs a change, nobody has replied (maflcko, w0xlt)

Thread notes: Reviewed thread up to 2026-10-09. Stood up objections from maflcko and w0xlt regarding regressions on index RPC queries remain open with no author response, and architectural disagreement with josibake over exception translation remains contested. / Added maflcko's suggestion regarding agreement on CheckDiskSpace error handling consistency. Standing objections from w0xlt and maflcko on index regressions remain open without author reply, and josibake's approach NACK remains contested.

Objections, suggestions, and questions:

Reviewer	Type	Harm, request, or question	Status	Must be	Author replied	Quote
maflcko (member)	objection, maintenance	adding nodiscard to block storage functions whose return values must naturally be consumed adds unnecessary verbosity and code bloat	fixed · fix pushed · commit was dropped in response to review	changed	yes	2026-05-16: 'Personally I don't think we should be adding `[[nodiscard]]` to all those functions, because this is extra verbosity/bloat' Status from: 2026-05-16: maflcko: 'review ACK 01daa552c3ce06c8c8bfdab8c5b93a662cee6e79'
w0xlt (contributor)	objection, correctness	in unsupported test environments where file permissions cannot be simulated, tests silently pass without executing assertions, creating false coverage	fixed · fix pushed · addressed and reviewer re-ACKed before test changes were further reworked	changed	yes	2026-06-04: 'in unsupported environments, the assertions never run, but the tests still pass. This can create false-positive test coverage.' Status from: 2026-06-04: w0xlt: 'ACK 400a5b16e396498d41ec1ae70ec91f7baf45d1db'
l0rinc (member)	objection, correctness	lock order assertion failure in blockmanager unit tests when run with DEBUG_LOCKORDER	fixed · fix pushed · fixed with proper locking and reviewer verified	changed	yes	2026-06-11: 'Assertion failed: lock ::cs_main not held in node/blockstorage.cpp:1138' Status from: 2026-06-25: l0rinc: 'No lock order problems now 👍'
josibake (member)	objection, approach	removing exceptions from the low-level file primitive and folding them into a nullptr return loses failure cause and context and expands an ambiguous nullable contract	contested · author replied defending current consistency fix, josibake maintained Approach NACK	changed	yes	2026-07-09: 'removing exceptions from the low level file primitive and folding it into a `nullptr` is the wrong direction.' Status from: 2026-07-09: josibake: 'removing exceptions from the low level file primitive and folding it into a `nullptr` is the wrong direction.'
maflcko (member)	objection, approach	merging without resolving follow-up architectural questions leaves inconsistent handling of rare filesystem exceptions across execution paths	contested · author defended pragmatic approach, discussion stalled over competing approaches	changed	yes	2026-07-09: '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.' Status from: 2026-07-09: maflcko: 'I don't think the pull will be merged as-is.'
maflcko (member)	objection, correctness	introduces a regression for index functions by ignoring errors, and leaves unresolved whether to fully fix this class of problem	open	changed	no	2026-09-23: 'It introduces a regression for indexes, and I think it is unclear if people want to fully fix this class of problem'
w0xlt (contributor)	objection, correctness	under -txindex, when the blocks directory is missing and cannot be recreated, getrawtransaction falsely reports the transaction was not found instead of reporting a filesystem error	open	changed	no	2026-09-23: 'when the blocks directory is missing and cannot be recreated, it previously reported a filesystem error. With the new `if (!read_only) guard`, it instead reports that the transaction wasn’t found.'
maflcko (member)	suggestion, approach	agree on what the expected user outcome should be when the same I/O error is hit via CheckDiskSpace	open	answered	no	2026-10-09: 'at least there should be agreement what the expected user outcome in the other case should be'

Support:

pinheadmz (member) (Concept ACK, 2026-04-06) [read]: minimal changes with lots of test coverage
frankomosh (contributor) (Tested ACK, 2026-04-08) [both: SendMessages, blockmanager_tests, flatfile_tests, p2p_handle_io_errors.py, test/functional/p2p_handle_io_errors.py, validation_chainstate_tests]: built from source, all new tests pass, and mutation testing confirmed the functional test detects silent failure regression in GETBLOCKTXN
maflcko (member) (ACK, 2026-04-14) [read: src/test/, src/test/flatfile_tests.cpp, src/test/util/common.h, test/functional/p2p_handle_io_errors.py]: added tests are useful to have proper coverage for this. Also, the code changes move in the right direction
rkrux (member) (ACK, 2026-05-08) [read: test/functional/p2p_handle_io_errors.py]: praised the simulate_io_error context manager for test coverage
sedited (member) (ACK, 2026-05-24) [read: file allocation path, indexes, src/node/blockstorage.cpp]: an improvement over the status quo with valuable additional tests, even if mechanics could be further refined later
w0xlt (contributor) (ACK, 2026-06-04): ACK 400a5b16e396498d41ec1ae70ec91f7baf45d1db [not substantive]
l0rinc (member) (ACK, 2026-06-30) [read: net_processing.cpp, src/net_processing.cpp, src/node/blockstorage.cpp, test/functional/p2p_handle_io_errors.py, tests]: changes look correct and new coverage exercises the relevant failure paths well

Participants:

pinheadmz (member, support, 1 statements 2026-04-06 to 2026-04-06): Concept ACK, praised detailed PR description and minimal changes with strong test coverage
frankomosh (contributor, support, 2 statements 2026-04-08 to 2026-04-15): Tested ACK, verified mutation tests and test coverage
maflcko (member, objection, 43 statements 2026-04-08 to 2026-10-09): Raised blocking objections regarding architectural error handling and regression for indexes
rkrux (member, support, 14 statements 2026-04-15 to 2026-05-25): ACK, reviewed and suggested test cleanups
sedited (member, support, 6 statements 2026-05-10 to 2026-09-20): ACK, questioned approach initially but considers it an improvement over status quo
l0rinc (member, support, 28 statements 2026-05-16 to 2026-07-02): ACK, reviewed extensively and verified fixes
w0xlt (contributor, objection, 3 statements 2026-06-04 to 2026-09-23): Raised objection regarding getrawtransaction -txindex regression when blocks directory is missing
josibake (member, objection, 14 statements 2026-06-10 to 2026-07-09): Approach NACK against folding filesystem_error into nullptr in FlatFileSeq::Open

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

The short text the ranking pass compares across PRs.

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 ledger, generated 2026-10-10T01:22, confidence high, input hash 4936272ce579f2ec
thread read through 2026-10-09 (182 statements), head 768014068b
labels: Validation, Needs rebase

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

Goal

Reviewability

Agreement

Files

Card

Data

Categories

P2P

Validation