[Chain] Prune entire orphaned chains.

If the index contains forks other than the active fork, then
removing one from an early part of the chain can cause startup to
assert `pindexWalk->pprev` on the remaining blocks in the fork which
no longer have parents in the index. (Issue #692)

This is made likely by the pruning process that only prunes orphans
based on height far enough from the best block.

Solution: After finding out which index entries are old enough to be
pruned, search the index for all entries that descend from any of those,
and add them to be pruned.

Additionally, CBlockIndex pointers are passed all over the codebase and
are never deleted, but their hash values are pointers to the
mapBlockIndex's keys. Normally this isn't a problem (unordered_map),
until the data is erased and the memory reused.

Author: Zannick

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());
+                }
+            }
+        }
     }
 }