Skip to content

Commit 4feaa28

Browse files
l0rincandrewtoth
l0rinc
and
andrewtoth
committed
refactor: Rely on returned value of GetCoin instead of parameter
Also removed the unused coin parameter of GetCoin. Co-authored-by: Andrew Toth <[email protected]>
1 parent 46dfbf1 commit 4feaa28

16 files changed

+85
-113
lines changed

‎src/coins.cpp

Lines changed: 15 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -9,20 +9,19 @@
99
#include <random.h>
1010
#include <util/trace.h>
1111

12-
std::optional<Coin> CCoinsView::GetCoin(const COutPoint& outpoint, Coin& coin) const { return std::nullopt; }
12+
std::optional<Coin> CCoinsView::GetCoin(const COutPoint& outpoint) const { return std::nullopt; }
1313
uint256 CCoinsView::GetBestBlock() const { return uint256(); }
1414
std::vector<uint256> CCoinsView::GetHeadBlocks() const { return std::vector<uint256>(); }
1515
bool CCoinsView::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { return false; }
1616
std::unique_ptr<CCoinsViewCursor> CCoinsView::Cursor() const { return nullptr; }
1717

1818
bool CCoinsView::HaveCoin(const COutPoint &outpoint) const
1919
{
20-
Coin coin;
21-
return GetCoin(outpoint, coin).has_value();
20+
return GetCoin(outpoint).has_value();
2221
}
2322

2423
CCoinsViewBacked::CCoinsViewBacked(CCoinsView *viewIn) : base(viewIn) { }
25-
std::optional<Coin> CCoinsViewBacked::GetCoin(const COutPoint& outpoint, Coin& coin) const { return base->GetCoin(outpoint, coin); }
24+
std::optional<Coin> CCoinsViewBacked::GetCoin(const COutPoint& outpoint) const { return base->GetCoin(outpoint); }
2625
bool CCoinsViewBacked::HaveCoin(const COutPoint &outpoint) const { return base->HaveCoin(outpoint); }
2726
uint256 CCoinsViewBacked::GetBestBlock() const { return base->GetBestBlock(); }
2827
std::vector<uint256> CCoinsViewBacked::GetHeadBlocks() const { return base->GetHeadBlocks(); }
@@ -45,26 +44,24 @@ size_t CCoinsViewCache::DynamicMemoryUsage() const {
4544
CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const {
4645
const auto [ret, inserted] = cacheCoins.try_emplace(outpoint);
4746
if (inserted) {
48-
if (!base->GetCoin(outpoint, ret->second.coin)) {
47+
if (auto coin{base->GetCoin(outpoint)}) {
48+
ret->second.coin = std::move(*coin);
49+
cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
50+
if (ret->second.coin.IsSpent()) { // TODO GetCoin cannot return spent coins
51+
// The parent only has an empty entry for this outpoint; we can consider our version as fresh.
52+
ret->second.AddFlags(CCoinsCacheEntry::FRESH, *ret, m_sentinel);
53+
}
54+
} else {
4955
cacheCoins.erase(ret);
5056
return cacheCoins.end();
5157
}
52-
if (ret->second.coin.IsSpent()) {
53-
// The parent only has an empty entry for this outpoint; we can consider our version as fresh.
54-
ret->second.AddFlags(CCoinsCacheEntry::FRESH, *ret, m_sentinel);
55-
}
56-
cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
5758
}
5859
return ret;
5960
}
6061

61-
std::optional<Coin> CCoinsViewCache::GetCoin(const COutPoint& outpoint, Coin& coin) const
62+
std::optional<Coin> CCoinsViewCache::GetCoin(const COutPoint& outpoint) const
6263
{
63-
CCoinsMap::const_iterator it = FetchCoin(outpoint);
64-
if (it != cacheCoins.end()) {
65-
coin = it->second.coin;
66-
if (!coin.IsSpent()) return coin;
67-
}
64+
if (auto it{FetchCoin(outpoint)}; it != cacheCoins.end() && !it->second.coin.IsSpent()) return it->second.coin;
6865
return std::nullopt;
6966
}
7067

@@ -382,9 +379,9 @@ static ReturnType ExecuteBackedWrapper(Func func, const std::vector<std::functio
382379
}
383380
}
384381

385-
std::optional<Coin> CCoinsViewErrorCatcher::GetCoin(const COutPoint& outpoint, Coin& coin) const
382+
std::optional<Coin> CCoinsViewErrorCatcher::GetCoin(const COutPoint& outpoint) const
386383
{
387-
return ExecuteBackedWrapper<std::optional<Coin>>([&]() { return CCoinsViewBacked::GetCoin(outpoint, coin); }, m_err_callbacks);
384+
return ExecuteBackedWrapper<std::optional<Coin>>([&]() { return CCoinsViewBacked::GetCoin(outpoint); }, m_err_callbacks);
388385
}
389386

390387
bool CCoinsViewErrorCatcher::HaveCoin(const COutPoint& outpoint) const

‎src/coins.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,7 @@ class CCoinsView
304304
{
305305
public:
306306
//! Retrieve the Coin (unspent transaction output) for a given outpoint.
307-
virtual std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const;
307+
virtual std::optional<Coin> GetCoin(const COutPoint& outpoint) const;
308308

309309
//! Just check whether a given outpoint is unspent.
310310
virtual bool HaveCoin(const COutPoint &outpoint) const;
@@ -341,7 +341,7 @@ class CCoinsViewBacked : public CCoinsView
341341

342342
public:
343343
CCoinsViewBacked(CCoinsView *viewIn);
344-
std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const override;
344+
std::optional<Coin> GetCoin(const COutPoint& outpoint) const override;
345345
bool HaveCoin(const COutPoint &outpoint) const override;
346346
uint256 GetBestBlock() const override;
347347
std::vector<uint256> GetHeadBlocks() const override;
@@ -381,7 +381,7 @@ class CCoinsViewCache : public CCoinsViewBacked
381381
CCoinsViewCache(const CCoinsViewCache &) = delete;
382382

383383
// Standard CCoinsView methods
384-
std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const override;
384+
std::optional<Coin> GetCoin(const COutPoint& outpoint) const override;
385385
bool HaveCoin(const COutPoint &outpoint) const override;
386386
uint256 GetBestBlock() const override;
387387
void SetBestBlock(const uint256 &hashBlock);
@@ -511,7 +511,7 @@ class CCoinsViewErrorCatcher final : public CCoinsViewBacked
511511
m_err_callbacks.emplace_back(std::move(f));
512512
}
513513

514-
std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const override;
514+
std::optional<Coin> GetCoin(const COutPoint& outpoint) const override;
515515
bool HaveCoin(const COutPoint &outpoint) const override;
516516

517517
private:

‎src/node/coin.cpp

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,11 @@ void FindCoins(const NodeContext& node, std::map<COutPoint, Coin>& coins)
1616
LOCK2(cs_main, node.mempool->cs);
1717
CCoinsViewCache& chain_view = node.chainman->ActiveChainstate().CoinsTip();
1818
CCoinsViewMemPool mempool_view(&chain_view, *node.mempool);
19-
for (auto& coin : coins) {
20-
if (!mempool_view.GetCoin(coin.first, coin.second)) {
21-
// Either the coin is not in the CCoinsViewCache or is spent. Clear it.
22-
coin.second.Clear();
19+
for (auto& [outpoint, coin] : coins) {
20+
if (auto c{mempool_view.GetCoin(outpoint)}) {
21+
coin = std::move(*c);
22+
} else {
23+
coin.Clear(); // Either the coin is not in the CCoinsViewCache or is spent
2324
}
2425
}
2526
}

‎src/node/interfaces.cpp

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -352,9 +352,7 @@ class NodeImpl : public Node
352352
std::optional<Coin> getUnspentOutput(const COutPoint& output) override
353353
{
354354
LOCK(::cs_main);
355-
Coin coin;
356-
if (chainman().ActiveChainstate().CoinsTip().GetCoin(output, coin)) return coin;
357-
return {};
355+
return chainman().ActiveChainstate().CoinsTip().GetCoin(output);
358356
}
359357
TransactionError broadcastTransaction(CTransactionRef tx, CAmount max_tx_fee, std::string& err_string) override
360358
{

‎src/rest.cpp

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -870,10 +870,9 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
870870
{
871871
auto process_utxos = [&vOutPoints, &outs, &hits, &active_height, &active_hash, &chainman](const CCoinsView& view, const CTxMemPool* mempool) EXCLUSIVE_LOCKS_REQUIRED(chainman.GetMutex()) {
872872
for (const COutPoint& vOutPoint : vOutPoints) {
873-
Coin coin;
874-
bool hit = (!mempool || !mempool->isSpent(vOutPoint)) && view.GetCoin(vOutPoint, coin);
875-
hits.push_back(hit);
876-
if (hit) outs.emplace_back(std::move(coin));
873+
auto coin = !mempool || !mempool->isSpent(vOutPoint) ? view.GetCoin(vOutPoint) : std::nullopt;
874+
hits.push_back(coin.has_value());
875+
if (coin) outs.emplace_back(std::move(*coin));
877876
}
878877
active_height = chainman.ActiveHeight();
879878
active_hash = chainman.ActiveTip()->GetBlockHash();

‎src/rpc/blockchain.cpp

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1140,35 +1140,32 @@ static RPCHelpMan gettxout()
11401140
if (!request.params[2].isNull())
11411141
fMempool = request.params[2].get_bool();
11421142

1143-
Coin coin;
11441143
Chainstate& active_chainstate = chainman.ActiveChainstate();
11451144
CCoinsViewCache* coins_view = &active_chainstate.CoinsTip();
11461145

1146+
std::optional<Coin> coin;
11471147
if (fMempool) {
11481148
const CTxMemPool& mempool = EnsureMemPool(node);
11491149
LOCK(mempool.cs);
11501150
CCoinsViewMemPool view(coins_view, mempool);
1151-
if (!view.GetCoin(out, coin) || mempool.isSpent(out)) {
1152-
return UniValue::VNULL;
1153-
}
1151+
if (!mempool.isSpent(out)) coin = view.GetCoin(out);
11541152
} else {
1155-
if (!coins_view->GetCoin(out, coin)) {
1156-
return UniValue::VNULL;
1157-
}
1153+
coin = coins_view->GetCoin(out);
11581154
}
1155+
if (!coin) return UniValue::VNULL;
11591156

11601157
const CBlockIndex* pindex = active_chainstate.m_blockman.LookupBlockIndex(coins_view->GetBestBlock());
11611158
ret.pushKV("bestblock", pindex->GetBlockHash().GetHex());
1162-
if (coin.nHeight == MEMPOOL_HEIGHT) {
1159+
if (coin->nHeight == MEMPOOL_HEIGHT) {
11631160
ret.pushKV("confirmations", 0);
11641161
} else {
1165-
ret.pushKV("confirmations", (int64_t)(pindex->nHeight - coin.nHeight + 1));
1162+
ret.pushKV("confirmations", (int64_t)(pindex->nHeight - coin->nHeight + 1));
11661163
}
1167-
ret.pushKV("value", ValueFromAmount(coin.out.nValue));
1164+
ret.pushKV("value", ValueFromAmount(coin->out.nValue));
11681165
UniValue o(UniValue::VOBJ);
1169-
ScriptToUniv(coin.out.scriptPubKey, /*out=*/o, /*include_hex=*/true, /*include_address=*/true);
1166+
ScriptToUniv(coin->out.scriptPubKey, /*out=*/o, /*include_hex=*/true, /*include_address=*/true);
11701167
ret.pushKV("scriptPubKey", std::move(o));
1171-
ret.pushKV("coinbase", (bool)coin.fCoinBase);
1168+
ret.pushKV("coinbase", (bool)coin->fCoinBase);
11721169

11731170
return ret;
11741171
},

‎src/test/coins_tests.cpp

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -44,18 +44,14 @@ class CCoinsViewTest : public CCoinsView
4444
public:
4545
CCoinsViewTest(FastRandomContext& rng) : m_rng{rng} {}
4646

47-
std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const override
47+
std::optional<Coin> GetCoin(const COutPoint& outpoint) const override
4848
{
49-
std::map<COutPoint, Coin>::const_iterator it = map_.find(outpoint);
50-
if (it == map_.end()) {
51-
return std::nullopt;
52-
}
53-
coin = it->second;
54-
if (coin.IsSpent() && m_rng.randbool() == 0) {
55-
// Randomly return std::nullopt in case of an empty entry.
56-
return std::nullopt;
49+
if (auto it{map_.find(outpoint)}; it != map_.end()) {
50+
if (!it->second.IsSpent() || m_rng.randbool()) {
51+
return it->second; // TODO spent coins shouldn't be returned
52+
}
5753
}
58-
return coin;
54+
return std::nullopt;
5955
}
6056

6157
uint256 GetBestBlock() const override { return hashBestBlock_; }

‎src/test/fuzz/coins_view.cpp

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -162,22 +162,20 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view)
162162
const bool exists_using_access_coin = !(coin_using_access_coin == EMPTY_COIN);
163163
const bool exists_using_have_coin = coins_view_cache.HaveCoin(random_out_point);
164164
const bool exists_using_have_coin_in_cache = coins_view_cache.HaveCoinInCache(random_out_point);
165-
Coin coin_using_get_coin;
166-
const bool exists_using_get_coin = coins_view_cache.GetCoin(random_out_point, coin_using_get_coin).has_value();
167-
if (exists_using_get_coin) {
168-
assert(coin_using_get_coin == coin_using_access_coin);
165+
if (auto coin{coins_view_cache.GetCoin(random_out_point)}) {
166+
assert(*coin == coin_using_access_coin);
167+
assert(exists_using_access_coin && exists_using_have_coin_in_cache && exists_using_have_coin);
168+
} else {
169+
assert(!exists_using_access_coin && !exists_using_have_coin_in_cache && !exists_using_have_coin);
169170
}
170-
assert((exists_using_access_coin && exists_using_have_coin_in_cache && exists_using_have_coin && exists_using_get_coin) ||
171-
(!exists_using_access_coin && !exists_using_have_coin_in_cache && !exists_using_have_coin && !exists_using_get_coin));
172171
// If HaveCoin on the backend is true, it must also be on the cache if the coin wasn't spent.
173172
const bool exists_using_have_coin_in_backend = backend_coins_view.HaveCoin(random_out_point);
174173
if (!coin_using_access_coin.IsSpent() && exists_using_have_coin_in_backend) {
175174
assert(exists_using_have_coin);
176175
}
177-
Coin coin_using_backend_get_coin;
178-
if (backend_coins_view.GetCoin(random_out_point, coin_using_backend_get_coin)) {
176+
if (auto coin{backend_coins_view.GetCoin(random_out_point)}) {
179177
assert(exists_using_have_coin_in_backend);
180-
// Note we can't assert that `coin_using_get_coin == coin_using_backend_get_coin` because the coin in
178+
// Note we can't assert that `coin_using_get_coin == *coin` because the coin in
181179
// the cache may have been modified but not yet flushed.
182180
} else {
183181
assert(!exists_using_have_coin_in_backend);

‎src/test/fuzz/coinscache_sim.cpp

Lines changed: 16 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -146,15 +146,11 @@ class CoinsViewBottom final : public CCoinsView
146146
std::map<COutPoint, Coin> m_data;
147147

148148
public:
149-
std::optional<Coin> GetCoin(const COutPoint& outpoint, Coin& coin) const final
149+
std::optional<Coin> GetCoin(const COutPoint& outpoint) const final
150150
{
151-
auto it = m_data.find(outpoint);
152-
if (it == m_data.end()) {
153-
return std::nullopt;
154-
} else {
155-
coin = it->second;
156-
return coin; // TODO GetCoin shouldn't return spent coins
157-
}
151+
// TODO GetCoin shouldn't return spent coins
152+
if (auto it = m_data.find(outpoint); it != m_data.end()) return it->second;
153+
return std::nullopt;
158154
}
159155

160156
bool HaveCoin(const COutPoint& outpoint) const final
@@ -265,17 +261,16 @@ FUZZ_TARGET(coinscache_sim)
265261
// Look up in simulation data.
266262
auto sim = lookup(outpointidx);
267263
// Look up in real caches.
268-
Coin realcoin;
269-
auto real = caches.back()->GetCoin(data.outpoints[outpointidx], realcoin);
264+
auto realcoin = caches.back()->GetCoin(data.outpoints[outpointidx]);
270265
// Compare results.
271266
if (!sim.has_value()) {
272-
assert(!real || realcoin.IsSpent());
267+
assert(!realcoin || realcoin->IsSpent());
273268
} else {
274-
assert(real && !realcoin.IsSpent());
269+
assert(realcoin && !realcoin->IsSpent());
275270
const auto& simcoin = data.coins[sim->first];
276-
assert(realcoin.out == simcoin.out);
277-
assert(realcoin.fCoinBase == simcoin.fCoinBase);
278-
assert(realcoin.nHeight == sim->second);
271+
assert(realcoin->out == simcoin.out);
272+
assert(realcoin->fCoinBase == simcoin.fCoinBase);
273+
assert(realcoin->nHeight == sim->second);
279274
}
280275
},
281276

@@ -460,16 +455,15 @@ FUZZ_TARGET(coinscache_sim)
460455

461456
// Compare the bottom coinsview (not a CCoinsViewCache) with sim_cache[0].
462457
for (uint32_t outpointidx = 0; outpointidx < NUM_OUTPOINTS; ++outpointidx) {
463-
Coin realcoin;
464-
auto real = bottom.GetCoin(data.outpoints[outpointidx], realcoin);
458+
auto realcoin = bottom.GetCoin(data.outpoints[outpointidx]);
465459
auto sim = lookup(outpointidx, 0);
466460
if (!sim.has_value()) {
467-
assert(!real || realcoin.IsSpent());
461+
assert(!realcoin || realcoin->IsSpent());
468462
} else {
469-
assert(real && !realcoin.IsSpent());
470-
assert(realcoin.out == data.coins[sim->first].out);
471-
assert(realcoin.fCoinBase == data.coins[sim->first].fCoinBase);
472-
assert(realcoin.nHeight == sim->second);
463+
assert(realcoin && !realcoin->IsSpent());
464+
assert(realcoin->out == data.coins[sim->first].out);
465+
assert(realcoin->fCoinBase == data.coins[sim->first].fCoinBase);
466+
assert(realcoin->nHeight == sim->second);
473467
}
474468
}
475469
}

‎src/test/fuzz/tx_pool.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -214,9 +214,8 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
214214
// Helper to query an amount
215215
const CCoinsViewMemPool amount_view{WITH_LOCK(::cs_main, return &chainstate.CoinsTip()), tx_pool};
216216
const auto GetAmount = [&](const COutPoint& outpoint) {
217-
Coin c;
218-
Assert(amount_view.GetCoin(outpoint, c));
219-
return c.out.nValue;
217+
auto coin{amount_view.GetCoin(outpoint).value()};
218+
return coin.out.nValue;
220219
};
221220

222221
LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 300)

0 commit comments

Comments
 (0)