Bugfix: stratum: Process the current job in newly started stratum threads - #21
Open
alphaminetech wants to merge 3 commits into
Open
alphaminetech wants to merge 3 commits into
alphaminetech wants to merge 3 commits into
Conversation
stratum_job_coinbaser_ready applied the 5 second timeout before looking at whether the job's coinbaser had arrived. A thread that first looks at a job more than 5 seconds after it was created therefore treated it as timed out even when the coinbaser was ready, and sent the class 0 coinbase instead of the full one until the next job. Check the coinbaser first, and only fall back after 5 seconds if it is still missing. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…eads Stratum threads are started as clients connect, up to stratum.max_threads. A new thread recorded the current job as already seen, so its loop never processed that job: - full_coinbase_ready stayed false, and every client on the thread was sent the class 0 coinbase (in pooled mining it pays only the pool) until the next job, up to bitcoind.work_update_seconds later. - A thread started while empty work for a new block was being sent counted towards the threads that have to send it, but never did, so the full work for that block waited for the template thread's 4 second timeout. Start the thread without a job, so the first loop iteration picks up the current one like any other job update. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… job Cover stratum_job_coinbaser_ready using a coinbaser that is already there even after the 5 second fallback, a client subscribing on a newly started stratum thread getting the current job with the full coinbase (class 4) rather than the pool-only one (class 0), and a thread started while empty work is being sent taking part in that send. Co-Authored-By: Claude Opus 5.5 (1M context) <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
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.
A stratum thread is started for each new connection until
stratum.max_threadsthreads exist (8 by default), and threads never stop, so this concerns the firststratum.max_threadsconnections after startup. A new thread records the job that is current when it starts as already seen, so its loop never processes that job:full_coinbase_readystays false, and every client that subscribes on the thread gets the class 0 coinbase instead of the full one. In pooled mining that is the coinbase that pays only the pool; in solo mining all classes are the same coinbase. It lasts until the thread's next job reaches the client: the next work update, at mostbitcoind.work_update_secondsafter the current job (or a new block, if one comes first), which is then paced across the thread's clients like any regular update. Only clients of a newly started thread are affected, and only during that thread's first job.Commits:
Bugfix: stratum: Check for the coinbaser before applying its timeout.
stratum_job_coinbaser_readychecked the 5 s fallback first, so a thread that first looks at a job more than 5 s after it was created fell back to class 0 even though the coinbaser was there. That is the case for most new threads (a job is older than 5 s for most of its life), and for a coinbaser that lands in the last loop interval (up to ~10 ms) before the 5 s mark. The timeout itself is unchanged: with no coinbaser after 5 s, the thread still goes ahead without it.One side effect: when the fetch gets no usable reply, it builds the job's coinbases with 0 outputs (every class is then the pool-only coinbase) and clears
need_coinbaser. A thread that first looks after that now sends this coinbase under the miner's class (4) instead of class 0, as master already does when the fetch gives up before the 5 s mark. The coinbase itself is the same.Bugfix: stratum: Process the current job in newly started stratum threads. Start the thread without a job, so its first loop iteration picks up the current one like any other job update. A thread always runs its loop once before it handles any client command, so the job is processed before a client on it can subscribe. This needs commit 1, since most new threads start on a job that is older than 5 s.
QA: stratum_tests: Check that new stratum threads process the current job. Three tests:
stratum_job_coinbaser_readywith a coinbaser that is there after the 5 s mark, a client subscribing on a new thread gets class 4, and a thread started during an empty-work send takes part in it. On master 4 asserts fail; without commit 1, 3 fail; without commit 2, 3 fail.--testpasses in the four CI configurations (API with gcc and with clang, Umbrel define, API off) with ASan+UBSan and leak detection, and with plain-Wall -Werrorgcc and clang, on Debian 12 (GCC 12.2, clang 14) and Fedora 44 (GCC 16.2, clang 22).To reproduce on master: start a gateway in pooled mode and connect a miner once the first job's coinbaser is in. The job id of its first
mining.notifyends in00(class 0), and only its thread's next job brings the miner's class (04). With this branch the first notify ends in04.End to end, against a local mock node and a mock DATUM pool. First
mining.notifyper client,stratum.max_threads3,bitcoind.work_update_seconds15, coinbaser answered at once:Other runs ("split" = the coinbaser's outputs):
Related PRs:
datum_protocol_coinbaser_fetch: a reply that arrives before the fetch starts waiting, or a reply to another request, is no longer lost or taken as the answer). This PR only changesdatum_stratum.c; protocol: Take the coinbaser lock before sending the request #9 alone leaves the 5 s ordering case above as it is (5 of the 35 jobs). The two merge cleanly, and together they pass--testand the checks above (no class 0 although the coinbaser was in before the 5 s mark; split used for 21 of the 35 jobs), while protocol: Take the coinbaser lock before sending the request #9's own cases (a reply before the fetch waits, a wrong reply first) get the split for 8 of 8 jobs each.full_coinbase_readyis false in pooled mode (subsidy-only instead of class 0). That is complementary: this PR avoids the fallback where the coinbaser is there, Never pair pool-only coinbase class 0 with a full template on a pooled job #13 changes the fallback. They merge cleanly.datum_stratum_tests.c.🤖 Generated with Claude Code