Merge bitcoin#21584: Fix assumeutxo crash due to invalid base_blockhash

fa340b8 refactor: Avoid magic value of all-zeros in assumeutxo base_blockhash (MarcoFalke)
fae33f9 Fix assumeutxo crash due to invalid base_blockhash (MarcoFalke)
fa5668b refactor: Use type-safe assumeutxo hash (MarcoFalke)
0000007 refactor: Remove unused code (MarcoFalke)
faa921f move-only: Add util/hash_type (MarcoFalke)

Pull request description:

  Starting with commit d6af06d, a block hash of all-zeros is invalid and will lead to a crash of the node. Can be tested by cherry-picking the test changes without the other changes.

  Stack trace (copied from bitcoin#21584 (comment)):

  ```
  #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
  bitcoin#1  0x00007ffff583c8b1 in __GI_abort () at abort.c:79
  #2  0x00007ffff582c42a in __assert_fail_base (fmt=0x7ffff59b3a38 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
      assertion=assertion@entry=0x555556c8b450 "!hashBlock.IsNull()", file=file@entry=0x555556c8b464 "txdb.cpp", line=line@entry=89,
      function=function@entry=0x555556c8b46d "virtual bool CCoinsViewDB::BatchWrite(CCoinsMap &, const uint256 &)") at assert.c:92
  #3  0x00007ffff582c4a2 in __GI___assert_fail (assertion=0x555556c8b450 "!hashBlock.IsNull()", file=0x555556c8b464 "txdb.cpp", line=89,
      function=0x555556c8b46d "virtual bool CCoinsViewDB::BatchWrite(CCoinsMap &, const uint256 &)") at assert.c:101
  #4  0x000055555636738b in CCoinsViewDB::BatchWrite (this=0x5555577975c0, mapCoins=std::unordered_map with 110 elements = {...}, hashBlock=...) at txdb.cpp:89
  #5  0x00005555564a2e80 in CCoinsViewBacked::BatchWrite (this=0x5555577975f8, mapCoins=std::unordered_map with 110 elements = {...}, hashBlock=...) at coins.cpp:30
  #6  0x00005555564a43de in CCoinsViewCache::Flush (this=0x55555778eaf0) at coins.cpp:223
  #7  0x00005555563fc11d in ChainstateManager::PopulateAndValidateSnapshot (this=0x55555740b038 <g_chainman>, snapshot_chainstate=..., coins_file=..., metadata=...)
      at validation.cpp:5422
  #8  0x00005555563fab3d in ChainstateManager::ActivateSnapshot (this=0x55555740b038 <g_chainman>, coins_file=..., metadata=..., in_memory=true) at validation.cpp:5299
  #9  0x0000555555e8c893 in validation_chainstatemanager_tests::CreateAndActivateUTXOSnapshot<validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method()::$_12>(NodeContext&, boost::filesystem::path, validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method()::$_12) (node=...,
      root=..., malleation=...) at test/validation_chainstatemanager_tests.cpp:199
  #10 0x0000555555e8877a in validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method (this=0x7fffffffc8d0)
      at test/validation_chainstatemanager_tests.cpp:262

ACKs for top commit:
  laanwj:
    Code review re-ACK fa340b8
  jamesob:
    ACK fa340b8 ([`jamesob/ackr/21584.1.MarcoFalke.fix_assumeutxo_crash_due`](https://github.com/jamesob/bitcoin/tree/ackr/21584.1.MarcoFalke.fix_assumeutxo_crash_due))

Tree-SHA512: c2c4e66c1abfd400ef18a04f22fec1f302f1ff4d27a18050f492f688319deb4ccdd165ff792eee0a1f816e7b69fb64080662b79517ab669e3d26b9eb77802851

Author: laanwj

src/Makefile.am

@@ -246,6 +246,7 @@ BITCOIN_CORE_H = \
   util/fees.h \
   util/getuniquepath.h \
   util/golombrice.h \
+  util/hash_type.h \
   util/hasher.h \
   util/macros.h \
   util/message.h \

src/chainparams.cpp

@@ -451,11 +451,11 @@ class CRegTestParams : public CChainParams {
         m_assumeutxo_data = MapAssumeutxo{
             {
                 110,
-                {uint256S("0x1ebbf5850204c0bdb15bf030f47c7fe91d45c44c712697e4509ba67adb01c618"), 110},
+                {AssumeutxoHash{uint256S("0x1ebbf5850204c0bdb15bf030f47c7fe91d45c44c712697e4509ba67adb01c618")}, 110},
             },
             {
                 210,
-                {uint256S("0x9c5ed99ef98544b34f8920b6d1802f72ac28ae6e2bd2bd4c316ff10c230df3f2"), 210},
+                {AssumeutxoHash{uint256S("0x9c5ed99ef98544b34f8920b6d1802f72ac28ae6e2bd2bd4c316ff10c230df3f2")}, 210},
             },
         };
 
@@ -559,9 +559,3 @@ void SelectParams(const std::string& network)
     SelectBaseParams(network);
     globalChainParams = CreateChainParams(gArgs, network);
 }
-
-std::ostream& operator<<(std::ostream& o, const AssumeutxoData& aud)
-{
-    o << strprintf("AssumeutxoData(%s, %s)", aud.hash_serialized.ToString(), aud.nChainTx);
-    return o;
-}

src/chainparams.h

@@ -10,6 +10,7 @@
 #include <consensus/params.h>
 #include <primitives/block.h>
 #include <protocol.h>
+#include <util/hash_type.h>
 
 #include <memory>
 #include <vector>
@@ -25,14 +26,18 @@ struct CCheckpointData {
     }
 };
 
+struct AssumeutxoHash : public BaseHash<uint256> {
+    explicit AssumeutxoHash(const uint256& hash) : BaseHash(hash) {}
+};
+
 /**
  * Holds configuration for use during UTXO snapshot load and validation. The contents
  * here are security critical, since they dictate which UTXO snapshots are recognized
  * as valid.
  */
 struct AssumeutxoData {
     //! The expected hash of the deserialized UTXO set.
-    const uint256 hash_serialized;
+    const AssumeutxoHash hash_serialized;
 
     //! Used to populate the nChainTx value, which is used during BlockManager::LoadBlockIndex().
     //!
@@ -41,8 +46,6 @@ struct AssumeutxoData {
     const unsigned int nChainTx;
 };
 
-std::ostream& operator<<(std::ostream& o, const AssumeutxoData& aud);
-
 using MapAssumeutxo = std::map<int, const AssumeutxoData>;
 
 /**

src/coins.h

@@ -20,8 +20,6 @@
 #include <functional>
 #include <unordered_map>
 
-class ChainstateManager;
-
 /**
  * A UTXO entry.
  *

src/script/standard.h

@@ -8,6 +8,7 @@
 
 #include <script/interpreter.h>
 #include <uint256.h>
+#include <util/hash_type.h>
 
 #include <string>
 #include <variant>
@@ -18,70 +19,6 @@ class CKeyID;
 class CScript;
 struct ScriptHash;
 
-template<typename HashType>
-class BaseHash
-{
-protected:
-    HashType m_hash;
-
-public:
-    BaseHash() : m_hash() {}
-    explicit BaseHash(const HashType& in) : m_hash(in) {}
-
-    unsigned char* begin()
-    {
-        return m_hash.begin();
-    }
-
-    const unsigned char* begin() const
-    {
-        return m_hash.begin();
-    }
-
-    unsigned char* end()
-    {
-        return m_hash.end();
-    }
-
-    const unsigned char* end() const
-    {
-        return m_hash.end();
-    }
-
-    operator std::vector<unsigned char>() const
-    {
-        return std::vector<unsigned char>{m_hash.begin(), m_hash.end()};
-    }
-
-    std::string ToString() const
-    {
-        return m_hash.ToString();
-    }
-
-    bool operator==(const BaseHash<HashType>& other) const noexcept
-    {
-        return m_hash == other.m_hash;
-    }
-
-    bool operator!=(const BaseHash<HashType>& other) const noexcept
-    {
-        return !(m_hash == other.m_hash);
-    }
-
-    bool operator<(const BaseHash<HashType>& other) const noexcept
-    {
-        return m_hash < other.m_hash;
-    }
-
-    size_t size() const
-    {
-        return m_hash.size();
-    }
-
-    unsigned char* data() { return m_hash.data(); }
-    const unsigned char* data() const { return m_hash.data(); }
-};
-
 /** A reference to a CScript: the Hash160 of its serialization (see script.h) */
 class CScriptID : public BaseHash<uint160>
 {

src/test/validation_chainstatemanager_tests.cpp

@@ -226,10 +226,8 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
 
     // Snapshot should refuse to load at this height.
     BOOST_REQUIRE(!CreateAndActivateUTXOSnapshot(m_node, m_path_root));
-    BOOST_CHECK(chainman.ActiveChainstate().m_from_snapshot_blockhash.IsNull());
-    BOOST_CHECK_EQUAL(
-        chainman.ActiveChainstate().m_from_snapshot_blockhash,
-        chainman.SnapshotBlockhash().value_or(uint256()));
+    BOOST_CHECK(!chainman.ActiveChainstate().m_from_snapshot_blockhash);
+    BOOST_CHECK(!chainman.SnapshotBlockhash());
 
     // Mine 10 more blocks, putting at us height 110 where a valid assumeutxo value can
     // be found.
@@ -260,6 +258,11 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
             // Coins count is smaller than coins in file
             metadata.m_coins_count -= 1;
     }));
+    BOOST_REQUIRE(!CreateAndActivateUTXOSnapshot(
+        m_node, m_path_root, [](CAutoFile& auto_infile, SnapshotMetadata& metadata) {
+            // Wrong hash
+            metadata.m_base_blockhash = uint256::ZERO;
+    }));
     BOOST_REQUIRE(!CreateAndActivateUTXOSnapshot(
         m_node, m_path_root, [](CAutoFile& auto_infile, SnapshotMetadata& metadata) {
             // Wrong hash
@@ -269,9 +272,9 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
     BOOST_REQUIRE(CreateAndActivateUTXOSnapshot(m_node, m_path_root));
 
     // Ensure our active chain is the snapshot chainstate.
-    BOOST_CHECK(!chainman.ActiveChainstate().m_from_snapshot_blockhash.IsNull());
+    BOOST_CHECK(!chainman.ActiveChainstate().m_from_snapshot_blockhash->IsNull());
     BOOST_CHECK_EQUAL(
-        chainman.ActiveChainstate().m_from_snapshot_blockhash,
+        *chainman.ActiveChainstate().m_from_snapshot_blockhash,
         *chainman.SnapshotBlockhash());
 
     const AssumeutxoData& au_data = *ExpectedAssumeutxo(snapshot_height, ::Params());
@@ -347,7 +350,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
 
     // Snapshot blockhash should be unchanged.
     BOOST_CHECK_EQUAL(
-        chainman.ActiveChainstate().m_from_snapshot_blockhash,
+        *chainman.ActiveChainstate().m_from_snapshot_blockhash,
         loaded_snapshot_blockhash);
 }
 

src/test/validation_tests.cpp

@@ -135,11 +135,11 @@ BOOST_AUTO_TEST_CASE(test_assumeutxo)
     }
 
     const auto out110 = *ExpectedAssumeutxo(110, *params);
-    BOOST_CHECK_EQUAL(out110.hash_serialized, uint256S("1ebbf5850204c0bdb15bf030f47c7fe91d45c44c712697e4509ba67adb01c618"));
+    BOOST_CHECK_EQUAL(out110.hash_serialized.ToString(), "1ebbf5850204c0bdb15bf030f47c7fe91d45c44c712697e4509ba67adb01c618");
     BOOST_CHECK_EQUAL(out110.nChainTx, (unsigned int)110);
 
     const auto out210 = *ExpectedAssumeutxo(210, *params);
-    BOOST_CHECK_EQUAL(out210.hash_serialized, uint256S("9c5ed99ef98544b34f8920b6d1802f72ac28ae6e2bd2bd4c316ff10c230df3f2"));
+    BOOST_CHECK_EQUAL(out210.hash_serialized.ToString(), "9c5ed99ef98544b34f8920b6d1802f72ac28ae6e2bd2bd4c316ff10c230df3f2");
     BOOST_CHECK_EQUAL(out210.nChainTx, (unsigned int)210);
 }
 

src/util/hash_type.h

@@ -0,0 +1,72 @@
+// Copyright (c) 2020-2021 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_UTIL_HASH_TYPE_H
+#define BITCOIN_UTIL_HASH_TYPE_H
+
+template <typename HashType>
+class BaseHash
+{
+protected:
+    HashType m_hash;
+
+public:
+    BaseHash() : m_hash() {}
+    explicit BaseHash(const HashType& in) : m_hash(in) {}
+
+    unsigned char* begin()
+    {
+        return m_hash.begin();
+    }
+
+    const unsigned char* begin() const
+    {
+        return m_hash.begin();
+    }
+
+    unsigned char* end()
+    {
+        return m_hash.end();
+    }
+
+    const unsigned char* end() const
+    {
+        return m_hash.end();
+    }
+
+    operator std::vector<unsigned char>() const
+    {
+        return std::vector<unsigned char>{m_hash.begin(), m_hash.end()};
+    }
+
+    std::string ToString() const
+    {
+        return m_hash.ToString();
+    }
+
+    bool operator==(const BaseHash<HashType>& other) const noexcept
+    {
+        return m_hash == other.m_hash;
+    }
+
+    bool operator!=(const BaseHash<HashType>& other) const noexcept
+    {
+        return !(m_hash == other.m_hash);
+    }
+
+    bool operator<(const BaseHash<HashType>& other) const noexcept
+    {
+        return m_hash < other.m_hash;
+    }
+
+    size_t size() const
+    {
+        return m_hash.size();
+    }
+
+    unsigned char* data() { return m_hash.data(); }
+    const unsigned char* data() const { return m_hash.data(); }
+};
+
+#endif // BITCOIN_UTIL_HASH_TYPE_H