Skip to content

Threading improvements#392

Merged
n13 merged 2 commits intomainfrom
threading_improvements
Mar 4, 2026
Merged

Threading improvements#392
n13 merged 2 commits intomainfrom
threading_improvements

Conversation

@n13
Copy link
Collaborator

@n13 n13 commented Mar 4, 2026

Remove blocking executor and unnecessary mutex from mining submission

The mining loop (qpow-mining) runs as an async task on the tokio thread pool. Block submission previously used futures::executor::block_on(worker_handle.submit(seal)), which parked the tokio worker thread for the duration of the block import. This was a workaround because parking_lot::MutexGuard is !Send, making the submit() future incompatible with .await in a tokio context.

The underlying issue was that block_import was wrapped in a parking_lot::Mutex despite not needing one -- BlockImport::import_block takes &self, and submit() is only ever called from the single mining loop. There is no concurrent access to synchronize.

Changes:

  • Remove the Mutex around block_import in MiningHandle, replacing Arc<Mutex<BoxBlockImport>> with Arc<BoxBlockImport>
  • Replace futures::executor::block_on(worker_handle.submit(seal)) with .await at both call sites
  • Make submit_mined_block async
  • Remove #[allow(clippy::await_holding_lock)]

@n13
Copy link
Collaborator Author

n13 commented Mar 4, 2026

Gemini loves it

This is an excellent PR! 🎉 I've checked out the code and reviewed the changes. Everything compiles perfectly and the logic is very sound.

Here is my review of the changes:

🌟 The Good Stuff

  • No More Thread Starvation: Removing futures::executor::block_on from the async tokio context is a huge win! Blocking calls inside a tokio worker thread can easily lead to thread pool starvation or even deadlocks, so replacing it with .await is definitely the right move.
  • Simplified Concurrency: Removing the parking_lot::Mutex around block_import in MiningHandle is a great cleanup. As the PR description rightly points out, submit() is only executed sequentially from within the mining_loop task, so concurrent mutable access synchronization was completely unnecessary.
  • Clean Async Propagation: Making submit_mined_block an async fn and properly propagating the .await up through handle_external_mining and the mining_loop keeps the async control flow very clean and idiomatic.
  • Cleaner Linting: Removing the #[allow(clippy::await_holding_lock)] attribute is always satisfying and proves the architecture is now naturally safe without requiring compiler exception workarounds.

💡 Conclusion

The code is completely solid, minimal, and fully adheres to Rust's concurrency guarantees (the compiler perfectly proves that the Mutex wasn't needed for trait satisfaction). This will definitely improve the responsiveness and performance of the mining loop!

@n13
Copy link
Collaborator Author

n13 commented Mar 4, 2026

GPT and Grok also love it

Copy link
Contributor

@illuzen illuzen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the fact that only one thread needs it also means that blocking would not make much of a difference, but I like the simplicity

@n13 n13 merged commit 8e37a91 into main Mar 4, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants