Merge bitcoin/bitcoin#25349: CBlockIndex/CDiskBlockIndex improvements for safety, consistent behavior

3a61fc56a0 refactor: move CBlockIndex#ToString() from header to implementation (Jon Atack)
57865eb512 CDiskBlockIndex: rename GetBlockHash() to ConstructBlockHash() (Jon Atack)
99e8ec8721 CDiskBlockIndex: remove unused ToString() class member (Jon Atack)
14aeece462 CBlockIndex: ensure phashBlock is not nullptr before dereferencing (Jon Atack)

Pull request description:

  Fix a few design issues, potential footguns and inconsistent behavior in the CBlockIndex and CDiskBlockIndex classes.

  - Ensure phashBlock in `CBlockIndex#GetBlockHash()` is not nullptr before dereferencing and remove a now-redundant assert preceding a GetBlockHash() caller.  This protects against UB here, and in case of failure (which would indicate a consensus bug), the debug log will print `bitcoind: chain.h:265: uint256 CBlockIndex::GetBlockHash() const: Assertion 'phashBlock != nullptr' failed. Aborted` instead of `Segmentation fault`.
  - Remove the unused `CDiskBlockIndex#ToString()` class member, and mark the inherited `CBlockIndex#ToString()` public interface member as deleted to disallow calling it in the derived CDiskBlockIndex class.
  - Rename the `CDiskBlockIndex GetBlockHash()` class member to `ConstructBlockHash()`, which also makes sense as they perform different operations to return a blockhash, and mark the inherited `CBlockIndex#GetBlockHash()` public interface member as deleted to disallow calling it in the derived CDiskBlockIndex class.
  - Move `CBlockIndex#ToString()` from header to implementation, which also allows dropping `tinyformat.h` from the header file.

  Rationale and discussion regarding the CDiskBlockIndex changes:

  Here is a failing test on master that demonstrates the inconsistent behavior of the current design: calling the same inherited public interface functions on the same CDiskBlockIndex object should yield identical behavior, but does not.

  ```diff
  diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
  index 6dc522b421..dac3840f32 100644
  --- a/src/test/validation_chainstatemanager_tests.cpp
  +++ b/src/test/validation_chainstatemanager_tests.cpp
  @@ -240,6 +240,15 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)

       const CBlockIndex* tip = chainman.ActiveTip();

       BOOST_CHECK_EQUAL(tip->nChainTx, au_data.nChainTx);

  +    // CDiskBlockIndex "is a" CBlockIndex, as it publicly inherits from it.
  +    // Test that calling the same inherited interface functions on the same
  +    // object yields identical behavior.
  +    CDiskBlockIndex index{tip};
  +    CBlockIndex *pB = &index;
  +    CDiskBlockIndex *pD = &index;
  +    BOOST_CHECK_EQUAL(pB->GetBlockHash(), pD->GetBlockHash());
  +    BOOST_CHECK_EQUAL(pB->ToString(), pD->ToString());
  ```

  (build and run: `$ ./src/test/test_bitcoin -t validation_chainstatemanager_tests`)

  The GetBlockHash() test assertion only passes on master because the different methods invoked by the current design happen to return the same result.  If one of the two is changed, it fails like the ToString() assertion does.

  Redefining inherited non-virtual functions is well-documented as incorrect design to avoid inconsistent behavior (see Scott Meyers, Effective C++, Item 36). Class usage is confusing when the behavior depends on the pointer definition instead of the object definition (static binding happening where dynamic binding was expected). This can lead to unsuspected or hard-to-track bugs.

  Outside of critical hot spots, correctness usually comes before optimisation, but the current design dates back to main.cpp and it may possibly have been chosen to avoid the overhead of dynamic dispatch.  This solution does the same: the class sizes are unchanged and no vptr or vtbl is added.

  There are better designs for doing this that use composition instead of inheritance, or that separate the public interface from the private implementations.  One example of the latter would be a non-virtual public interface that calls private virtual implementation methods, i.e. the Template pattern via the Non-Virtual Interface (NVI) idiom.

ACKs for top commit:
  vasild:
    ACK 3a61fc56a0

Tree-SHA512: 9ff358ab0a6d010b8f053ad8303c6d4d061e62d9c3755a56b9c9f5eab855d02f02bee42acc77dfa0cbf4bb5cb775daa72d675e1560610a29bd285c46faa85ab7

src/chain.cpp

@@ -4,6 +4,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <chain.h>
+#include <tinyformat.h>
 #include <util/time.h>
 
 std::string CBlockFileInfo::ToString() const
@@ -11,6 +12,12 @@ std::string CBlockFileInfo::ToString() const
     return strprintf("CBlockFileInfo(blocks=%u, size=%u, heights=%u...%u, time=%s...%s)", nBlocks, nSize, nHeightFirst, nHeightLast, FormatISO8601Date(nTimeFirst), FormatISO8601Date(nTimeLast));
 }
 
+std::string CBlockIndex::ToString() const
+{
+    return strprintf("CBlockIndex(pprev=%p, nHeight=%d, merkle=%s, hashBlock=%s)",
+                     pprev, nHeight, hashMerkleRoot.ToString(), GetBlockHash().ToString());
+}
+
 void CChain::SetTip(CBlockIndex *pindex) {
     if (pindex == nullptr) {
         vChain.clear();

src/chain.h

@@ -11,7 +11,6 @@
 #include <flatfile.h>
 #include <primitives/block.h>
 #include <sync.h>
-#include <tinyformat.h>
 #include <uint256.h>
 
 #include <vector>
@@ -263,6 +262,7 @@ public:
 
     uint256 GetBlockHash() const
     {
+        assert(phashBlock != nullptr);
         return *phashBlock;
     }
 
@@ -301,13 +301,7 @@ public:
         return pbegin[(pend - pbegin) / 2];
     }
 
-    std::string ToString() const
-    {
-        return strprintf("CBlockIndex(pprev=%p, nHeight=%d, merkle=%s, hashBlock=%s)",
-            pprev, nHeight,
-            hashMerkleRoot.ToString(),
-            GetBlockHash().ToString());
-    }
+    std::string ToString() const;
 
     //! Check whether this block index entry is valid up to the passed validity level.
     bool IsValid(enum BlockStatus nUpTo = BLOCK_VALID_TRANSACTIONS) const
@@ -402,7 +396,7 @@ public:
         READWRITE(obj.nNonce);
     }
 
-    uint256 GetBlockHash() const
+    uint256 ConstructBlockHash() const
     {
         CBlockHeader block;
         block.nVersion = nVersion;
@@ -414,16 +408,8 @@ public:
         return block.GetHash();
     }
 
-
-    std::string ToString() const
-    {
-        std::string str = "CDiskBlockIndex(";
-        str += CBlockIndex::ToString();
-        str += strprintf("\n                hashBlock=%s, hashPrev=%s)",
-            GetBlockHash().ToString(),
-            hashPrev.ToString());
-        return str;
-    }
+    uint256 GetBlockHash() = delete;
+    std::string ToString() = delete;
 };
 
 /** An in-memory indexed chain of blocks. */

src/test/fuzz/chain.cpp

@@ -23,7 +23,7 @@ FUZZ_TARGET(chain)
     disk_block_index->phashBlock = &zero;
     {
         LOCK(::cs_main);
-        (void)disk_block_index->GetBlockHash();
+        (void)disk_block_index->ConstructBlockHash();
         (void)disk_block_index->GetBlockPos();
         (void)disk_block_index->GetBlockTime();
         (void)disk_block_index->GetBlockTimeMax();
@@ -31,7 +31,6 @@ FUZZ_TARGET(chain)
         (void)disk_block_index->GetUndoPos();
         (void)disk_block_index->HaveTxsDownloaded();
         (void)disk_block_index->IsValid();
-        (void)disk_block_index->ToString();
     }
 
     const CBlockHeader block_header = disk_block_index->GetBlockHeader();

src/txdb.cpp

@@ -310,7 +310,7 @@ bool CBlockTreeDB::LoadBlockIndexGuts(const Consensus::Params& consensusParams,
             CDiskBlockIndex diskindex;
             if (pcursor->GetValue(diskindex)) {
                 // Construct block index object
-                CBlockIndex* pindexNew = insertBlockIndex(diskindex.GetBlockHash());
+                CBlockIndex* pindexNew = insertBlockIndex(diskindex.ConstructBlockHash());
                 pindexNew->pprev          = insertBlockIndex(diskindex.hashPrev);
                 pindexNew->nHeight        = diskindex.nHeight;
                 pindexNew->nFile          = diskindex.nFile;

src/validation.cpp

@@ -2266,7 +2266,6 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state,
         m_blockman.m_dirty_blockindex.insert(pindex);
     }
 
-    assert(pindex->phashBlock);
     // add this block to the view's block chain
     view.SetBestBlock(pindex->GetBlockHash());