From 615c1adfb07b9b466173166dc2e53ace540e4b32 Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Wed, 16 Jun 2021 16:27:20 -0400 Subject: [PATCH 1/3] refactor: wrap CCoinsViewCursor in unique_ptr Specifically with CCoinsViewDB, if a raw cursor is allocated and not freed, a cryptic leveldb assertion failure occurs on CCoinsViewDB destruction. See: https://github.com/google/leveldb/issues/142#issuecomment-414418135 --- src/coins.cpp | 4 ++-- src/coins.h | 6 +++--- src/rpc/blockchain.cpp | 4 ++-- src/test/fuzz/coins_view.cpp | 4 ++-- src/txdb.cpp | 5 +++-- src/txdb.h | 6 +++--- 6 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index d52851cadd3..ce0b131de6d 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -13,7 +13,7 @@ bool CCoinsView::GetCoin(const COutPoint &outpoint, Coin &coin) const { return f uint256 CCoinsView::GetBestBlock() const { return uint256(); } std::vector CCoinsView::GetHeadBlocks() const { return std::vector(); } bool CCoinsView::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) { return false; } -CCoinsViewCursor *CCoinsView::Cursor() const { return nullptr; } +std::unique_ptr CCoinsView::Cursor() const { return nullptr; } bool CCoinsView::HaveCoin(const COutPoint &outpoint) const { @@ -28,7 +28,7 @@ uint256 CCoinsViewBacked::GetBestBlock() const { return base->GetBestBlock(); } std::vector 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); } -CCoinsViewCursor *CCoinsViewBacked::Cursor() const { return base->Cursor(); } +std::unique_ptr CCoinsViewBacked::Cursor() const { return base->Cursor(); } size_t CCoinsViewBacked::EstimateSize() const { return base->EstimateSize(); } CCoinsViewCache::CCoinsViewCache(CCoinsView *baseIn) : CCoinsViewBacked(baseIn), cachedCoinsUsage(0) {} diff --git a/src/coins.h b/src/coins.h index 816b4864a38..3151a260d94 100644 --- a/src/coins.h +++ b/src/coins.h @@ -180,7 +180,7 @@ public: virtual bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock); //! Get a cursor to iterate over the whole state - virtual CCoinsViewCursor *Cursor() const; + virtual std::unique_ptr Cursor() const; //! As we use CCoinsViews polymorphically, have a virtual destructor virtual ~CCoinsView() {} @@ -204,7 +204,7 @@ public: std::vector GetHeadBlocks() const override; void SetBackend(CCoinsView &viewIn); bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) override; - CCoinsViewCursor *Cursor() const override; + std::unique_ptr Cursor() const override; size_t EstimateSize() const override; }; @@ -237,7 +237,7 @@ public: uint256 GetBestBlock() const override; void SetBestBlock(const uint256 &hashBlock); bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) override; - CCoinsViewCursor* Cursor() const override { + std::unique_ptr Cursor() const override { throw std::logic_error("CCoinsViewCache cursor iteration not supported."); } diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 63897e0e05b..4d4d3fb7d08 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -2391,7 +2391,7 @@ static RPCHelpMan scantxoutset() LOCK(cs_main); CChainState& active_chainstate = chainman.ActiveChainstate(); active_chainstate.ForceFlushStateToDisk(); - pcursor = std::unique_ptr(active_chainstate.CoinsDB().Cursor()); + pcursor = active_chainstate.CoinsDB().Cursor(); CHECK_NONFATAL(pcursor); tip = active_chainstate.m_chain.Tip(); CHECK_NONFATAL(tip); @@ -2590,7 +2590,7 @@ UniValue CreateUTXOSnapshot(NodeContext& node, CChainState& chainstate, CAutoFil throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to read UTXO set"); } - pcursor = std::unique_ptr(chainstate.CoinsDB().Cursor()); + pcursor = chainstate.CoinsDB().Cursor(); tip = chainstate.m_blockman.LookupBlockIndex(stats.hashBlock); CHECK_NONFATAL(tip); } diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 42f19d16c6c..f4526966893 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -183,8 +183,8 @@ FUZZ_TARGET_INIT(coins_view, initialize_coins_view) } { - const CCoinsViewCursor* coins_view_cursor = backend_coins_view.Cursor(); - assert(coins_view_cursor == nullptr); + std::unique_ptr coins_view_cursor = backend_coins_view.Cursor(); + assert(!coins_view_cursor); (void)backend_coins_view.EstimateSize(); (void)backend_coins_view.GetBestBlock(); (void)backend_coins_view.GetHeadBlocks(); diff --git a/src/txdb.cpp b/src/txdb.cpp index 762f71feb10..bd0e9f7317f 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -168,9 +168,10 @@ bool CBlockTreeDB::ReadLastBlockFile(int &nFile) { return Read(DB_LAST_BLOCK, nFile); } -CCoinsViewCursor *CCoinsViewDB::Cursor() const +std::unique_ptr CCoinsViewDB::Cursor() const { - CCoinsViewDBCursor *i = new CCoinsViewDBCursor(const_cast(*m_db).NewIterator(), GetBestBlock()); + auto i = std::make_unique( + const_cast(*m_db).NewIterator(), GetBestBlock()); /* It seems that there are no "const iterators" for LevelDB. Since we only need read operations on it, use a const-cast to get around that restriction. */ diff --git a/src/txdb.h b/src/txdb.h index 0cf7e2f1b8b..e1096ffeba0 100644 --- a/src/txdb.h +++ b/src/txdb.h @@ -60,7 +60,7 @@ public: uint256 GetBestBlock() const override; std::vector GetHeadBlocks() const override; bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) override; - CCoinsViewCursor *Cursor() const override; + std::unique_ptr Cursor() const override; //! Attempt to update from an older database format. Returns whether an error occurred. bool Upgrade(); @@ -74,6 +74,8 @@ public: class CCoinsViewDBCursor: public CCoinsViewCursor { public: + CCoinsViewDBCursor(CDBIterator* pcursorIn, const uint256 &hashBlockIn): + CCoinsViewCursor(hashBlockIn), pcursor(pcursorIn) {} ~CCoinsViewDBCursor() {} bool GetKey(COutPoint &key) const override; @@ -84,8 +86,6 @@ public: void Next() override; private: - CCoinsViewDBCursor(CDBIterator* pcursorIn, const uint256 &hashBlockIn): - CCoinsViewCursor(hashBlockIn), pcursor(pcursorIn) {} std::unique_ptr pcursor; std::pair keyTmp; From 0f8a5a4dd530549d37c43da52c923ac3b2af1a03 Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Fri, 18 Jun 2021 14:14:15 -0400 Subject: [PATCH 2/3] move-only(ish): don't expose CCoinsViewDBCursor No need for this to be a part of the header anymore. Includes a small reference type style change. --- src/txdb.cpp | 22 ++++++++++++++++++++++ src/txdb.h | 22 ---------------------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/txdb.cpp b/src/txdb.cpp index bd0e9f7317f..e6469ee1599 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -168,6 +168,28 @@ bool CBlockTreeDB::ReadLastBlockFile(int &nFile) { return Read(DB_LAST_BLOCK, nFile); } +/** Specialization of CCoinsViewCursor to iterate over a CCoinsViewDB */ +class CCoinsViewDBCursor: public CCoinsViewCursor +{ +public: + CCoinsViewDBCursor(CDBIterator* pcursorIn, const uint256&hashBlockIn): + CCoinsViewCursor(hashBlockIn), pcursor(pcursorIn) {} + ~CCoinsViewDBCursor() {} + + bool GetKey(COutPoint &key) const override; + bool GetValue(Coin &coin) const override; + unsigned int GetValueSize() const override; + + bool Valid() const override; + void Next() override; + +private: + std::unique_ptr pcursor; + std::pair keyTmp; + + friend class CCoinsViewDB; +}; + std::unique_ptr CCoinsViewDB::Cursor() const { auto i = std::make_unique( diff --git a/src/txdb.h b/src/txdb.h index e1096ffeba0..845d80788f2 100644 --- a/src/txdb.h +++ b/src/txdb.h @@ -70,28 +70,6 @@ public: void ResizeCache(size_t new_cache_size) EXCLUSIVE_LOCKS_REQUIRED(cs_main); }; -/** Specialization of CCoinsViewCursor to iterate over a CCoinsViewDB */ -class CCoinsViewDBCursor: public CCoinsViewCursor -{ -public: - CCoinsViewDBCursor(CDBIterator* pcursorIn, const uint256 &hashBlockIn): - CCoinsViewCursor(hashBlockIn), pcursor(pcursorIn) {} - ~CCoinsViewDBCursor() {} - - bool GetKey(COutPoint &key) const override; - bool GetValue(Coin &coin) const override; - unsigned int GetValueSize() const override; - - bool Valid() const override; - void Next() override; - -private: - std::unique_ptr pcursor; - std::pair keyTmp; - - friend class CCoinsViewDB; -}; - /** Access to the block database (blocks/index/) */ class CBlockTreeDB : public CDBWrapper { From 7ad414f4bfa74595ee5726e66f3527045c02a977 Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Fri, 18 Jun 2021 14:15:39 -0400 Subject: [PATCH 3/3] doc: add comment about CCoinsViewDBCursor constructor --- src/txdb.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/txdb.cpp b/src/txdb.cpp index e6469ee1599..4b76bee5ab8 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -172,6 +172,8 @@ bool CBlockTreeDB::ReadLastBlockFile(int &nFile) { class CCoinsViewDBCursor: public CCoinsViewCursor { public: + // Prefer using CCoinsViewDB::Cursor() since we want to perform some + // cache warmup on instantiation. CCoinsViewDBCursor(CDBIterator* pcursorIn, const uint256&hashBlockIn): CCoinsViewCursor(hashBlockIn), pcursor(pcursorIn) {} ~CCoinsViewDBCursor() {}