From 36dbebafb9b54764005e6fffa7ad28d4cadfe5e4 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Thu, 21 Nov 2024 12:14:25 +0100 Subject: [PATCH] rpc: Remove submitblock coinbase pre-check The coinbase check is repeated again early during ProcessNewBlock. Pre-checking it may also shadow more fundamental problems with a block. In most cases the block header is checked first, before validating the transactions. Checking the coinbase first therefore masks potential issues with the header. Fix this by removing the pre-check. The pre-check was likely introduced on top of ada0caa165905b50db351a56ec124518c922085a to fix UB in GetWitnessCommitmentIndex in case a block's transactions are empty. This code path could only be reached because of the call to UpdateUncommittedBlockStructures in submitblock, but cannot be reached through net_processing. Add some functional test cases to cover the previous conditions that lead to a "Block does not start with a coinbase" json rpc error being returned. --- With the introduction of a mining ipc interface and the potential future introduction of a kernel library API it becomes increasingly important to offer common behaviour between them. An example of this is ProcessNewBlock, which is used by ipc, rpc, net_processing and (potentially) the kernel library. Having divergent behaviour on suggested pre-checks and checks for these functions is confusing to both developers and users and is a maintenance burden. The rpc interface for ProcessNewBlock (submitblock) currently pre-checks if the block has a coinbase transaction and whether it has been processed before. While the current example binary for how to use the kernel library, bitcoin-chainstate, imitates these checks, the other interfaces do not. --- src/rpc/mining.cpp | 4 ---- test/functional/mining_basic.py | 16 +++++++++++++--- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index c1af397872c..7726d250994 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -1015,10 +1015,6 @@ static RPCHelpMan submitblock() throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Block decode failed"); } - if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) { - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Block does not start with a coinbase"); - } - ChainstateManager& chainman = EnsureAnyChainman(request.context); uint256 hash = block.GetHash(); { diff --git a/test/functional/mining_basic.py b/test/functional/mining_basic.py index aca71933ec5..decee34e99c 100755 --- a/test/functional/mining_basic.py +++ b/test/functional/mining_basic.py @@ -239,9 +239,19 @@ class MiningTest(BitcoinTestFramework): bad_block.vtx[0].rehash() assert_template(node, bad_block, 'bad-cb-missing') - self.log.info("submitblock: Test invalid coinbase transaction") - assert_raises_rpc_error(-22, "Block does not start with a coinbase", node.submitblock, CBlock().serialize().hex()) - assert_raises_rpc_error(-22, "Block does not start with a coinbase", node.submitblock, bad_block.serialize().hex()) + self.log.info("submitblock: Test bad input hash for coinbase transaction") + bad_block.solve() + assert_equal("bad-cb-missing", node.submitblock(hexdata=bad_block.serialize().hex())) + + self.log.info("submitblock: Test block with no transactions") + no_tx_block = copy.deepcopy(block) + no_tx_block.vtx.clear() + no_tx_block.hashMerkleRoot = 0 + no_tx_block.solve() + assert_equal("bad-blk-length", node.submitblock(hexdata=no_tx_block.serialize().hex())) + + self.log.info("submitblock: Test empty block") + assert_equal('high-hash', node.submitblock(hexdata=CBlock().serialize().hex())) self.log.info("getblocktemplate: Test truncated final transaction") assert_raises_rpc_error(-22, "Block decode failed", node.getblocktemplate, {