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/35003.

Reviews

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

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #35731 (Indexes: Harden the flush-error notification invariant by arejula27)
• #35676 (util: Abort in CheckDiskSpace/FlatFileSeq::Open on rare exceptions by maflcko)
• #35139 (test: Add thread-safe fast-failing test macros by maflcko)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

• txospenderindex -> txspendingindex [misspelled index name in the docstring; the intended term is unclear otherwise]

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

• FlatFileSeq(data_dir, "a", 100) in src/test/flatfile_tests.cpp

Possible places where comparison-specific test macros should replace generic comparisons:

• src/test/flatfile_tests.cpp BOOST_CHECK_THROW(seq.Allocate(FlatFilePos(0, 0), 1, out_of_space), fs::filesystem_error); -> consider using BOOST_CHECK_EXCEPTION with a predicate that checks the expected error message/details instead of only the exception type.

^{2026-07-02 15:13:56}

concept ACK
Thanks for the thorough PR description! Definitely helps me to understand the code changes before even looking at them. Can also see its minimal changes with lots of test coverage, will review.

Line 5915 in SendMessages has assert(ret) after a ReadBlock call in the compact block announcement path. I think this is the same pattern as the GETBLOCKTXN assert fixed in commit 4. I believe this is the "another assertion scenario in compact block relay" mentioned in the PR description. Is this planned as a follow-up? Happy to take it if so.

Yes @frankomosh. All yours :).

Looks like the test fails on the second commit, or so?

It seems the assertion error message is libc-dependent.
Linux logs "Assertion `expr' failed" while MacOS logs "Assertion failed: (expr), ...".

Will make the wait_until_stopped expected error a bit more general and check for "Assertion" wording only. That is more than enough to check the crash reason.
The last commit makes it clear with the fatal error change anyway.

Haven't read all of the discussion threads, but since my last review, the only change seems to be moving the chmod, which doesn't seem like a good change. (See inline comment)

Also, the suggestion by @josibake to use exceptions here sounds interesting. However, I haven't looked at the details and code, so it is harder to tell which is the right fit here. I'd be happy to review either approach, and wonder what other reviewers prefer here?

lgtm ACK d4f9539 🐻

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK d4f95399b5af4643b90bb396c4d172e908ae87c2 🐻
oH+HVBgFxqvZd9iiXWWh2X30aRqlpbsZRmLXAWKmHc3lkuFDKeMRDVJRktd+GT8xFT0DR5CtMzWrWtrUHDDUCA==

I hopefully tackled all remaining feedback. Also added coverage for the GETCFILTERS p2p path (swallowed exception) and the RPC getrawtransaction and gettxspendingprevout paths (low-level filesystem exception reaching the user).

It seems to me compact-block announcements still abort on the same kind of failure:

bitcoin/src/net_processing.cpp

Lines 5997 to 5998 in dc282ff

	const bool ret{m_chainman.m_blockman.ReadBlock(block, *pBestIndex)};
	assert(ret);

Is that deliberate, or could we use the same fatal-error style there?

Yes, it is in the PR description. You probably forgot this one #35003 (review) and my reply #35003 (comment).

reACK 7680140

test-doc-only re-ACK 7680140 🍏

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: test-doc-only re-ACK 768014068b8ccdb205b4ad8e666eed07db247221  🍏
eTqlGYFVii78opeRBwUyzB67YFv2RzmwLLXHJWOpZnMwKSqaIkoE59cn2wV8OTyGy5qmBLInA6fVRY01kz3RAQ==

@rkrux @w0xlt @frankomosh @josibake can you take another look here?

Approach NACK

While I certainly appreciate the work @furszy has put into this, the PR in its current form is not the direction I think we should be going. I spent some time reading and thinking through a more comprehensive error handling strategy for the codebase in order to convince myself I'm not pushing back simply for aesthetics, and I am convinced removing exceptions from the low level file primitive and folding it into a nullptr is the wrong direction.

I would much prefer something like this in, instead of the FlatFileSeq commit:

index b060108c18..df89984d53 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -821,13 +821,23 @@ void BlockManager::UnlinkPrunedFiles(const std::set<int>& setFilesToPrune) const

 AutoFile BlockManager::OpenBlockFile(const FlatFilePos& pos, bool fReadOnly) const
 {
-    return AutoFile{m_block_file_seq.Open(pos, fReadOnly), m_obfuscation};
+    try {
+        return AutoFile{m_block_file_seq.Open(pos, fReadOnly), m_obfuscation};
+    } catch (const fs::filesystem_error& e) {
+        LogError("OpenBlockFile failed for %s: %s", pos.ToString(), e.what());
+        return AutoFile{nullptr, m_obfuscation};
+    }
 }

 /** Open an undo file (rev?????.dat) */
 AutoFile BlockManager::OpenUndoFile(const FlatFilePos& pos, bool fReadOnly) const
 {
-    return AutoFile{m_undo_file_seq.Open(pos, fReadOnly), m_obfuscation};
+    try {
+        return AutoFile{m_undo_file_seq.Open(pos, fReadOnly), m_obfuscation};
+    } catch (const fs::filesystem_error& e) {
+        LogError("OpenUndoFile failed for %s: %s", pos.ToString(), e.what());
+        return AutoFile{nullptr, m_obfuscation};
+    }
 }

 fs::path BlockManager::GetBlockPosFilename(const FlatFilePos& pos) const

This preserves the exception, and moves the translation of that error into BlockManager which I have argued is the appropriate layer for handling the exception. This directly solves the stated problem: exceptions were being swallowed at a higher layer and not handled. It solves the problem by handling the exception at the appropriate layer, without needing to touch the lower level file primitives.

What my proposal does not address is the BlockFilterIndex uses of FlatFileSeq. I think thats fine, as those are not in the critical path. I'd argue its even desirable, as we may want to have a different policy on how FlatFileSeq failures are handled there, further demonstrating the strength of this approach.

I do understand that the approach in this PR was done with consistency in mind, but I don't think that is enough of a justification when the thing being made consistent is, as I've tried to argue here, a bad design.

Let's recall this pull has more than 150 comments and is sitting for 3 months (which is reasonable for its size). However, it doesn't seem to be getting traction and has outstanding issues, some of which are enough to NACK it.

So, 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.

I think it would be good to step back and answer a meta question:

• Do we even care about providing fallbacks for those exceptions? Let's recall they can only happen when there is a coding bug, hardware corruption, or the users physically fiddles with the hardware (e.g. disconnect an external blocksdir)

There are different answers:

• "They are so rare, we don't care about them at all". Then I think this pull (and other ones) can be closed and we can all move on to other things.
• "They are rare, but providing a fatal shutdown path in all of them is too tedious". Then, I think it is fine to manually terminate/abort in the functions (See util: Abort in CheckDiskSpace/FlatFileSeq::Open on rare exceptions #35676)
• "They are rare, but providing a fatal shutdown path in all of them should be done". Then, I think there needs to be a single large pull request to do it in all paths.
• ... Some other answer I am not seeing?

Thanks for zooming out @maflcko , I think its helpful. I'll respond here, but this is starting to feel like a discussion that can be moved to an issue around error handling in general, considering the discussion now spans two PRs.

Do we even care about providing fallbacks for those exceptions?

This is a policy question. I think its up to the caller to decide, which is why I've been pushing back on folding multiple errors into a nullptr, and pushing back on having the low level function abort instead of propagating the error up. Said differently, the answer to this question will change in the future, and it will likely be different for different callers.

Some other answer I am not seeing?

I think we should stop swallowing errors with broad catch alls that do nothing. Instead, if we can't catch it, let it terminate. If we would prefer a graceful fatal shutdown instead, have broad catch all that executes a safe shut down. Even better, have each boundary own their own error handling policy as I recommend here: BlockManager should own storage errors, BlockfilterIndex should manage storage errors related to indexes, etc etc.

In each of these boundaries, if they have the information they need to know that a graceful fatal shutdown is possible/safe/etc, they can do that. If the cant make that call, they should allow the error to propagate.

Restating the discussion we already had: #35003 (comment).

Today, every filesystem error in FlatFileSeq::Open is handled through the same path except for this one case. Your proposal @josibake keeps that one case special, how leaving a function with two error-signaling paths for the same class of failure makes sense to you? That is just error-prone. It doesn't really matter which kind of error we are talking about.

Overall, following the by the books approach without pondering the actual sources is just bad in my view. And treating this bug fix PR as a broader architectural design discussion spot, instead of just creating a new issue or PR (as it was suggested in my first replies to you), is just blocking progress unnecessarily.

Let's recall this pull has more than 150 comments and is sitting for 3 months (which is reasonable for its size)

The PR is 97% about adding tests. It changes 5 lines of code in the node at most.

Let's recall this pull has more than 150 comments and is sitting for 3 months (which is reasonable for its size)

The PR is 97% about adding tests. It changes 5 lines of code in the node at most.

Sorry for being unclear, with size I didn't mean the LOC in ./src, but the review size, also approximated by the number of diverse tests. Let's recall that the exceptions involved here concern all threads: scheduler, indexes, initload, main, p2p, rpc, except for the script and fetcher threads. Also, the exceptions here (or the return value) can propagate up ~anywhere in the codebase (except maybe for wallet and unrelated low-level utils). So any reviewer must be familiar with the whole codebase.

I can't provide an answer here, but I think reviewers should ask themselves the meta question (#35003 (comment)) and then hopefully there is rough consensus on the answer, whatever that will be.