validation: check invariants when inserting into m_blocks_unlinked

For an entry A -> B in m_blocks_unlinked, the entry B was added into
m_blocks_unlinked either because:
- some ancestor of B was never received (or)
- some ancestor of B was pruned away.

Every insert must satisfy two invariants:
1. B has BLOCK_HAVE_DATA set.
2. No duplicate A -> B entries in m_blocks_unlinked (this is UB zone if
	this entry gets popped twice in ReceivedBlockTransactions and
	happens to be in setBlockIndexCandidates)

2 bugs (#35070 and #35168) discovered recently stemmed from the
m_blocks_unlinked insertion sites not enforcing these invariants.
So add a helper which wraps around insertion sites of m_blocks_unlinked
with these invariants.

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

src/node/blockstorage.cpp

@@ -255,6 +255,18 @@ CBlockIndex* BlockManager::AddToBlockIndex(const CBlockHeader& block, CBlockInde
     return pindexNew;
 }
 
+void BlockManager::AddUnlinkedBlock(CBlockIndex* block)
+{
+    AssertLockHeld(cs_main);
+    Assume(block != nullptr);
+    Assume(block->nStatus & BLOCK_HAVE_DATA);
+    auto range = m_blocks_unlinked.equal_range(block->pprev);
+    for (auto it = range.first; it != range.second; ++it) {
+        if (it->second == block) return;  // don't insert duplicates
+    }
+    m_blocks_unlinked.emplace(block->pprev, block);
+}
+
 void BlockManager::PruneOneBlockFile(const int fileNumber)
 {
     AssertLockHeld(cs_main);
@@ -487,7 +499,7 @@ bool BlockManager::LoadBlockIndex(const std::optional<uint256>& snapshot_blockha
                 } else {
                     pindex->m_chain_tx_count = 0;
                     if (pindex->nStatus & BLOCK_HAVE_DATA) {
-                        m_blocks_unlinked.insert(std::make_pair(pindex->pprev, pindex));
+                        AddUnlinkedBlock(pindex);
                     }
                 }
             } else {

src/node/blockstorage.h

@@ -354,6 +354,7 @@ public:
      * All pairs A->B, where A (or one of its ancestors) misses transactions, but B has transactions.
      */
     std::multimap<CBlockIndex*, CBlockIndex*> m_blocks_unlinked;
+    void AddUnlinkedBlock(CBlockIndex* block) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     std::unique_ptr<BlockTreeDB> m_block_tree_db GUARDED_BY(::cs_main);
 

src/validation.cpp

@@ -3149,10 +3149,7 @@ CBlockIndex* Chainstate::FindMostWorkChain()
                         // processed twice in ReceivedBlockTransactions(), it may be re-added to
                         // setBlockIndexCandidates with a modified nSequenceId, breaking ordering
                         // guarantees and leading to undefined behavior.
-                        auto range = m_blockman.m_blocks_unlinked.equal_range(pindexFailed->pprev);
-                        if (!std::any_of(range.first, range.second, [&](const auto& p) { return p.second == pindexFailed; })) {
-                            m_blockman.m_blocks_unlinked.emplace(pindexFailed->pprev, pindexFailed);
-                        }
+                        m_blockman.AddUnlinkedBlock(pindexFailed);
                     }
                     setBlockIndexCandidates.erase(pindexFailed);
                 }
@@ -3816,7 +3813,7 @@ void ChainstateManager::ReceivedBlockTransactions(const CBlock& block, CBlockInd
         }
     } else {
         if (pindexNew->pprev && pindexNew->pprev->IsValid(BLOCK_VALID_TREE)) {
-            m_blockman.m_blocks_unlinked.insert(std::make_pair(pindexNew->pprev, pindexNew));
+            m_blockman.AddUnlinkedBlock(pindexNew);
         }
     }
 }