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
ada0caa165 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.
This commit is contained in:
TheCharlatan 2024-11-21 12:14:25 +01:00
parent da10e0bab4
commit 36dbebafb9
No known key found for this signature in database
GPG key ID: 9B79B45691DB4173
2 changed files with 13 additions and 7 deletions

View file

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

View file

@ -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, {