rpc: return error when invalidateblock fails #36555

<!-- *** Please remove the following help text before submitting: *** Pull requests may be closed immediately if they: - do not have a rationale and clear improvement - do not adhere to doc/AI_POLICY.md GUI-related pull requests should be opened against https://github.com/bitcoin-core/gui first. See CONTRIBUTING.md -->

<!-- Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly: * Any test improvements or new tests that improve coverage are always welcome. * All other changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`). Contributors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change. * Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed. * Features are welcome, but might be rejected due to design or scope issues. If a feature is based on a lot of dependencies, contributors should first consider building the system outside of Bitcoin Core, if possible. * Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. -->

<!-- Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time. -->

invalidateblock returns null even when the block was not invalidated because the RPC does not pay attention to Chainstate::InvalidateBlock() return value. If there is a problem disconnecting the block (for example, the undo data is corrupt), the chain remains partially rolled back when telling the user that the operation succeeded. This creates an error in every failure case, including the genesis block case, and makes RPC look at the return value. The error reports a failed block and how far the chain was rolled back before the block returned.

Fixes #36546

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

External sites

• Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36555
• Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36555.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36555#author-ralph

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Before we spend precious human resources reviewing your submission, can you guarantee us that you have read our AI Policy and this particular contribution conforms to that policy?

It's important for us to know that real human users have encountered the problem you are trying to solve and there is a worthwhile motivation for this work.

Before we spend precious human resources reviewing your submission, can you guarantee us that you have read our AI Policy and this particular contribution conforms to that policy?

It's important for us to know that real human users have encountered the problem you are trying to solve and there is a worthwhile motivation for this work.

I’ve read and completely understand the AI policy.

Just to be clear, I wrote this patch myself with full understanding of the code. I have a background in blockchain development and have verified the fix end-to-end with unit tests and devnet testing.

Great! Please add a thorough description to the PR.

I haven't spent a lot of time reviewing the change, but instead of the complicated manual corruption we can probably reproduce the same partial rollback is even simpler to reproduce with AssumeUTXO without corrupting or deleting files.

But given that I'm working on removing AssumeUTXO now, I think this remaining partial-state behavior should be documented as a known limitation instead of adding more AssumeUTXO-specific validation or regression code.

<details> <summary>Functional test reproducing the partial rollback</summary>

diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
index 8727a9925ec..250d31a4b09 100755
--- a/test/functional/feature_assumeutxo.py
+++ b/test/functional/feature_assumeutxo.py
@@ -660,6 +660,24 @@ class AssumeutxoTest(BitcoinTestFramework):
         self.assert_only_network_limited_service(n1)
         dumb_sync_blocks(src=n0, dst=n1, height=PAUSE_HEIGHT)
 
+        self.log.info("Test invalidateblock stops at the unavailable snapshot base")
+
+        def active_state():
+            info = n1.gettxoutsetinfo(use_index=False)
+            return {key: info[key] for key in ("bestblock", "hash_serialized_3")}
+
+        before = active_state()
+        snapshot_base = n1.getblockhash(SNAPSHOT_BASE_HEIGHT)
+        first_removed = n1.getblockhash(SNAPSHOT_BASE_HEIGHT + 1)
+        assert_equal(n1.invalidateblock(snapshot_base), None)  # TODO: should raise an RPC error
+        after = active_state()
+        assert_equal(after["bestblock"], snapshot_base)  # TODO: should be before["bestblock"]
+        assert_not_equal(after["hash_serialized_3"], before["hash_serialized_3"])  # TODO: should be unchanged
+
+        self.log.info("Restore the snapshot chain after the characterization")
+        n1.reconsiderblock(first_removed)
+        assert_equal(active_state(), before)
+
         self.log.info("Checking that blocks are segmented on disk")
         assert self.has_blockfile(n1, "00000"), "normal blockfile missing"
         assert self.has_blockfile(n1, "00001"), "assumed blockfile missing"

</details>

Please try to simplify the tests and maybe add the AssumeUTXO reproducer in a collapsible section in the PR (if you agree that it characterizes the same problem).

Reviewed the diff; I have not built or tested the branch.

Concept makes sense — invalidateblock reporting success after a partial rollback is clearly wrong, and routing the failures through state so the existing RPC error path picks them up is cleaner than patching the RPC helper.

Two things on the implementation.

The error message in the third hunk names the wrong block.

if (m_chain.Contains(*to_mark_failed)) {
    return state.Error(strprintf("Block %s was not invalidated because it is still in the active chain (interrupted?)", pindex->GetBlockHash().ToString()));
}

The condition tests to_mark_failed, but the message prints pindex. to_mark_failed is reassigned inside the disconnect loop (to_mark_failed = disconnected_tip;), so once at least one iteration has run the two are different blocks — to_mark_failed is the last disconnected block, a descendant of pindex. In that case the message names a block that may well have been disconnected successfully, while the block actually still in the chain goes unnamed. to_mark_failed->GetBlockHash() would match the condition.

state.Error() for the genesis case.

ModeState in consensus/validation.h documents M_ERROR as //!< run-time error, and the RPC maps IsError() to RPC_DATABASE_ERROR. So passing the genesis hash now returns -20 Database error: Cannot invalidate genesis block. That is a caller error, not a runtime one, and the neighbouring "Block not found" case in the same helper uses RPC_INVALID_ADDRESS_OR_KEY (-5). Rejecting height 0 in the RPC helper next to the existing LookupBlockIndex() check, with RPC_INVALID_PARAMETER, would give a more accurate code and keep M_ERROR meaning an actual internal failure — which the disconnect-failure hunk legitimately is.

nit: the two commits have near-identical subjects, and the second largely reverts the first's changes to src/rpc/blockchain.cpp and the functional test. They look like a fixup pair that wants squashing; both commit bodies are also empty.

Does this want a release note? invalidateblock now throws where it previously returned null.

Finally, @l0rinc's point about documenting the partial-state behaviour instead of adding regression code is still open — if that direction wins, most of the above is moot.

@yusufyildiz34 Thanks for the review. You're right that it should print the block the check is about. It's to_mark_failed now.

Genesis: agreed, a bad argument isn't a runtime error, the RPC now rejects it with -8 next to "Block not found" and InvalidateBlock() retains its old return false.

Squashed the commits and added a body and a release note.

I also tested on regtest with a corrupted undo record: invalidateblock now fails with the rollback height in the error and reconsiderblock restores the chain.