Fix/enqueue block submission - #58
Open
secretnamebasis wants to merge 4 commits into
Open
Conversation
…ing it connection (the package-level *websocket.Conn) is reassigned on every reconnect in getwork()'s dial loop. mineblock() captures myjob before hashing and, on finding a valid hash, later writes a SubmitBlock_Params carrying that job's JobID over whatever "connection" currently is. If a reconnect lands in the window between finding the hash and finishing the submit (not instantaneous -- there's a mutex acquisition and a network write in between), the submission goes out over the new session's connection but references the old session's JobID, which the server has no reason to recognize. The share is silently lost with no error, no log line, and no counter -- indistinguishable from a share that was simply never found. This exact bug class has been independently found and fixed in at least four sibling DERO miner implementations (Go, Rust, C++, Zig). Go/Rust fix it by draining a submit queue at the reconnect boundary; this dero-miner has no queue to drain (submission is synchronous, straight from the hashing goroutine), so this ports the C++/Zig shape instead: a per-item epoch re-check immediately before the write. connectionEpoch is a counter bumped once per successful reconnect, under connection_mutex, in the same critical section as the connection assignment itself -- getwork()'s dial call wasn't taking that lock before this change, which was a plain data race on top of the staleness bug (mineblock's submit closures already read/wrote connection under connection_mutex; getwork()'s assignment wasn't synchronized with that at all). mineblock() captures the epoch alongside the job when it starts hashing, and the new submitShare() helper (replacing two identical inline closures, one per side of the MAJOR_HF2_HEIGHT branch) re-checks it inside connection_mutex immediately before WriteJSON -- if the epoch moved, a reconnect happened in between, so the share is dropped instead of sent into the void, and counted via the new staleSubmitsDropped so the race is observable rather than silent. Deliberately scoped to just this bug: a smaller residual race some sibling miners close with a session-gate (workers keep mining the old job for a short window after reconnect until the new session's first job arrives) and separately-known gaps (missing read deadlines, flat retry backoff) are not addressed here. go build/go vet/gofmt/go build -race all clean. No live reconnect-race reproduction attempted -- would need a test harness that can force a mid-submit reconnect deterministically; correctness argued from code review plus the fail-safe property that a bug in this logic can only ever cause an occasional over-cautious skip, the same risk category as the bug being fixed, never a wrong or corrupted submission. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review feedback on the previous commit: submitShare ran inline on whichever mining goroutine found a share, blocking that goroutine on connection_mutex plus a synchronous network write (WriteJSON) before it could get back to hashing. Under lock contention or a slow/stalling connection, that's real hash time lost on the thread that just did the work to find a share -- worse than just "a mutex is briefly held," since a network write's latency is not bounded the way a plain in-memory critical section's is. Replaced with a channel (submitQueue) and one dedicated submitWorker goroutine, started once from main() alongside getwork(). Mining threads now call enqueueShare(), a non-blocking channel send, and go straight back to hashing -- all real submission I/O (the mutex, the network write) happens on submitWorker alone. The connectionEpoch staleness check from the previous commit moves with it: submitWorker re-checks the epoch immediately before WriteJSON, same correctness property as before (a reconnect between finding a hash and the actual write means the share gets dropped, not sent with a stale JobID), just off the hot path now. submitQueueSize (64) is generous relative to realistic share-find rates, so the new submitQueueOverflowed counter (distinct from staleSubmitsDropped -- different failure mode, a stuck submitter rather than a merely-late one) should stay at 0 in practice; a full queue would mean the submitter itself is stuck, not normal load. go build/go vet/gofmt/go build -race all clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous two commits added the counter but never displayed it anywhere -- an operator had no way to actually see the race being defended against without attaching a debugger. Adds it next to Rejected in the readline prompt, the same place every other live mining stat already surfaces. Verified live against the simulator (with the not-yet-merged fix/simulator-get-work-server getwork-server patch applied locally, for testing only -- not part of this branch): normal mining end-to-end through the new enqueueShare/submitWorker path works cleanly (job fetch, hashing, share submission, blocks accepted, height climbing steadily, StaleSubmits at 0 under normal operation). Restarting the simulator mid-mining to force real reconnects is the next step, watching this exact field to confirm it moves instead of the miner silently losing shares. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Found live, not by inspection: ran the fixed miner against the simulator (with fix/simulator-get-work-server's getwork patch applied locally for testing) and repeatedly killed/restarted the simulator mid-mining to force real reconnects. submitWorker panicked -- recovered by the existing globals.Recover(1), so no crash, but firing on every queued share for the whole 10s retry-sleep window after a failed dial (100k+ times in a few seconds at 8 threads' hash rate). Root cause: connectionEpoch only advances on a *successful* dial, but getwork's dial call assigns connection = nil on a *failed* one regardless (websocket.Dial's return value on error) without bumping the epoch. So a share whose epoch still matches connectionEpoch isn't actually guaranteed to have a live connection to submit over -- it only guarantees no *successful* reconnect happened since the job was fetched. A failed reconnect attempt leaves epoch unchanged while connection sits nil for the whole retry-sleep window, and submitWorker's epoch check alone let those through to a WriteJSON call on a nil *websocket.Conn. Fixed with a nil check inside connection_mutex, alongside the epoch check, counted through the same staleSubmitsDropped -- a nil connection is the same kind of "can't submit this right now" as a stale epoch, not a different failure class worth its own counter. Re-ran the same repeated-kill/restart test after this fix: 0 panics, all 8 reconnect cycles survived, StaleSubmits climbed into the hundreds of thousands (expected at this aggressiveness -- restarting roughly every 1.5s against 8 threads hashing at simulator-trivial difficulty), and the miner kept mining and successfully submitting blocks throughout (BLOCKS counter kept climbing across and after every cycle). This is the strongest evidence in this branch that the actual race is being handled correctly, not just plausibly-argued from code review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes a real, reproducible bug in
dero-miner's share-submission path:connection(the shared*websocket.Conn) can be reassigned by a reconnect ingetwork()at any moment, including in the narrow window between a mining thread finding a valid hash and finishing its submission. When that happens, the submission goes out over the new session's connection but carries the old session'sJobID, which the server has no reason to recognize — the share is silently lost with no error, no log line, and no counter. This exact bug class has been independently found and fixed in several sibling DERO miner implementations in other languages.While building and verifying the fix (against the simulator, repeatedly killing/restarting it mid-mining to force real reconnects — see Testing below), also found and fixed a second, related bug: a failed reconnect attempt leaves
connectionnilwithout advancing the new staleness-detection counter, so a queued share could still pass the staleness check and then panic on a nil connection. Not theoretical — this fired over 100,000 times in a few seconds during testing before the fix.What changed
connectionEpoch— a counter bumped once per successful reconnect, under the same lock (connection_mutex) that guards theconnectionvariable itself. (Also fixes a latent, unrelated data race:getwork()'sconnection = ...assignment wasn't previously taking this lock at all, even though every other reader/writer ofconnectionwas.)enqueueShare()/submitQueue/submitWorker()— found-hash handling no longer blocks a mining thread on a mutex and a synchronous network write. Mining threads hand a found share off to a bounded channel and immediately return to hashing; one dedicated goroutine (submitWorker) owns all submission I/O, re-checkingconnectionEpochimmediately beforeWriteJSONand dropping (not sending) anything whose epoch has moved — i.e. a reconnect happened since the share's job was fetched.submitWorker, alongside the epoch check — closes the second bug described above.staleSubmitsDroppedsurfaced in the live status line (next toRejected), so the race being defended against is actually observable to an operator, not just an internal counter.Deliberately out of scope
A smaller residual race some sibling miners close with a session-gate (workers keep mining the old job for a short window after reconnect until the new session's first job arrives), and separately-known gaps (missing read deadlines on the websocket connection, flat 10s retry backoff with no jitter/cap). Real, but different bugs — kept this PR scoped to the one race plus what testing surfaced along the way.
Testing
go build/go vet/gofmt/go build -raceall clean throughout.Live-verified against
cmd/simulator(locally, withfix/simulator-get-work-server's getwork-server patch applied on top for testing — not part of this PR): pointed a realdero-minerbinary at the simulator's getwork endpoint and mined normally end-to-end (job fetch → hash → share found → submitted → block accepted, height and block count climbing steadily) through the newenqueueShare/submitWorkerpath.Then forced the actual race: repeatedly killed and restarted the simulator process while mining continued (8 threads, ~13 KH/s, simulator-trivial difficulty, roughly one reconnect every 1.5s for 8 cycles).
submitWorkerpanicked (recovered, not a crash, but firing continuously) every time a reconnect attempt failed and leftconnectionnil.StaleSubmitsclimbed into the hundreds of thousands (expected, given how aggressively the test was cycling reconnects against a low simulator difficulty) — real confirmation the race fires and is being caught, not just plausible from code review. The miner kept mining and successfully submitting new blocks throughout and after every cycle (BLOCKSkept climbing).Type of change
Which part is impacted ?
Checklist:
License
I am contributing & releasing the code under DERO Research License (which can be found here).