diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index f4c42e204a7..943d34e75d9 100644 --- a/src/interfaces/mining.h +++ b/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 diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 836b828b5c4..574e293ac11 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -184,14 +184,19 @@ std::unique_ptr 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(nHeight - 1); coinbase_tx.lock_time = coinbaseTx.nLockTime; @@ -221,7 +226,6 @@ std::unique_ptr 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())); } diff --git a/src/node/types.h b/src/node/types.h index 01e74528e06..cc0c404892f 100644 --- a/src/node/types.h +++ b/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. diff --git a/test/functional/interface_ipc_mining.py b/test/functional/interface_ipc_mining.py index 0f54bb92efc..2740662f1c0 100755 --- a/test/functional/interface_ipc_mining.py +++ b/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()))