From 79cedc36afe2e72e42839d861734d73d545d21b8 Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Tue, 3 Dec 2019 13:25:35 -0500 Subject: coins: add Sync() method to allow flush without cacheCoins drop In certain circumstances, we may want to flush to disk without emptying `cacheCoins`, which affects performance. UTXO snapshot activation is one such case. This method is currently unused and this commit does not change any behavior. Incorporates feedback from John Newbery. Co-authored-by: Suhas Daftuar --- src/test/coins_tests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/test/coins_tests.cpp') diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index b5f961a239..d55dea4f83 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -53,7 +53,7 @@ public: uint256 GetBestBlock() const override { return hashBestBlock_; } - bool BatchWrite(CCoinsMap& mapCoins, const uint256& hashBlock) override + bool BatchWrite(CCoinsMap& mapCoins, const uint256& hashBlock, bool erase = true) override { for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); ) { if (it->second.flags & CCoinsCacheEntry::DIRTY) { -- cgit v1.2.3 From 6d8affca96c7a34f5f104c5a3122e7420ffc083c Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Tue, 3 Dec 2019 13:31:24 -0500 Subject: test: refactor: clarify the coins simulation Adds comments, slight refactor clarifications to make the code easier to follow. --- src/test/coins_tests.cpp | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) (limited to 'src/test/coins_tests.cpp') diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index d55dea4f83..e5e5a6e215 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -154,9 +154,16 @@ void SimulationTest(CCoinsView* base, bool fake_best_block) bool test_havecoin_after = InsecureRandBits(2) == 0; bool result_havecoin = test_havecoin_before ? stack.back()->HaveCoin(COutPoint(txid, 0)) : false; - const Coin& entry = (InsecureRandRange(500) == 0) ? AccessByTxid(*stack.back(), txid) : stack.back()->AccessCoin(COutPoint(txid, 0)); + + // Infrequently, test usage of AccessByTxid instead of AccessCoin - the + // former just delegates to the latter and returns the first unspent in a txn. + const Coin& entry = (InsecureRandRange(500) == 0) ? + AccessByTxid(*stack.back(), txid) : stack.back()->AccessCoin(COutPoint(txid, 0)); BOOST_CHECK(coin == entry); - BOOST_CHECK(!test_havecoin_before || result_havecoin == !entry.IsSpent()); + + if (test_havecoin_before) { + BOOST_CHECK(result_havecoin == !entry.IsSpent()); + } if (test_havecoin_after) { bool ret = stack.back()->HaveCoin(COutPoint(txid, 0)); @@ -167,24 +174,29 @@ void SimulationTest(CCoinsView* base, bool fake_best_block) Coin newcoin; newcoin.out.nValue = InsecureRand32(); newcoin.nHeight = 1; + + // Infrequently test adding unspendable coins. if (InsecureRandRange(16) == 0 && coin.IsSpent()) { newcoin.out.scriptPubKey.assign(1 + InsecureRandBits(6), OP_RETURN); BOOST_CHECK(newcoin.out.scriptPubKey.IsUnspendable()); added_an_unspendable_entry = true; } else { - newcoin.out.scriptPubKey.assign(InsecureRandBits(6), 0); // Random sizes so we can test memory usage accounting + // Random sizes so we can test memory usage accounting + newcoin.out.scriptPubKey.assign(InsecureRandBits(6), 0); (coin.IsSpent() ? added_an_entry : updated_an_entry) = true; coin = newcoin; } - stack.back()->AddCoin(COutPoint(txid, 0), std::move(newcoin), !coin.IsSpent() || InsecureRand32() & 1); + bool is_overwrite = !coin.IsSpent() || InsecureRand32() & 1; + stack.back()->AddCoin(COutPoint(txid, 0), std::move(newcoin), is_overwrite); } else { + // Spend the coin. removed_an_entry = true; coin.Clear(); BOOST_CHECK(stack.back()->SpendCoin(COutPoint(txid, 0))); } } - // One every 10 iterations, remove a random entry from the cache + // Once every 10 iterations, remove a random entry from the cache if (InsecureRandRange(10) == 0) { COutPoint out(txids[InsecureRand32() % txids.size()], 0); int cacheid = InsecureRand32() % stack.size(); -- cgit v1.2.3 From 2c3cbd6c007a588e667751024027462268626fdb Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Tue, 3 Dec 2019 13:32:43 -0500 Subject: test: add use of Sync() to coins tests --- src/test/coins_tests.cpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) (limited to 'src/test/coins_tests.cpp') diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index e5e5a6e215..2070646158 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -55,7 +55,7 @@ public: bool BatchWrite(CCoinsMap& mapCoins, const uint256& hashBlock, bool erase = true) override { - for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); ) { + for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); it = erase ? mapCoins.erase(it) : ++it) { if (it->second.flags & CCoinsCacheEntry::DIRTY) { // Same optimization used in CCoinsViewDB is to only write dirty entries. map_[it->first] = it->second.coin; @@ -64,7 +64,6 @@ public: map_.erase(it->first); } } - mapCoins.erase(it++); } if (!hashBlock.IsNull()) hashBestBlock_ = hashBlock; @@ -126,6 +125,7 @@ void SimulationTest(CCoinsView* base, bool fake_best_block) bool found_an_entry = false; bool missed_an_entry = false; bool uncached_an_entry = false; + bool flushed_without_erase = false; // A simple map to track what we expect the cache stack to represent. std::map result; @@ -228,7 +228,9 @@ void SimulationTest(CCoinsView* base, bool fake_best_block) if (stack.size() > 1 && InsecureRandBool() == 0) { unsigned int flushIndex = InsecureRandRange(stack.size() - 1); if (fake_best_block) stack[flushIndex]->SetBestBlock(InsecureRand256()); - BOOST_CHECK(stack[flushIndex]->Flush()); + bool should_erase = InsecureRandRange(4) < 3; + BOOST_CHECK(should_erase ? stack[flushIndex]->Flush() : stack[flushIndex]->Sync()); + flushed_without_erase |= !should_erase; } } if (InsecureRandRange(100) == 0) { @@ -236,7 +238,9 @@ void SimulationTest(CCoinsView* base, bool fake_best_block) if (stack.size() > 0 && InsecureRandBool() == 0) { //Remove the top cache if (fake_best_block) stack.back()->SetBestBlock(InsecureRand256()); - BOOST_CHECK(stack.back()->Flush()); + bool should_erase = InsecureRandRange(4) < 3; + BOOST_CHECK(should_erase ? stack.back()->Flush() : stack.back()->Sync()); + flushed_without_erase |= !should_erase; delete stack.back(); stack.pop_back(); } @@ -272,6 +276,7 @@ void SimulationTest(CCoinsView* base, bool fake_best_block) BOOST_CHECK(found_an_entry); BOOST_CHECK(missed_an_entry); BOOST_CHECK(uncached_an_entry); + BOOST_CHECK(flushed_without_erase); } // Run the above simulation for multiple base types. -- cgit v1.2.3 From 1d7935b45ac61791399989effc18aece8b368fbb Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Tue, 3 Dec 2019 13:33:10 -0500 Subject: test: add test for coins view flush behavior using Sync() Thanks to Marco Falke for help with move semantics. --- src/test/coins_tests.cpp | 205 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 203 insertions(+), 2 deletions(-) (limited to 'src/test/coins_tests.cpp') diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 2070646158..74f9992ef4 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -606,9 +606,9 @@ static size_t InsertCoinsMapEntry(CCoinsMap& map, CAmount value, char flags) return inserted.first->second.coin.DynamicMemoryUsage(); } -void GetCoinsMapEntry(const CCoinsMap& map, CAmount& value, char& flags) +void GetCoinsMapEntry(const CCoinsMap& map, CAmount& value, char& flags, const COutPoint& outp = OUTPOINT) { - auto it = map.find(OUTPOINT); + auto it = map.find(outp); if (it == map.end()) { value = ABSENT; flags = NO_ENTRY; @@ -894,4 +894,205 @@ BOOST_AUTO_TEST_CASE(ccoins_write) CheckWriteCoins(parent_value, child_value, parent_value, parent_flags, child_flags, parent_flags); } + +Coin MakeCoin() +{ + Coin coin; + coin.out.nValue = InsecureRand32(); + coin.nHeight = InsecureRandRange(4096); + coin.fCoinBase = 0; + return coin; +} + + +//! For CCoinsViewCache instances backed by either another cache instance or +//! leveldb, test cache behavior and flag state (DIRTY/FRESH) by +//! +//! 1. Adding a random coin to the child-most cache, +//! 2. Flushing all caches (without erasing), +//! 3. Ensure the entry still exists in the cache and has been written to parent, +//! 4. (if `do_erasing_flush`) Flushing the caches again (with erasing), +//! 5. (if `do_erasing_flush`) Ensure the entry has been written to the parent and is no longer in the cache, +//! 6. Spend the coin, ensure it no longer exists in the parent. +//! +void TestFlushBehavior( + CCoinsViewCacheTest* view, + CCoinsViewDB& base, + std::vector& all_caches, + bool do_erasing_flush) +{ + CAmount value; + char flags; + size_t cache_usage; + + auto flush_all = [&all_caches](bool erase) { + // Flush in reverse order to ensure that flushes happen from children up. + for (auto i = all_caches.rbegin(); i != all_caches.rend(); ++i) { + auto cache = *i; + // hashBlock must be filled before flushing to disk; value is + // unimportant here. This is normally done during connect/disconnect block. + cache->SetBestBlock(InsecureRand256()); + erase ? cache->Flush() : cache->Sync(); + } + }; + + uint256 txid = InsecureRand256(); + COutPoint outp = COutPoint(txid, 0); + Coin coin = MakeCoin(); + // Ensure the coins views haven't seen this coin before. + BOOST_CHECK(!base.HaveCoin(outp)); + BOOST_CHECK(!view->HaveCoin(outp)); + + // --- 1. Adding a random coin to the child cache + // + view->AddCoin(outp, Coin(coin), false); + + cache_usage = view->DynamicMemoryUsage(); + // `base` shouldn't have coin (no flush yet) but `view` should have cached it. + BOOST_CHECK(!base.HaveCoin(outp)); + BOOST_CHECK(view->HaveCoin(outp)); + + GetCoinsMapEntry(view->map(), value, flags, outp); + BOOST_CHECK_EQUAL(value, coin.out.nValue); + BOOST_CHECK_EQUAL(flags, DIRTY|FRESH); + + // --- 2. Flushing all caches (without erasing) + // + flush_all(/*erase=*/ false); + + // CoinsMap usage should be unchanged since we didn't erase anything. + BOOST_CHECK_EQUAL(cache_usage, view->DynamicMemoryUsage()); + + // --- 3. Ensuring the entry still exists in the cache and has been written to parent + // + GetCoinsMapEntry(view->map(), value, flags, outp); + BOOST_CHECK_EQUAL(value, coin.out.nValue); + BOOST_CHECK_EQUAL(flags, 0); // Flags should have been wiped. + + // Both views should now have the coin. + BOOST_CHECK(base.HaveCoin(outp)); + BOOST_CHECK(view->HaveCoin(outp)); + + if (do_erasing_flush) { + // --- 4. Flushing the caches again (with erasing) + // + flush_all(/*erase=*/ true); + + // Memory usage should have gone down. + BOOST_CHECK(view->DynamicMemoryUsage() < cache_usage); + + // --- 5. Ensuring the entry is no longer in the cache + // + GetCoinsMapEntry(view->map(), value, flags, outp); + BOOST_CHECK_EQUAL(value, ABSENT); + BOOST_CHECK_EQUAL(flags, NO_ENTRY); + + view->AccessCoin(outp); + GetCoinsMapEntry(view->map(), value, flags, outp); + BOOST_CHECK_EQUAL(value, coin.out.nValue); + BOOST_CHECK_EQUAL(flags, 0); + } + + // Can't overwrite an entry without specifying that an overwrite is + // expected. + BOOST_CHECK_THROW( + view->AddCoin(outp, Coin(coin), /*possible_overwrite=*/ false), + std::logic_error); + + // --- 6. Spend the coin. + // + BOOST_CHECK(view->SpendCoin(outp)); + + // The coin should be in the cache, but spent and marked dirty. + GetCoinsMapEntry(view->map(), value, flags, outp); + BOOST_CHECK_EQUAL(value, SPENT); + BOOST_CHECK_EQUAL(flags, DIRTY); + BOOST_CHECK(!view->HaveCoin(outp)); // Coin should be considered spent in `view`. + BOOST_CHECK(base.HaveCoin(outp)); // But coin should still be unspent in `base`. + + flush_all(/*erase=*/ false); + + // Coin should be considered spent in both views. + BOOST_CHECK(!view->HaveCoin(outp)); + BOOST_CHECK(!base.HaveCoin(outp)); + + // Spent coin should not be spendable. + BOOST_CHECK(!view->SpendCoin(outp)); + + // --- Bonus check: ensure that a coin added to the base view via one cache + // can be spent by another cache which has never seen it. + // + txid = InsecureRand256(); + outp = COutPoint(txid, 0); + coin = MakeCoin(); + BOOST_CHECK(!base.HaveCoin(outp)); + BOOST_CHECK(!all_caches[0]->HaveCoin(outp)); + BOOST_CHECK(!all_caches[1]->HaveCoin(outp)); + + all_caches[0]->AddCoin(outp, std::move(coin), false); + all_caches[0]->Sync(); + BOOST_CHECK(base.HaveCoin(outp)); + BOOST_CHECK(all_caches[0]->HaveCoin(outp)); + BOOST_CHECK(!all_caches[1]->HaveCoinInCache(outp)); + + BOOST_CHECK(all_caches[1]->SpendCoin(outp)); + flush_all(/*erase=*/ false); + BOOST_CHECK(!base.HaveCoin(outp)); + BOOST_CHECK(!all_caches[0]->HaveCoin(outp)); + BOOST_CHECK(!all_caches[1]->HaveCoin(outp)); + + flush_all(/*erase=*/ true); // Erase all cache content. + + // --- Bonus check 2: ensure that a FRESH, spent coin is deleted by Sync() + // + txid = InsecureRand256(); + outp = COutPoint(txid, 0); + coin = MakeCoin(); + CAmount coin_val = coin.out.nValue; + BOOST_CHECK(!base.HaveCoin(outp)); + BOOST_CHECK(!all_caches[0]->HaveCoin(outp)); + BOOST_CHECK(!all_caches[1]->HaveCoin(outp)); + + // Add and spend from same cache without flushing. + all_caches[0]->AddCoin(outp, std::move(coin), false); + + // Coin should be FRESH in the cache. + GetCoinsMapEntry(all_caches[0]->map(), value, flags, outp); + BOOST_CHECK_EQUAL(value, coin_val); + BOOST_CHECK_EQUAL(flags, DIRTY|FRESH); + + // Base shouldn't have seen coin. + BOOST_CHECK(!base.HaveCoin(outp)); + + BOOST_CHECK(all_caches[0]->SpendCoin(outp)); + all_caches[0]->Sync(); + + // Ensure there is no sign of the coin after spend/flush. + GetCoinsMapEntry(all_caches[0]->map(), value, flags, outp); + BOOST_CHECK_EQUAL(value, ABSENT); + BOOST_CHECK_EQUAL(flags, NO_ENTRY); + BOOST_CHECK(!all_caches[0]->HaveCoinInCache(outp)); + BOOST_CHECK(!base.HaveCoin(outp)); +} + +BOOST_AUTO_TEST_CASE(ccoins_flush_behavior) +{ + // Create two in-memory caches atop a leveldb view. + CCoinsViewDB base{"test", /*nCacheSize=*/ 1 << 23, /*fMemory=*/ true, /*fWipe=*/ false}; + std::vector caches; + caches.push_back(new CCoinsViewCacheTest(&base)); + caches.push_back(new CCoinsViewCacheTest(caches.back())); + + for (CCoinsViewCacheTest* view : caches) { + TestFlushBehavior(view, base, caches, /*do_erasing_flush=*/ false); + TestFlushBehavior(view, base, caches, /*do_erasing_flush=*/ true); + } + + // Clean up the caches. + while (caches.size() > 0) { + delete caches.back(); + caches.pop_back(); + } +} + BOOST_AUTO_TEST_SUITE_END() -- cgit v1.2.3