Skip to content

Conversation

@illuzen
Copy link
Contributor

@illuzen illuzen commented Feb 2, 2026

  • refactored into smaller files
  • support quic mining
  • support running against --dev node
  • support changing chains
  • various small ui issues fixed

@illuzen
Copy link
Contributor Author

illuzen commented Feb 2, 2026

we should not merge this until we have releases for the quic miner and quic node

@n13
Copy link
Collaborator

n13 commented Feb 4, 2026

PR #377 Review: QUIC Mining & Refactoring

Summary

This PR introduces significant improvements to the miner application, most notably:

  • Refactoring the monolithic MinerProcess into a robust MiningOrchestrator pattern.
  • Adding support for QUIC mining.
  • Centralizing configuration and process management.
  • Improving error handling and logging.

The changes are substantial (36 files, ~6.7k lines) but result in a much more maintainable and scalable architecture.

Architecture

The shift from a single MinerProcess class to a service-based architecture is a major improvement.

  • MiningOrchestrator: Acts as the central brain, managing the state machine and coordinating sub-services. This makes the logic easier to reason about compared to a single large class.
  • Process Managers: NodeProcessManager and MinerProcessManager encapsulate the specifics of running the respective binaries. This separation allows for independent evolution and testing.
  • ProcessCleanupService: A dedicated service for cleaning up zombie processes and ports is crucial for stability, especially across restarts.

Key Changes & Observations

1. QUIC Mining Support

The PR successfully wires up QUIC mining support:

  • MinerConfig defines the default QUIC port (9833).
  • NodeProcessManager passes --miner-listen-port.
  • MinerProcessManager connects via --node-addr using the node's QUIC port.

2. Configuration Management

The introduction of MinerConfig is excellent. It centralizes:

  • Ports (QUIC, RPC, Metrics, P2P).
  • Timeouts and retry policies.
  • Chain configurations (dev vs dirac).
  • URLs and file paths.

This eliminates magic numbers scattered throughout the codebase.

3. Process Management & Cleanup

ProcessCleanupService is very robust:

  • It handles both Windows and Unix process killing.
  • It includes logic to clean up database lock files (LOCK), which is a common pain point when the node crashes.
  • It verifies port availability and tries alternative ports for metrics if needed.

4. Logging

The new AppLogger and LogStreamProcessor provide better observability:

  • Structured logging with levels (Debug, Info, Warn, Error).
  • Source tagging (Node, Miner, Orchestrator).
  • Filtering logic to reduce noise (e.g., during sync).

Suggestions & Nitpicks

Node Process Arguments

In miner-app/lib/src/services/node_process_manager.dart:

// Only use --base-path for non-dev chains (dev uses temp storage for fresh state)
if (config.chainId != 'dev') ...['--base-path', basePath],
// ...
if (config.chainId == 'dev') '--dev' else ...['--chain', config.chainId],

Observation: While --dev implies a temporary DB by default in Substrate, implicitly preventing --base-path for dev chain means users cannot persist their dev chain state even if they wanted to. This is likely intended behavior for "fresh state" dev mode, but worth noting if persistence is ever needed for dev testing.

Error Handling

The MiningOrchestrator handles crashes well:

  • It distinguishes between Node and Miner crashes.
  • It has a forceStop mechanism for cleanup.
  • It emits typed MinerError events for the UI to consume.

Conclusion

This is a high-quality PR that significantly improves the codebase's maturity. The architecture changes set a solid foundation for future features.

Verdict: Approve 🚀

Copy link
Collaborator

@n13 n13 left a comment

Choose a reason for hiding this comment

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

Gemini loves it see above

I guess it needs new releases of external miner (quantus miner) and node?!

CI fail is just formatting, easy to fix

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