Eight on_fatal calls in the connect batch run while the UTXO write window is held. The handler is caller-supplied, so what it does under that window is not something this code can see — and the two things it is most likely to reach for are exactly the two the window excludes.
The sites
The window opened in src/node/src/sync/block_tasks.cpp covers steps 4 to 9, lines 2181–2350. Inside it:
| line |
condition reported |
| 2189 |
the UTXO delta could not be applied |
| 2212 |
the deletions the batch owes could not be applied |
| 2279 |
a deletion the batch did not account for |
| 2286 |
the deletion sweep exhausted its attempts |
| 2293 |
undo data could not be captured |
| 2316 |
undo data could not be flushed |
| 2337 |
the durability barrier failed |
| 2347 |
the batch could not be published |
The five below step 9 are outside it (2380, 2391, 2417, 2436) and are not affected.
The risk
The node has two locks and one order: the transaction organizer takes validation_mutex_ and then, inside validator_.accept(), a UTXO read lease. A handler that takes either while the window is held is acquiring them in the opposite order.
Since #651 neither of those deadlocks silently — mempool_remove_for_block raises utxo_lock_order_error and a read lease from the window-holding thread raises utxo_reentry_error — so the outcome today is an exception thrown out of a fatal handler rather than a hang. That is better than a stopped node and still wrong: the fatal condition being reported is replaced by a second failure, and the operator reads about the lock order instead of the disk error that actually killed the batch. It also depends on a guard from another change rather than on this code being correct.
The window is not needed to report anything. Every one of these sites has already decided the batch is over.
Fix
Record the reason, let the window scope end, then invoke on_fatal — or state a contract that the handler may take no other lock and check it. The first is simpler and does not constrain callers.
Control
A handler that takes validation_mutex_ (through mempool_remove_for_block, which is the reachable form) must not raise utxo_lock_order_error when a batch reports a fatal condition. Today it does, which is what makes the negation red.
Found by review during #654. Not fixed there: that change is about the connected-tip marker and this is the window's scope, which came from #651.
Eight
on_fatalcalls in the connect batch run while the UTXO write window is held. The handler is caller-supplied, so what it does under that window is not something this code can see — and the two things it is most likely to reach for are exactly the two the window excludes.The sites
The window opened in
src/node/src/sync/block_tasks.cppcovers steps 4 to 9, lines 2181–2350. Inside it:The five below step 9 are outside it (2380, 2391, 2417, 2436) and are not affected.
The risk
The node has two locks and one order: the transaction organizer takes
validation_mutex_and then, insidevalidator_.accept(), a UTXO read lease. A handler that takes either while the window is held is acquiring them in the opposite order.Since #651 neither of those deadlocks silently —
mempool_remove_for_blockraisesutxo_lock_order_errorand a read lease from the window-holding thread raisesutxo_reentry_error— so the outcome today is an exception thrown out of a fatal handler rather than a hang. That is better than a stopped node and still wrong: the fatal condition being reported is replaced by a second failure, and the operator reads about the lock order instead of the disk error that actually killed the batch. It also depends on a guard from another change rather than on this code being correct.The window is not needed to report anything. Every one of these sites has already decided the batch is over.
Fix
Record the reason, let the window scope end, then invoke
on_fatal— or state a contract that the handler may take no other lock and check it. The first is simpler and does not constrain callers.Control
A handler that takes
validation_mutex_(throughmempool_remove_for_block, which is the reachable form) must not raiseutxo_lock_order_errorwhen a batch reports a fatal condition. Today it does, which is what makes the negation red.Found by review during #654. Not fixed there: that change is about the connected-tip marker and this is the window's scope, which came from #651.