Skip to content

stratum: Serve every miner the largest coinbase class on BLAKE2b work - #10

Open
iohzrd wants to merge 3 commits into
CONVOYMining:masterfrom
iohzrd:master
Open

iohzrd wants to merge 3 commits into
CONVOYMining:masterfrom
iohzrd:master

Conversation

@iohzrd

@iohzrd iohzrd commented Sep 5, 2026

Copy link
Copy Markdown

The size classes were sized to what SHA256d firmware could accept, since
those miners receive coinb1/coinb2 and hash the coinbase. On BLAKE2b work
the miner receives 000000 || H2 || 00000000 as coinb1 and an empty coinb2,
and the work root is blake2b(0x00 || coinb1 || extranonce), so the coinbase
never reaches the miner. A smaller class therefore only omits some of the
pool's dictated outputs from the block; their value is paid to the pool's
address as the remainder. With the default class 2 (755 bytes) that is
about 18 P2WPKH outputs with the default coinbase tags; a pool with more
payout outputs than that in its split had the rest omitted from every block
found by such a miner, and an Antminer A3 was served class 0, which pays
only the pool address.

datum_stratum_coinbase_index now returns COINBASE_TYPE_YUGE for every miner
once the full coinbase is ready (job state at or above
JOB_STATE_FULL_PRIORITY_WAIT_COINBASER and full_coinbase_ready set);
fingerprinting keeps only the NiceHash minimum difficulty. Classes 1, 2, 3
and 5 are still built, but their indexes never appear in a job id.

coinbaser: Enforce the block sigop limit on payout outputs

With every miner served the largest class, a split can carry hundreds of
outputs, so the block's sigop limit must be enforced here. The budget is
the template's sigoplimit minus the sigop cost of its transactions and of
the pool output. Cost is four per P2PKH output and zero for every other
script, as recorded in available_coinbase_outputs[].sigops by the coinbaser
parser. The check is applied in both passes of
generate_coinbase_txns_for_stratum_job_subtypebysize so the output count
written to the varint matches the outputs written.

… miner on BLAKE2b work the largest coinbase class.
…and count the coinbase's static bytes as 124 instead of 119.

@jasonsopko jasonsopko left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tested ACK 40cf813.

The premise holds: send_mining_notify hands every miner the same 39-byte coinb1 (000000 || H2 || 00000000), an empty coinb2 and no merkle branches whatever the class, so the class only decides which of the pool's outputs make it into the block. Serving the largest one is the right call.

What I ran:

  • Built with -Wall -Werror, --test passes.
  • Drove generate_coinbase_txns_for_stratum_job_subtypebysize from a small harness across 396 configurations: output space 0 to 30000 bytes, template sigop usage 0 to 90000, P2WPKH and P2PKH pool scripts, and all three extranonce placements including the class-2 coinb1 split. Every result parsed as a transaction: the varint matched the outputs written, the tx ended exactly at the locktime, payout sigops stayed within the budget, and the values summed to coinbase_value with the remainder on the pool output. The two-pass parity you describe holds under everything I threw at it.
  • Merges clean with #7 and #9. The submit path already bounds-checks the class byte in the job id, and the 4 MB DATUM message buffer takes the 16 KB coinbase upload without trouble.

One thing for a follow-up rather than this PR: datum_stratum_coinbase_fit_to_template still shrinks the class to whatever weight the template leaves. With Knots' default blockreservedweight=8000 a full block leaves room for about 55 P2WPKH outputs, and the README's blockmaxweight=785000 about 176. Holding the whole class needs about 64 kWU of headroom, so blockreservedweight=66000 or blockmaxweight=743000. Blocks are nowhere near full right now, so this is a README line, not a bug here. Do you want to update that line in this PR, or should I open one?

nit: the sigops kludge (first byte 0x76) undercounts a P2PK or bare-multisig script if a pool ever dictated one, and the parser accepts any 2 to 64 byte script. Counting OP_CHECKSIG as 4 and OP_CHECKMULTISIG as 80 is a dozen lines. Pre-existing, this PR only relies on it, so optional.

@jasonsopko jasonsopko left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tested ACK 7491a50 for the second commit as well.

The weight accounting it corrects is real. I drove datum_stratum_coinbase_fit_to_template and the generator across full templates (transaction weight 700,000 up to the limit, 512 mixed P2WPKH/P2PKH outputs, extranonce in the coinbase) and assembled the block weight from the result: on 40cf813, in templates where the gateway believed the largest class fit, the block came out up to 288 weight units over 800,000; on this head the same cases are 68 under. The 124-byte static count and the 47-byte witness commitment output check out against the serialized coinbase. The 396-configuration parity check from this morning still passes on this head, and --test does.

For what it is worth, Luke's #8 now carries 4d0a045, which also drops the per-miner class selection, so the first commit here overlaps it. The sigop budget and this weight correction are the parts only this PR has.

@innerhat-dev

innerhat-dev commented Sep 7, 2026 •

Copy link
Copy Markdown

ACK #10, NACK #8 in favor of #10

Edit: being too narrow here in my language, I mean NACK #8 in favor of #10 for this bug. There are other potentially useful things in #8 which is why I haven't NACKed directly on it.

innerhat-dev added a commit to Blockvase/datum_gateway that referenced this pull request Sep 8, 2026
…er work subsidy-only.

Blake jobs use YUGE after the coinbaser and EMPTY while pooled and late, plus the remaining open-PR bounds/RPC/logger fixes.
innerhat-dev added a commit to Blockvase/datum_gateway that referenced this pull request Sep 8, 2026
…er work subsidy-only.

Blake jobs use YUGE after the coinbaser and EMPTY while pooled and late, plus the remaining open-PR bounds/RPC/logger fixes.

Co-authored-by: iohzrd <3422336+iohzrd@users.noreply.github.com>
Co-authored-by: Jason Sopko <22288016+jasonsopko@users.noreply.github.com>
Co-authored-by: AwokenLazarus <262377448+AwokenLazarus@users.noreply.github.com>
Co-authored-by: f4u57ox <72417751+f4u57ox@users.noreply.github.com>
Co-authored-by: Luke Dashjr <1095675+luke-jr@users.noreply.github.com>
…nd the coinbaser output limit to 1024, and remove miner fingerprinting.
@iohzrd

iohzrd commented Sep 24, 2026

Copy link
Copy Markdown
Author

Pushed two commits on top of the original one:

7491a50: Coinbase size accounting for BLAKE2b blocks

  • datum_stratum_coinbase_fit_to_template reserved 340 weight units for the header and tx count, which assumes an 80-byte SHA256d header. It now uses the 164-byte BLAKE2b header plus the 5-byte count.
  • The coinbase's static byte count goes from 119 to 124. The old value was 3 to 5 bytes under the real transaction size, so a coinbase built to fill the template's remaining room could exceed the block weight limit by up to 20 WU.

c031568: Coinbase classes reduced to TINY and YUGE, fingerprinting removed

  • Only TINY (pays the pool address only) and YUGE are built now. Classes 1, 2, 3 and 5 are gone, and so is the special_coinb1 path, since no miner is served any of them.
  • YUGE max goes from 16000 to 32000 bytes (MAX_DICTATED_COINBASE_SIZE), and the coinbaser output limit goes from 512 to 1024 (MAX_COINBASER_OUTPUTS). The coinbaser buffer, STRATUM_COINBASE2_MAX_LEN and the POW message size limit are raised so a 1024-output split fits. A unit test covers the 3-byte output count varint at that size.
  • Miner fingerprinting is removed: stratum.fingerprint_miners, the user-agent table, the NiceHash forced minimum difficulty, and the "Coinbase" column on the clients page.

The PR description covers only the first commit. It says classes 1, 2, 3 and 5 are still built and fingerprinting keeps the NiceHash minimum difficulty, and neither is true anymore.

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.

3 participants