Repository navigation
coinbaser: Enforce the sigop limit on payout outputs and count the BLAKE2b header in the coinbase weight limit - #24
Conversation
ebabaeb to
76d748e
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The sigop heuristic misses valid scripts such as bare P2PK and multisig, allowing invalid over-limit blocks.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Updates coinbase construction to respect block weight and sigop limits.
Changes:
- Accounts for the 164-byte BLAKE2b header and corrected coinbase size.
- Adds payout-output sigop budgeting and tests.
| File | Description |
|---|---|
src/datum_coinbaser.c |
Enforces limits during coinbase generation. |
src/datum_coinbaser_tests.c |
Tests weight and sigop accounting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // parser: 4 for a script whose first byte is OP_DUP (0x76, P2PKH) and 0 | ||
| // for every other script. The pool output is charged the same way. An |
There was a problem hiding this comment.
Fixed in 993e261. datum_script_sigop_cost takes the legacy count the way GetSigOpCount(false) does (OP_CHECKSIG and OP_CHECKSIGVERIFY 1, OP_CHECKMULTISIG and OP_CHECKMULTISIGVERIFY 20, pushed data skipped, stop at a truncated push) and multiplies it by 4. It replaces the OP_DUP first-byte check for dictated outputs in datum_coinbaser_v2_parse and for the pool output in generate_coinbase_txns_for_stratum_job_subtypebysize. datum_script_sigop_cost_tests covers bare P2PK (4) and bare multisig (80); datum_coinbaser_parse_sigops_tests and the bare P2PK pool case in datum_blake2b_coinbase_sigops_tests cover both call sites.
…and count the coinbase's static bytes as 124 instead of 119.
Please don't list PRs like this |

Replaces #10, whose head branch was my fork's master. Reduced to the two changes CONVOY master does not have:
COINBASE_TYPE_YUGE, anddatum_stratum_coinbase_indexkeeps itsminerargument for per-miner selection later.Rebased onto 08123fc. The Tested ACKs on #10 were for 40cf813 and 7491a50. 993e261 is 40cf813's
datum_coinbaser.canddatum_coinbaser_tests.cchanges, with sigops now counted per script (below). 157500e is 7491a50 unchanged.993e261: Enforce the template sigop limit on coinbaser outputs
Every miner is served
COINBASE_TYPE_YUGEsince 4d0a045, so a split can put hundreds of outputs in the coinbase, and the block's sigop limit has to be enforced on them. The budget is the template's sigoplimit minus the sigop cost of its transactions and of the pool output. Each output is chargeddatum_script_sigop_costof its script: Bitcoin's legacy count (GetSigOpCount(false)) times 4. That counts OP_CHECKSIG and OP_CHECKSIGVERIFY as 1 and OP_CHECKMULTISIG and OP_CHECKMULTISIGVERIFY as 20, skips pushed data, and stops at a push that runs past the end. The coinbaser parser stores it inavailable_coinbase_outputs[].sigops; before, it charged 4 only for a script starting with OP_DUP. The pool output is charged the same way. Tests cover bare P2PK (4), bare multisig (80), the VERIFY forms, opcode bytes inside pushed data, and truncated pushes. The check is applied in both passes ofgenerate_coinbase_txns_for_stratum_job_subtypebysize, so the output count written to the varint matches the outputs written.157500e: Coinbase size accounting for BLAKE2b blocks
datum_stratum_coinbase_fit_to_templatereserved 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.Each commit builds with
-Wall -Werrorand passes--test. Merges without conflicts with each open PR that merges with master: #2, #3, #4, #6, #9, #11, #12, #13, #18, #20, #21, #26 and #27.