fix: improve miner thread robustness - #7510
Conversation
Fixed the miner thread exiting (and stalling the chain for the remainder of the tenure) on transient error. The miner now retries when it hits DB contention, a parent block that has not been processed yet, a new parent block discovered mid-mining, or a mempool cache reset failure, instead of giving up on the tenure.
There was a problem hiding this comment.
Pull request overview
Improves miner resilience by retrying recoverable failures instead of ending the tenure’s mining thread.
Changes:
- Adds abort-aware retry handling for parent changes, DB errors, and timestamp validation.
- Retries failed mempool cache resets.
- Preserves underlying DB errors for retry classification.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
stacks-node/src/nakamoto_node/miner.rs |
Adds miner retry and abort behavior. |
changelog.d/miner-thread-retry-transient-errors.fixed |
Documents the robustness fix. |
Suppressed comments (1)
stacks-node/src/nakamoto_node/miner.rs:826
- This arm classifies all
DBErrorvalues as transient, but the wrapped error type also includes permanent conditions such asCorruption,ReadOnly,OldSchema,TooOldForEpoch, andBlockHeightOutOfRange(stackslib/src/util_lib/db.rs:54-91). Such faults will now loop and log until the tenure changes. Narrow this arm to explicitly retryable contention errors (and any intentionally retryable not-found cases), allowing all other errors to reach the fatal path.
Err(
ref e @ (NakamotoNodeError::MiningFailure(ChainstateError::DBError(_))
| NakamotoNodeError::DBError(_)),
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if let Err(e) = reset_result { | ||
| // A mempool DB error here is likely transient (e.g. lock | ||
| // contention); sleep and retry rather than exiting the miner | ||
| // thread. | ||
| warn!("Miner: failed to reset mempool caches, will try again: {e:?}"); |
There was a problem hiding this comment.
I don't think it hurts anything if the miner keeps retrying with no possibility of recovery, and that case might be more obvious in the logs. I'll take a closer look.
There was a problem hiding this comment.
Only thing that I'd be careful of is the potential for this warn! to spam the logs 5x/second if it is e.g. some db/storage (or other persistent) issue 🙈 But the expect() above would maybe crash the thread then..
EDIT: But I guess we do that in a bunch of other places here already, so likely moot. I don't think it hurts anything, either -- the node will crash for other reasons if out of space or there's a corrupt db somewhere, etc.
Coverage Report for CI Build 31841396132Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.04%) to 86.625%Details
Uncovered Changes
Coverage Regressions90 previously-covered lines in 25 files lost coverage.
Coverage Stats
💛 - Coveralls |
Replace a boolean flag that was set manually with an optional block id to ensure that we reset the caches when appropriate.
cylewitruk-stacks
left a comment
cylewitruk-stacks
left a comment
There was a problem hiding this comment.
At a first glance this lgtm 👍 Not approving just yet only because I want to circle back with more focus and follow the error paths more closely.
| /// mining attempt pops and returns one error until the list is empty. | ||
| pub static TEST_MINE_TRANSIENT_ERRORS: LazyLock<TestFlag<Vec<TestTransientError>>> = | ||
| LazyLock::new(TestFlag::default); | ||
|
|
There was a problem hiding this comment.
Maybe we could squeeze something like this in somewhere?
| // TODO: Move all of these test flags into a `cfg(test)`-gated `testing` module. |
| Err( | ||
| ref e @ (NakamotoNodeError::MiningFailure(ChainstateError::DBError(_)) | ||
| | NakamotoNodeError::DBError(_)), | ||
| ) => { |
Fixed the miner thread exiting (and stalling the chain for the remainder of the tenure) on transient error. The miner now retries when it hits DB contention, a parent block that has not been processed yet, a new parent block discovered mid-mining, or a mempool cache reset failure, instead of giving up on the tenure.
These changes are difficult to test and not worth the effort in my opinion.Never mind -- this does need testing. I'm working on it.
Checklist
docs/property-testing.md)changelog.d/README.md). If this PR breaksanything for node operators or users, or requires them to manually do
anything (such as adjust a setting), use the breaking category.
rpc/openapi.yamlfor RPC endpoints,event-dispatcher.mdfor new events)clarity-benchmarkingrepo