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

We place stdlib include headers after the ones to our codebase.

Are there any written guidelines on this? I couldn't find anything in the developer notes and noticed both conventions being used, e.g. blockfilter.h for the opposite. Browsing through some more header files, it seems most indeed include system headers last, so I'll update it.

Could be a good addition to https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#source-code-organization if it's generally-agreed practice

Agreed, would be a good addition. Updated all 4 index header files to import system headers last.

Can the temporary here (and in txindex.h) be omitted? (also, why static)

We need to return a const std::string& reference, so the underlying object can't be destroyed after the function returns, which is achieved with the static. I don't know of a better way to implement this but I'm open to suggestions?

Ah, right.

<details><summary>A slightly different approach, for what it's worth (tested with a clang 16 debug build)</summary><p>

diff --git a/src/index/coinstatsindex.h b/src/index/coinstatsindex.h
index 91a30fb9d9..609bbcdab6 100644
--- a/src/index/coinstatsindex.h
+++ b/src/index/coinstatsindex.h
@@ -19,6 +19,7 @@
 class CoinStatsIndex final : public BaseIndex
 {
 private:
+    const std::string index_name{"coinstatsindex"};
     std::string m_name;
     std::unique_ptr<BaseIndex::DB> m_db;
 
@@ -51,11 +52,7 @@ protected:
 
     BaseIndex::DB& GetDB() const override { return *m_db; }
 
-    const std::string& GetName() const override
-    {
-        static std::string name{"coinstatsindex"};
-        return name;
-    }
+    const std::string& GetName() const override { return index_name; }
 
 public:
     // Constructs the index, which becomes available to be queried.
diff --git a/src/index/txindex.h b/src/index/txindex.h
index cf0ecf498f..ec6ea1d55b 100644
--- a/src/index/txindex.h
+++ b/src/index/txindex.h
@@ -20,6 +20,7 @@ protected:
     class DB;
 
 private:
+    const std::string index_name{"txindex"};
     const std::unique_ptr<DB> m_db;
 
     bool AllowPrune() const override { return false; }
@@ -29,11 +30,7 @@ protected:
 
     BaseIndex::DB& GetDB() const override;
 
-    const std::string& GetName() const override
-    {
-        static std::string name{"txindex"};
-        return name;
-    }
+    const std::string& GetName() const override { return index_name; }
 
 public:

(Edit: making the index_name definitions const is optional)

</p></details>

No strong feelings, but I'm not sure it's an improvement:

• doesn't reduce the amount of code (disregarding newlines)
• moves the name further away from the only place where it's used, so imo slightly harder to read
• requires index_name to be initialized for every instance of the class instead of just once

So with that in mind, I think I'd rather keep it as is?

It may also be possible to return a string_view (from a lifetime perspective this is no different than a reference), which works for strings and char-string literals.

string_view should work with tinyformat as well as std::string::operator+ (edit: not operator+, but append)

I considered that, but are you sure about string_view having implemented operator+? It's not in the reference. Mostly because of the lack of concatenation, I thought it'd be best for these getters to use a const string& to not make too many assumptions around how the return values will be used.

Edit: to clarify, for example in util::ThreadRename, when passed a string_view you wouldn't be able to concatenate "b-" + name, would have to be "b-" + name.data() https://github.com/bitcoin/bitcoin/blob/fa5c224d444802dabec5841009e029b9754c92f1/src/util/threadnames.cpp#L59-L63

That's not a huge issue on its own, but I felt the const string& approach was a bit cleaner partly for that reason.

You can't concatenate string_views, as string_views are pointer/length references to continuous characters in memory. The concatenation of two is not continuous anymore.

That doesn't matter though; a string_view can be implicitly converted to a string, so concatenating a string to a string_view will yield a string. EDIT: I'm wrong.

That said, I don't think string_view is much of a benefit here; it's not like these names get sliced. See my suggestion below.

<details><summary>Another approach :smile: </summary><p>

diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp
index b9029e946a..a554cd4e3b 100644
--- a/src/index/coinstatsindex.cpp
+++ b/src/index/coinstatsindex.cpp
@@ -23,6 +23,7 @@ using node::UndoReadFromDisk;
 static constexpr uint8_t DB_MUHASH{'M'};
+static const std::string index_name{"coinstatsindex"};
 
@@ -392,6 +393,8 @@ bool CoinStatsIndex::CustomInit(const std::optional<interfaces::BlockKey>& block
 
+const std::string& CoinStatsIndex::GetName() const { return index_name; }
+
 bool CoinStatsIndex::CustomCommit(CDBBatch& batch)
diff --git a/src/index/coinstatsindex.h b/src/index/coinstatsindex.h
index 91a30fb9d9..54fff61e53 100644
--- a/src/index/coinstatsindex.h
+++ b/src/index/coinstatsindex.h
@@ -51,11 +51,7 @@ protected:
 
-    const std::string& GetName() const override
-    {
-        static std::string name{"coinstatsindex"};
-        return name;
-    }
+    const std::string& GetName() const override;
 
 public:
     // Constructs the index, which becomes available to be queried.
diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp
index b719aface8..489bcd53c0 100644
--- a/src/index/txindex.cpp
+++ b/src/index/txindex.cpp
@@ -12,6 +12,7 @@
 constexpr uint8_t DB_TXINDEX{'t'};
+static const std::string index_name{"txindex"};
  
@@ -54,6 +55,8 @@ TxIndex::TxIndex(std::unique_ptr<interfaces::Chain> chain, size_t n_cache_size,
 
+const std::string& TxIndex::GetName() const { return index_name; }
+
 bool TxIndex::CustomAppend(const interfaces::BlockInfo& block)
 {
     // Exclude genesis block transaction because outputs are not spendable.
diff --git a/src/index/txindex.h b/src/index/txindex.h
index cf0ecf498f..c54ad562d8 100644
--- a/src/index/txindex.h
+++ b/src/index/txindex.h
@@ -29,11 +29,7 @@ protected:
 
-    const std::string& GetName() const override
-    {
-        static std::string name{"txindex"};
-        return name;
-    }
+    const std::string& GetName() const override;
 
 public:
 ``
</p></details>

Edit: to clarify, for example in util::ThreadRename, when passed a string_view you wouldn't be able to concatenate "b-" + name, would have to be "b-" + name.data()

"b-" + name.data() is undefined behaviour if it compiles.

I was thinking that you'd be able to write "b-"s + name, but I was wrong.

Edit: To clarify https://en.cppreference.com/w/cpp/string/basic_string/append does accept it, so you could write

std::string{"b-"}.append(name)

Thank you for the input everyone, much appreciated!

I've incorporated sipa's suggestion to move the name logic to BaseIndex, which I think is an elegant solution and should hopefully also satisfy jonatack's concerns? The only downside to this approach I see is that there is some code duplication in the BlockFilterIndex constructor as we need to derive the filter name from the filter type, but it's minimal enough imo.

I've also updated the thread name parameters to string_view wherever it made sense.

so you could write...

Thanks for the clarification! The call to SetInternalName() on the next line also requires a string, so I think it's best to just expect a string type as parameter (reverting it to what it was before).

I think this addresses everyone's concerns voiced here, but I'll leave the comment open for a bit longer so people can react if I misunderstood anything.

Closing this thread as resolved

void util::TraceThread(std::string thread_name, std::function<void()> thread_func)

I think the overhead of creating a thread is much higher than creating a copy of a few bytes. And having not to think about lifetime (or run into lifetime issues in never expected corner cases) just isn't worth the questionable benefits here that are given by a char* or string_view.

I agree, thanks, good catch. Made it a const std::string instead, and since we're already copying here I re-reverted the logic for ThreadRename() to use an lvalue ref again.

You probably want a const std::string& to avoid a copy.

Do you mean for util::TraceThread()? I think we explicitly want a copy here to not have to worry about lifetime as per MarcoFalke's comment? The copying I mentioned was further downstream for when we pass thread_name to util::ThreadRename(), where indeed a const std::string& is used to avoid copying again.

Is there a reference stored to this thread_name variable somewhere? If not, there cannot be any lifetime issues.

TraceThread does not return before the end of the thread, so the reference is implicitly stored as a bound argument for the whole duration of the thread, no?

If TraceThread doesn't return before the thread is done executing, then the object the caller references can't disappear before the thread exits (because the caller doesn't even get to run) and there cannot be any lifetime issues.

@MarcoFalke if you agree I'll revert to string_view again, I'm not feeling very confident here but what sipa says makes sense to me. At the very least, it shouldn't introduce any new lifetime issues compared to what we had before with const char*?

Oh, I see. The arguments to std::thread are copied, unless they are passed as std::ref ( https://en.cppreference.com/w/cpp/thread/thread/thread )

So const std::string& is indeed fine here, as it is a reference to the copy that was created by the std::thread constructor. (However, std::string should be fine as well and come with only the overhead equivalent to constructing an empty std::string)

For reference, the following gives an AddressSanitizer: stack-use-after-scope, because the argument was passed with std::ref, but does not if the argument is not passed with std::ref.

<details><summary>`std::thread`/`std::ref` test</summary>

#include <chrono>
#include <iostream>
#include <thread>

void fun(const std::string &a) {
  std::this_thread::sleep_for(std::chrono::milliseconds{10});
  std::cout << a << std::endl;
}

int main() {
  std::thread a;
  {
    std::string s{"s"};
    a = std::thread{fun, std::ref(s)};
  }
  a.join();
}

</details>

To clarify, anything is fine here, but string_view can lead to lifetime issues if there is a leftover caller that uses c_str on a temporary string. For example, this is UB:

#include <chrono>
#include <iostream>
#include <thread>

void fun(std::string_view a) {
  std::this_thread::sleep_for(std::chrono::milliseconds{10});
  std::cout << a << std::endl;
}

int main() {
  std::thread a;
  {
    std::string s{"s"};
    a = std::thread{fun, s.c_str()};
  }
  a.join();
}

Thanks for the background info MarcoFalke, that's really helpful.

can lead to lifetime issues if there is a leftover caller that uses c_str on a temporary string

Would you agree that that would just be bad implementation, and not something we should expect downstream functions to try and fix?

Previously passing a std::string was only possible by calling c_str() on it, so this UB may persist in a very unlikely and unfortunate silent merge conflict. (Changing const char* to std::string_view shouldn't introduce any new UB, but it wouldn't fix it either, if it existed previously). Though, again that seems unlikely so I think anything is fine here and this is almost a style question.

FWIW, in our code base, all util::TraceThread callers pass either a string literal (in most cases) or a BaseIndex::GetName() return value of const std::string& type (the only case).

Isn't it the case that we don't need the #include <string> include here or in txindex.h or coinstatsindex.h?

Yeah thanks, good catch. Removed all 3 includes, leftover from previous versions of the PR.

Nit: the prefix in "pszThread" means "Pointer to String, Zero-terminated". We don't use that naming style anymore in new code, and while it's ok to leave it as is to avoid needing to touch code all over the place, please don't leave the name when its actual meaning actually mismatches the code.

Oh that's good to know, thanks. I've added 26cf9ea to fix that with a scripted-diff.

nit: Why #include <utility>?

std::move (Edit: Oh yeah. That one isn't used.)

$ git grep move -- src//util/thread.cpp | wc -l
0

iwyu:

util/thread.h should add these lines:
#include <string_view>  // for string_view

util/thread.h should remove these lines:
- #include <string>  // lines 9-9

The full include-list for util/thread.h:
#include <functional>   // for function
#include <string_view>  // for string_view
---

util/thread.cpp should add these lines:

util/thread.cpp should remove these lines:
- #include <utility>  // lines 14-14

The full include-list for util/thread.cpp:
#include <util/thread.h>
#include <logging.h>           // for LogPrintf_, LogPrintf
#include <util/system.h>       // for PrintExceptionContinue
#include <util/threadnames.h>  // for ThreadRename
#include <exception>           // for exception
#include <functional>          // for function
#include <string>              // for allocator, string
---