Merge bitcoin/bitcoin#28685: coinstats, assumeutxo: fix hash_serialized2 calculation

4bfaad4eca01674a9c84a447a17594dc2b9a4c39 chainparams, assumeutxo: Fix signet txoutset hash (Fabian Jahr)
a503cd0f0b55736743bcf8d2c46d271064772bef chainparams, assumeutxo: Fix testnet txoutset hash (Fabian Jahr)
f6213929c519d0e615cacd3d6f479f1517be1662 assumeutxo: Check deserialized coins for out of range values (Fabian Jahr)
66865446a771327be9e972cdaf01154ea1bdff6d docs: Add release notes for #28685 (Fabian Jahr)
cb0336817edc2b6aee2eca818f133841f613a767 scripted-diff: Rename hash_serialized_2 to hash_serialized_3 (Fabian Jahr)
351370a1d211615e3d5b158ccb0400cd79c5c085 coinstats: Fix hash_serialized2 calculation (Fabian Jahr)

Pull request description:

  Closes #28675

  The last commit demonstrates that theStack's analysis [here](https://github.com/bitcoin/bitcoin/issues/28675#issuecomment-1770389468) seems to be correct. There will be more changes needed for the rest of the test suite but the `feature_assumeutxo.py` with my additional tests pass.

ACKs for top commit:
  achow101:
    ACK 4bfaad4eca01674a9c84a447a17594dc2b9a4c39
  theStack:
    Code-review ACK 4bfaad4eca01674a9c84a447a17594dc2b9a4c39
  ryanofsky:
    Code review ACK 4bfaad4eca01674a9c84a447a17594dc2b9a4c39

Tree-SHA512: 2f6abc92b282f7c5da46391803cf0804d13978d191d541f2509b532c538abccd0a081e46cda23d80d47206a05fa2b5d41b7ab246e6a263db7a7461d6292116ef

7 files changed, 52 insertions, 54 deletions

diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp
index 9dab8ca901..ecd3fd21b5 100644
--- a/src/index/coinstatsindex.cpp
+++ b/src/index/coinstatsindex.cpp
@@ -15,9 +15,10 @@
 #include <undo.h>
 #include <validation.h>
 
+using kernel::ApplyCoinHash;
 using kernel::CCoinsStats;
 using kernel::GetBogoSize;
-using kernel::TxOutSer;
+using kernel::RemoveCoinHash;
 
 static constexpr uint8_t DB_BLOCK_HASH{'s'};
 static constexpr uint8_t DB_BLOCK_HEIGHT{'t'};
@@ -166,7 +167,7 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block)
                     continue;
                 }
 
-                m_muhash.Insert(MakeUCharSpan(TxOutSer(outpoint, coin)));
+                ApplyCoinHash(m_muhash, outpoint, coin);
 
                 if (tx->IsCoinBase()) {
                     m_total_coinbase_amount += coin.out.nValue;
@@ -187,7 +188,7 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block)
                     Coin coin{tx_undo.vprevout[j]};
                     COutPoint outpoint{tx->vin[j].prevout.hash, tx->vin[j].prevout.n};
 
-                    m_muhash.Remove(MakeUCharSpan(TxOutSer(outpoint, coin)));
+                    RemoveCoinHash(m_muhash, outpoint, coin);
 
                     m_total_prevout_spent_amount += coin.out.nValue;
 
@@ -443,7 +444,7 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
                 continue;
             }
 
-            m_muhash.Remove(MakeUCharSpan(TxOutSer(outpoint, coin)));
+            RemoveCoinHash(m_muhash, outpoint, coin);
 
             if (tx->IsCoinBase()) {
                 m_total_coinbase_amount -= coin.out.nValue;
@@ -464,7 +465,7 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
                 Coin coin{tx_undo.vprevout[j]};
                 COutPoint outpoint{tx->vin[j].prevout.hash, tx->vin[j].prevout.n};
 
-                m_muhash.Insert(MakeUCharSpan(TxOutSer(outpoint, coin)));
+                ApplyCoinHash(m_muhash, outpoint, coin);
 
                 m_total_prevout_spent_amount -= coin.out.nValue;
 
diff --git a/src/kernel/chainparams.cpp b/src/kernel/chainparams.cpp
index 3ac8756e41..73ba330ff0 100644
--- a/src/kernel/chainparams.cpp
+++ b/src/kernel/chainparams.cpp
@@ -269,7 +269,7 @@ public:
         m_assumeutxo_data = {
             {
                 .height = 2'500'000,
-                .hash_serialized = AssumeutxoHash{uint256S("0x2a8fdefef3bf75fa00540ccaaaba4b5281bea94229327bdb0f7416ef1e7a645c")},
+                .hash_serialized = AssumeutxoHash{uint256S("0xf841584909f68e47897952345234e37fcd9128cd818f41ee6c3ca68db8071be7")},
                 .nChainTx = 66484552,
                 .blockhash = uint256S("0x0000000000000093bcb68c03a9a168ae252572d348a2eaeba2cdf9231d73206f")
             }
@@ -378,7 +378,7 @@ public:
         m_assumeutxo_data = {
             {
                 .height = 160'000,
-                .hash_serialized = AssumeutxoHash{uint256S("0x5225141cb62dee63ab3be95f9b03d60801f264010b1816d4bd00618b2736e7be")},
+                .hash_serialized = AssumeutxoHash{uint256S("0xfe0a44309b74d6b5883d246cb419c6221bcccf0b308c9b59b7d70783dbdf928a")},
                 .nChainTx = 2289496,
                 .blockhash = uint256S("0x0000003ca3c99aff040f2563c2ad8f8ec88bd0fd6b8f0895cfaf1ef90353a62c")
             }
@@ -494,14 +494,14 @@ public:
         m_assumeutxo_data = {
             {
                 .height = 110,
-                .hash_serialized = AssumeutxoHash{uint256S("0x1ebbf5850204c0bdb15bf030f47c7fe91d45c44c712697e4509ba67adb01c618")},
+                .hash_serialized = AssumeutxoHash{uint256S("0x6657b736d4fe4db0cbc796789e812d5dba7f5c143764b1b6905612f1830609d1")},
                 .nChainTx = 111,
                 .blockhash = uint256S("0x696e92821f65549c7ee134edceeeeaaa4105647a3c4fd9f298c0aec0ab50425c")
             },
             {
                 // For use by test/functional/feature_assumeutxo.py
                 .height = 299,
-                .hash_serialized = AssumeutxoHash{uint256S("0xef45ccdca5898b6c2145e4581d2b88c56564dd389e4bd75a1aaf6961d3edd3c0")},
+                .hash_serialized = AssumeutxoHash{uint256S("0x61d9c2b29a2571a5fe285fe2d8554f91f93309666fc9b8223ee96338de25ff53")},
                 .nChainTx = 300,
                 .blockhash = uint256S("0x7e0517ef3ea6ecbed9117858e42eedc8eb39e8698a38dcbd1b3962a283233f4c")
             },
diff --git a/src/kernel/coinstats.cpp b/src/kernel/coinstats.cpp
index 527433f45e..9bd755ed27 100644
--- a/src/kernel/coinstats.cpp
+++ b/src/kernel/coinstats.cpp
@@ -48,15 +48,35 @@ uint64_t GetBogoSize(const CScript& script_pub_key)
            script_pub_key.size() /* scriptPubKey */;
 }
 
-DataStream TxOutSer(const COutPoint& outpoint, const Coin& coin)
+template <typename T>
+static void TxOutSer(T& ss, const COutPoint& outpoint, const Coin& coin)
 {
-    DataStream ss{};
     ss << outpoint;
-    ss << static_cast<uint32_t>(coin.nHeight * 2 + coin.fCoinBase);
+    ss << static_cast<uint32_t>((coin.nHeight << 1) + coin.fCoinBase);
     ss << coin.out;
-    return ss;
 }
 
+static void ApplyCoinHash(HashWriter& ss, const COutPoint& outpoint, const Coin& coin)
+{
+    TxOutSer(ss, outpoint, coin);
+}
+
+void ApplyCoinHash(MuHash3072& muhash, const COutPoint& outpoint, const Coin& coin)
+{
+    DataStream ss{};
+    TxOutSer(ss, outpoint, coin);
+    muhash.Insert(MakeUCharSpan(ss));
+}
+
+void RemoveCoinHash(MuHash3072& muhash, const COutPoint& outpoint, const Coin& coin)
+{
+    DataStream ss{};
+    TxOutSer(ss, outpoint, coin);
+    muhash.Remove(MakeUCharSpan(ss));
+}
+
+static void ApplyCoinHash(std::nullptr_t, const COutPoint& outpoint, const Coin& coin) {}
+
 //! Warning: be very careful when changing this! assumeutxo and UTXO snapshot
 //! validation commitments are reliant on the hash constructed by this
 //! function.
@@ -69,32 +89,13 @@ DataStream TxOutSer(const COutPoint& outpoint, const Coin& coin)
 //! It is also possible, though very unlikely, that a change in this
 //! construction could cause a previously invalid (and potentially malicious)
 //! UTXO snapshot to be considered valid.
-static void ApplyHash(HashWriter& ss, const uint256& hash, const std::map<uint32_t, Coin>& outputs)
-{
-    for (auto it = outputs.begin(); it != outputs.end(); ++it) {
-        if (it == outputs.begin()) {
-            ss << hash;
-            ss << VARINT(it->second.nHeight * 2 + it->second.fCoinBase ? 1u : 0u);
-        }
-
-        ss << VARINT(it->first + 1);
-        ss << it->second.out.scriptPubKey;
-        ss << VARINT_MODE(it->second.out.nValue, VarIntMode::NONNEGATIVE_SIGNED);
-
-        if (it == std::prev(outputs.end())) {
-            ss << VARINT(0u);
-        }
-    }
-}
-
-static void ApplyHash(std::nullptr_t, const uint256& hash, const std::map<uint32_t, Coin>& outputs) {}
-
-static void ApplyHash(MuHash3072& muhash, const uint256& hash, const std::map<uint32_t, Coin>& outputs)
+template <typename T>
+static void ApplyHash(T& hash_obj, const uint256& hash, const std::map<uint32_t, Coin>& outputs)
 {
     for (auto it = outputs.begin(); it != outputs.end(); ++it) {
         COutPoint outpoint = COutPoint(hash, it->first);
         Coin coin = it->second;
-        muhash.Insert(MakeUCharSpan(TxOutSer(outpoint, coin)));
+        ApplyCoinHash(hash_obj, outpoint, coin);
     }
 }
 
@@ -118,8 +119,6 @@ static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, c
     std::unique_ptr<CCoinsViewCursor> pcursor(view->Cursor());
     assert(pcursor);
 
-    PrepareHash(hash_obj, stats);
-
     uint256 prevkey;
     std::map<uint32_t, Coin> outputs;
     while (pcursor->Valid()) {
@@ -180,15 +179,6 @@ std::optional<CCoinsStats> ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsV
     return stats;
 }
 
-// The legacy hash serializes the hashBlock
-static void PrepareHash(HashWriter& ss, const CCoinsStats& stats)
-{
-    ss << stats.hashBlock;
-}
-// MuHash does not need the prepare step
-static void PrepareHash(MuHash3072& muhash, CCoinsStats& stats) {}
-static void PrepareHash(std::nullptr_t, CCoinsStats& stats) {}
-
 static void FinalizeHash(HashWriter& ss, CCoinsStats& stats)
 {
     stats.hashSerialized = ss.GetHash();
diff --git a/src/kernel/coinstats.h b/src/kernel/coinstats.h
index 54d0e4f664..c0c363a842 100644
--- a/src/kernel/coinstats.h
+++ b/src/kernel/coinstats.h
@@ -6,6 +6,7 @@
 #define BITCOIN_KERNEL_COINSTATS_H
 
 #include <consensus/amount.h>
+#include <crypto/muhash.h>
 #include <streams.h>
 #include <uint256.h>
 
@@ -72,7 +73,8 @@ struct CCoinsStats {
 
 uint64_t GetBogoSize(const CScript& script_pub_key);
 
-DataStream TxOutSer(const COutPoint& outpoint, const Coin& coin);
+void ApplyCoinHash(MuHash3072& muhash, const COutPoint& outpoint, const Coin& coin);
+void RemoveCoinHash(MuHash3072& muhash, const COutPoint& outpoint, const Coin& coin);
 
 std::optional<CCoinsStats> ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsView* view, node::BlockManager& blockman, const std::function<void()>& interruption_point = {});
 } // namespace kernel
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 6d2b84cb6c..7b84747a3f 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -820,7 +820,7 @@ static RPCHelpMan pruneblockchain()
 
 CoinStatsHashType ParseHashType(const std::string& hash_type_input)
 {
-    if (hash_type_input == "hash_serialized_2") {
+    if (hash_type_input == "hash_serialized_3") {
         return CoinStatsHashType::HASH_SERIALIZED;
     } else if (hash_type_input == "muhash") {
         return CoinStatsHashType::MUHASH;
@@ -867,7 +867,7 @@ static RPCHelpMan gettxoutsetinfo()
                 "\nReturns statistics about the unspent transaction output set.\n"
                 "Note this call may take some time if you are not using coinstatsindex.\n",
                 {
-                    {"hash_type", RPCArg::Type::STR, RPCArg::Default{"hash_serialized_2"}, "Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'."},
+                    {"hash_type", RPCArg::Type::STR, RPCArg::Default{"hash_serialized_3"}, "Which UTXO set hash should be calculated. Options: 'hash_serialized_3' (the legacy algorithm), 'muhash', 'none'."},
                     {"hash_or_height", RPCArg::Type::NUM, RPCArg::DefaultHint{"the current best block"}, "The block hash or height of the target height (only available with coinstatsindex).",
                      RPCArgOptions{
                          .skip_type_check = true,
@@ -882,7 +882,7 @@ static RPCHelpMan gettxoutsetinfo()
                         {RPCResult::Type::STR_HEX, "bestblock", "The hash of the block at which these statistics are calculated"},
                         {RPCResult::Type::NUM, "txouts", "The number of unspent transaction outputs"},
                         {RPCResult::Type::NUM, "bogosize", "Database-independent, meaningless metric indicating the UTXO set size"},
-                        {RPCResult::Type::STR_HEX, "hash_serialized_2", /*optional=*/true, "The serialized hash (only present if 'hash_serialized_2' hash_type is chosen)"},
+                        {RPCResult::Type::STR_HEX, "hash_serialized_3", /*optional=*/true, "The serialized hash (only present if 'hash_serialized_3' hash_type is chosen)"},
                         {RPCResult::Type::STR_HEX, "muhash", /*optional=*/true, "The serialized hash (only present if 'muhash' hash_type is chosen)"},
                         {RPCResult::Type::NUM, "transactions", /*optional=*/true, "The number of transactions with unspent outputs (not available when coinstatsindex is used)"},
                         {RPCResult::Type::NUM, "disk_size", /*optional=*/true, "The estimated size of the chainstate on disk (not available when coinstatsindex is used)"},
@@ -942,7 +942,7 @@ static RPCHelpMan gettxoutsetinfo()
         }
 
         if (hash_type == CoinStatsHashType::HASH_SERIALIZED) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, "hash_serialized_2 hash type cannot be queried for a specific block");
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "hash_serialized_3 hash type cannot be queried for a specific block");
         }
 
         if (!index_requested) {
@@ -971,7 +971,7 @@ static RPCHelpMan gettxoutsetinfo()
         ret.pushKV("txouts", (int64_t)stats.nTransactionOutputs);
         ret.pushKV("bogosize", (int64_t)stats.nBogoSize);
         if (hash_type == CoinStatsHashType::HASH_SERIALIZED) {
-            ret.pushKV("hash_serialized_2", stats.hashSerialized.GetHex());
+            ret.pushKV("hash_serialized_3", stats.hashSerialized.GetHex());
         }
         if (hash_type == CoinStatsHashType::MUHASH) {
             ret.pushKV("muhash", stats.hashSerialized.GetHex());
diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp
index 2692037273..14440571eb 100644
--- a/src/test/validation_tests.cpp
+++ b/src/test/validation_tests.cpp
@@ -137,11 +137,11 @@ BOOST_AUTO_TEST_CASE(test_assumeutxo)
     }
 
     const auto out110 = *params->AssumeutxoForHeight(110);
-    BOOST_CHECK_EQUAL(out110.hash_serialized.ToString(), "1ebbf5850204c0bdb15bf030f47c7fe91d45c44c712697e4509ba67adb01c618");
+    BOOST_CHECK_EQUAL(out110.hash_serialized.ToString(), "6657b736d4fe4db0cbc796789e812d5dba7f5c143764b1b6905612f1830609d1");
     BOOST_CHECK_EQUAL(out110.nChainTx, 111U);
 
     const auto out110_2 = *params->AssumeutxoForBlockhash(uint256S("0x696e92821f65549c7ee134edceeeeaaa4105647a3c4fd9f298c0aec0ab50425c"));
-    BOOST_CHECK_EQUAL(out110_2.hash_serialized.ToString(), "1ebbf5850204c0bdb15bf030f47c7fe91d45c44c712697e4509ba67adb01c618");
+    BOOST_CHECK_EQUAL(out110_2.hash_serialized.ToString(), "6657b736d4fe4db0cbc796789e812d5dba7f5c143764b1b6905612f1830609d1");
     BOOST_CHECK_EQUAL(out110_2.nChainTx, 111U);
 }
 
diff --git a/src/validation.cpp b/src/validation.cpp
index 2600f0f9fe..a6cab6b095 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -5399,6 +5399,11 @@ bool ChainstateManager::PopulateAndValidateSnapshot(
                       coins_count - coins_left);
             return false;
         }
+        if (!MoneyRange(coin.out.nValue)) {
+            LogPrintf("[snapshot] bad snapshot data after deserializing %d coins - bad tx out value\n",
+                      coins_count - coins_left);
+            return false;
+        }
 
         coins_cache.EmplaceCoinInternalDANGER(std::move(outpoint), std::move(coin));