Mining: reuse FiroPoW inputs and join CPU workers - #1949
Conversation
Reuse the managed light epoch cache and prepare fixed header inputs once per mining batch. Serialize worker replacement, join interrupted workers, and keep extra nonce history per thread. Check template/tip consistency and bound stale work by elapsed time as well as nonce count. Add regression tests for prepared headers, concurrent epoch changes, independent extra nonce histories, and worker restart/stop completion.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
reubenyap
left a comment
There was a problem hiding this comment.
Reviewed exact head 1b6c585c11a4f3a85922dbb42959e48d5d344670; one actionable issue remains.
-
[P1] Make the lock-not-held assertion safe on fresh threads —
src/miner.cpp:1254AssertLockNotHeld(cs_main)runs before any tracked lock has initialized this thread’slockstack. The new lifecycle test callsGenerateBitcoinsfrom a freshstd::asyncthread, and Debug CI deterministically SIGSEGVs while dereferencing that null stack inAssertLockNotHeldInternal; the base revision has no such call, while Release omits the debug lock-order check. This blocks the Debug build and can crash any otherwise valid fresh-thread caller before mining state is touched. Please make the assertion helper treat a missing stack as “no locks held,” or otherwise perform a safe non-locking initialization, then rerunminer_tests.
The prepared-header vectors, concurrent epoch checks, Release build, and the remaining platform matrix pass.
PR intention
Remove repeated FiroPoW header preparation from nonce loops and make the built-in CPU miner safe to start, stop, and use concurrently. Keep the existing light-cache implementation and single-worker default for testing.
Code changes brief
cs_mainin both mining paths. End background batches after 256 unsuccessful hashes or one second, checked after each hash, and rebuild after a solution.Safety evaluation: hashing algorithms, byte conversions, difficulty rules, and normal block validation are preserved. The hashing path reads the light cache; there is no full-DAG mutation or new dependency. All current miner-controller callers were checked to be outside
cs_main, so joining workers does not hold that chain lock. Stop still waits for in-progress hashing/context initialization or validation to return. Idle threads can retain an older light cache until their next epoch request or exit. Review found no outstanding correctness issue in the changes.Validation:
firod,firo-cli, andtest_firo, using pinneddependspackages with assertions and hardening retained.ctest --test-dir <build> -R '^(firpow_tests|miner_tests|progpow_tests)$' --output-on-failure -j1: all three suites passed (34.51 seconds), including the 55 existing FiroPoW vectors and transition/epoch-limit tests.test_firo.git diff --checkpassed.Performance evaluation: an Apple M3
-O2hashing-kernel benchmark used 2,048 nonces per sample, three interleaved repeats, and untimed context/worker setup. Median rates were:The initial single-worker median was 12.1% lower. A follow-up comparison on the same persistent thread, alternating old/new order, did not reproduce a consistent regression: paired throughput ratios ranged from 0.918 to 1.217. Warm managed-context lookup averaged about 6.5 ns against roughly 1 ms per hash. These variable local timings support neither a reliable single-thread speedup claim nor a consistent regression. The multicore results demonstrate concurrent hashing throughput; they do not compare against the old miner with multiple workers or establish end-to-end mining gains.