QUIC miner

[illuzen]

• use QUIC instead of HTTP (no polling, just bidirectional push)
• simplify protocol to two message types

[n13]

Minor comment on if-else above - not a blocker though

Gemini Review:

Review of PR #363: QUIC miner

Summary

This PR replaces the HTTP-based external miner protocol with a QUIC-based protocol. This is a significant architectural improvement that enables:

• Lower Latency: Server-push for new jobs (no polling).
• Efficiency: Persistent connections and bidirectional streams.
• Simplicity: Reduced protocol complexity (only 2 main message types).

The changes involve:

1. Node Side: A new MinerServer implementation using quinn (QUIC).
2. Protocol: Definition of MinerMessage types in miner-api.
3. Integration: Updates to node/src/service.rs to use the new server.
4. Cleanup: Removal of the old external_miner_client.rs.

Protocol Design

The move to QUIC is well-justified for a mining protocol where latency is critical. The "push" model for jobs ensures miners start working on new blocks immediately.

Message Types

The protocol is clean, but there is a slight discrepancy between EXTERNAL_MINER_PROTOCOL.md and the implementation:

• Documentation: Lists only NewJob and JobResult.
• Implementation: Includes a Ready message sent by the miner immediately after connection.
  • Reason: To establish the bidirectional stream (QUIC streams are lazily initialized).
  • Recommendation: Update EXTERNAL_MINER_PROTOCOL.md to document the Ready message so independent miner implementers are aware of it.

Code Review

node/src/miner_server.rs

The implementation of the QUIC server is solid.

• Concurrency: Uses Arc<RwLock<HashMap>> for miner management, which is appropriate.
• Resource Management: Miners are correctly registered and unregistered upon connection/disconnection.
• Broadcasting: broadcast_job iterates through all connected miners. This is efficient for the expected number of miners.
• Security:
  • Enforces MAX_MESSAGE_SIZE (16MB) to prevent OOM attacks.
  • Uses self-signed certificates. This is acceptable for the current stage, but the documentation correctly advises using proper CAs or pinning for production.

node/src/service.rs

The integration logic in mining_loop is robust.

• Verification: Crucially, worker_handle.verify_seal(&seal) is called before submitting the block. This prevents invalid work from clogging the import queue.
• Stale Work: The logic handles stale results gracefully by checking job_id.
• Cancellation: The cancellation_token is checked in the wait loop, ensuring the node shuts down cleanly.

miner-api/src/lib.rs

• The data structures are well-defined using serde.
• MiningResult includes elapsed_time and hash_count, which are useful for metrics.

Suggestions

1. Update Documentation: Add the Ready message to the protocol specification.
2. Metrics: Consider exposing the number of connected miners as a Prometheus metric (currently only logged).
3. Configuration: The miner_listen_port is optional. Ensure the default behavior (when not set) is clearly communicated (i.e., external mining disabled).

Conclusion

This is a high-quality PR that significantly improves the mining infrastructure. The code is clean, safe, and follows Rust idioms.

Verdict: ✅ Approve (with minor documentation update)

[illuzen]

Took suggestions except for adding miner count as it requires an additional internal server.