Merge bitcoin/bitcoin#25971: refactor: Use std::string for thread and index names

26cf9ea8e4 scripted-diff: rename pszThread to thread_name (stickies-v)
200d84d568 refactor: use std::string for index names (stickies-v)
97f5b20c12 refactor: use std::string for thread names (stickies-v)

Pull request description:

  As a follow-up to https://github.com/bitcoin/bitcoin/pull/25967#discussion_r959637189, this PR changes the return type of [`BaseIndex::GetName()`](fa5c224d44/src/index/base.h (L120)) to `const std::string&` instead of `const char*`. The first commit is not essential for this change, but since the code is touched and index names are commonly used to specify thread names, I've made the same update there.

  No behaviour change, just refactoring to further phase out C-style strings.

  Note: `util::ThreadRename()` used to take an rvalue ref, but since it then passes this to `SetInternalName()` by value, I don't think there's any benefit to having both an rvalue and lvalue ref function so I just changed it into lvalue ref. Not 100% sure I'm missing something?

ACKs for top commit:
  MarcoFalke:
    review ACK 26cf9ea8e4 only change is new scripted-diff 😀
  hebasto:
    ACK 26cf9ea8e4, I have reviewed the code and it looks OK.
  w0xlt:
    reACK 26cf9ea8e4

Tree-SHA512: 44a03ebf2bb86ca1411a36222a575217cdba8ee3a3c985e74d74c934516f002b27336147fa22f59eda7dac21204a93951563317005d475da95b23c427014d77b

src/index/base.cpp

@@ -18,6 +18,9 @@
 #include <validation.h> // For g_chainman
 #include <warnings.h>
 
+#include <string>
+#include <utility>
+
 using node::ReadBlockFromDisk;
 
 constexpr uint8_t DB_BEST_BLOCK{'B'};
@@ -62,8 +65,8 @@ void BaseIndex::DB::WriteBestBlock(CDBBatch& batch, const CBlockLocator& locator
     batch.Write(DB_BEST_BLOCK, locator);
 }
 
-BaseIndex::BaseIndex(std::unique_ptr<interfaces::Chain> chain)
-    : m_chain{std::move(chain)} {}
+BaseIndex::BaseIndex(std::unique_ptr<interfaces::Chain> chain, std::string name)
+    : m_chain{std::move(chain)}, m_name{std::move(name)} {}
 
 BaseIndex::~BaseIndex()
 {

src/index/base.h

@@ -10,6 +10,8 @@
 #include <threadinterrupt.h>
 #include <validationinterface.h>
 
+#include <string>
+
 class CBlock;
 class CBlockIndex;
 class Chainstate;
@@ -95,6 +97,7 @@ private:
 protected:
     std::unique_ptr<interfaces::Chain> m_chain;
     Chainstate* m_chainstate{nullptr};
+    const std::string m_name;
 
     void BlockConnected(const std::shared_ptr<const CBlock>& block, const CBlockIndex* pindex) override;
 
@@ -117,13 +120,13 @@ protected:
     virtual DB& GetDB() const = 0;
 
     /// Get the name of the index for display in logs.
-    virtual const char* GetName() const = 0;
+    const std::string& GetName() const LIFETIMEBOUND { return m_name; }
 
     /// Update the internal best block index as well as the prune lock.
     void SetBestBlockIndex(const CBlockIndex* block);
 
 public:
-    BaseIndex(std::unique_ptr<interfaces::Chain> chain);
+    BaseIndex(std::unique_ptr<interfaces::Chain> chain, std::string name);
     /// Destructor interrupts sync thread if running and blocks until it exits.
     virtual ~BaseIndex();
 

src/index/blockfilterindex.cpp

@@ -97,7 +97,8 @@ static std::map<BlockFilterType, BlockFilterIndex> g_filter_indexes;
 
 BlockFilterIndex::BlockFilterIndex(std::unique_ptr<interfaces::Chain> chain, BlockFilterType filter_type,
                                    size_t n_cache_size, bool f_memory, bool f_wipe)
-    : BaseIndex(std::move(chain)), m_filter_type(filter_type)
+    : BaseIndex(std::move(chain), BlockFilterTypeName(filter_type) + " block filter index")
+    , m_filter_type(filter_type)
 {
     const std::string& filter_name = BlockFilterTypeName(filter_type);
     if (filter_name.empty()) throw std::invalid_argument("unknown filter_type");
@@ -105,7 +106,6 @@ BlockFilterIndex::BlockFilterIndex(std::unique_ptr<interfaces::Chain> chain, Blo
     fs::path path = gArgs.GetDataDirNet() / "indexes" / "blockfilter" / fs::u8path(filter_name);
     fs::create_directories(path);
 
-    m_name = filter_name + " block filter index";
     m_db = std::make_unique<BaseIndex::DB>(path / "db", n_cache_size, f_memory, f_wipe);
     m_filter_fileseq = std::make_unique<FlatFileSeq>(std::move(path), "fltr", FLTR_FILE_CHUNK_SIZE);
 }

src/index/blockfilterindex.h

@@ -26,7 +26,6 @@ class BlockFilterIndex final : public BaseIndex
 {
 private:
     BlockFilterType m_filter_type;
-    std::string m_name;
     std::unique_ptr<BaseIndex::DB> m_db;
 
     FlatFilePos m_next_filter_pos;
@@ -52,8 +51,6 @@ protected:
 
     BaseIndex::DB& GetDB() const LIFETIMEBOUND override { return *m_db; }
 
-    const char* GetName() const LIFETIMEBOUND override { return m_name.c_str(); }
-
 public:
     /** Constructs the index, which becomes available to be queried. */
     explicit BlockFilterIndex(std::unique_ptr<interfaces::Chain> chain, BlockFilterType filter_type,

src/index/coinstatsindex.cpp

@@ -105,7 +105,7 @@ struct DBHashKey {
 std::unique_ptr<CoinStatsIndex> g_coin_stats_index;
 
 CoinStatsIndex::CoinStatsIndex(std::unique_ptr<interfaces::Chain> chain, size_t n_cache_size, bool f_memory, bool f_wipe)
-    : BaseIndex(std::move(chain))
+    : BaseIndex(std::move(chain), "coinstatsindex")
 {
     fs::path path{gArgs.GetDataDirNet() / "indexes" / "coinstats"};
     fs::create_directories(path);

src/index/coinstatsindex.h

@@ -20,7 +20,6 @@ struct CCoinsStats;
 class CoinStatsIndex final : public BaseIndex
 {
 private:
-    std::string m_name;
     std::unique_ptr<BaseIndex::DB> m_db;
 
     MuHash3072 m_muhash;
@@ -52,8 +51,6 @@ protected:
 
     BaseIndex::DB& GetDB() const override { return *m_db; }
 
-    const char* GetName() const override { return "coinstatsindex"; }
-
 public:
     // Constructs the index, which becomes available to be queried.
     explicit CoinStatsIndex(std::unique_ptr<interfaces::Chain> chain, size_t n_cache_size, bool f_memory = false, bool f_wipe = false);

src/index/txindex.cpp

@@ -49,7 +49,7 @@ bool TxIndex::DB::WriteTxs(const std::vector<std::pair<uint256, CDiskTxPos>>& v_
 }
 
 TxIndex::TxIndex(std::unique_ptr<interfaces::Chain> chain, size_t n_cache_size, bool f_memory, bool f_wipe)
-    : BaseIndex(std::move(chain)), m_db(std::make_unique<TxIndex::DB>(n_cache_size, f_memory, f_wipe))
+    : BaseIndex(std::move(chain), "txindex"), m_db(std::make_unique<TxIndex::DB>(n_cache_size, f_memory, f_wipe))
 {}
 
 TxIndex::~TxIndex() = default;

src/index/txindex.h

@@ -27,8 +27,6 @@ protected:
 
     BaseIndex::DB& GetDB() const override;
 
-    const char* GetName() const override { return "txindex"; }
-
 public:
     /// Constructs the index, which becomes available to be queried.
     explicit TxIndex(std::unique_ptr<interfaces::Chain> chain, size_t n_cache_size, bool f_memory = false, bool f_wipe = false);

src/qt/guiutil.cpp

@@ -982,7 +982,7 @@ void PrintSlotException(
     std::string description = sender->metaObject()->className();
     description += "->";
     description += receiver->metaObject()->className();
-    PrintExceptionContinue(exception, description.c_str());
+    PrintExceptionContinue(exception, description);
 }
 
 void ShowModalDialogAsynchronously(QDialog* dialog)

src/util/system.cpp

@@ -831,7 +831,7 @@ std::string HelpMessageOpt(const std::string &option, const std::string &message
            std::string("\n\n");
 }
 
-static std::string FormatException(const std::exception* pex, const char* pszThread)
+static std::string FormatException(const std::exception* pex, std::string_view thread_name)
 {
 #ifdef WIN32
     char pszModule[MAX_PATH] = "";
@@ -841,15 +841,15 @@ static std::string FormatException(const std::exception* pex, const char* pszThr
 #endif
     if (pex)
         return strprintf(
-            "EXCEPTION: %s       \n%s       \n%s in %s       \n", typeid(*pex).name(), pex->what(), pszModule, pszThread);
+            "EXCEPTION: %s       \n%s       \n%s in %s       \n", typeid(*pex).name(), pex->what(), pszModule, thread_name);
     else
         return strprintf(
-            "UNKNOWN EXCEPTION       \n%s in %s       \n", pszModule, pszThread);
+            "UNKNOWN EXCEPTION       \n%s in %s       \n", pszModule, thread_name);
 }
 
-void PrintExceptionContinue(const std::exception* pex, const char* pszThread)
+void PrintExceptionContinue(const std::exception* pex, std::string_view thread_name)
 {
-    std::string message = FormatException(pex, pszThread);
+    std::string message = FormatException(pex, thread_name);
     LogPrintf("\n\n************************\n%s\n", message);
     tfm::format(std::cerr, "\n\n************************\n%s\n", message);
 }

src/util/system.h

@@ -51,7 +51,7 @@ bool error(const char* fmt, const Args&... args)
     return false;
 }
 
-void PrintExceptionContinue(const std::exception *pex, const char* pszThread);
+void PrintExceptionContinue(const std::exception* pex, std::string_view thread_name);
 
 /**
  * Ensure file contents are fully committed to disk, using a platform-specific

src/util/thread.cpp

@@ -10,10 +10,12 @@
 
 #include <exception>
 #include <functional>
+#include <string>
+#include <utility>
 
-void util::TraceThread(const char* thread_name, std::function<void()> thread_func)
+void util::TraceThread(std::string_view thread_name, std::function<void()> thread_func)
 {
-    util::ThreadRename(thread_name);
+    util::ThreadRename(std::string{thread_name});
     try {
         LogPrintf("%s thread start\n", thread_name);
         thread_func();

src/util/thread.h

@@ -6,12 +6,13 @@
 #define BITCOIN_UTIL_THREAD_H
 
 #include <functional>
+#include <string>
 
 namespace util {
 /**
  * A wrapper for do-something-once thread functions.
  */
-void TraceThread(const char* thread_name, std::function<void()> thread_func);
+void TraceThread(std::string_view thread_name, std::function<void()> thread_func);
 
 } // namespace util