From ec5a06e5d1d3fbd537ad2e1cb34a33a59bb548b2 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 4 May 2020 07:48:11 -0400 Subject: [PATCH] Merge #18859: Remove CCoinsViewCache::GetValueIn(...) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit b56607a89ba112083f2b0a7b64ab18d66b26e2be Remove CCoinsViewCache::GetValueIn(...) (practicalswift) Pull request description: Remove `CCoinsViewCache::GetValueIn(...)`. Fixes #18858. It seems like `GetValueIn` was added in #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 b56607a89ba112083f2b0a7b64ab18d66b26e2be promag: Code review ACK b56607a89ba112083f2b0a7b64ab18d66b26e2be. jb55: ACK b56607a89ba112083f2b0a7b64ab18d66b26e2be hebasto: ACK b56607a89ba112083f2b0a7b64ab18d66b26e2be, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged. Tree-SHA512: 2c8402b5753ec96703d12c57c3eda8eccf999ed3519134a87faaf0838cfe44b94ef384296af2a524c06c8756c0245418d181af9083548e360905fac9d79215e6 --- src/bench/ccoins_caching.cpp | 2 -- src/coins.cpp | 12 ------------ src/coins.h | 10 ---------- src/primitives/transaction.h | 2 -- src/test/transaction_tests.cpp | 1 - 5 files changed, 27 deletions(-) diff --git a/src/bench/ccoins_caching.cpp b/src/bench/ccoins_caching.cpp index e2984bf03e2543..1e914c6a19cbe0 100644 --- a/src/bench/ccoins_caching.cpp +++ b/src/bench/ccoins_caching.cpp @@ -83,8 +83,6 @@ static void CCoinsCaching(benchmark::Bench& bench) bench.run([&] { bool success = AreInputsStandard(tx_1, coins); assert(success); - CAmount value = coins.GetValueIn(tx_1); - assert(value == (50 + 21 + 22) * COIN); }); ECC_Stop(); } diff --git a/src/coins.cpp b/src/coins.cpp index 103967a02f3096..46df86ca993bae 100644 --- a/src/coins.cpp +++ b/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()) { diff --git a/src/coins.h b/src/coins.h index 14d6e9177b3f24..74aa452d63d281 100644 --- a/src/coins.h +++ b/src/coins.h @@ -318,16 +318,6 @@ class CCoinsViewCache : public CCoinsViewBacked //! Calculate the size of the cache (in bytes) size_t DynamicMemoryUsage() const; - /** - * Amount of dash 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; diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index 724cd99cf3391c..3e0fe28cd4af5a 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -239,8 +239,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. diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index f0e89e8b78df49..586475a6787825 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -313,7 +313,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); } BOOST_AUTO_TEST_CASE(test_IsStandard)