#36109 test: fix coinbases built on arbitrary parents
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36109 test: fix coinbases built on arbitrary parents | Ready | 0 | Strong | 0 + 63 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Extracts arbitrary-parent block template reconstruction into a shared RebuildBlockForParent test helper used by BuildChain and MinerTestingSetup::Block. The helper updates the header hash and timestamp, recalculates the proof-of-work target and block subsidy from the target parent, appends a dummy OP_0 to satisfy minimum coinbase script length on low heights, and asserts coinbase-only templates.
Problem: Test utilities creating fork blocks by repointing active-tip templates could retain the active tip's work target or subsidy, and produced coinbase scripts below the minimum length requirement at heights 1 through 16. Existing test callers either avoided these boundary conditions or worked around them locally, but new tests constructing forks could produce invalid blocks.
Evidence of importance:
- PR description: 'test helpers attaching active-tip block templates to arbitrary parents could retain the wrong work target or subsidy. Rebuilding the coinbase input script for heights 1 through 16 without padding also produces a script below the minimum length. Existing callers avoid these cases or handle them locally, so no current test fails.'
- Commit 15ffe40719: 'Current chain-building callers use regtest with retargeting disabled, so this prevents future invalid test cases rather than fixing an existing failure.'
Risk of a mistake: low (tests). Touches only unit test utilities and test cases without affecting consensus, node, or production code.
Changed since the previous assessment: Enforced coinbase-only templates by asserting block.vtx.size() == 1 in RebuildBlockForParent and passing use_mempool = false in MinerTestingSetup::Block to prevent fee stripping when rewriting coinbase subsidies.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: jeanpablojp to review the fixes and rebase 2026-09-25
Agreement
Strong
substantive support, no open objection (jeanpablojp)
Thread notes: l0rinc replied to and implemented fixes for both of jeanpablojp's objections, pushing a rebase with tests and coinbase-only enforcement.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| jeanpablojp (contributor) | objection, correctness | rebuilding the coinbase reward strictly with the block subsidy drops transaction fees when a template contains mempool transactions | fixed · fix pushed · author asserted template is coinbase-only and disabled mempool in MinerTestingSetup::Block() · v1, not yet re-checked | answered | yes | 2026-09-08: 'This is an assignment rather than an adjustment, so the template's fees go with it.' Status from: 2026-09-25: l0rinc: 'Sure, added `.use_mempool = false` to `MinerTestingSetup::Block()` and asserted the shared helper receives a coinbase-only block.' |
| jeanpablojp (contributor) | objection, correctness | lack of test coverage for target recalculation and coinbase adjustments in arbitrary parent rebuilding | fixed · fix pushed · author added tests covering the target recalculation and coinbase adjustments · v1, not yet re-checked | answered | yes | 2026-09-08: 'Removing the target, subsidy or padding line here leaves blockfilter_index_tests and baseindex_tests green, so nothing on that side covers what the branch changes.' Status from: 2026-09-25: l0rinc: 'Thanks! I added similar tests, with the coinbase cases combined into one fork test.' |
Support:
- jeanpablojp (contributor) (Concept ACK, 2026-09-08) [tested]: Built each commit and ran the unit tests, with a before and after of both cases against master.
Participants:
- jeanpablojp (contributor, objection, 3 statements 2026-09-08 to 2026-09-08): Concept ACK, but objected that coinbase fees are lost with mempool templates and noted missing test coverage
Files
78 lines under test/bench/ci.
- src/test/validation_block_tests.cpp +39/-6
- src/test/util/mining.cpp +17/-9
- src/test/util/mining.h +7/-0
Card
The short text the ranking pass compares across PRs.
PR #36109 unifies block reconstruction logic for arbitrary-parent test helpers (BuildChain and MinerTestingSetup::Block) into a shared RebuildBlockForParent helper. It recalculates the parent-dependent work target and block subsidy, pads coinbase scriptSigs with OP_0 to satisfy length requirements at heights 1-16, and enforces coinbase-only templates so fee rewards cannot be silently overwritten. The author notes no existing tests fail today because current callers avoid these edge cases, making this preventative test harness hardening.
Data
thread read through 2026-09-25 (6 statements), head 23a99695ef
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: Tests
Categories
Test infrastructure
P3 · cleanup
Hardens test block creation utilities by extracting arbitrary-parent rebuilding logic into a shared helper that correctly recalculates targets, subsidies, and coinbase script padding. The commit notes explain that 'no current test fails' and 'this prevents future invalid test cases rather than fixing an existing failure,' making this a worthwhile infrastructure improvement that can be safely deferred.
Membership: Matches the 'Tests' label and modifies shared unit test mining utilities in src/test/util/mining.{cpp,h}.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 1