Merge bitcoin/bitcoin#28039: wallet: don't include bdb files from our headers

8b5397c wallet: bdb: include bdb header from our implementation files only (Cory Fields)
6e01062 wallet: bdb: don't use bdb define in header (Cory Fields)
004b184 wallet: bdb: move BerkeleyDatabase constructor to cpp file (Cory Fields)
b3582ba wallet: bdb: move SafeDbt to cpp file (Cory Fields)
e5e5aa1 wallet: bdb: move SpanFromDbt to below SafeDbt's implementation (Cory Fields)
4216f69 wallet: bdb: move TxnBegin to cpp file since it uses a bdb function (Cory Fields)
43369f3 wallet: bdb: drop default parameter (Cory Fields)

Pull request description:

  Only `#include` upstream bdb headers from our cpp files.

  It's generally good practice to avoid including 3rd party deps in headers as otherwise they tend to sneak into new compilation units. IMO this makes for a nice cleanup.

  There's a good bit of code movement here, but each commit is small and _should_ be obviously correct.

  Note: in the future, the buildsystem can add the bdb include path for `bdb.cpp` and `salvage.cpp` only, rather than all wallet sources.

ACKs for top commit:
  achow101:
    reACK 8b5397c
  hebasto:
    ACK 8b5397c

Tree-SHA512: 0ef6e8a9c4c6e2d1e5d6a3534495f91900e4175143911a5848258c56da54535b85fad67b6d573da5f7b96e7881299b5a8ca2327e708f305b317b9a3e85038d66

Author: achow101

src/wallet/bdb.cpp

@@ -18,6 +18,7 @@
 
 #include <stdint.h>
 
+#include <db_cxx.h>
 #include <sys/stat.h>
 
 // Windows may not define S_IRUSR or S_IWUSR. We define both
@@ -29,12 +30,10 @@
 #endif
 #endif
 
+static_assert(BDB_DB_FILE_ID_LEN == DB_FILE_ID_LEN, "DB_FILE_ID_LEN should be 20.");
+
 namespace wallet {
 namespace {
-Span<const std::byte> SpanFromDbt(const SafeDbt& dbt)
-{
-    return {reinterpret_cast<const std::byte*>(dbt.get_data()), dbt.get_size()};
-}
 
 //! Make sure database has a unique fileid within the environment. If it
 //! doesn't, throw an error. BDB caches do not work properly when more than one
@@ -236,6 +235,26 @@ BerkeleyEnvironment::BerkeleyEnvironment() : m_use_shared_memory(false)
     fMockDb = true;
 }
 
+/** RAII class that automatically cleanses its data on destruction */
+class SafeDbt final
+{
+    Dbt m_dbt;
+
+public:
+    // construct Dbt with internally-managed data
+    SafeDbt();
+    // construct Dbt with provided data
+    SafeDbt(void* data, size_t size);
+    ~SafeDbt();
+
+    // delegate to Dbt
+    const void* get_data() const;
+    uint32_t get_size() const;
+
+    // conversion operator to access the underlying Dbt
+    operator Dbt*();
+};
+
 SafeDbt::SafeDbt()
 {
     m_dbt.set_flags(DB_DBT_MALLOC);
@@ -275,6 +294,18 @@ SafeDbt::operator Dbt*()
     return &m_dbt;
 }
 
+static Span<const std::byte> SpanFromDbt(const SafeDbt& dbt)
+{
+    return {reinterpret_cast<const std::byte*>(dbt.get_data()), dbt.get_size()};
+}
+
+BerkeleyDatabase::BerkeleyDatabase(std::shared_ptr<BerkeleyEnvironment> env, fs::path filename, const DatabaseOptions& options) :
+    WalletDatabase(), env(std::move(env)), m_filename(std::move(filename)), m_max_log_mb(options.max_log_mb)
+{
+    auto inserted = this->env->m_databases.emplace(m_filename, std::ref(*this));
+    assert(inserted.second);
+}
+
 bool BerkeleyDatabase::Verify(bilingual_str& errorStr)
 {
     fs::path walletDir = env->Directory();
@@ -462,6 +493,15 @@ void BerkeleyEnvironment::ReloadDbEnv()
     Open(open_err);
 }
 
+DbTxn* BerkeleyEnvironment::TxnBegin(int flags)
+{
+    DbTxn* ptxn = nullptr;
+    int ret = dbenv->txn_begin(nullptr, &ptxn, flags);
+    if (!ptxn || ret != 0)
+        return nullptr;
+    return ptxn;
+}
+
 bool BerkeleyDatabase::Rewrite(const char* pszSkip)
 {
     while (true) {
@@ -742,7 +782,7 @@ bool BerkeleyBatch::TxnBegin()
 {
     if (!pdb || activeTxn)
         return false;
-    DbTxn* ptxn = env->TxnBegin();
+    DbTxn* ptxn = env->TxnBegin(DB_TXN_WRITE_NOSYNC);
     if (!ptxn)
         return false;
     activeTxn = ptxn;

src/wallet/bdb.h

@@ -21,14 +21,21 @@
 #include <unordered_map>
 #include <vector>
 
-#include <db_cxx.h>
-
 struct bilingual_str;
 
+class DbEnv;
+class DbTxn;
+class Db;
+class Dbc;
+
+// This constant was introduced in BDB 4.0.14 and has never changed, but there
+// is a belt-and-suspenders check in the cpp file just in case.
+#define BDB_DB_FILE_ID_LEN 20 /* Unique file ID length. */
+
 namespace wallet {
 
 struct WalletDatabaseFileId {
-    uint8_t value[DB_FILE_ID_LEN];
+    uint8_t value[BDB_DB_FILE_ID_LEN];
     bool operator==(const WalletDatabaseFileId& rhs) const;
 };
 
@@ -67,14 +74,7 @@ class BerkeleyEnvironment
     void CloseDb(const fs::path& filename);
     void ReloadDbEnv();
 
-    DbTxn* TxnBegin(int flags = DB_TXN_WRITE_NOSYNC)
-    {
-        DbTxn* ptxn = nullptr;
-        int ret = dbenv->txn_begin(nullptr, &ptxn, flags);
-        if (!ptxn || ret != 0)
-            return nullptr;
-        return ptxn;
-    }
+    DbTxn* TxnBegin(int flags);
 };
 
 /** Get BerkeleyEnvironment given a directory path. */
@@ -91,12 +91,7 @@ class BerkeleyDatabase : public WalletDatabase
     BerkeleyDatabase() = delete;
 
     /** Create DB handle to real database */
-    BerkeleyDatabase(std::shared_ptr<BerkeleyEnvironment> env, fs::path filename, const DatabaseOptions& options) :
-        WalletDatabase(), env(std::move(env)), m_filename(std::move(filename)), m_max_log_mb(options.max_log_mb)
-    {
-        auto inserted = this->env->m_databases.emplace(m_filename, std::ref(*this));
-        assert(inserted.second);
-    }
+    BerkeleyDatabase(std::shared_ptr<BerkeleyEnvironment> env, fs::path filename, const DatabaseOptions& options);
 
     ~BerkeleyDatabase() override;
 
@@ -159,26 +154,6 @@ class BerkeleyDatabase : public WalletDatabase
     std::unique_ptr<DatabaseBatch> MakeBatch(bool flush_on_close = true) override;
 };
 
-/** RAII class that automatically cleanses its data on destruction */
-class SafeDbt final
-{
-    Dbt m_dbt;
-
-public:
-    // construct Dbt with internally-managed data
-    SafeDbt();
-    // construct Dbt with provided data
-    SafeDbt(void* data, size_t size);
-    ~SafeDbt();
-
-    // delegate to Dbt
-    const void* get_data() const;
-    uint32_t get_size() const;
-
-    // conversion operator to access the underlying Dbt
-    operator Dbt*();
-};
-
 class BerkeleyCursor : public DatabaseCursor
 {
 private:

src/wallet/salvage.cpp

@@ -11,6 +11,8 @@
 #include <wallet/wallet.h>
 #include <wallet/walletdb.h>
 
+#include <db_cxx.h>
+
 namespace wallet {
 /* End of headers, beginning of key/value data */
 static const char *HEADER_END = "HEADER=END";
@@ -175,7 +177,7 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil
         return false;
     }
 
-    DbTxn* ptxn = env->TxnBegin();
+    DbTxn* ptxn = env->TxnBegin(DB_TXN_WRITE_NOSYNC);
     CWallet dummyWallet(nullptr, "", std::make_unique<DummyDatabase>());
     for (KeyValPair& row : salvagedData)
     {