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
ada0caa 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.

Author: TheCharlatan

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();
     {

test/functional/mining_basic.py

@@ -239,9 +239,19 @@ def assert_submitblock(block, result_str_1, result_str_2=None):
         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, {