blockstorage: Defer error notifications to callers #36171

w0xlt commented at 11:41 PM on September 4, 2026: contributor

BlockManager currently invokes fatalError and flushError callbacks synchronously during storage operations, making it unsafe to protect those operations with a non-recursive mutex.

This PR introduces BlockStorageOutcome<T> to return the operation result together with ordered notifications. Callers dispatch them after the storage operation returns, including when a successful rollover produces multiple flush errors.

It also removes the notification dependency from BlockManager and adds coverage for ordered flush errors and fatal errors.

DrahtBot added the label Block storage on Sep 4, 2026

DrahtBot commented at 11:41 PM on September 4, 2026: contributor

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

Reviews

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

Type	Reviewers
Concept ACK	jeanpablojp

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36066 (validation: Separate check-only version of ConnectBlock by optout21)
#35731 (Indexes: Harden the flush-error notification invariant by arejula27)
#35714 (validation: stop writes after flush failure by l0rinc)
#35646 (RFC: Separate out runtime errors from BlockValidationState using util::Expected by yuvicc)
#35570 (refactor: Change some validation.cpp methods to return BlockValidationState by optout21)
#35307 (blockstorage: keep snapshot base in normal blockfile range by shuv-amp)
#35003 (validation: improve block data I/O error handling in P2P paths by furszy)
#32554 (bench: replace embedded raw block with configurable block generator by l0rinc)
#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.

blockstorage: Return notification errors to callers

Block storage currently invokes fatalError and flushError callbacks
from inside storage operations. This prevents those operations from
being protected by a non-recursive mutex without exposing callback
re-entry to that mutex.

Return the operation value together with ordered notification errors,
and have validation deliver them synchronously after the storage call
returns. A result object also represents successful writes accompanied
by rollover flush errors without output parameters.

bbfaa44a36

blockstorage: Remove notification dependency

BlockManager no longer invokes application notifications after
returning storage errors to validation. Remove the unused notification
reference from its options and construction sites so future storage
code cannot accidentally call back through BlockManager.

4383922309

test: Cover returned block storage errors

Exercise a successful rollover that accumulates block and undo flush
errors in order, and the missing-assumeutxo path that returns a fatal
error. Also assert ordinary writes return no notification errors.

8543c51d0d

w0xlt force-pushed on Sep 5, 2026

DrahtBot added the label CI failed on Sep 5, 2026

DrahtBot commented at 12:17 AM on September 5, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/33930447031/job/101207781617</sub> <sub>LLM reason (✨ experimental): CI failed because IWYU detected missing/incorrect #includes (generated “Failure generated from IWYU” and exited non-zero).</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label CI failed on Sep 5, 2026

jeanpablojp commented at 2:21 AM on September 6, 2026: contributor

Concept ACK

Ran the full unit suite and the block functional tests on the head, and a forced block write failure in regtest matches the merge-base line for line.

in src/node/blockstorage.h:202 in 8543c51d0d

 197 | +    bilingual_str message;
 198 | +};
 199 | +
 200 | +using BlockStorageErrors = std::vector<BlockStorageError>;
 201 | +
 202 | +/**

jeanpablojp commented at 2:21 AM on September 6, 2026:

The comment does not say the caller has to deliver the notifications, which is what this moves out of BlockManager. Worth adding a line here? And [[nodiscard]] on the struct itself might be worth it, it builds clean on bitcoind, test_bitcoin and bench_bitcoin and would catch WriteBlock, WriteBlockUndo and LoadBlockIndexDB, the three public ones without it. WriteBlock's @returns and the two Return false if comments still describe the old return type.

in src/node/blockstorage.cpp:942 in 8543c51d0d

 940 | @@ -925,15 +941,16 @@ FlatFilePos BlockManager::FindNextBlockPos(unsigned int nAddSize, unsigned int n
 941 |      bool out_of_space;
 942 |      size_t bytes_allocated = m_block_file_seq.Allocate(pos, nAddSize, out_of_space);

jeanpablojp commented at 2:21 AM on September 6, 2026:

Whatever notifications the rollover flush produced sit in outcome until AcceptBlock delivers them, and in between Allocate calls CheckDiskSpace, which uses the throwing fs::space overload. If it throws, outcome is gone before AcceptBlock gets it, and the catch there reports only the generic error. The log lines stay, but the flush callbacks never fire, only fatal_error does. Is discarding the notifications here intended?

in src/test/blockmanager_tests.cpp:124 in 8543c51d0d

 121 | +    const auto outcome{blockman.LoadBlockIndexDB(uint256::ONE)};
 122 | +    BOOST_CHECK(!outcome.value);
 123 | +    BOOST_REQUIRE_EQUAL(outcome.notifications.size(), 1);
 124 | +    BOOST_CHECK(outcome.notifications[0].type == node::BlockStorageErrorType::FATAL);
 125 | +    BOOST_CHECK_NE(outcome.notifications[0].message.original.find("Assumeutxo data not found for the given blockhash"), std::string::npos);
 126 |  }

jeanpablojp commented at 2:21 AM on September 6, 2026:

The two new tests cover the FLUSH pair and LoadBlockIndexDB's FATAL. Nothing covers the FATAL that WriteBlock returns when it cannot open the block file, and deleting the push_back for Failed to write block. leaves the whole suite green. The same directory trick from the flush test works here. I ran this one on the head, it passes, and it fails with that push_back removed. Worth adding?

}

BOOST_AUTO_TEST_CASE(blockmanager_returns_write_fatal_error)
{
    const auto params{CreateChainParams(ArgsManager{}, ChainType::MAIN)};
    const BlockManager::Options blockman_opts{
        .chainparams = *params,
        .blocks_dir = m_args.GetBlocksDirPath(),
        .block_tree_db_params = DBParams{
            .path = m_args.GetDataDirNet() / "blocks" / "index",
            .cache_bytes = 0,
        },
    };
    BlockManager blockman{*Assert(m_node.shutdown_signal), blockman_opts};

    // Make the block file the next write would open impossible to open.
    BOOST_REQUIRE(fs::create_directory(m_args.GetBlocksDirPath() / "blk00000.dat"));

    CBlock block;
    LOCK(::cs_main);
    const auto outcome{blockman.WriteBlock(block, /*nHeight=*/0)};
    BOOST_CHECK(outcome.value.IsNull());
    BOOST_REQUIRE_EQUAL(outcome.notifications.size(), 1U);
    BOOST_CHECK(outcome.notifications[0].type == node::BlockStorageErrorType::FATAL);
    BOOST_CHECK_EQUAL(outcome.notifications[0].message.original, "Failed to write block.");
}

in src/validation.cpp:2141 in 8543c51d0d

2137 | @@ -2138,6 +2138,20 @@ bool FatalError(Notifications& notifications, BlockValidationState& state, const
2138 |      return state.Error(message.original);
2139 |  }
2140 |  
2141 | +static void NotifyBlockStorageErrors(Notifications& notifications, node::BlockStorageErrors errors)

jeanpablojp commented at 2:21 AM on September 6, 2026:

This one is new and has five callers. In the tests it runs constantly with an empty list, and only one caller ever delivers an actual notification, the assumeutxo path feature_assumeutxo.py covers end to end. I forced the other four by hand in regtest and they all deliver, but nothing here observes them. Worth a test for one of them?