diff --git a/src/coins.cpp b/src/coins.cpp index 058ba779f2e..cb09aa2e7aa 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -9,7 +9,7 @@ #include #include -std::optional CCoinsView::GetCoin(const COutPoint& outpoint, Coin& coin) const { return std::nullopt; } +std::optional CCoinsView::GetCoin(const COutPoint& outpoint) const { return std::nullopt; } uint256 CCoinsView::GetBestBlock() const { return uint256(); } std::vector CCoinsView::GetHeadBlocks() const { return std::vector(); } bool CCoinsView::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { return false; } @@ -17,12 +17,11 @@ std::unique_ptr CCoinsView::Cursor() const { return nullptr; } bool CCoinsView::HaveCoin(const COutPoint &outpoint) const { - Coin coin; - return GetCoin(outpoint, coin).has_value(); + return GetCoin(outpoint).has_value(); } CCoinsViewBacked::CCoinsViewBacked(CCoinsView *viewIn) : base(viewIn) { } -std::optional CCoinsViewBacked::GetCoin(const COutPoint& outpoint, Coin& coin) const { return base->GetCoin(outpoint, coin); } +std::optional CCoinsViewBacked::GetCoin(const COutPoint& outpoint) const { return base->GetCoin(outpoint); } bool CCoinsViewBacked::HaveCoin(const COutPoint &outpoint) const { return base->HaveCoin(outpoint); } uint256 CCoinsViewBacked::GetBestBlock() const { return base->GetBestBlock(); } std::vector CCoinsViewBacked::GetHeadBlocks() const { return base->GetHeadBlocks(); } @@ -45,26 +44,24 @@ size_t CCoinsViewCache::DynamicMemoryUsage() const { CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const { const auto [ret, inserted] = cacheCoins.try_emplace(outpoint); if (inserted) { - if (!base->GetCoin(outpoint, ret->second.coin)) { + if (auto coin{base->GetCoin(outpoint)}) { + ret->second.coin = std::move(*coin); + cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage(); + if (ret->second.coin.IsSpent()) { // TODO GetCoin cannot return spent coins + // The parent only has an empty entry for this outpoint; we can consider our version as fresh. + ret->second.AddFlags(CCoinsCacheEntry::FRESH, *ret, m_sentinel); + } + } else { cacheCoins.erase(ret); return cacheCoins.end(); } - if (ret->second.coin.IsSpent()) { - // The parent only has an empty entry for this outpoint; we can consider our version as fresh. - ret->second.AddFlags(CCoinsCacheEntry::FRESH, *ret, m_sentinel); - } - cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage(); } return ret; } -std::optional CCoinsViewCache::GetCoin(const COutPoint& outpoint, Coin& coin) const +std::optional CCoinsViewCache::GetCoin(const COutPoint& outpoint) const { - CCoinsMap::const_iterator it = FetchCoin(outpoint); - if (it != cacheCoins.end()) { - coin = it->second.coin; - if (!coin.IsSpent()) return coin; - } + if (auto it{FetchCoin(outpoint)}; it != cacheCoins.end() && !it->second.coin.IsSpent()) return it->second.coin; return std::nullopt; } @@ -382,9 +379,9 @@ static ReturnType ExecuteBackedWrapper(Func func, const std::vector CCoinsViewErrorCatcher::GetCoin(const COutPoint& outpoint, Coin& coin) const +std::optional CCoinsViewErrorCatcher::GetCoin(const COutPoint& outpoint) const { - return ExecuteBackedWrapper>([&]() { return CCoinsViewBacked::GetCoin(outpoint, coin); }, m_err_callbacks); + return ExecuteBackedWrapper>([&]() { return CCoinsViewBacked::GetCoin(outpoint); }, m_err_callbacks); } bool CCoinsViewErrorCatcher::HaveCoin(const COutPoint& outpoint) const diff --git a/src/coins.h b/src/coins.h index 54458105590..a2449e1b815 100644 --- a/src/coins.h +++ b/src/coins.h @@ -304,7 +304,7 @@ class CCoinsView { public: //! Retrieve the Coin (unspent transaction output) for a given outpoint. - virtual std::optional GetCoin(const COutPoint& outpoint, Coin& coin) const; + virtual std::optional GetCoin(const COutPoint& outpoint) const; //! Just check whether a given outpoint is unspent. virtual bool HaveCoin(const COutPoint &outpoint) const; @@ -341,7 +341,7 @@ protected: public: CCoinsViewBacked(CCoinsView *viewIn); - std::optional GetCoin(const COutPoint& outpoint, Coin& coin) const override; + std::optional GetCoin(const COutPoint& outpoint) const override; bool HaveCoin(const COutPoint &outpoint) const override; uint256 GetBestBlock() const override; std::vector GetHeadBlocks() const override; @@ -381,7 +381,7 @@ public: CCoinsViewCache(const CCoinsViewCache &) = delete; // Standard CCoinsView methods - std::optional GetCoin(const COutPoint& outpoint, Coin& coin) const override; + std::optional GetCoin(const COutPoint& outpoint) const override; bool HaveCoin(const COutPoint &outpoint) const override; uint256 GetBestBlock() const override; void SetBestBlock(const uint256 &hashBlock); @@ -511,7 +511,7 @@ public: m_err_callbacks.emplace_back(std::move(f)); } - std::optional GetCoin(const COutPoint& outpoint, Coin& coin) const override; + std::optional GetCoin(const COutPoint& outpoint) const override; bool HaveCoin(const COutPoint &outpoint) const override; private: diff --git a/src/node/coin.cpp b/src/node/coin.cpp index 221854c5f67..eba67f4cb50 100644 --- a/src/node/coin.cpp +++ b/src/node/coin.cpp @@ -16,10 +16,11 @@ void FindCoins(const NodeContext& node, std::map& coins) LOCK2(cs_main, node.mempool->cs); CCoinsViewCache& chain_view = node.chainman->ActiveChainstate().CoinsTip(); CCoinsViewMemPool mempool_view(&chain_view, *node.mempool); - for (auto& coin : coins) { - if (!mempool_view.GetCoin(coin.first, coin.second)) { - // Either the coin is not in the CCoinsViewCache or is spent. Clear it. - coin.second.Clear(); + for (auto& [outpoint, coin] : coins) { + if (auto c{mempool_view.GetCoin(outpoint)}) { + coin = std::move(*c); + } else { + coin.Clear(); // Either the coin is not in the CCoinsViewCache or is spent } } } diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 510541dfda9..2fe943035be 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -352,9 +352,7 @@ public: std::optional getUnspentOutput(const COutPoint& output) override { LOCK(::cs_main); - Coin coin; - if (chainman().ActiveChainstate().CoinsTip().GetCoin(output, coin)) return coin; - return {}; + return chainman().ActiveChainstate().CoinsTip().GetCoin(output); } TransactionError broadcastTransaction(CTransactionRef tx, CAmount max_tx_fee, std::string& err_string) override { diff --git a/src/rest.cpp b/src/rest.cpp index 4732922a156..89932e2e0ba 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -870,10 +870,9 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std:: { auto process_utxos = [&vOutPoints, &outs, &hits, &active_height, &active_hash, &chainman](const CCoinsView& view, const CTxMemPool* mempool) EXCLUSIVE_LOCKS_REQUIRED(chainman.GetMutex()) { for (const COutPoint& vOutPoint : vOutPoints) { - Coin coin; - bool hit = (!mempool || !mempool->isSpent(vOutPoint)) && view.GetCoin(vOutPoint, coin); - hits.push_back(hit); - if (hit) outs.emplace_back(std::move(coin)); + auto coin = !mempool || !mempool->isSpent(vOutPoint) ? view.GetCoin(vOutPoint) : std::nullopt; + hits.push_back(coin.has_value()); + if (coin) outs.emplace_back(std::move(*coin)); } active_height = chainman.ActiveHeight(); active_hash = chainman.ActiveTip()->GetBlockHash(); diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index a9cea44779d..cdcf2ca5970 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1140,35 +1140,32 @@ static RPCHelpMan gettxout() if (!request.params[2].isNull()) fMempool = request.params[2].get_bool(); - Coin coin; Chainstate& active_chainstate = chainman.ActiveChainstate(); CCoinsViewCache* coins_view = &active_chainstate.CoinsTip(); + std::optional coin; if (fMempool) { const CTxMemPool& mempool = EnsureMemPool(node); LOCK(mempool.cs); CCoinsViewMemPool view(coins_view, mempool); - if (!view.GetCoin(out, coin) || mempool.isSpent(out)) { - return UniValue::VNULL; - } + if (!mempool.isSpent(out)) coin = view.GetCoin(out); } else { - if (!coins_view->GetCoin(out, coin)) { - return UniValue::VNULL; - } + coin = coins_view->GetCoin(out); } + if (!coin) return UniValue::VNULL; const CBlockIndex* pindex = active_chainstate.m_blockman.LookupBlockIndex(coins_view->GetBestBlock()); ret.pushKV("bestblock", pindex->GetBlockHash().GetHex()); - if (coin.nHeight == MEMPOOL_HEIGHT) { + if (coin->nHeight == MEMPOOL_HEIGHT) { ret.pushKV("confirmations", 0); } else { - ret.pushKV("confirmations", (int64_t)(pindex->nHeight - coin.nHeight + 1)); + ret.pushKV("confirmations", (int64_t)(pindex->nHeight - coin->nHeight + 1)); } - ret.pushKV("value", ValueFromAmount(coin.out.nValue)); + ret.pushKV("value", ValueFromAmount(coin->out.nValue)); UniValue o(UniValue::VOBJ); - ScriptToUniv(coin.out.scriptPubKey, /*out=*/o, /*include_hex=*/true, /*include_address=*/true); + ScriptToUniv(coin->out.scriptPubKey, /*out=*/o, /*include_hex=*/true, /*include_address=*/true); ret.pushKV("scriptPubKey", std::move(o)); - ret.pushKV("coinbase", (bool)coin.fCoinBase); + ret.pushKV("coinbase", (bool)coin->fCoinBase); return ret; }, diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 7c08cb9ac2d..ec4720966b0 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -44,18 +44,14 @@ class CCoinsViewTest : public CCoinsView public: CCoinsViewTest(FastRandomContext& rng) : m_rng{rng} {} - std::optional GetCoin(const COutPoint& outpoint, Coin& coin) const override + std::optional GetCoin(const COutPoint& outpoint) const override { - std::map::const_iterator it = map_.find(outpoint); - if (it == map_.end()) { - return std::nullopt; + if (auto it{map_.find(outpoint)}; it != map_.end()) { + if (!it->second.IsSpent() || m_rng.randbool()) { + return it->second; // TODO spent coins shouldn't be returned + } } - coin = it->second; - if (coin.IsSpent() && m_rng.randbool() == 0) { - // Randomly return std::nullopt in case of an empty entry. - return std::nullopt; - } - return coin; + return std::nullopt; } uint256 GetBestBlock() const override { return hashBestBlock_; } diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index ea40d717361..54bfb93b491 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -162,22 +162,20 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) const bool exists_using_access_coin = !(coin_using_access_coin == EMPTY_COIN); const bool exists_using_have_coin = coins_view_cache.HaveCoin(random_out_point); const bool exists_using_have_coin_in_cache = coins_view_cache.HaveCoinInCache(random_out_point); - Coin coin_using_get_coin; - const bool exists_using_get_coin = coins_view_cache.GetCoin(random_out_point, coin_using_get_coin).has_value(); - if (exists_using_get_coin) { - assert(coin_using_get_coin == coin_using_access_coin); + if (auto coin{coins_view_cache.GetCoin(random_out_point)}) { + assert(*coin == coin_using_access_coin); + assert(exists_using_access_coin && exists_using_have_coin_in_cache && exists_using_have_coin); + } else { + assert(!exists_using_access_coin && !exists_using_have_coin_in_cache && !exists_using_have_coin); } - assert((exists_using_access_coin && exists_using_have_coin_in_cache && exists_using_have_coin && exists_using_get_coin) || - (!exists_using_access_coin && !exists_using_have_coin_in_cache && !exists_using_have_coin && !exists_using_get_coin)); // If HaveCoin on the backend is true, it must also be on the cache if the coin wasn't spent. const bool exists_using_have_coin_in_backend = backend_coins_view.HaveCoin(random_out_point); if (!coin_using_access_coin.IsSpent() && exists_using_have_coin_in_backend) { assert(exists_using_have_coin); } - Coin coin_using_backend_get_coin; - if (backend_coins_view.GetCoin(random_out_point, coin_using_backend_get_coin)) { + if (auto coin{backend_coins_view.GetCoin(random_out_point)}) { assert(exists_using_have_coin_in_backend); - // Note we can't assert that `coin_using_get_coin == coin_using_backend_get_coin` because the coin in + // Note we can't assert that `coin_using_get_coin == *coin` because the coin in // the cache may have been modified but not yet flushed. } else { assert(!exists_using_have_coin_in_backend); diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp index a43cba65601..6000d52fc97 100644 --- a/src/test/fuzz/coinscache_sim.cpp +++ b/src/test/fuzz/coinscache_sim.cpp @@ -146,15 +146,11 @@ class CoinsViewBottom final : public CCoinsView std::map m_data; public: - std::optional GetCoin(const COutPoint& outpoint, Coin& coin) const final + std::optional GetCoin(const COutPoint& outpoint) const final { - auto it = m_data.find(outpoint); - if (it == m_data.end()) { - return std::nullopt; - } else { - coin = it->second; - return coin; // TODO GetCoin shouldn't return spent coins - } + // TODO GetCoin shouldn't return spent coins + if (auto it = m_data.find(outpoint); it != m_data.end()) return it->second; + return std::nullopt; } bool HaveCoin(const COutPoint& outpoint) const final @@ -265,17 +261,16 @@ FUZZ_TARGET(coinscache_sim) // Look up in simulation data. auto sim = lookup(outpointidx); // Look up in real caches. - Coin realcoin; - auto real = caches.back()->GetCoin(data.outpoints[outpointidx], realcoin); + auto realcoin = caches.back()->GetCoin(data.outpoints[outpointidx]); // Compare results. if (!sim.has_value()) { - assert(!real || realcoin.IsSpent()); + assert(!realcoin || realcoin->IsSpent()); } else { - assert(real && !realcoin.IsSpent()); + assert(realcoin && !realcoin->IsSpent()); const auto& simcoin = data.coins[sim->first]; - assert(realcoin.out == simcoin.out); - assert(realcoin.fCoinBase == simcoin.fCoinBase); - assert(realcoin.nHeight == sim->second); + assert(realcoin->out == simcoin.out); + assert(realcoin->fCoinBase == simcoin.fCoinBase); + assert(realcoin->nHeight == sim->second); } }, @@ -460,16 +455,15 @@ FUZZ_TARGET(coinscache_sim) // Compare the bottom coinsview (not a CCoinsViewCache) with sim_cache[0]. for (uint32_t outpointidx = 0; outpointidx < NUM_OUTPOINTS; ++outpointidx) { - Coin realcoin; - auto real = bottom.GetCoin(data.outpoints[outpointidx], realcoin); + auto realcoin = bottom.GetCoin(data.outpoints[outpointidx]); auto sim = lookup(outpointidx, 0); if (!sim.has_value()) { - assert(!real || realcoin.IsSpent()); + assert(!realcoin || realcoin->IsSpent()); } else { - assert(real && !realcoin.IsSpent()); - assert(realcoin.out == data.coins[sim->first].out); - assert(realcoin.fCoinBase == data.coins[sim->first].fCoinBase); - assert(realcoin.nHeight == sim->second); + assert(realcoin && !realcoin->IsSpent()); + assert(realcoin->out == data.coins[sim->first].out); + assert(realcoin->fCoinBase == data.coins[sim->first].fCoinBase); + assert(realcoin->nHeight == sim->second); } } } diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index 64861311dbd..39aa404484e 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -214,9 +214,8 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool) // Helper to query an amount const CCoinsViewMemPool amount_view{WITH_LOCK(::cs_main, return &chainstate.CoinsTip()), tx_pool}; const auto GetAmount = [&](const COutPoint& outpoint) { - Coin c; - Assert(amount_view.GetCoin(outpoint, c)); - return c.out.nValue; + auto coin{amount_view.GetCoin(outpoint).value()}; + return coin.out.nValue; }; LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 300) diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index fa89ceb332a..2246d3010ad 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -432,9 +432,8 @@ std::pair TestChain100Setup::CreateValidTransactio std::map input_coins; CAmount inputs_amount{0}; for (const auto& outpoint_to_spend : inputs) { - // - Use GetCoin to properly populate utxo_to_spend, - Coin utxo_to_spend; - assert(coins_cache.GetCoin(outpoint_to_spend, utxo_to_spend)); + // Use GetCoin to properly populate utxo_to_spend + auto utxo_to_spend{coins_cache.GetCoin(outpoint_to_spend).value()}; input_coins.insert({outpoint_to_spend, utxo_to_spend}); inputs_amount += utxo_to_spend.out.nValue; } diff --git a/src/txdb.cpp b/src/txdb.cpp index c211479b3bc..1622039d63b 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -65,10 +65,10 @@ void CCoinsViewDB::ResizeCache(size_t new_cache_size) } } -std::optional CCoinsViewDB::GetCoin(const COutPoint& outpoint, Coin& coin) const +std::optional CCoinsViewDB::GetCoin(const COutPoint& outpoint) const { - if (m_db->Read(CoinEntry(&outpoint), coin)) return coin; - else return std::nullopt; + if (Coin coin; m_db->Read(CoinEntry(&outpoint), coin)) return coin; + return std::nullopt; } bool CCoinsViewDB::HaveCoin(const COutPoint &outpoint) const { diff --git a/src/txdb.h b/src/txdb.h index e95c04c4955..565b060dbea 100644 --- a/src/txdb.h +++ b/src/txdb.h @@ -57,7 +57,7 @@ protected: public: explicit CCoinsViewDB(DBParams db_params, CoinsViewOptions options); - std::optional GetCoin(const COutPoint& outpoint, Coin& coin) const override; + std::optional GetCoin(const COutPoint& outpoint) const override; bool HaveCoin(const COutPoint &outpoint) const override; uint256 GetBestBlock() const override; std::vector GetHeadBlocks() const override; diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 80c33f2ba1f..4756c202096 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -988,13 +988,12 @@ bool CTxMemPool::HasNoInputsOf(const CTransaction &tx) const CCoinsViewMemPool::CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn) : CCoinsViewBacked(baseIn), mempool(mempoolIn) { } -std::optional CCoinsViewMemPool::GetCoin(const COutPoint& outpoint, Coin& coin) const +std::optional CCoinsViewMemPool::GetCoin(const COutPoint& outpoint) const { // Check to see if the inputs are made available by another tx in the package. // These Coins would not be available in the underlying CoinsView. if (auto it = m_temp_added.find(outpoint); it != m_temp_added.end()) { - coin = it->second; - return coin; + return it->second; } // If an entry in the mempool exists, always return that one, as it's guaranteed to never @@ -1003,14 +1002,13 @@ std::optional CCoinsViewMemPool::GetCoin(const COutPoint& outpoint, Coin& CTransactionRef ptx = mempool.get(outpoint.hash); if (ptx) { if (outpoint.n < ptx->vout.size()) { - coin = Coin(ptx->vout[outpoint.n], MEMPOOL_HEIGHT, false); + Coin coin(ptx->vout[outpoint.n], MEMPOOL_HEIGHT, false); m_non_base_coins.emplace(outpoint); return coin; - } else { - return std::nullopt; } + return std::nullopt; } - return base->GetCoin(outpoint, coin); + return base->GetCoin(outpoint); } void CCoinsViewMemPool::PackageAddTransaction(const CTransactionRef& tx) diff --git a/src/txmempool.h b/src/txmempool.h index b9b0ba0f846..f914cbd729e 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -851,7 +851,7 @@ public: CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn); /** GetCoin, returning whether it exists and is not spent. Also updates m_non_base_coins if the * coin is not fetched from base. */ - std::optional GetCoin(const COutPoint& outpoint, Coin& coin) const override; + std::optional GetCoin(const COutPoint& outpoint) const override; /** Add the coins created by this transaction. These coins are only temporarily stored in * m_temp_added and cannot be flushed to the back end. Only used for package validation. */ void PackageAddTransaction(const CTransactionRef& tx); diff --git a/src/validation.cpp b/src/validation.cpp index 38fcda55af6..5c33d3f8b63 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -180,18 +180,14 @@ std::optional> CalculatePrevHeights( std::vector prev_heights; prev_heights.resize(tx.vin.size()); for (size_t i = 0; i < tx.vin.size(); ++i) { - const CTxIn& txin = tx.vin[i]; - Coin coin; - if (!coins.GetCoin(txin.prevout, coin)) { + if (auto coin{coins.GetCoin(tx.vin[i].prevout)}) { + prev_heights[i] = coin->nHeight == MEMPOOL_HEIGHT + ? tip.nHeight + 1 // Assume all mempool transaction confirm in the next block. + : coin->nHeight; + } else { LogPrintf("ERROR: %s: Missing input %d in transaction \'%s\'\n", __func__, i, tx.GetHash().GetHex()); return std::nullopt; } - if (coin.nHeight == MEMPOOL_HEIGHT) { - // Assume all mempool transaction confirm in the next block. - prev_heights[i] = tip.nHeight + 1; - } else { - prev_heights[i] = coin.nHeight; - } } return prev_heights; }