Merge #18859: Remove CCoinsViewCache::GetValueIn(...)

b56607a Remove CCoinsViewCache::GetValueIn(...) (practicalswift)

Pull request description:

  Remove `CCoinsViewCache::GetValueIn(...)`.

  Fixes #18858.

  It seems like `GetValueIn` was added in bitcoin-core#748 ("Pay-to-script-hash (OP_EVAL replacement)", merged in 2012) and the last use in validation code was removed in #8498 ("Near-Bugfix: Optimization: Minimize the number of times it is checked that no money...", merged in 2017).

  `CCoinsViewCache::GetValueIn(…)` performs money summation like this:

  ```c++
  CAmount CCoinsViewCache::GetValueIn(const CTransaction& tx) const
  {
      if (tx.IsCoinBase())
          return 0;

      CAmount nResult = 0;
      for (unsigned int i = 0; i < tx.vin.size(); i++)
          nResult += AccessCoin(tx.vin[i].prevout).out.nValue;

      return nResult;
  }
  ```

  Note that no check is done to make sure that the resulting `nResult` is such that it stays within the money bounds (`MoneyRange(nResult)`), or that the summation does not trigger a signed integer overflow.

  Proof of concept output:

  ```
  coins.cpp:243:17: runtime error: signed integer overflow: 9223200000000000000 + 2100000000000000 cannot be represented in type 'long'
  GetValueIn = -9221444073709551616
  ```

  Proof of concept code:

  ```c++
  CMutableTransaction mutable_transaction;
  mutable_transaction.vin.resize(4393);

  Coin coin;
  coin.out.nValue = MAX_MONEY;
  assert(MoneyRange(coin.out.nValue));

  CCoinsCacheEntry coins_cache_entry;
  coins_cache_entry.coin = coin;
  coins_cache_entry.flags = CCoinsCacheEntry::DIRTY;

  CCoinsView backend_coins_view;
  CCoinsViewCache coins_view_cache{&backend_coins_view};
  CCoinsMap coins_map;
  coins_map.emplace(COutPoint{}, std::move(coins_cache_entry));
  coins_view_cache.BatchWrite(coins_map, {});

  const CAmount total_value_in = coins_view_cache.GetValueIn(CTransaction{mutable_transaction});
  std::cout << "GetValueIn = " << total_value_in << std::endl;
  ```

ACKs for top commit:
  MarcoFalke:
    ACK b56607a
  promag:
    Code review ACK b56607a.
  jb55:
    ACK b56607a
  hebasto:
    ACK b56607a, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.

Tree-SHA512: 2c8402b5753ec96703d12c57c3eda8eccf999ed3519134a87faaf0838cfe44b94ef384296af2a524c06c8756c0245418d181af9083548e360905fac9d79215e6

Author: MarcoFalke

src/bench/ccoins_caching.cpp

@@ -47,8 +47,6 @@ static void CCoinsCaching(benchmark::State& state)
     while (state.KeepRunning()) {
         bool success = AreInputsStandard(tx_1, coins);
         assert(success);
-        CAmount value = coins.GetValueIn(tx_1);
-        assert(value == (50 + 21 + 22) * COIN);
     }
     ECC_Stop();
 }

src/coins.cpp

@@ -233,18 +233,6 @@ unsigned int CCoinsViewCache::GetCacheSize() const {
     return cacheCoins.size();
 }
 
-CAmount CCoinsViewCache::GetValueIn(const CTransaction& tx) const
-{
-    if (tx.IsCoinBase())
-        return 0;
-
-    CAmount nResult = 0;
-    for (unsigned int i = 0; i < tx.vin.size(); i++)
-        nResult += AccessCoin(tx.vin[i].prevout).out.nValue;
-
-    return nResult;
-}
-
 bool CCoinsViewCache::HaveInputs(const CTransaction& tx) const
 {
     if (!tx.IsCoinBase()) {

src/coins.h

@@ -315,16 +315,6 @@ class CCoinsViewCache : public CCoinsViewBacked
     //! Calculate the size of the cache (in bytes)
     size_t DynamicMemoryUsage() const;
 
-    /**
-     * Amount of bitcoins coming in to a transaction
-     * Note that lightweight clients may not know anything besides the hash of previous transactions,
-     * so may not be able to calculate this.
-     *
-     * @param[in] tx    transaction for which we are checking input total
-     * @return  Sum of value of all inputs (scriptSigs)
-     */
-    CAmount GetValueIn(const CTransaction& tx) const;
-
     //! Check whether all prevouts of the transaction are present in the UTXO set represented by this view
     bool HaveInputs(const CTransaction& tx) const;
 

src/primitives/transaction.h

@@ -324,8 +324,6 @@ class CTransaction
 
     // Return sum of txouts.
     CAmount GetValueOut() const;
-    // GetValueIn() is a method on CCoinsViewCache, because
-    // inputs must be known to compute value in.
 
     /**
      * Get the total transaction size in bytes, including witness data.

src/test/transaction_tests.cpp

@@ -305,7 +305,6 @@ BOOST_AUTO_TEST_CASE(test_Get)
     t1.vout[0].scriptPubKey << OP_1;
 
     BOOST_CHECK(AreInputsStandard(CTransaction(t1), coins));
-    BOOST_CHECK_EQUAL(coins.GetValueIn(CTransaction(t1)), (50+21+22)*CENT);
 }
 
 static void CreateCreditAndSpend(const FillableSigningProvider& keystore, const CScript& outscript, CTransactionRef& output, CMutableTransaction& input, bool success = true)