mining: pad coinbase to fix createNewBlock at heights <=16

Since #32420, createNewBlock has thrown `bad-cb-length` errors when called at
low block heights because `OP_0` padding stopped being added to coinbase
transactions. (#32420 did add an `include_dummy_extranonce` option which could
bypass this, but it was not exposed to IPC clients.) Fix the problem by padding
coinbase transactions with `OP_0` when necessary to produce valid blocks.

Additionally this commit stops adding `OP_0` padding to the template
`script_sig_prefix` field when `include_dummy_extranonce` is true. This is safe
because non-IPC clients don't use this field, and IPC clients could never set
the option to true, and are expected to add their own nonces in any case.

This also improves documentation about the `script_sig_prefix` field and
`getCoinbaseTx` method.

src/interfaces/mining.h

@@ -34,7 +34,8 @@ public:
     virtual ~BlockTemplate() = default;
 
     virtual CBlockHeader getBlockHeader() = 0;
-    // Block contains a dummy coinbase transaction that should not be used.
+    // Block contains a dummy coinbase transaction that should not be used and
+    // it may not match a transaction constructed from getCoinbaseTx().
     virtual CBlock getBlock() = 0;
 
     // Fees per transaction, not including coinbase transaction.
@@ -64,6 +65,10 @@ public:
      * @note unlike the submitblock RPC, this method does NOT add the
      *       coinbase witness automatically.
      *
+     * @note for heights <= 16, the BIP34 height push in getCoinbaseTx().script_sig_prefix
+     *       is only one byte long, so the coinbase scriptSig needs at least
+     *       one additional byte of data to avoid bad-cb-length.
+     *
      * @returns if the block was processed, does not necessarily indicate validity.
      *
      * @note Returns true if the block is already known, which can happen if

src/node/miner.cpp

@@ -184,14 +184,19 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
     // increasing its length would reduce the space they can use and may break
     // existing clients.
     coinbaseTx.vin[0].scriptSig = CScript() << nHeight;
-    if (m_options.include_dummy_extranonce) {
+    // Set script_sig_prefix here, so IPC mining clients are not affected by
+    // the optional scriptSig padding below. They provide their own extraNonce,
+    // and in a typical setup a pool name or realistic extraNonce already makes
+    // the scriptSig long enough.
+    coinbase_tx.script_sig_prefix = coinbaseTx.vin[0].scriptSig;
+    if (nHeight <= 16 || m_options.include_dummy_extranonce) {
         // For blocks at heights <= 16, the BIP34-encoded height alone is only
         // one byte. Consensus requires coinbase scriptSigs to be at least two
-        // bytes long (bad-cb-length), so tests and regtest include a dummy
-        // extraNonce (OP_0)
+        // bytes long (bad-cb-length), so an OP_0 is always appended at those
+        // heights. At greater heights it is only added when the caller
+        // requests it (e.g. RPC and test code that rely on stable hashes).
         coinbaseTx.vin[0].scriptSig << OP_0;
     }
-    coinbase_tx.script_sig_prefix = coinbaseTx.vin[0].scriptSig;
     Assert(nHeight > 0);
     coinbaseTx.nLockTime = static_cast<uint32_t>(nHeight - 1);
     coinbase_tx.lock_time = coinbaseTx.nLockTime;
@@ -221,7 +226,6 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
     pblock->nNonce         = 0;
 
     if (m_options.test_block_validity) {
-        // if nHeight <= 16, and include_dummy_extranonce=false this will fail due to bad-cb-length.
         if (BlockValidationState state{TestBlockValidity(m_chainstate, *pblock, /*check_pow=*/false, /*check_merkle_root=*/false)}; !state.IsValid()) {
             throw std::runtime_error(strprintf("TestBlockValidity failed: %s", state.ToString()));
         }

src/node/types.h

@@ -74,6 +74,9 @@ struct BlockCreateOptions {
     CScript coinbase_output_script{CScript() << OP_TRUE};
     /**
      * Whether to include an OP_0 as a dummy extraNonce in the template's coinbase
+     *
+     * TODO: this can be dropped after regenerating hardcoded block and transaction
+     *       hashes in the test suite.
      */
     bool include_dummy_extranonce{false};
 };
@@ -125,7 +128,9 @@ struct CoinbaseTx {
      * Prefix which needs to be placed at the beginning of the scriptSig.
      * Clients may append extra data to this as long as the overall scriptSig
      * size is 100 bytes or less, to avoid the block being rejected with
-     * "bad-cb-length" error.
+     * "bad-cb-length" error. At heights <= 16 the BIP 34 height push is only
+     * one byte long, so clients must append at least one additional byte to
+     * meet the consensus minimum scriptSig length of two bytes.
      *
      * Currently with BIP 34, the prefix is guaranteed to be less than 8 bytes,
      * but future soft forks could require longer prefixes.

test/functional/interface_ipc_mining.py

@@ -63,7 +63,7 @@ class IPCMiningTest(BitcoinTestFramework):
         # as it is being called before knowing whether capnp is available).
         self.capnp_modules = load_capnp_modules(self.config)
 
-    async def build_coinbase_test(self, template, ctx, miniwallet):
+    async def build_coinbase_test(self, template, ctx, miniwallet, extra_nonce=b""):
         self.log.debug("Build coinbase transaction using getCoinbaseTx()")
         assert template is not None
         coinbase_res = await mining_get_coinbase_tx(template, ctx)
@@ -79,7 +79,7 @@ class IPCMiningTest(BitcoinTestFramework):
         assert_equal(coinbase_res.scriptSigPrefix, bip34_prefix)
 
         # Typically a mining pool appends its name and an extraNonce
-        coinbase_tx.vin[0].scriptSig = coinbase_res.scriptSigPrefix
+        coinbase_tx.vin[0].scriptSig = coinbase_res.scriptSigPrefix + extra_nonce
 
         # We currently always provide a coinbase witness, even for empty
         # blocks, but this may change, so always check:
@@ -405,7 +405,12 @@ class IPCMiningTest(BitcoinTestFramework):
 
     def run_low_height_test(self):
         """Test that IPC createNewBlock() works at low block heights on a
-        clean chain. Currently fails with bad-cb-length and falls back to RPC."""
+        clean chain, in particular with regard to bad-cb-length.
+
+        createNewBlock pads the scriptSig with a dummy extranonce to pass
+        its internal CheckBlock(). This dummy is omitted from the
+        getCoinbaseTx() script_sig_prefix field. The client provides its
+        own extraNonce via submitSolution()."""
         self.log.info("Running low block height test")
 
         node = self.nodes[0]
@@ -416,29 +421,29 @@ class IPCMiningTest(BitcoinTestFramework):
         node.wait_for_rpc_connection()
         assert_equal(node.getblockcount(), 0)
 
+        miniwallet = MiniWallet(node)
+
         async def async_routine():
             ctx, mining = await make_mining_ctx(self)
             opts = self.capnp_modules['mining'].BlockCreateOptions()
 
-            # IPC createNewBlock() currently fails with bad-cb-length at heights
-            # <= 16, so use the generate RPC as a workaround. The next commit
-            # fixes this and replaces the loop body with a createNewBlock() /
-            # submitSolution() flow.
-            for i in range(17):
+            # Mine blocks 1-17 to exercise the boundary at height 16, where the
+            # internal scriptSig padding is no longer needed.
+            for height in range(1, 18):
                 async with AsyncExitStack() as stack:
-                    try:
-                        # Disable cooldown to avoid hanging in the IBD loop on a fresh chain
-                        template = await mining_create_block_template(mining, stack, ctx, opts, cooldown=False)
-                        assert template is not None
-                        # createNewBlock() only succeeds once the BIP34 height
-                        # encoding is >= 2 bytes, i.e. for height >= 17.
-                        if i != 16:
-                            raise AssertionError(f"createNewBlock() should have failed at height {i + 1}")
-                    except capnp.lib.capnp.KjException as e:
-                        assert i < 16
-                        assert_capnp_failed(e, "remote exception: std::exception: TestBlockValidity failed: bad-cb-length")
-                    self.generate(node, 1, sync_fun=self.no_op)
-                    assert_equal(node.getblockcount(), i + 1)
+                    # Disable cooldown to avoid hanging in the IBD loop on a fresh chain
+                    template = await mining_create_block_template(mining, stack, ctx, opts, cooldown=False)
+                    assert template is not None
+                    block = await mining_get_block(template, ctx)
+                    # Heights <= 16 need extra nonce padding.
+                    extra_nonce = b'\xaa\xbb\xcc\xdd' if height <= 16 else b""
+                    coinbase = await self.build_coinbase_test(template, ctx, miniwallet, extra_nonce=extra_nonce)
+                    block.vtx[0] = coinbase
+                    block.hashMerkleRoot = block.calc_merkle_root()
+                    block.solve()
+                    submitted = (await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())).result
+                    assert_equal(submitted, True)
+                    assert_equal(node.getblockcount(), height)
 
         asyncio.run(capnp.run(async_routine()))