test: fix coinbases built on arbitrary parents #36109

pull l0rinc wants to merge 5 commits into bitcoin:master from l0rinc:l0rinc/test-build-valid-fork-blocks changing 3 files +63 −15
  1. l0rinc commented at 12:17 AM on August 28, 2026: contributor

    Problem: Test helpers that attach active-tip templates to arbitrary parents retain the template's subsidy. Existing reorg tests can underpay without failing, while a fork crossing a halving can overpay and fail with bad-cb-amount. Bare height scripts at heights 1 through 16 also fail with bad-cb-length.

    Fix: Share parent-dependent reconstruction between BuildChain() and MinerTestingSetup::Block(), deriving difficulty and coinbase fields from the actual parent. Pad rebuilt coinbase input scripts and require coinbase-only templates so recalculating the subsidy cannot discard transaction fees.

  2. DrahtBot added the label Tests on Aug 28, 2026
  3. DrahtBot commented at 12:17 AM on August 28, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36109.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline and AI policy for information on the review process.

    Type Reviewers
    Concept ACK jeanpablojp

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #36521 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36521.svg"></sub> (Keep witness stacks in serialized form in CScriptWitness by sipa)
    • #35570 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35570.svg"></sub> (refactor: Change some validation.cpp methods to return BlockValidationState by optout21)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. jeanpablojp commented at 6:27 PM on September 8, 2026: contributor

    Concept ACK

    Built each commit and ran the unit tests, with a before and after of both cases against master.

  5. in src/test/util/mining.cpp:89 in 6052be3135
      84 | +        const int height{parent.nHeight + 1};
      85 | +        CMutableTransaction tx_coinbase{*block.vtx.at(0)};
      86 | +        tx_coinbase.nLockTime = static_cast<uint32_t>(parent.nHeight);
      87 | +        // Include a dummy OP_0 so low-height coinbase input scripts meet the minimum length
      88 | +        tx_coinbase.vin.at(0).scriptSig = CScript{} << height << OP_0;
      89 | +        tx_coinbase.vout.at(0).nValue = GetBlockSubsidy(height, params);
    


    jeanpablojp commented at 6:27 PM on September 8, 2026:

    This is an assignment rather than an adjustment, so the template's fees go with it. MinerTestingSetup::Block asks for its template with the mempool on, and with one transaction in it the coinbase reward comes out at 5000001000 on master and 5000000000 here, with the block still carrying that transaction.

    Nothing hits it today, since the mempool is empty at every call. Would .use_mempool = false on that template call be simpler? It makes the site match BuildChain, so the overwrite is exact, and the three suites stay green with it.


    l0rinc commented at 1:18 AM on September 25, 2026:

    Sure, added .use_mempool = false to MinerTestingSetup::Block() and asserted the shared helper receives a coinbase-only block.

  6. in src/test/util/mining.cpp:78 in 6052be3135 outdated
      74 | @@ -75,6 +75,23 @@ std::vector<std::shared_ptr<CBlock>> CreateBlockChain(size_t total_height, const
      75 |      return ret;
      76 |  }
      77 |  
      78 | +void RebuildBlockForParent(CBlock& block, const CBlockIndex& parent, uint32_t time, const Consensus::Params& params)
    


    jeanpablojp commented at 6:27 PM on September 8, 2026:

    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. The new assertions in MinerTestingSetup::Block catch the subsidy and the padding, and the target has nothing anywhere.

    These two cover all four fields, if they are worth having. The target one calls the helper directly, which is the only way in, since on regtest the recalculated target always equals the template's and no chain can tell the line apart. The coinbase one passes here and fails on master, first on the script length and then at the halving block. Both compile in blockfilter_index_tests.cpp.

    BOOST_FIXTURE_TEST_CASE(rebuild_block_for_parent_target, RegTestingSetup)
    {
        const auto& consensus{Params().GetConsensus()};
        const auto genesis{Params().GenesisBlock()};
        const auto parent_hash{genesis.GetHash()};
        CBlockIndex parent{genesis};
        parent.phashBlock = &parent_hash;
        --parent.nBits; // A valid target, different from the one the template carries.
    
        // Regtest allows min-difficulty blocks, and parent has no pprev, so
        // GetNextWorkRequired returns the parent's target below the threshold and the
        // pow limit above it.
        const auto threshold{uint32_t(2 * consensus.nPowTargetSpacing)};
        for (const auto delay : {1U, threshold, threshold + 1}) {
            CBlock block{genesis};
            RebuildBlockForParent(block, parent, parent.nTime + delay, consensus);
            BOOST_CHECK_EQUAL(block.nBits, delay > threshold ? genesis.nBits : parent.nBits);
        }
    }
    
    BOOST_FIXTURE_TEST_CASE(rebuild_block_for_parent_coinbase, TestChain100Setup)
    {
        const auto& consensus{Params().GetConsensus()};
        auto process = [&](const std::vector<std::shared_ptr<CBlock>>& chain) {
            for (const auto& block : chain) {
                BOOST_CHECK(m_node.chainman->ProcessNewBlock(block, /*force_processing=*/true, /*min_pow_checked=*/true, nullptr));
            }
        };
    
        // A fork rooted below height 17 needs the dummy OP_0, since the BIP34
        // height alone is one byte there (bad-cb-length).
        {
            const CBlockIndex* genesis{WITH_LOCK(cs_main, return m_node.chainman->ActiveChain().Genesis())};
            std::vector<std::shared_ptr<CBlock>> fork;
            BOOST_REQUIRE(BuildChain(m_node, genesis, CScript() << OP_TRUE, 3, fork));
            BOOST_CHECK_EQUAL(fork.front()->vtx[0]->vin[0].scriptSig.size(), 2U);
            process(fork);
        }
    
        // BuildChain only submits headers, so every template keeps being built on
        // the active tip. A chain long enough to pass the halving would otherwise
        // carry the tip's subsidy and fail to connect (bad-cb-amount).
        {
            const CBlockIndex* tip{WITH_LOCK(cs_main, return m_node.chainman->ActiveChain().Tip())};
            const int start{tip->nHeight};
            const int length{consensus.nSubsidyHalvingInterval + 10 - start};
            BOOST_REQUIRE(length > 0);
            std::vector<std::shared_ptr<CBlock>> chain;
            BOOST_REQUIRE(BuildChain(m_node, tip, CScript() << OP_TRUE, length, chain));
            for (int i = 0; i < length; ++i) {
                BOOST_CHECK_EQUAL(chain.at(i)->vtx[0]->vout[0].nValue, GetBlockSubsidy(start + 1 + i, consensus));
            }
            process(chain);
            BOOST_CHECK_EQUAL(WITH_LOCK(cs_main, return m_node.chainman->ActiveChain().Height()), start + length);
        }
    }
    

    l0rinc commented at 1:26 AM on September 25, 2026:

    Thanks! I added similar tests, with the coinbase cases combined into one fork test. I also credited you as a co-author on the commits containing them.

  7. DrahtBot added the label Needs rebase on Sep 24, 2026
  8. l0rinc force-pushed on Sep 25, 2026
  9. l0rinc commented at 1:32 AM on September 25, 2026: contributor

    Rebased to resolve conflicts and applied the review suggestions. The block’s difficulty now uses its actual parent and timestamp, and its coinbase pays the subsidy for its resulting height. I also added focused tests for the target and coinbase fields through the first halving.

  10. DrahtBot removed the label Needs rebase on Sep 25, 2026
  11. test: characterize fork coinbase fields
    Record the subsidy and input-script length of a chain built from genesis through the first halving.
    The active-tip template reward is retained at the halving, and the unpadded low-height coinbases prevent the fork from connecting.
    
    Co-authored-by: JP <jeanpablo.jp@hotmail.com>
    d319b4654f
  12. refactor: extract fork block reconstruction
    Move the existing parent-dependent field rewrites out of `BuildChain()` without changing the generated fields.
    A direct helper test records the copied target using a synthetic parent with a different target on either side of regtest's min-difficulty delay.
    
    Co-authored-by: JP <jeanpablo.jp@hotmail.com>
    4e5aeb3535
  13. test: derive fork difficulty from parent
    Recalculate `nBits` from the actual parent and child timestamp instead of retaining the active-tip template's target.
    Existing chain-building callers use regtest's minimum difficulty, so their generated targets are unchanged.
    28990b4ff3
  14. test: derive fork subsidy from parent
    Derive the coinbase subsidy from the actual parent height and share the reconstruction with `MinerTestingSetup::Block()`.
    Existing reorg tests cross halvings and underpay some coinbases, which remains valid and does not fail the tests. A fork extending across a halving can instead overpay and fail with `bad-cb-amount`.
    
    Require coinbase-only templates so replacing the reward cannot discard transaction fees.
    9312297e15
  15. test: pad fork coinbase input scripts
    Append a dummy `OP_0` to rebuilt coinbase input scripts so heights 1 through 16 meet the minimum length.
    The fork from genesis can then connect through the first halving. Remove the validation helper's duplicate padding.
    23a99695ef
  16. l0rinc force-pushed on Oct 1, 2026
  17. l0rinc renamed this:
    test: harden arbitrary-parent block creation
    test: fix coinbases built on arbitrary parents
    on Oct 4, 2026
Labels

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-10-23 09:51 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me