Merge #962: [Chain] Prune entire orphaned chains.

2054946 [Chain] Prune entire orphaned chains. (Zannick)

Pull request description:

  ### Problem

  Pruned blocks sometimes cause other blocks in the index (mostly descendants of orphans) to have invalid parents, which may trip an assert `pindexWalk->pprev` (#692) on startup. Additionally, because block indexes *don't own their own hash values*, removing the index from `mapBlockIndex` where the hash is allocated may lead to a use-after-free corrupting the hash of a block written to disk (may be the cause of #930 (comment)).

  ### Solution

  Select pruning candidates in two rounds: first, based on height. Then, add all descendants of the blocks chosen in the first round.

  For the memory issue, add a `shared_ptr` field to CBlockIndex that provides the hash value when the block is removed from mapBlockIndex. This will manage the memory of the hash for the lifetime of the object (which is forever anyway) and allows for efficient sharing and copying as usual (unique_ptr does not).

  ### Testing

  Via regtest. See #692 (comment)

Tree-SHA512: ef000928dff3ad45e214f07e23795d92b1a75c6221180982e02a82f28a69349975a46813a34d966c1bb73bd927d007ce892e0d85afe1cac055c1da3931e3ea5b

Author: CaveSpectre11

src/chain.cpp

@@ -109,8 +109,8 @@ const CBlockIndex* CBlockIndex::GetAncestor(int height) const
             heightWalk = heightSkip;
         } else {
             if (!pindexWalk->pprev) {
-                LogPrintf("%s: pindexWalk failed:  Hash: %s, Current Height: %d, Working Height: %d\n",
-                          __func__, pindexWalk->phashBlock->GetHex(), height, nHeight);
+                LogPrintf("%s: pindexWalk:  Hash: %s, Looking for Height: %d, Current Height: %d, Working Height: %d, pindexWalk height: %d\n",
+                          __func__, pindexWalk->phashBlock->GetHex(), height, heightWalk, nHeight, pindexWalk->nHeight);
             }
             assert(pindexWalk->pprev);
             pindexWalk = pindexWalk->pprev;
@@ -275,6 +275,14 @@ uint256 CBlockIndex::GetProgPowHash(uint256& mix_hash) const
     return ProgPowHash(header, mix_hash);
 }
 
+void CBlockIndex::CopyBlockHashIntoIndex()
+{
+    if (phashBlock) {
+        _phashBlock.reset(new uint256(*phashBlock));
+        phashBlock = _phashBlock.get();
+    }
+}
+
 // We are going to be performing a block hash for RandomX. To see if we need to spin up a new
 // cache, we can first check to see if we can use the current validation cache
 uint256 GetRandomXBlockHash(const int32_t& height, const uint256& hash_blob ) {

src/chain.h

@@ -15,8 +15,9 @@
 #include <uint256.h>
 #include <libzerocoin/bignum.h>
 
-#include <vector>
 #include <map>
+#include <memory>
+#include <vector>
 
 /**
  * Maximum amount of time that a block timestamp is allowed to exceed the
@@ -178,8 +179,11 @@ enum BlockMemFlags: uint32_t {
  */
 class CBlockIndex
 {
+private:
+    //! owned pointer to the hash of the block, for use when erased from mapBlockIndex.
+    std::shared_ptr<const uint256> _phashBlock;
 public:
-    //! pointer to the hash of the block, if any. Memory is owned by mapBlockIndex
+    //! pointer to the hash of the block, if any. Memory is owned by mapBlockIndex, if this index is in mapBlockIndex
     const uint256* phashBlock{nullptr};
 
     //! pointer to the index of the predecessor of this block
@@ -572,6 +576,8 @@ class CBlockIndex
     //! Adds all accumulators to the block idnex given an accumulator map
     void AddAccumulator(AccumulatorMap mapAccumulator);
 
+    //! Copies the value of phashBlock (if any) into a new uint256 owned by the CBlockIndex.
+    void CopyBlockHashIntoIndex();
 };
 
 arith_uint256 GetBlockProof(const CBlockIndex& block);

src/validation.cpp

@@ -3606,10 +3606,11 @@ void PruneStaleBlockIndexes()
     // This isn't going to change while we're processing, so just leave and try again later.
     if (!pindexBestHeader) return;
 
-    std::set<uint256> setDelete;
     uint32_t irrelevantIndexes = 0;
     int64_t lowestPrunedHeight = -1;
     int64_t currentHeight;
+    std::set<const CBlockIndex*> setDelete;
+    std::set<const CBlockIndex*> setMaybeDelete;
     std::vector<std::pair<uint256, CBlockIndex*>> checkPrunable;
     {
         LOCK(cs_mapblockindex);
@@ -3642,29 +3643,62 @@ void PruneStaleBlockIndexes()
 
             // if it's also old enough, add it to the prune list.
             if (currentHeight > static_cast<int>(pindex->nHeight + PRUNE_DEPTH)) {
-                setDelete.emplace(p.first);
+                setDelete.emplace(pindex);
 
                 // save the lowest height that we're purging, even if it's lower than the previous
                 // lastPrunedHeight
                 if (pindex->nHeight < lowestPrunedHeight || lowestPrunedHeight == -1) {
                     lowestPrunedHeight = pindex->nHeight;
                 }
+            } else if (!chainActive.Contains(pindex->pprev) && !IsAncestor(pindexBestHeader, pindex->pprev)) {
+                // Or, if an ancestor might be pruned, in which case we would have to prune, note it to check later.
+                setMaybeDelete.emplace(pindex);
+            }
+        }
+    }
+
+    int descendants = 0;
+    for (const CBlockIndex* pindex : setMaybeDelete) {
+        for (const CBlockIndex* prune : setDelete) {
+            if (IsAncestor(pindex, prune)) {
+                ++descendants;
+                setDelete.emplace(pindex);
+                break;
             }
         }
     }
 
     if (setDelete.size() > 0) {
-         LogPrintf("%s: Erasing %d of %d irrelevant indexes.  LastPrunedHeight now %d.\n",
-                   __func__, setDelete.size(), irrelevantIndexes, lowestPrunedHeight);
+        LogPrintf("%s: Erasing %d of %d irrelevant indexes, including %d descendants.  LastPrunedHeight now %d.\n",
+                  __func__, setDelete.size(), irrelevantIndexes, descendants, lowestPrunedHeight);
 
-         LOCK(cs_mapblockindex);
-         // Purge the ones we flagged
-         for (const uint256& hash : setDelete) {
-             mapBlockIndex.erase(hash);
-         }
+        LOCK(cs_mapblockindex);
+        // Purge the ones we flagged
+        for (const CBlockIndex* pindex : setDelete) {
+            auto p = mapBlockIndex.find(*pindex->phashBlock);
+            if (p != mapBlockIndex.end()) {
+                p->second->CopyBlockHashIntoIndex();
+                mapBlockIndex.erase(p);
+            }
+        }
 
-         // Step back a little for safety
-         lastPrunedHeight = lowestPrunedHeight;
+        // Step back a little for safety
+        lastPrunedHeight = lowestPrunedHeight;
+
+        // Double-check remaining unpruned indexes
+        for (const auto& p : mapBlockIndex) {
+            const CBlockIndex* pindex = p.second;
+
+            if (!chainActive.Contains(pindex)) {
+                if (setDelete.find(pindex->pprev) != setDelete.end()) {
+                    LogPrintf("%s: DANGER: Potentially dangling pprev pointer at orphan hash=%s height=%d. Chain height=%d\n",
+                              __func__, pindex->phashBlock->GetHex(), pindex->nHeight, chainActive.Height());
+                } else if (setDelete.find(pindex->pskip) != setDelete.end()) {
+                    LogPrintf("%s: DANGER: Potentially dangling pskip pointer at orphan hash=%s height=%d. Chain height=%d\n",
+                              __func__, pindex->phashBlock->GetHex(), pindex->nHeight, chainActive.Height());
+                }
+            }
+        }
     }
 }