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

8b5397c00e821d7eaab22f512e9d71a1a0392ebf wallet: bdb: include bdb header from our implementation files only (Cory Fields)
6e010626af7ed51f1748323ece2f46335e145f2f wallet: bdb: don't use bdb define in header (Cory Fields)
004b184b027520a4f9019d1432a816e6ec891fe3 wallet: bdb: move BerkeleyDatabase constructor to cpp file (Cory Fields)
b3582baa3a2f84db7d2fb5a681121a5f2d6de3a1 wallet: bdb: move SafeDbt to cpp file (Cory Fields)
e5e5aa1da261633c8f73b97d5aefe5dc450a7db9 wallet: bdb: move SpanFromDbt to below SafeDbt's implementation (Cory Fields)
4216f69250937b1ca4650dc0c21678a8444c6650 wallet: bdb: move TxnBegin to cpp file since it uses a bdb function (Cory Fields)
43369f37060a1b4c987672707c500d35c9a27c1d 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 8b5397c00e821d7eaab22f512e9d71a1a0392ebf
  hebasto:
    ACK 8b5397c00e821d7eaab22f512e9d71a1a0392ebf

Tree-SHA512: 0ef6e8a9c4c6e2d1e5d6a3534495f91900e4175143911a5848258c56da54535b85fad67b6d573da5f7b96e7881299b5a8ca2327e708f305b317b9a3e85038d66

3 files changed, 60 insertions, 43 deletions

diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp
index 658500e456..9ea43ca67c 100644
--- a/src/wallet/bdb.cpp
+++ b/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;
diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h
index 1073d32e0f..630630ebe0 100644
--- a/src/wallet/bdb.h
+++ b/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 @@ public:
     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 @@ public:
     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 @@ public:
     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:
diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp
index da16435f04..0a0745b1c5 100644
--- a/src/wallet/salvage.cpp
+++ b/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)
     {