#36555 rpc: return error when invalidateblock fails

ReviewabilityReviewsAgreementSize
#36555 ja7ad rpc: return error when invalidateblock failsReady0Strong19 + 14 tests
  • Stops invalidateblock from reporting success when block disconnection fails and leaves a partial rollback
  • Explicitly rejects attempts to invalidate the genesis block with an invalid parameter error
  • Ready for review
  • No review verdicts recorded
  • Supports routing disconnection failures through state rather than reporting success (yusufyildiz34)
  • Suggests documenting partial rollback as a known limitation instead of adding regression code (l0rinc)
  • Requested simplifying tests and expanding the pull request description (l0rinc, pinheadmz)
  • 19 lines added or modified outside tests
  • 14 lines added or modified in tests
  • 4 lines removed in total
  • 6 files, 3 commits
RPC / REST / ZMQ#21 of 55
P3 · bug fix
  • P3 because invalidateblock is an administrative and testing tool rather than an everyday RPC method
  • Prevents callers from assuming a rollback succeeded when block disconnection actually failed
  • Ranking pass: Moved up 6: an RPC reporting success after a partial chain rollback is a false-success bug of the same class as the ones ranked just above.
Ranking notes
Validation#28 of 53
P3 · bug fix
  • P3 because it improves error accounting during rollbacks without altering consensus rules
  • Surfaces disconnection failures in BlockValidationState instead of silently ignoring them
  • Ranking pass: Moved up: fixes a reported bug (#36546) where invalidateblock reports success after a partial rollback, more concrete than the rest of P3
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

Populates BlockValidationState with an error message when Chainstate::InvalidateBlock fails to disconnect a block or encounters an active-chain conflict during invalidation. Adds an explicit RPC parameter check to invalidateblock to reject invalidating the genesis block with RPC_INVALID_PARAMETER. Extends unit tests for disconnect failures due to missing undo data and functional tests for genesis block invalidation.

Problem: When InvalidateBlock fails during block disconnection (for example, due to corrupted undo data), the chain is left partially rolled back while the invalidateblock RPC silently ignores the failure and returns success (null) to the caller. Additionally, attempting to invalidate the genesis block is not explicitly validated up front.

Evidence of importance:

  • Issue #36546: 'rpc: invalidateblock reports success when a block can't be disconnected'
  • PR description: '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.'

Scope: CMakeLists.txt contains an apparent stray change adding src/pubkey.cpp to the core_interface target.

Risk of a mistake: medium (rpc_api, validation, build). Touches block disconnection and invalidation error handling in validation, and includes a stray change to CMake build definitions.

Changed since the previous assessment: Added an accidental modification to CMakeLists.txt linking src/pubkey.cpp to core_interface; the core logic for validation error reporting and genesis check remains the same.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • author: response to l0rinc regarding documenting the limitation versus adding regression code 2026-10-20
  • reviewer: yusufyildiz34 to review author's pushed fixes 2026-10-20

Agreement

Strong

substantive support, no open objection (yusufyildiz34)

Thread notes: Added review comments from l0rinc and yusufyildiz34. Yusufyildiz34's objections regarding the block name in the error message and the genesis block error code were addressed and fixed by ja7ad.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
pinheadmz (member)question, usefulnessCan the author guarantee having read the AI policy, that the contribution conforms to it, and that real human users have encountered the problem being solved?accepted · author confirmed writing the code and understanding the AI policy; reviewer acceptedansweredyes2026-10-16: 'can you guarantee us that you have read our AI Policy and this particular contribution conforms to that policy?'
Status from: 2026-10-17: pinheadmz: 'Great!'
pinheadmz (member)suggestion, maintenanceAdd a thorough description to the pull requestopenansweredno2026-10-17: 'Please add a thorough description to the PR.'
l0rinc (member)suggestion, approachDocument the partial rollback behavior as a known limitation instead of adding validation or regression codeopenansweredno2026-10-20: 'I think this remaining partial-state behavior should be documented as a known limitation instead of adding more AssumeUTXO-specific validation or regression code.'
l0rinc (member)suggestion, maintenanceSimplify the tests and consider adding the AssumeUTXO reproducer in a collapsible sectionopenansweredno2026-10-20: 'Please try to simplify the tests and maybe add the AssumeUTXO reproducer in a collapsible section in the PR'
yusufyildiz34 (none)objection, correctnessThe error message names the wrong block (pindex instead of to_mark_failed)fixed · fix pushed · author updated the error message to name to_mark_failedchangedyes2026-10-20: 'The error message in the third hunk names the wrong block.'
Status from: 2026-10-20: ja7ad: 'It's to_mark_failed now.'
yusufyildiz34 (none)objection, interfaceUsing state.Error() for the genesis block returns database error (-20) rather than an invalid parameter error (-8 / RPC_INVALID_PARAMETER)fixed · fix pushed · author updated the RPC helper to reject the genesis block with -8changedyes2026-10-20: 'Rejecting height 0 in the RPC helper next to the existing LookupBlockIndex() check, with RPC_INVALID_PARAMETER, would give a more accurate code'
Status from: 2026-10-20: ja7ad: 'the RPC now rejects it with -8 next to "Block not found"'
yusufyildiz34 (none)question, maintenanceDoes this change need a release note?answered · fix pushed · author confirmed adding a release noteansweredyes2026-10-20: 'Does this want a release note? invalidateblock now throws where it previously returned null.'
Status from: 2026-10-20: ja7ad: 'Squashed the commits and added a body and a release note.'

Support:

  • yusufyildiz34 (none) (2026-10-20) [read: src/rpc/blockchain.cpp, consensus/validation.h]: invalidateblock reporting success after a partial rollback is clearly wrong, and routing the failures through state is cleaner than patching the RPC helper

Participants:

  • pinheadmz (member, question, 2 statements 2026-10-16 to 2026-10-17): asked about AI policy compliance and requested a thorough PR description
  • l0rinc (member, neutral, 1 statements 2026-10-20 to 2026-10-20): suggested documenting partial-state behavior as a known limitation and simplifying tests
  • yusufyildiz34 (none, objection, 1 statements 2026-10-20 to 2026-10-20): supported the concept but objected to the error message naming the wrong block and returning database error for genesis

Files

15 lines under test/bench/ci.

  • src/test/blockchain_tests.cpp +10/-1
  • doc/release-notes-36555.md +9/-0
  • src/validation.cpp +5/-2
  • test/functional/rpc_invalidateblock.py +4/-0
  • CMakeLists.txt +2/-1
  • src/rpc/blockchain.cpp +3/-0

Card

The short text the ranking pass compares across PRs.

Ensures the invalidateblock RPC and validation layer properly report errors when block invalidation fails. Previously, if block disconnection failed midway (such as from missing or corrupt undo data), InvalidateBlock returned false without setting an error state, causing the RPC to report success despite leaving the active chain partially rolled back (issue #36546). The PR records specific error messages into BlockValidationState, adds an explicit RPC check preventing invalidation of the genesis block, and adds unit and functional test coverage.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-22T01:13, confidence high, input hash 677410b05c570999
thread read through 2026-10-20 (6 statements), head 4aeb6a8fad
labels: RPC/REST/ZMQ

Categories

RPC / REST / ZMQ

P3 · bug fix (ranking pass: Moved up 6: an RPC reporting success after a partial chain rollback is a false-success bug of the same class as the ones ranked just above.)

Fixes a bug where invalidateblock silently returned success despite a failure disconnecting blocks that left the chain partially rolled back. Because invalidateblock is primarily an administrative and testing tool rather than a standard hot method, the impact on everyday RPC callers is limited.

Membership: Carries the RPC/REST/ZMQ label and modifies src/rpc/blockchain.cpp to report errors on failed invalidations and validate parameters.

Factors: security/stability 1, bug 1, performance 0, user value 1, leverage 0

Validation

P3 · bug fix (ranking pass: Moved up: fixes a reported bug (#36546) where invalidateblock reports success after a partial rollback, more concrete than the rest of P3)

Surfaces block disconnection failures in Chainstate::InvalidateBlock by populating BlockValidationState instead of silently ignoring them. The change improves internal state accounting during rollbacks without modifying consensus rules or core validation algorithms.

Membership: Changes Chainstate::InvalidateBlock error state propagation in src/validation.cpp and adds blockchain unit tests for disconnect failures.

Factors: security/stability 1, bug 1, performance 0, user value 1, leverage 0