Merge bitcoin/bitcoin#25311: refactor: remove CBlockIndex copy construction

36c201feb7 remove CBlockIndex copy construction (James O'Beirne)

Pull request description:

  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 members (e.g. pprev).

  (See also https://github.com/bitcoin/bitcoin/pull/24008#discussion_r891949166)

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

ACKs for top commit:
  ajtowns:
    ACK 36c201feb7 - code review only
  MarcoFalke:
    re-ACK 36c201feb7  🏻

Tree-SHA512: b1cf9a1cb992464a4377dad609713eea63cc099435df374e4553bfe62d362a4eb5e3c6c6649177832f38c0905b23841caf9d62196cef8e3084bfea0bfc26374b

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

src/test/fuzz/pow.cpp

@@ -26,7 +26,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) {
@@ -34,9 +34,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;
@@ -58,7 +59,6 @@ FUZZ_TARGET_INIT(pow, initialize_pow)
             } else {
                 current_block.nChainWork = ConsumeArithUInt256(fuzzed_data_provider);
             }
-            blocks.push_back(current_block);
         }
         {
             (void)GetBlockProof(current_block);
@@ -68,9 +68,9 @@ FUZZ_TARGET_INIT(pow, initialize_pow)
             }
         }
         {
-            const CBlockIndex* to = &PickValue(fuzzed_data_provider, blocks);
+            const auto& to = PickValue(fuzzed_data_provider, blocks);
-            const CBlockIndex* from = &PickValue(fuzzed_data_provider, blocks);
+            const auto& from = PickValue(fuzzed_data_provider, blocks);
-            const CBlockIndex* tip = &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&) {

src/test/miner_tests.cpp

@@ -87,11 +87,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;
+    auto index{std::make_unique<CBlockIndex>()};
-    index.nHeight = nHeight;
+    index->nHeight = nHeight;
-    index.pprev = active_chain_tip;
+    index->pprev = active_chain_tip;
     return index;
 }
 
@@ -438,7 +438,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std::
 
     {
         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
@@ -455,7 +455,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std::
         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) {