remove CBlockIndex copy construction

Copy construction of CBlockIndex objects is a footgun because of the
wide use of equality-by-pointer comparison in the code base. There are
also potential lifetime confusions of using copied instances, since
there are recursive pointer references (e.g. pprev).

We can't just delete the copy constructors because they are used for
derived classes (CDiskBlockIndex), so we mark them protected.

Delete move constructors and declare the destructor to satisfy the
"rule of 5."

3 files changed, 31 insertions, 17 deletions

diff --git a/src/chain.h b/src/chain.h
index 2d3b084b9b..e052b074a7 100644
--- a/src/chain.h
+++ b/src/chain.h
@@ -213,10 +213,6 @@ public:
     //! (memory only) Maximum nTime in the chain up to and including this block.
     unsigned int nTimeMax{0};
 
-    CBlockIndex()
-    {
-    }
-
     explicit CBlockIndex(const CBlockHeader& block)
         : nVersion{block.nVersion},
           hashMerkleRoot{block.hashMerkleRoot},
@@ -355,6 +351,24 @@ public:
     //! Efficiently find an ancestor of this block.
     CBlockIndex* GetAncestor(int height);
     const CBlockIndex* GetAncestor(int height) const;
+
+    CBlockIndex() = default;
+    ~CBlockIndex() = default;
+
+protected:
+    //! CBlockIndex should not allow public copy construction because equality
+    //! comparison via pointer is very common throughout the codebase, making
+    //! use of copy a footgun. Also, use of copies do not have the benefit
+    //! of simplifying lifetime considerations due to attributes like pprev and
+    //! pskip, which are at risk of becoming dangling pointers in a copied
+    //! instance.
+    //!
+    //! We declare these protected instead of simply deleting them so that
+    //! CDiskBlockIndex can reuse copy construction.
+    CBlockIndex(const CBlockIndex&) = default;
+    CBlockIndex& operator=(const CBlockIndex&) = delete;
+    CBlockIndex(CBlockIndex&&) = delete;
+    CBlockIndex& operator=(CBlockIndex&&) = delete;
 };
 
 arith_uint256 GetBlockProof(const CBlockIndex& block);
diff --git a/src/test/fuzz/pow.cpp b/src/test/fuzz/pow.cpp
index 507ce57ec0..eecf50f475 100644
--- a/src/test/fuzz/pow.cpp
+++ b/src/test/fuzz/pow.cpp
@@ -25,7 +25,7 @@ FUZZ_TARGET_INIT(pow, initialize_pow)
 {
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
     const Consensus::Params& consensus_params = Params().GetConsensus();
-    std::vector<CBlockIndex> blocks;
+    std::vector<std::unique_ptr<CBlockIndex>> blocks;
     const uint32_t fixed_time = fuzzed_data_provider.ConsumeIntegral<uint32_t>();
     const uint32_t fixed_bits = fuzzed_data_provider.ConsumeIntegral<uint32_t>();
     LIMITED_WHILE(fuzzed_data_provider.remaining_bytes() > 0, 10000) {
@@ -33,9 +33,10 @@ FUZZ_TARGET_INIT(pow, initialize_pow)
         if (!block_header) {
             continue;
         }
-        CBlockIndex current_block{*block_header};
+        CBlockIndex& current_block{
+            *blocks.emplace_back(std::make_unique<CBlockIndex>(*block_header))};
         {
-            CBlockIndex* previous_block = blocks.empty() ? nullptr : &PickValue(fuzzed_data_provider, blocks);
+            CBlockIndex* previous_block = blocks.empty() ? nullptr : PickValue(fuzzed_data_provider, blocks).get();
             const int current_height = (previous_block != nullptr && previous_block->nHeight != std::numeric_limits<int>::max()) ? previous_block->nHeight + 1 : 0;
             if (fuzzed_data_provider.ConsumeBool()) {
                 current_block.pprev = previous_block;
@@ -57,7 +58,6 @@ FUZZ_TARGET_INIT(pow, initialize_pow)
             } else {
                 current_block.nChainWork = ConsumeArithUInt256(fuzzed_data_provider);
             }
-            blocks.push_back(current_block);
         }
         {
             (void)GetBlockProof(current_block);
@@ -67,9 +67,9 @@ FUZZ_TARGET_INIT(pow, initialize_pow)
             }
         }
         {
-            const CBlockIndex* to = &PickValue(fuzzed_data_provider, blocks);
-            const CBlockIndex* from = &PickValue(fuzzed_data_provider, blocks);
-            const CBlockIndex* tip = &PickValue(fuzzed_data_provider, blocks);
+            const auto& to = PickValue(fuzzed_data_provider, blocks);
+            const auto& from = PickValue(fuzzed_data_provider, blocks);
+            const auto& tip = PickValue(fuzzed_data_provider, blocks);
             try {
                 (void)GetBlockProofEquivalentTime(*to, *from, *tip, consensus_params);
             } catch (const uint_error&) {
diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp
index 9f5fb17b60..2f091492e1 100644
--- a/src/test/miner_tests.cpp
+++ b/src/test/miner_tests.cpp
@@ -78,11 +78,11 @@ constexpr static struct {
               {0, 792342903},  {6, 678455063},  {6, 773251385},  {5, 186617471}, {6, 883189502},  {7, 396077336},
               {8, 254702874},  {0, 455592851}};
 
-static CBlockIndex CreateBlockIndex(int nHeight, CBlockIndex* active_chain_tip) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
+static std::unique_ptr<CBlockIndex> CreateBlockIndex(int nHeight, CBlockIndex* active_chain_tip) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
-    CBlockIndex index;
-    index.nHeight = nHeight;
-    index.pprev = active_chain_tip;
+    auto index{std::make_unique<CBlockIndex>()};
+    index->nHeight = nHeight;
+    index->pprev = active_chain_tip;
     return index;
 }
 
@@ -394,7 +394,7 @@ void MinerTestingSetup::TestBasicMining(const CChainParams& chainparams, const C
 
     {
         CBlockIndex* active_chain_tip = m_node.chainman->ActiveChain().Tip();
-        BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, CreateBlockIndex(active_chain_tip->nHeight + 2, active_chain_tip))); // Sequence locks pass on 2nd block
+        BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, *CreateBlockIndex(active_chain_tip->nHeight + 2, active_chain_tip))); // Sequence locks pass on 2nd block
     }
 
     // relative time locked
@@ -411,7 +411,7 @@ void MinerTestingSetup::TestBasicMining(const CChainParams& chainparams, const C
         m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime += SEQUENCE_LOCK_TIME; // Trick the MedianTimePast
     {
         CBlockIndex* active_chain_tip = m_node.chainman->ActiveChain().Tip();
-        BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, CreateBlockIndex(active_chain_tip->nHeight + 1, active_chain_tip)));
+        BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, *CreateBlockIndex(active_chain_tip->nHeight + 1, active_chain_tip)));
     }
 
     for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i) {