InvalidateBlock concurrency issues #36117

While reviewing #34254, I asked Claude to examine for any multi-threaded issues with InvalidateBlock. It came up with the following crash which I verified:

• [rpc thread] in InvalidateBlock, populate the list of highpow_outofchain_headers here and then release cs_main. These headers are all better than the new tip we should be getting and will be used to update m_best_header later.
• [other thread] acquire cs_main and call AddToBlockIndex with a header that is not better than m_best_header but is better than every header in highpow_outofchain_headers. This means AddToBlockIndex won't update m_best_header here
• [rpc thread] acquire cs_main in the loop and update m_best_header based on the candidates in highpow_outofchain_headers here
• [rpc thread] calls CheckBlockIndex which fails on the assert since there is actually a header with more work than m_best_header: https://github.com/bitcoin/bitcoin/blob/05e49b342faa1412266951429c135e9f5daa30c2/src/validation.cpp#L5317-L5318

Haven't really thought about how to fix this too much.

Vibe-coded functional test: https://github.com/Crypt-iQ/bitcoin/tree/08282026/rpc_invalidateblock_crash Run with: <build_dir>/test/functional/test_runner.py rpc_invalidateblock.py --race-attempts=5000 and inspect stderr of node2 in the tmp test directory.

cc @mzumsande

there are a few known issues that invalidateblock has, including this one. I took advantage of one of them to trigger other issues that are harder to reach without it #35003 (review) (this is essentially a lack of atomicity). It would be good to fix that one too now that we are going to touch this.

@furszy is there a list of these issues so we can triage them? I think Claude found another one and I'm unsure if it's a dupe.

I drop them into random PR comments to make the scavenger hunt slightly more interesting. Jokes aside, no single issue listing them all I am aware of. We can start grouping them.

This m_best_header assert in CheckBlockIndex can be hit if an invalid block is given while an InvalidateBlock is happening:

• The loop to populate highpow_outofchain_headers only considers valid headers, this runs here
• If we receive the block (via p2p/rpc) for a header in this multimap and it happens to be invalid, the check that may update m_best_header to one of these candidates headers does not then check if the candidate was marked invalid in the meantime here.
• This may update m_best_header to an invalid block

This CheckBlockIndex assert can be hit if reconsiderblock runs at the same time as invalidateblock:

• the main loop will lock cs_main, mark disconnected blocks (and things in the candidate set) as BLOCK_FAILED_VALID, and release cs_main
• ReconsiderBlock will lock cs_main and may call ResetBlockFailureFlags on one of those blocks here
• the disconnect loop continues to run
• the assert is hit because there are valid blocks descending from invalid blocks

Vibe-coded reproducers for these two issues here. The first issue could be fixed by checking that the candidates are valid before updating m_best_header. The second issue could be fixed by preventing reconsiderblock from competing with invalidateblock via a dedicated mutex or perhaps by moving the CheckBlockIndex call after the InvalidChainFound here?

On a high-level, I tend to think that maybe InvalidateBlock just wants too much for a debug-only rpc that wouldn't really need to reverse thousands of blocks for any serious production purposes: It wants to be very fast, so the normal process of disconnecting blocks is being shortcut in various ways to avoid unnecessary work by caches such as highpow_outofchain_headers, avoiding loops over the entire block index. It also wants to release cs_main after each iteration, so that GUI etc. doesn't become unresponsive, leading to these concurrency issues (which at least only occur in rather unusual situations as far as I understand, the normal case of a mostly linear chain is very likely not affected).

Considering all that, I'd say that the options are

1. try to fix these issues one by one, potentially making the function even more complicated
2. simplify the function, at the cost of performance and/or longer locking periods (e.g. release cs_main every 20 blocks or so, but drop the optimizations and do the full loops over the block index before each release)
3. accept some of the issues as not super relevant in practice

Considering all that, I'd say that the options are

I think option 2 or 3 makes the most sense. The current design seems to have been introduced in #15402 which broke up cs_main locking. I think this was done to make it safely interruptible, limit unbounded growth of the scheduler queue causing high memory usage, and allow other threads to acquire cs_main. This comment by @willcl-ark seems to indicate that high memory usage is still present with really deep invalidateblock calls, so maybe we don't need to worry about that if we end up locking cs_main for longer.

Also, this is just an rpc call so perhaps we could add a disclaimer saying that asserts may happen in unexpected scenarios?

I encountered the same stale-state issue in a different situation, and I think it's worth fixing, even though it is triggered by an RPC call that is unlikely to be used often.

Leaving this race condition unresolved can affect other projects that update this part of the code and silently make the problem worse. At the very least, the tests created by @Crypt-iQ should be added to acknowledge the problem.

I vibe-coded a solution in the direction of the simplification mentioned by @mzumsande: https://github.com/w0xlt/bitcoin/commit/db8f4202c3c2544af8aae8723c41f704c1835698

It drops highpow_outofchain_headers and checks the current block index after each disconnection, keeping the block state consistent before releasing cs_main.

The fix keeps the state consistent with simpler code, but scanning the block index after each disconnection may make deep invalidations slower and delay other operations waiting for cs_main.

These extra scans happen only during explicit invalidateblock, not normal block processing, IBD, or natural reorgs.