fix(database): abort P39 cascade recovery when pop_block() makes no progress (last write-lock deadlock loop) - #118
Merged
Conversation
…rogress #117 added a no-progress guard to the three fork-switch pop loops in _push_block() but left the fourth pop loop — the P39 cascade-recovery path — unguarded. It has the identical deadlock: when pop_block()'s undo() is a no-op (committed/empty undo session while head is still above LIB), head_block_num() never decreases, so neither '> original_head' nor '> lib_num' can ever become false and the loop spins forever holding the global chainbase write lock — the same wedge (writer_held_ms climbing for hours) observed during the minority-fork reorg incident. Mirror #117's recovery-loop pattern: detect that head_block_id() did not change after pop_block() and break. The existing _fork_db.reset() + start_block() right after the loop already restores fork_db to the database head, so break leaves consistent state. This closes the last unguarded no-progress pop loop in _push_block().
chiliec
marked this pull request as ready for review
June 16, 2026 04:47
web3blind
pushed a commit
to web3blind/viz-cpp-node
that referenced
this pull request
Jul 3, 2026
Observability backstop for the global chainbase write lock. A wedged push_block() holding the write lock takes the whole node down (every reader + the validator's own block production block on it; observed: writer_held_ms climbing for hours during a minority-fork reorg). A monitor thread watches the per-push_block hold time and, when a single push_block holds the write lock past PUSH_BLOCK_STALL_WARN_SEC (60s) with no operation applied in that window, emits a loud, throttled stderr diagnostic. It also exposes push_block_lock_held_ms() for external healthchecks. LOG-ONLY by design: it never calls std::_Exit and never mutates state, so a false positive can only add a log line, never a node death. A force-exit watchdog was deliberately rejected: on a deterministic heavy block (e.g. a future hardfork) it would self-destruct the whole network at once -- far worse than the localized hang it would guard against. Restart decisions stay with the operator / orchestrator, driven by push_block_lock_held_ms(). Complements the pop-loop no-progress guards (VIZ-Blockchain#117/VIZ-Blockchain#118): those fix the known infinite loops; this only makes any remaining/future write-lock stall visible.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #117. That PR fixed the write-lock deadlock in the three fork-switch pop loops of
_push_block(), but a fourth pop loop — the P39 cascade-recovery path (libraries/chain/database.cpp, thewhile (head_block_num() > original_head && head_block_num() > lib_num)loop) — was left unguarded. It carries the identical deadlock.This PR closes it, so no unguarded no-progress pop loop remains in
_push_block().The bug
When
pop_block()'sundo()is a no-op — a committed/empty undo session while head is still above LIB —head_block_num()never decreases. Neither loop condition (> original_head,> lib_num) can ever become false, so the loop spins forever holding the global chainbase write lock (writer_held_msclimbing for hours), starving every reader and the validator's own block-production loop. This is the same wedge observed in the minority-fork reorg incident that motivated #117.The fix
Mirror #117's recovery-loop pattern exactly: capture
head_block_id()beforepop_block(), and if it is unchanged afterward,break. The_fork_db.reset()+start_block()that already run immediately after this loop restorefork_dbto the database head, so breaking leaves consistent state. Minimal, no behavior change on the healthy path.Verification status
Known follow-ups (out of scope here — deliberately not patched blind)
These do not cause the deadlock and are noted for tracking rather than fixed speculatively:
pop_block()advancesfork_dbbefore the no-opundo().pop_block()runs_fork_db.pop_block()thenundo(). On a no-op pop the fork_db head moves back one while the chainbase head does not, so the abort paths in fix(database): abort fork switch when pop_block() makes no progress (write-lock deadlock) #117's main-switch branch (_fork_db.remove(new_head)+ throw) can leavefork_dband chainbase desynced by one block. Likely self-heals on the next resync; worth confirming. (This loop is unaffected — it resets fork_db right after.)undo_all()s cleanly). The fix prevents the catastrophic lock-starvation; it does not guarantee automatic fork convergence.Possible cleanup
The no-progress guard now appears in four near-identical copies. A small private helper (e.g. a guarded pop returning
falseon no-progress) would de-duplicate them — left out here to avoid churning the freshly-merged #117 loops.