Merge bitcoin/bitcoin#31175: rpc: Remove submitblock pre-checks

73db95c kernel: Make bitcoin-chainstate's block validation mirror submitblock's (TheCharlatan)
bb53ce9 tests: Add functional test for submitting a previously pruned block (Greg Sanders)
1f7fc73 rpc: Remove submitblock duplicate pre-check (TheCharlatan)
e62a8ab rpc: Remove submitblock invalid-duplicate precheck (TheCharlatan)
36dbeba rpc: Remove submitblock coinbase pre-check (TheCharlatan)

Pull request description:

  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.

  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.

  Similary the duplicate checks are repeated early in the contextual checks of ProcessNewBlock. If duplicate blocks are detected much of their validation is skipped. Depending on the constitution of the block, validating the merkle root of the block is part of the more intensive workload when validating a block. This could be an argument for moving the pre-checks into block processing. In net_processing this would have a smaller effect however, since the block mutation check, which also validates the merkle root, is done before.

  Testing spamming a node with valid, but duplicate unrequested blocks seems to exhaust a CPU thread, but does not seem to significantly impact keeping up with the tip. The benefits of adding these checks to net_processing are questionable, especially since there are other ways to trigger the more CPU-intensive checks without submitting a duplicate block. Since these DOS concerns apply even less to the RPC interface, which does not have banning mechanics built in, remove them too.

  Finally, also remove the pre-checks from `bitcoin-chainstate.cpp`.

  ---

  This PR is part of the [libbitcoinkernel project](bitcoin/bitcoin#27587).

ACKs for top commit:
  Sjors:
    re-utACK 73db95c
  achow101:
    ACK 73db95c
  instagibbs:
    ACK 73db95c
  mzumsande:
    ACK 73db95c

Tree-SHA512: 2d02e851cf402ecf6a1968c058df3576aac407e200cbf922a1a6391b7f97b4f42c6d9f6b0a78b9d1af0a6d40bdd529a7b11a1e6d88885bd7b8b090f6d1411861

Author: achow101

doc/release-notes-31175.md

@@ -0,0 +1,8 @@
+RPC
+---
+
+Duplicate blocks submitted with `submitblock` will now persist their block data
+even if it was previously pruned. If pruning is activated, the data will be
+pruned again eventually once the block file it is persisted in is selected for
+pruning. This is consistent with the behaviour of `getblockfrompeer` where the
+block is persisted as well even when pruning.

src/bitcoin-chainstate.cpp

@@ -180,27 +180,6 @@ int main(int argc, char* argv[])
             break;
         }
 
-        if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) {
-            std::cerr << "Block does not start with a coinbase" << std::endl;
-            break;
-        }
-
-        uint256 hash = block.GetHash();
-        {
-            LOCK(cs_main);
-            const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(hash);
-            if (pindex) {
-                if (pindex->IsValid(BLOCK_VALID_SCRIPTS)) {
-                    std::cerr << "duplicate" << std::endl;
-                    break;
-                }
-                if (pindex->nStatus & BLOCK_FAILED_MASK) {
-                    std::cerr << "duplicate-invalid" << std::endl;
-                    break;
-                }
-            }
-        }
-
         {
             LOCK(cs_main);
             const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock);

src/rpc/mining.cpp

@@ -1024,25 +1024,7 @@ 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();
-    {
-        LOCK(cs_main);
-        const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(hash);
-        if (pindex) {
-            if (pindex->IsValid(BLOCK_VALID_SCRIPTS)) {
-                return "duplicate";
-            }
-            if (pindex->nStatus & BLOCK_FAILED_MASK) {
-                return "duplicate-invalid";
-            }
-        }
-    }
-
     {
         LOCK(cs_main);
         const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock);

src/validation.cpp

@@ -4323,7 +4323,7 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida
                 *ppindex = pindex;
             if (pindex->nStatus & BLOCK_FAILED_MASK) {
                 LogDebug(BCLog::VALIDATION, "%s: block %s is marked invalid\n", __func__, hash.ToString());
-                return state.Invalid(BlockValidationResult::BLOCK_CACHED_INVALID, "duplicate");
+                return state.Invalid(BlockValidationResult::BLOCK_CACHED_INVALID, "duplicate-invalid");
             }
             return true;
         }

test/functional/mining_basic.py

@@ -56,7 +56,12 @@ def assert_template(node, block, expect, rehash=True):
 
 class MiningTest(BitcoinTestFramework):
     def set_test_params(self):
-        self.num_nodes = 2
+        self.num_nodes = 3
+        self.extra_args = [
+            [],
+            [],
+            ["-fastprune", "-prune=1"]
+        ]
         self.setup_clean_chain = True
         self.supports_cli = False
 
@@ -169,6 +174,21 @@ def test_timewarp(self):
         bad_block.solve()
         node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex())
 
+    def test_pruning(self):
+        self.log.info("Test that submitblock stores previously pruned block")
+        prune_node = self.nodes[2]
+        self.generate(prune_node, 400, sync_fun=self.no_op)
+        pruned_block = prune_node.getblock(prune_node.getblockhash(2), verbosity=0)
+        pruned_height = prune_node.pruneblockchain(400)
+        assert_greater_than_or_equal(pruned_height, 2)
+        pruned_blockhash = prune_node.getblockhash(2)
+
+        assert_raises_rpc_error(-1, 'Block not available (pruned data)', prune_node.getblock, pruned_blockhash)
+
+        result = prune_node.submitblock(pruned_block)
+        assert_equal(result, "inconclusive")
+        assert_equal(prune_node.getblock(pruned_blockhash, verbosity=0), pruned_block)
+
     def run_test(self):
         node = self.nodes[0]
         self.wallet = MiniWallet(node)
@@ -240,9 +260,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, {
@@ -377,6 +407,7 @@ def chain_tip(b_hash, *, status='headers-only', branchlen=1):
 
         self.test_blockmintxfee_parameter()
         self.test_timewarp()
+        self.test_pruning()
 
 
 if __name__ == '__main__':