Skip to content

Commit

Permalink
Merge #6098: backport: merge bitcoin#22278, bitcoin#24103, bitcoin#24235
Browse files Browse the repository at this point in the history 
, bitcoin#24177, bitcoin#24299, bitcoin#24917, bitcoin#22564, bitcoin#25349, bitcoin#25571, bitcoin#17487, bitcoin#26999, bitcoin#27011 (blockstorage backports: part 2)

b658637 merge bitcoin#27011: Add simulation-based `CCoinsViewCache` fuzzer (Kittywhiskers Van Gogh)
1d0e410 merge bitcoin#26999: A few follow-ups to bitcoin#17487 (Kittywhiskers Van Gogh)
7d837ea merge bitcoin#17487: allow write to disk without cache drop (Kittywhiskers Van Gogh)
2c758f4 merge bitcoin#25571: Make mapBlocksUnknownParent local, and rename it (Kittywhiskers Van Gogh)
70a91e1 merge bitcoin#25349: CBlockIndex/CDiskBlockIndex improvements for safety, consistent behavior (Kittywhiskers Van Gogh)
eca0a64 merge bitcoin#22564: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager (Kittywhiskers Van Gogh)
916b3f0 merge bitcoin#24917: Make BlockManager::LoadBlockIndex private (Kittywhiskers Van Gogh)
e10ca27 merge bitcoin#24299: UnloadBlockIndex and ChainstateManager::Reset thread safety cleanups (Kittywhiskers Van Gogh)
18aa55b merge bitcoin#24177: add missing thread safety lock assertions (Kittywhiskers Van Gogh)
678e67c merge bitcoin#24235: use stronger EXCLUSIVE_LOCKS_REQUIRED() (Kittywhiskers Van Gogh)
edc665c merge bitcoin#24103: Replace RecursiveMutex m_cs_chainstate with Mutex, and rename it (Kittywhiskers Van Gogh)
d19ffd6 merge bitcoin#22278: Add LIFETIMEBOUND to CScript where needed (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependent on #6097
  * Dependency for #6067
  * Test failures in `linux64_multiprocess-build` ([build](https://gitlab.com/dashpay/dash/-/jobs/7550924662)) and `linux64_tsan-test` ([build](https://gitlab.com/dashpay/dash/-/jobs/7550924666)) do not stem from this PR but are pre-existing failures in `develop` ([build](https://gitlab.com/dashpay/dash/-/jobs/7550859495), [build](https://gitlab.com/dashpay/dash/-/jobs/7550859499)). A fix for the build failures has been opened as a separate PR.

  ## Breaking Changes

  None observed.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    LGTM, utACK b658637
  PastaPastaPasta:
    utACK b658637

Tree-SHA512: 1a9c3a41617af274db169db47a9c9fce7ba7ce0b2d68aa75617640a55da11a0fa095cb25ce6a2d38f06d3f6a6cc4c08cb4cf82dca4bdc74192e8882fd5f7052f
  • Loading branch information
@PastaPastaPasta
PastaPastaPasta committed Aug 25, 2024
2 parents 045e178 + b658637 commit 114d787
Show file tree
Hide file tree
Showing 23 changed files with 1,030 additions and 233 deletions.
1 change: 1 addition & 0 deletions src/Makefile.test.include
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ test_fuzz_fuzz_SOURCES = \
test/fuzz/chain.cpp \
test/fuzz/checkqueue.cpp \
test/fuzz/coins_view.cpp \
test/fuzz/coinscache_sim.cpp \
test/fuzz/connman.cpp \
test/fuzz/crypto.cpp \
test/fuzz/crypto_aes256.cpp \
Expand Down
14 changes: 1 addition & 13 deletions src/chain.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,10 @@

#include <tinyformat.h>

std::string CDiskBlockIndex::ToString() const
{
std::string str = "CDiskBlockIndex(";
str += CBlockIndex::ToString();
str += strprintf("\n hashBlock=%s, hashPrev=%s)",
GetBlockHash().ToString(),
hashPrev.ToString());
return str;
}

std::string CBlockIndex::ToString() const
{
return strprintf("CBlockIndex(pprev=%p, nHeight=%d, merkle=%s, hashBlock=%s)",
pprev, nHeight,
hashMerkleRoot.ToString(),
GetBlockHash().ToString());
pprev, nHeight, hashMerkleRoot.ToString(), GetBlockHash().ToString());
}

/**
Expand Down
7 changes: 4 additions & 3 deletions src/chain.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ class CBlockIndex

uint256 GetBlockHash() const
{
assert(phashBlock != nullptr);
return *phashBlock;
}

Expand Down Expand Up @@ -393,7 +394,7 @@ class CDiskBlockIndex : public CBlockIndex
READWRITE(obj.nNonce);
}

uint256 GetBlockHash() const
uint256 ConstructBlockHash() const
{
if(hash != uint256()) return hash;
// should never really get here, keeping this as a fallback
Expand All @@ -407,8 +408,8 @@ class CDiskBlockIndex : public CBlockIndex
return block.GetHash();
}

std::string ToString() const;

uint256 GetBlockHash() = delete;
std::string ToString() = delete;
};

/** An in-memory indexed chain of blocks. */
Expand Down
76 changes: 66 additions & 10 deletions src/coins.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
bool CCoinsView::GetCoin(const COutPoint &outpoint, Coin &coin) const { return false; }
uint256 CCoinsView::GetBestBlock() const { return uint256(); }
std::vector<uint256> CCoinsView::GetHeadBlocks() const { return std::vector<uint256>(); }
bool CCoinsView::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) { return false; }
bool CCoinsView::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase) { return false; }
std::unique_ptr<CCoinsViewCursor> CCoinsView::Cursor() const { return nullptr; }

bool CCoinsView::HaveCoin(const COutPoint &outpoint) const
Expand All @@ -27,11 +27,14 @@ bool CCoinsViewBacked::HaveCoin(const COutPoint &outpoint) const { return base->
uint256 CCoinsViewBacked::GetBestBlock() const { return base->GetBestBlock(); }
std::vector<uint256> CCoinsViewBacked::GetHeadBlocks() const { return base->GetHeadBlocks(); }
void CCoinsViewBacked::SetBackend(CCoinsView &viewIn) { base = &viewIn; }
bool CCoinsViewBacked::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) { return base->BatchWrite(mapCoins, hashBlock); }
bool CCoinsViewBacked::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase) { return base->BatchWrite(mapCoins, hashBlock, erase); }
std::unique_ptr<CCoinsViewCursor> CCoinsViewBacked::Cursor() const { return base->Cursor(); }
size_t CCoinsViewBacked::EstimateSize() const { return base->EstimateSize(); }

CCoinsViewCache::CCoinsViewCache(CCoinsView *baseIn) : CCoinsViewBacked(baseIn), cachedCoinsUsage(0) {}
CCoinsViewCache::CCoinsViewCache(CCoinsView* baseIn, bool deterministic) :
CCoinsViewBacked(baseIn), m_deterministic(deterministic),
cacheCoins(0, SaltedOutpointHasher(/*deterministic=*/deterministic))
{}

size_t CCoinsViewCache::DynamicMemoryUsage() const {
return memusage::DynamicUsage(cacheCoins) + cachedCoinsUsage;
Expand Down Expand Up @@ -163,8 +166,10 @@ void CCoinsViewCache::SetBestBlock(const uint256 &hashBlockIn) {
hashBlock = hashBlockIn;
}

bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn) {
for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); it = mapCoins.erase(it)) {
bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn, bool erase) {
for (CCoinsMap::iterator it = mapCoins.begin();
it != mapCoins.end();
it = erase ? mapCoins.erase(it) : std::next(it)) {
// Ignore non-dirty entries (optimization).
if (!(it->second.flags & CCoinsCacheEntry::DIRTY)) {
continue;
Expand All @@ -177,7 +182,14 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn
// Create the coin in the parent cache, move the data up
// and mark it as dirty.
CCoinsCacheEntry& entry = cacheCoins[it->first];
entry.coin = std::move(it->second.coin);
if (erase) {
// The `move` call here is purely an optimization; we rely on the
// `mapCoins.erase` call in the `for` expression to actually remove
// the entry from the child map.
entry.coin = std::move(it->second.coin);
} else {
entry.coin = it->second.coin;
}
cachedCoinsUsage += entry.coin.DynamicMemoryUsage();
entry.flags = CCoinsCacheEntry::DIRTY;
// We can mark it FRESH in the parent if it was FRESH in the child
Expand Down Expand Up @@ -205,7 +217,14 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn
} else {
// A normal modification.
cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
itUs->second.coin = std::move(it->second.coin);
if (erase) {
// The `move` call here is purely an optimization; we rely on the
// `mapCoins.erase` call in the `for` expression to actually remove
// the entry from the child map.
itUs->second.coin = std::move(it->second.coin);
} else {
itUs->second.coin = it->second.coin;
}
cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage();
itUs->second.flags |= CCoinsCacheEntry::DIRTY;
// NOTE: It isn't safe to mark the coin as FRESH in the parent
Expand All @@ -220,12 +239,32 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn
}

bool CCoinsViewCache::Flush() {
bool fOk = base->BatchWrite(cacheCoins, hashBlock);
cacheCoins.clear();
bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/true);
if (fOk && !cacheCoins.empty()) {
/* BatchWrite must erase all cacheCoins elements when erase=true. */
throw std::logic_error("Not all cached coins were erased");
}
cachedCoinsUsage = 0;
return fOk;
}

bool CCoinsViewCache::Sync()
{
bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/false);
// Instead of clearing `cacheCoins` as we would in Flush(), just clear the
// FRESH/DIRTY flags of any coin that isn't spent.
for (auto it = cacheCoins.begin(); it != cacheCoins.end(); ) {
if (it->second.coin.IsSpent()) {
cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
it = cacheCoins.erase(it);
} else {
it->second.flags = 0;
++it;
}
}
return fOk;
}

void CCoinsViewCache::Uncache(const COutPoint& hash)
{
CCoinsMap::iterator it = cacheCoins.find(hash);
Expand Down Expand Up @@ -256,7 +295,24 @@ void CCoinsViewCache::ReallocateCache()
// Cache should be empty when we're calling this.
assert(cacheCoins.size() == 0);
cacheCoins.~CCoinsMap();
::new (&cacheCoins) CCoinsMap();
::new (&cacheCoins) CCoinsMap(0, SaltedOutpointHasher(/*deterministic=*/m_deterministic));
}

void CCoinsViewCache::SanityCheck() const
{
size_t recomputed_usage = 0;
for (const auto& [_, entry] : cacheCoins) {
unsigned attr = 0;
if (entry.flags & CCoinsCacheEntry::DIRTY) attr |= 1;
if (entry.flags & CCoinsCacheEntry::FRESH) attr |= 2;
if (entry.coin.IsSpent()) attr |= 4;
// Only 5 combinations are possible.
assert(attr != 2 && attr != 4 && attr != 7);

// Recompute cachedCoinsUsage.
recomputed_usage += entry.coin.DynamicMemoryUsage();
}
assert(recomputed_usage == cachedCoinsUsage);
}

static const size_t MAX_OUTPUTS_PER_BLOCK = MaxBlockSize() / ::GetSerializeSize(CTxOut(), PROTOCOL_VERSION);
Expand Down
30 changes: 23 additions & 7 deletions src/coins.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ class CCoinsView

//! Do a bulk modification (multiple Coin changes + BestBlock change).
//! The passed mapCoins can be modified.
virtual bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock);
virtual bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true);

//! Get a cursor to iterate over the whole state
virtual std::unique_ptr<CCoinsViewCursor> Cursor() const;
Expand All @@ -203,7 +203,7 @@ class CCoinsViewBacked : public CCoinsView
uint256 GetBestBlock() const override;
std::vector<uint256> GetHeadBlocks() const override;
void SetBackend(CCoinsView &viewIn);
bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) override;
bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true) override;
std::unique_ptr<CCoinsViewCursor> Cursor() const override;
size_t EstimateSize() const override;
};
Expand All @@ -212,6 +212,9 @@ class CCoinsViewBacked : public CCoinsView
/** CCoinsView that adds a memory cache for transactions to another CCoinsView */
class CCoinsViewCache : public CCoinsViewBacked
{
private:
const bool m_deterministic;

protected:
/**
* Make mutable so that we can "fill the cache" even from Get-methods
Expand All @@ -221,10 +224,10 @@ class CCoinsViewCache : public CCoinsViewBacked
mutable CCoinsMap cacheCoins;

/* Cached dynamic memory usage for the inner Coin objects. */
mutable size_t cachedCoinsUsage;
mutable size_t cachedCoinsUsage{0};

public:
CCoinsViewCache(CCoinsView *baseIn);
CCoinsViewCache(CCoinsView *baseIn, bool deterministic = false);

/**
* By deleting the copy constructor, we prevent accidentally using it when one intends to create a cache on top of a base cache.
Expand All @@ -236,7 +239,7 @@ class CCoinsViewCache : public CCoinsViewBacked
bool HaveCoin(const COutPoint &outpoint) const override;
uint256 GetBestBlock() const override;
void SetBestBlock(const uint256 &hashBlock);
bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) override;
bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true) override;
std::unique_ptr<CCoinsViewCursor> Cursor() const override {
throw std::logic_error("CCoinsViewCache cursor iteration not supported.");
}
Expand Down Expand Up @@ -283,12 +286,22 @@ class CCoinsViewCache : public CCoinsViewBacked
bool SpendCoin(const COutPoint &outpoint, Coin* moveto = nullptr);

/**
* Push the modifications applied to this cache to its base.
* Failure to call this method before destruction will cause the changes to be forgotten.
* Push the modifications applied to this cache to its base and wipe local state.
* Failure to call this method or Sync() before destruction will cause the changes
* to be forgotten.
* If false is returned, the state of this cache (and its backing view) will be undefined.
*/
bool Flush();

/**
* Push the modifications applied to this cache to its base while retaining
* the contents of this cache (except for spent coins, which we erase).
* Failure to call this method or Flush() before destruction will cause the changes
* to be forgotten.
* If false is returned, the state of this cache (and its backing view) will be undefined.
*/
bool Sync();

/**
* Removes the UTXO with the given outpoint from the cache, if it is
* not modified.
Expand All @@ -311,6 +324,9 @@ class CCoinsViewCache : public CCoinsViewBacked
//! See: https://stackoverflow.com/questions/42114044/how-to-release-unordered-map-memory
void ReallocateCache();

//! Run an internal sanity check on the cache data structure. */
void SanityCheck() const;

private:
/**
* @note this is marked const, but may actually append to `cacheCoins`, increasing
Expand Down
Loading

0 comments on commit 114d787

Please sign in to comment.