From a9b7f5614c24fe6f386448604c325ec4fa6c98a5 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 30 May 2024 11:25:04 -0400 Subject: [PATCH] refactor: Add Chainstate::StoragePath() method Use to simplify code determining the chainstate leveldb paths. New method is the now the only code that needs to figure out the storage path, so the path doesn't need to be constructed multiple places and backed out of leveldb. --- src/dbwrapper.cpp | 2 +- src/dbwrapper.h | 14 ------ src/node/chainstate.cpp | 2 +- src/node/utxo_snapshot.cpp | 2 +- src/test/util/chainstate.h | 2 +- src/txdb.h | 3 -- src/validation.cpp | 98 ++++++++++++++------------------------ src/validation.h | 11 ++--- 8 files changed, 45 insertions(+), 89 deletions(-) diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp index 608f61f07c6..b3f08cb20d6 100644 --- a/src/dbwrapper.cpp +++ b/src/dbwrapper.cpp @@ -214,7 +214,7 @@ struct LevelDBContext { }; CDBWrapper::CDBWrapper(const DBParams& params) - : m_db_context{std::make_unique()}, m_name{fs::PathToString(params.path.stem())}, m_path{params.path}, m_is_memory{params.memory_only} + : m_db_context{std::make_unique()}, m_name{fs::PathToString(params.path.stem())} { DBContext().penv = nullptr; DBContext().readoptions.verify_checksums = true; diff --git a/src/dbwrapper.h b/src/dbwrapper.h index c770df8e206..b2ce67c7c2e 100644 --- a/src/dbwrapper.h +++ b/src/dbwrapper.h @@ -191,12 +191,6 @@ private: //! obfuscation key storage key, null-prefixed to avoid collisions inline static const std::string OBFUSCATION_KEY{"\000obfuscate_key", 14}; // explicit size to avoid truncation at leading \0 - //! path to filesystem storage - const fs::path m_path; - - //! whether or not the database resides in memory - bool m_is_memory; - std::optional ReadImpl(std::span key) const; bool ExistsImpl(std::span key) const; size_t EstimateSizeImpl(std::span key1, std::span key2) const; @@ -237,14 +231,6 @@ public: WriteBatch(batch, fSync); } - //! @returns filesystem path to the on-disk data. - std::optional StoragePath() { - if (m_is_memory) { - return {}; - } - return m_path; - } - template bool Exists(const K& key) const { diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index 5a03e581626..bba518d1825 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -202,7 +202,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize // do nothing; expected case } else if (snapshot_completion == SnapshotCompletionResult::SUCCESS) { LogInfo("[snapshot] cleaning up unneeded background chainstate, then reinitializing"); - if (!chainman.ValidatedSnapshotCleanup()) { + if (!chainman.ValidatedSnapshotCleanup(validated_cs, *assumeutxo_cs)) { return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated("Background chainstate cleanup failed unexpectedly.")}; } diff --git a/src/node/utxo_snapshot.cpp b/src/node/utxo_snapshot.cpp index e159d9e0213..abd7bcc19b6 100644 --- a/src/node/utxo_snapshot.cpp +++ b/src/node/utxo_snapshot.cpp @@ -25,7 +25,7 @@ bool WriteSnapshotBaseBlockhash(Chainstate& snapshot_chainstate) AssertLockHeld(::cs_main); assert(snapshot_chainstate.m_from_snapshot_blockhash); - const std::optional chaindir = snapshot_chainstate.CoinsDB().StoragePath(); + const std::optional chaindir = snapshot_chainstate.StoragePath(); assert(chaindir); // Sanity check that chainstate isn't in-memory. const fs::path write_to = *chaindir / node::SNAPSHOT_BLOCKHASH_FILENAME; diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h index aac7816e0e3..2d5919c2a8f 100644 --- a/src/test/util/chainstate.h +++ b/src/test/util/chainstate.h @@ -81,7 +81,7 @@ CreateAndActivateUTXOSnapshot( Chainstate& chain = node.chainman->ActiveChainstate(); Assert(chain.LoadGenesisBlock()); // These cache values will be corrected shortly in `MaybeRebalanceCaches`. - chain.InitCoinsDB(1 << 20, true, false, ""); + chain.InitCoinsDB(1 << 20, /*in_memory=*/true, /*should_wipe=*/false); chain.InitCoinsCache(1 << 20); chain.CoinsTip().SetBestBlock(gen_hash); chain.setBlockIndexCandidates.insert(node.chainman->m_blockman.LookupBlockIndex(gen_hash)); diff --git a/src/txdb.h b/src/txdb.h index ea0cf9d77e5..89e94473333 100644 --- a/src/txdb.h +++ b/src/txdb.h @@ -53,9 +53,6 @@ public: //! Dynamically alter the underlying leveldb cache size. void ResizeCache(size_t new_cache_size) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - - //! @returns filesystem path to on-disk storage or std::nullopt if in memory. - std::optional StoragePath() { return m_db->StoragePath(); } }; #endif // BITCOIN_TXDB_H diff --git a/src/validation.cpp b/src/validation.cpp index 15981708f58..a9bf12b5495 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1885,6 +1885,15 @@ Chainstate::Chainstate( m_assumeutxo(from_snapshot_blockhash ? Assumeutxo::UNVALIDATED : Assumeutxo::VALIDATED), m_from_snapshot_blockhash(from_snapshot_blockhash) {} +fs::path Chainstate::StoragePath() const +{ + fs::path path{m_chainman.m_options.datadir / "chainstate"}; + if (m_from_snapshot_blockhash) { + path += node::SNAPSHOT_CHAINSTATE_SUFFIX; + } + return path; +} + const CBlockIndex* Chainstate::SnapshotBase() const { if (!m_from_snapshot_blockhash) return nullptr; @@ -1918,16 +1927,11 @@ void Chainstate::SetTargetBlockHash(uint256 block_hash) void Chainstate::InitCoinsDB( size_t cache_size_bytes, bool in_memory, - bool should_wipe, - fs::path leveldb_name) + bool should_wipe) { - if (m_from_snapshot_blockhash) { - leveldb_name += node::SNAPSHOT_CHAINSTATE_SUFFIX; - } - m_coins_views = std::make_unique( DBParams{ - .path = m_chainman.m_options.datadir / leveldb_name, + .path = StoragePath(), .cache_bytes = cache_size_bytes, .memory_only = in_memory, .wipe_data = should_wipe, @@ -5757,7 +5761,7 @@ util::Result ChainstateManager::ActivateSnapshot( LOCK(::cs_main); snapshot_chainstate->InitCoinsDB( static_cast(current_coinsdb_cache_size * SNAPSHOT_CACHE_PERC), - in_memory, false, "chainstate"); + in_memory, /*should_wipe=*/false); snapshot_chainstate->InitCoinsCache( static_cast(current_coinstip_cache_size * SNAPSHOT_CACHE_PERC)); } @@ -6298,46 +6302,34 @@ bool IsBIP30Unspendable(const uint256& block_hash, int block_height) (block_height==91812 && block_hash == uint256{"00000000000af0aed4792b1acee3d966af36cf5def14935db8de83d6f9306f2f"}); } -static fs::path GetSnapshotCoinsDBPath(Chainstate& cs) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) -{ - AssertLockHeld(::cs_main); - // Should never be called on a non-snapshot chainstate. - assert(cs.m_from_snapshot_blockhash); - auto storage_path_maybe = cs.CoinsDB().StoragePath(); - // Should never be called with a non-existent storage path. - assert(storage_path_maybe); - return *storage_path_maybe; -} - util::Result Chainstate::InvalidateCoinsDBOnDisk() { - fs::path snapshot_datadir = GetSnapshotCoinsDBPath(*this); + // Should never be called on a non-snapshot chainstate. + assert(m_from_snapshot_blockhash); // Coins views no longer usable. m_coins_views.reset(); - auto invalid_path = snapshot_datadir + "_INVALID"; - std::string dbpath = fs::PathToString(snapshot_datadir); - std::string target = fs::PathToString(invalid_path); - LogInfo("[snapshot] renaming snapshot datadir %s to %s", dbpath, target); + const fs::path db_path{StoragePath()}; + const fs::path invalid_path{db_path + "_INVALID"}; + const std::string db_path_str{fs::PathToString(db_path)}; + const std::string invalid_path_str{fs::PathToString(invalid_path)}; + LogInfo("[snapshot] renaming snapshot datadir %s to %s", db_path_str, invalid_path_str); - // The invalid snapshot datadir is simply moved and not deleted because we may + // The invalid storage directory is simply moved and not deleted because we may // want to do forensics later during issue investigation. The user is instructed // accordingly in MaybeValidateSnapshot(). try { - fs::rename(snapshot_datadir, invalid_path); + fs::rename(db_path, invalid_path); } catch (const fs::filesystem_error& e) { - auto src_str = fs::PathToString(snapshot_datadir); - auto dest_str = fs::PathToString(invalid_path); - LogError("While invalidating the coins db: Error renaming file '%s' -> '%s': %s", - src_str, dest_str, e.what()); + db_path_str, invalid_path_str, e.what()); return util::Error{strprintf(_( "Rename of '%s' -> '%s' failed. " "You should resolve this by manually moving or deleting the invalid " "snapshot directory %s, otherwise you will encounter the same error again " "on the next startup."), - src_str, dest_str, src_str)}; + db_path_str, invalid_path_str, db_path_str)}; } return {}; } @@ -6381,33 +6373,17 @@ void ChainstateManager::RecalculateBestHeader() } } -bool ChainstateManager::ValidatedSnapshotCleanup() +bool ChainstateManager::ValidatedSnapshotCleanup(Chainstate& validated_cs, Chainstate& unvalidated_cs) { AssertLockHeld(::cs_main); - auto get_storage_path = [](auto& chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) -> std::optional { - if (!(chainstate && chainstate->HasCoinsViews())) { - return {}; - } - return chainstate->CoinsDB().StoragePath(); - }; - std::optional ibd_chainstate_path_maybe = get_storage_path(m_ibd_chainstate); - std::optional snapshot_chainstate_path_maybe = get_storage_path(m_snapshot_chainstate); - if (!this->IsSnapshotValidated()) { // No need to clean up. return false; } - // If either path doesn't exist, that means at least one of the chainstates - // is in-memory, in which case we can't do on-disk cleanup. You'd better be - // in a unittest! - if (!ibd_chainstate_path_maybe || !snapshot_chainstate_path_maybe) { - LogError("[snapshot] snapshot chainstate cleanup cannot happen with " - "in-memory chainstates. You are testing, right?"); - return false; - } - const auto& snapshot_chainstate_path = *snapshot_chainstate_path_maybe; - const auto& ibd_chainstate_path = *ibd_chainstate_path_maybe; + const fs::path validated_path{validated_cs.StoragePath()}; + const fs::path assumed_valid_path{unvalidated_cs.StoragePath()}; + const fs::path delete_path{validated_path + "_todelete"}; // Since we're going to be moving around the underlying leveldb filesystem content // for each chainstate, make sure that the chainstates (and their constituent @@ -6421,9 +6397,7 @@ bool ChainstateManager::ValidatedSnapshotCleanup() assert(this->GetAll().size() == 0); LogInfo("[snapshot] deleting background chainstate directory (now unnecessary) (%s)", - fs::PathToString(ibd_chainstate_path)); - - fs::path tmp_old{ibd_chainstate_path + "_todelete"}; + fs::PathToString(validated_path)); auto rename_failed_abort = [this]( fs::path p_old, @@ -6438,32 +6412,32 @@ bool ChainstateManager::ValidatedSnapshotCleanup() }; try { - fs::rename(ibd_chainstate_path, tmp_old); + fs::rename(validated_path, delete_path); } catch (const fs::filesystem_error& e) { - rename_failed_abort(ibd_chainstate_path, tmp_old, e); + rename_failed_abort(validated_path, delete_path, e); throw; } LogInfo("[snapshot] moving snapshot chainstate (%s) to " "default chainstate directory (%s)", - fs::PathToString(snapshot_chainstate_path), fs::PathToString(ibd_chainstate_path)); + fs::PathToString(assumed_valid_path), fs::PathToString(validated_path)); try { - fs::rename(snapshot_chainstate_path, ibd_chainstate_path); + fs::rename(assumed_valid_path, validated_path); } catch (const fs::filesystem_error& e) { - rename_failed_abort(snapshot_chainstate_path, ibd_chainstate_path, e); + rename_failed_abort(assumed_valid_path, validated_path, e); throw; } - if (!DeleteCoinsDBFromDisk(tmp_old, /*is_snapshot=*/false)) { + if (!DeleteCoinsDBFromDisk(delete_path, /*is_snapshot=*/false)) { // No need to FatalError because once the unneeded bg chainstate data is // moved, it will not interfere with subsequent initialization. LogWarning("Deletion of %s failed. Please remove it manually, as the " "directory is now unnecessary.", - fs::PathToString(tmp_old)); + fs::PathToString(delete_path)); } else { LogInfo("[snapshot] deleted background chainstate directory (%s)", - fs::PathToString(ibd_chainstate_path)); + fs::PathToString(validated_path)); } return true; } diff --git a/src/validation.h b/src/validation.h index 6fe31117034..37c6051e5b6 100644 --- a/src/validation.h +++ b/src/validation.h @@ -579,6 +579,9 @@ public: ChainstateManager& chainman, std::optional from_snapshot_blockhash = std::nullopt); + //! Return path to chainstate leveldb directory. + fs::path StoragePath() const; + //! Return the current role of the chainstate. See `ChainstateManager` //! documentation for a description of the different types of chainstates. //! @@ -594,8 +597,7 @@ public: void InitCoinsDB( size_t cache_size_bytes, bool in_memory, - bool should_wipe, - fs::path leveldb_name = "chainstate"); + bool should_wipe); //! Initialize the in-memory coins cache (to be done after the health of the on-disk database //! is verified). @@ -703,9 +705,6 @@ public: //! Destructs all objects related to accessing the UTXO set. void ResetCoinsViews() { m_coins_views.reset(); } - //! Does this chainstate have a UTXO set attached? - bool HasCoinsViews() const { return (bool)m_coins_views; } - //! The cache size of the on-disk coins view. size_t m_coinsdb_cache_size_bytes{0}; @@ -1335,7 +1334,7 @@ public: //! directories are moved or deleted. //! //! @sa node/chainstate:LoadChainstate() - bool ValidatedSnapshotCleanup() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + bool ValidatedSnapshotCleanup(Chainstate& validated_cs, Chainstate& unvalidated_cs) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); //! @returns the chainstate that indexes should consult when ensuring that an //! index is synced with a chain where we can expect block index entries to have