Merge bitcoin/bitcoin#23411: refactor: Avoid integer overflow in ApplyStats when activating snapshot

fa996c58e8a31ebe610d186cef408b6dd3b385a8 refactor: Avoid integer overflow in ApplyStats when activating snapshot (MarcoFalke)
fac01888d17423d6c23a9ce15d98fc88fb34e3cc Move AdditionOverflow to util, Add CheckedAdd with unit tests (MarcoFalke)
fa526d8fb6ab8f2678a30d4536aa9c45218f5269 Add dev doc to CCoinsStats::m_hash_type and make it const (MarcoFalke)
faff051560552d4405896e01920a18f698155a56 style: Remove unused whitespace (MarcoFalke)

Pull request description:

  A snapshot contains the utxo set, including the out value. To activate the snapshot, the hash needs to be calculated. As a side-effect, the total amount in the snapshot is calculated (as the sum of all out values), but never used. Instead of running into an integer overflow in an unused result, don't calculate the result in the first place.

  Other code paths (using the active utxo set) can not run into an integer overflow, since the active utxo set is valid.

  Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=39716

ACKs for top commit:
  shaavan:
    reACK fa996c58e8a31ebe610d186cef408b6dd3b385a8
  vasild:
    ACK fa996c58e8a31ebe610d186cef408b6dd3b385a8

Tree-SHA512: 4f207f634841f6f634fd02ae1e5907e343fd767524fd0e8149aa99fa9a1834fe50167d14874834d45236e9c325d567925f28129bacb7d80be29cf22277a16a14

src/Makefile.am

@@ -248,6 +248,7 @@ BITCOIN_CORE_H = \
   util/macros.h \
   util/message.h \
   util/moneystr.h \
+  util/overflow.h \
   util/overloaded.h \
   util/rbf.h \
   util/readwritefile.h \

src/index/coinstatsindex.cpp

@@ -321,7 +321,7 @@ bool CoinStatsIndex::LookUpStats(const CBlockIndex* block_index, CCoinsStats& co
     coins_stats.hashSerialized = entry.muhash;
     coins_stats.nTransactionOutputs = entry.transaction_output_count;
     coins_stats.nBogoSize = entry.bogo_size;
-    coins_stats.nTotalAmount = entry.total_amount;
+    coins_stats.total_amount = entry.total_amount;
     coins_stats.total_subsidy = entry.total_subsidy;
     coins_stats.total_unspendable_amount = entry.total_unspendable_amount;
     coins_stats.total_prevout_spent_amount = entry.total_prevout_spent_amount;

src/node/coinstats.cpp

@@ -11,6 +11,7 @@
 #include <index/coinstatsindex.h>
 #include <serialize.h>
 #include <uint256.h>
+#include <util/overflow.h>
 #include <util/system.h>
 #include <validation.h>
 
@@ -82,7 +83,9 @@ static void ApplyStats(CCoinsStats& stats, const uint256& hash, const std::map<u
     stats.nTransactions++;
     for (auto it = outputs.begin(); it != outputs.end(); ++it) {
         stats.nTransactionOutputs++;
-        stats.nTotalAmount += it->second.out.nValue;
+        if (stats.total_amount.has_value()) {
+            stats.total_amount = CheckedAdd(*stats.total_amount, it->second.out.nValue);
+        }
         stats.nBogoSize += GetBogoSize(it->second.out.scriptPubKey);
     }
 }
@@ -95,10 +98,8 @@ static bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats&
     assert(pcursor);
 
     if (!pindex) {
-        {
-            LOCK(cs_main);
-            pindex = blockman.LookupBlockIndex(view->GetBestBlock());
-        }
+        LOCK(cs_main);
+        pindex = blockman.LookupBlockIndex(view->GetBestBlock());
     }
     stats.nHeight = Assert(pindex)->nHeight;
     stats.hashBlock = pindex->GetBlockHash();

src/node/coinstats.h

@@ -24,9 +24,10 @@ enum class CoinStatsHashType {
     NONE,
 };
 
-struct CCoinsStats
-{
-    CoinStatsHashType m_hash_type;
+struct CCoinsStats {
+    //! Which hash type to use
+    const CoinStatsHashType m_hash_type;
+
     int nHeight{0};
     uint256 hashBlock{};
     uint64_t nTransactions{0};
@@ -34,7 +35,8 @@ struct CCoinsStats
     uint64_t nBogoSize{0};
     uint256 hashSerialized{};
     uint64_t nDiskSize{0};
-    CAmount nTotalAmount{0};
+    //! The total amount, or nullopt if an overflow occurred calculating it
+    std::optional<CAmount> total_amount{0};
 
     //! The number of coins contained.
     uint64_t coins_count{0};

src/rpc/blockchain.cpp

@@ -1274,9 +1274,10 @@ static RPCHelpMan gettxoutsetinfo()
             ret.pushKV("hash_serialized_2", stats.hashSerialized.GetHex());
         }
         if (hash_type == CoinStatsHashType::MUHASH) {
-              ret.pushKV("muhash", stats.hashSerialized.GetHex());
+            ret.pushKV("muhash", stats.hashSerialized.GetHex());
         }
-        ret.pushKV("total_amount", ValueFromAmount(stats.nTotalAmount));
+        CHECK_NONFATAL(stats.total_amount.has_value());
+        ret.pushKV("total_amount", ValueFromAmount(stats.total_amount.value()));
         if (!stats.index_used) {
             ret.pushKV("transactions", static_cast<int64_t>(stats.nTransactions));
             ret.pushKV("disk_size", stats.nDiskSize);

src/test/fuzz/addition_overflow.cpp

@@ -5,6 +5,7 @@
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
+#include <util/overflow.h>
 
 #include <cstdint>
 #include <string>

src/test/fuzz/crypto_chacha20_poly1305_aead.cpp

@@ -7,6 +7,7 @@
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
+#include <util/overflow.h>
 
 #include <cassert>
 #include <cstdint>

src/test/fuzz/integer.cpp

@@ -26,6 +26,7 @@
 #include <univalue.h>
 #include <util/check.h>
 #include <util/moneystr.h>
+#include <util/overflow.h>
 #include <util/strencodings.h>
 #include <util/string.h>
 #include <util/system.h>

src/test/fuzz/pow.cpp

@@ -9,6 +9,7 @@
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
+#include <util/overflow.h>
 
 #include <cstdint>
 #include <optional>

src/test/fuzz/util.cpp

@@ -8,6 +8,7 @@
 #include <pubkey.h>
 #include <test/fuzz/util.h>
 #include <test/util/script.h>
+#include <util/overflow.h>
 #include <util/rbf.h>
 #include <util/time.h>
 #include <version.h>

src/test/fuzz/util.h

@@ -193,17 +193,6 @@ template <typename T>
     }
 }
 
-template <class T>
-[[nodiscard]] bool AdditionOverflow(const T i, const T j) noexcept
-{
-    static_assert(std::is_integral<T>::value, "Integral required.");
-    if (std::numeric_limits<T>::is_signed) {
-        return (i > 0 && j > std::numeric_limits<T>::max() - i) ||
-               (i < 0 && j < std::numeric_limits<T>::min() - i);
-    }
-    return std::numeric_limits<T>::max() - i < j;
-}
-
 [[nodiscard]] bool ContainsSpentInput(const CTransaction& tx, const CCoinsViewCache& inputs) noexcept;
 
 /**

src/test/util_tests.cpp

@@ -15,6 +15,7 @@
 #include <util/getuniquepath.h>
 #include <util/message.h> // For MessageSign(), MessageVerify(), MESSAGE_MAGIC
 #include <util/moneystr.h>
+#include <util/overflow.h>
 #include <util/spanparsing.h>
 #include <util/strencodings.h>
 #include <util/string.h>
@@ -1463,6 +1464,38 @@ BOOST_AUTO_TEST_CASE(test_IsDigit)
     BOOST_CHECK_EQUAL(IsDigit(9), false);
 }
 
+/* Check for overflow */
+template <typename T>
+static void TestAddMatrixOverflow()
+{
+    constexpr T MAXI{std::numeric_limits<T>::max()};
+    BOOST_CHECK(!CheckedAdd(T{1}, MAXI));
+    BOOST_CHECK(!CheckedAdd(MAXI, MAXI));
+    BOOST_CHECK_EQUAL(0, CheckedAdd(T{0}, T{0}).value());
+    BOOST_CHECK_EQUAL(MAXI, CheckedAdd(T{0}, MAXI).value());
+    BOOST_CHECK_EQUAL(MAXI, CheckedAdd(T{1}, MAXI - 1).value());
+}
+
+/* Check for overflow or underflow */
+template <typename T>
+static void TestAddMatrix()
+{
+    TestAddMatrixOverflow<T>();
+    constexpr T MINI{std::numeric_limits<T>::min()};
+    constexpr T MAXI{std::numeric_limits<T>::max()};
+    BOOST_CHECK(!CheckedAdd(T{-1}, MINI));
+    BOOST_CHECK(!CheckedAdd(MINI, MINI));
+    BOOST_CHECK_EQUAL(MINI, CheckedAdd(T{0}, MINI).value());
+    BOOST_CHECK_EQUAL(MINI, CheckedAdd(T{-1}, MINI + 1).value());
+    BOOST_CHECK_EQUAL(-1, CheckedAdd(MINI, MAXI).value());
+}
+
+BOOST_AUTO_TEST_CASE(util_overflow)
+{
+    TestAddMatrixOverflow<unsigned>();
+    TestAddMatrix<signed>();
+}
+
 BOOST_AUTO_TEST_CASE(test_ParseInt32)
 {
     int32_t n;

src/util/overflow.h

@@ -0,0 +1,31 @@
+// Copyright (c) 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_OVERFLOW_H
+#define BITCOIN_UTIL_OVERFLOW_H
+
+#include <limits>
+#include <type_traits>
+
+template <class T>
+[[nodiscard]] bool AdditionOverflow(const T i, const T j) noexcept
+{
+    static_assert(std::is_integral<T>::value, "Integral required.");
+    if (std::numeric_limits<T>::is_signed) {
+        return (i > 0 && j > std::numeric_limits<T>::max() - i) ||
+               (i < 0 && j < std::numeric_limits<T>::min() - i);
+    }
+    return std::numeric_limits<T>::max() - i < j;
+}
+
+template <class T>
+[[nodiscard]] std::optional<T> CheckedAdd(const T i, const T j) noexcept
+{
+    if (AdditionOverflow(i, j)) {
+        return std::nullopt;
+    }
+    return i + j;
+}
+
+#endif // BITCOIN_UTIL_OVERFLOW_H