ipc: Fail-stop the node when Bitcoin IPC queue overflows#436
ipc: Fail-stop the node when Bitcoin IPC queue overflows#436pradhyum6144 wants to merge 1 commit into
Conversation
|
Flagging before review: #318 has two asks, this PR only covers one. Covered (requirement 1- "halt + noisy logs"): Normal/High/Critical overflow now emits error! + std::process::abort(). Note: the previous panic! never actually halted the node it ran inside tokio::task::spawn_local, so Tokio caught it and the processor task died silently while callers blocked on oneshot channels. abort() bypasses that. Not covered (requirement 2 - "ensure this can never happen"): Scoped out; I believe it's blocked on #283 (cmempoold split) + p2p rate limiting. Direction question: @krishi-agrawal suggested unbounding Critical + High queues so overflow is unreachable - a different philosophy than this PR (bounded + fail-stop). Happy to pivot: (a) Land as-is, track (2) against #283. |
zaidmstrr
left a comment
There was a problem hiding this comment.
Concept ACK db6e8f8
I don't like the idea of introducing EnqueueOutcome enum this is making code complex without adding much meaning, even though the test verifies overflow detection but not the abort response, so you end up testing the enum shape rather than the behaviour. Although your concern about the abort() is correct. The cleaner and simpler approach here is to use abort() inside enqueue() and don't treat low priority requests differently than critical and high. Because if a queue is full on low priority, then still that means the Bitcoin node is unresponsive.
Drop the EnqueueOutcome enum and inline std::process::abort() inside PriorityRequestQueue::enqueue. A full queue at any priority (including Low) means bitcoind is unresponsive, so fail-stop uniformly. Addresses zaidmstrr's review on braidpool#436.
zaidmstrr
left a comment
There was a problem hiding this comment.
Thanks for working on the things I suggested previously. This now looks way better than before, but there are some more suggestions below you can look at.
| BitcoinRequest::SubmitSolution { priority, .. } => *priority, | ||
| }; | ||
|
|
||
| let result = match priority { |
There was a problem hiding this comment.
Removing this piece of code is not the intended fix for this issue. I suggest just introducing a simple abort() and leaving the old code unchanged.
…#318) panic!() inside tokio::task::spawn_local is silently swallowed by the Tokio runtime and never surfaces to operators. Replace with std::process::abort() which bypasses unwinding and SIGABRTs the process unconditionally, making the failure visible. All existing queue-management logic is unchanged.
1aa9694 to
7c86eab
Compare
| if self.critical_queue.len() >= self.max_queue_sizes.critical { | ||
| error!("BITCOIN IPC QUEUE FULL - CRITICAL QUEUE EXCEEDED LIMIT"); | ||
| panic!("FATAL: Cannot communicate with Bitcoin Core. Queue is full."); | ||
| error!("FATAL: Bitcoin IPC critical queue full - aborting to prevent consensus desync. See issue #318."); |
There was a problem hiding this comment.
We don't need to change the previous logs please leave them as they are. And sign your commits.
| if self.normal_queue.len() >= self.max_queue_sizes.normal { | ||
| error!("BITCOIN IPC QUEUE FULL - NORMAL PRIORITY QUEUE EXCEEDED LIMIT"); | ||
| panic!("FATAL: Cannot communicate with Bitcoin Core. Queue is full."); | ||
| error!("FATAL: Bitcoin IPC normal queue full - aborting to prevent consensus desync. See issue #318."); |
There was a problem hiding this comment.
Why is the issue number listed in running logs not required and imo we can revert to previous log messages only .
Issue : #318
Changes
The existing
panic!inPriorityRequestQueue::enqueuenever halted the node: it runs insidetokio::task::spawn_local, where panics are caught by Tokio and only surface viaJoinHandle::await. The processor task died silently while callers blocked ononeshotchannels, leaving the node alive but unable to satisfy the consensus-critical IPC required bydocs/braidpool_spec.md.Replace the silent panic with
std::process::abort()on Normal/High/Critical overflow so operators see the failure. Low-priority overflow stays a soft drop (warn + continue) since it is non-consensus.Updates
EnqueueOutcomeenum returned byenqueue, matched by the processor task: Low → warn + drop, Normal/High/Critical → error +std::process::abort().is_overloaded()now checks all four priority queues.