coinstats: Fix hash_serialized2 calculation

The legacy serialization was vulnerable to maleation and is fixed by
adopting the same serialization procedure as was already in use for
MuHash.

This also includes necessary test fixes where the hash_serialized2 was
hardcoded as well as correction of the regtest chainparams.

Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>

5 files changed, 40 insertions, 47 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..f928520d3e 100644
--- a/src/kernel/chainparams.cpp
+++ b/src/kernel/chainparams.cpp
@@ -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/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);
 }