Adds a --without-bdb option to configure which disables the compilation of the BDB stuff. Legacy wallets will not be created when BDB is not compiled. A legacy-sqlite wallet can be loaded, but we will not create them.
Based on #20156 to resolve the situation where both --without-sqlite and --without-bdb are provided. In that case, the wallet is disabled and --disable-wallet is effectively set.
77@@ -71,8 +78,12 @@ AC_DEFUN([BITCOIN_FIND_BDB48],[
78 ])
79 done
80 if test "x$BDB_LIBS" = "x"; then
81- AC_MSG_ERROR([libdb_cxx missing, ]AC_PACKAGE_NAME[ requires this library for wallet functionality (--disable-wallet to disable wallet functionality)])
82+ AC_MSG_ERROR([libdb_cxx missing, ]AC_PACKAGE_NAME[ requires this library for BDB wallet functionality (--without-bdb to disable BDB wallet functionality)])
1256+ fi
1257+ AC_MSG_RESULT([$use_sqlite])
1258+
1259+ dnl Disable wallet if both --without-bdb and --without-sqlite
1260+ if test "x$use_bdb$use_sqlite" = "xnono"; then
1261+ enable_wallet=no
--enable-wallet is explicitly specified, this should AC_MSG_ERROR
75 argsman.AddArg("-dblogsize=<n>", strprintf("Flush wallet database activity from memory to disk log every <n> megabytes (default: %u)", DEFAULT_WALLET_DBLOGSIZE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::WALLET_DEBUG_TEST);
76- argsman.AddArg("-flushwallet", strprintf("Run a thread to flush wallet periodically (default: %u)", DEFAULT_FLUSHWALLET), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::WALLET_DEBUG_TEST);
77 argsman.AddArg("-privdb", strprintf("Sets the DB_PRIVATE flag in the wallet db environment (default: %u)", DEFAULT_WALLET_PRIVDB), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::WALLET_DEBUG_TEST);
78+#endif
79+
80+ argsman.AddArg("-flushwallet", strprintf("Run a thread to flush wallet periodically (default: %u)", DEFAULT_FLUSHWALLET), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::WALLET_DEBUG_TEST);
12 #include <wallet/salvage.h>
13 #include <wallet/wallet.h>
14 #include <wallet/walletdb.h>
15
16+#ifndef USE_BDB
17+// If BDB is not compiled, we don't have a salvage
1076@@ -1064,5 +1077,9 @@ std::unique_ptr<WalletDatabase> CreateDummyWalletDatabase()
1077 /** Return object for accessing temporary in-memory database. */
1078 std::unique_ptr<WalletDatabase> CreateMockWalletDatabase()
1079 {
1080+#ifdef USE_BDB
1081 return MakeUnique<BerkeleyDatabase>(std::make_shared<BerkeleyEnvironment>(), "");
1082+#elif USE_SQLITE
For a followup PR…
Should we be testing with both mock dbs when bdb+sqlite are built?
33@@ -34,6 +34,15 @@ CreateWalletDialog::CreateWalletDialog(QWidget* parent) :
34 ui->disable_privkeys_checkbox->setChecked(false);
35 }
36 });
37+
38+#ifndef USE_SQLITE
39+ ui->descriptor_checkbox->setToolTip(tr("Compiled without sqlite support (required for descriptor wallets)"));
40+ ui->descriptor_checkbox->setEnabled(false);
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
1761@@ -1746,6 +1762,7 @@ echo " multiprocess = $build_multiprocess"
1762 echo " with wallet = $enable_wallet"
1763 if test "x$enable_wallet" != "xno"; then
1764 echo " with sqlite = $use_sqlite"
1765+ echo " with bdb = $use_bdb"
bb93bf607b9d83218cff562e54f6bd00505c3ec7: nit: move = to the right a bit:
0 with wallet = yes
1 with sqlite = yes
2 with bdb = no
Concept ACK. Tested bb93bf607b9d83218cff562e54f6bd00505c3ec7 on macOS.
This breaks a whole bunch of functional tests. It would be better if those are skipped.
Having this configuration on one of the CI machines makes sense to me as well. As we work towards #20160 all tests should (also) use descriptor wallets. Perhaps merging #18788 first makes that easier.
This breaks a whole bunch of functional tests. It would be better if those are skipped.
Having this configuration on one of the CI machines makes sense to me as well. As we work towards #20160 all tests should (also) use descriptor wallets. Perhaps merging #18788 first makes that easier.
It’d be a lot easier if tests used the MiniWallet. I think we can do those as a followup.
0C:\projects\bitcoin\src\wallet\walletdb.cpp(1087): error C4716: 'CreateMockWalletDatabase': must return a value [C:\projects\bitcoin\build_msvc\test_bitcoin-qt\test_bitcoin-qt.vcxproj]
1LINK : fatal error LNK1257: code generation failed [C:\projects\bitcoin\build_msvc\test_bitcoin-qt\test_bitcoin-qt.vcxproj]
2db_tests.obj : error LNK2001: unresolved external symbol "class std::shared_ptr<class BerkeleyEnvironment> __cdecl GetWalletEnv(class boost::filesystem::path const &,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > &)" (?GetWalletEnv@@YA?AV?$shared_ptr@VBerkeleyEnvironment@@@std@@AEBVpath@filesystem@boost@@AEAV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@2@@Z) [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
3C:\projects\bitcoin\build_msvc\x64\Release\test_bitcoin.exe : fatal error LNK1120: 1 unresolved externals [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
Instead of prompting users to specify descriptors=True, how would people feel about just defaulting to descriptors=True and making all newly created wallets descriptor-sqlite wallets when BDB is not compiled?
I think that this would be really helpful in our test framework to ensure that tests that shouldn’t be restricted to one wallet type do pass with both. So we can stop making tests that are dependent on legacy wallet behavior.
IMO that should wait for descriptor wallets to leave experimental status.
Descriptor wallets also shouldn’t change behaviour vs legacy…
IMO that should wait for descriptor wallets to leave experimental status.
Agree. If you accidentally build without BDB support (e.g. by not knowing that it’s necessary for non-experimental wallets), you shouldn’t be automatically opted into an experimental feature.
Descriptor wallets also shouldn’t change behaviour vs legacy…
Not sure what you mean here. In many ways, descriptor wallets were created exactly because some behavior of legacy wallets is just insane and the only way to avoid it was creating a new type of wallet for it, so something better can be introduced without being bound by compatibility.
2748@@ -2749,6 +2749,12 @@ static RPCHelpMan createwallet()
2749 warnings.emplace_back(Untranslated("Wallet is an experimental descriptor wallet"));
2750 }
2751
2752+#ifndef USE_BDB
In commit “RPC: Require descriptors=True for createwallet when BDB is not compiled” (d2781aa656cce419700ba57a4551a999756750a4)
I think it would be good to drop the #ifndef USE_BDB here and the #ifndef USE_SQLITE a few lines above by just writing:
0options.require_format = wallet_creation_flags & WALLET_FLAG_DESCRIPTORS ? DatabaseFormat::SQLITE : DatabaseFormat::BERKELEY;
in CreateWallet. It would be good for not having so many ifdefs, not having to hardcode the same database format assumption so many places, and having a separation of concerns where MakeDatabase callers don’t need to figure out what kind of databases MakeDatabase can create before calling it
125@@ -126,14 +126,20 @@ AC_ARG_ENABLE([wallet],
126 [AS_HELP_STRING([--disable-wallet],
127 [disable wallet (enabled by default)])],
128 [enable_wallet=$enableval],
129- [enable_wallet=yes])
130+ [enable_wallet=auto])
In commit “Allow disabling BDB in configure with –without-bdb” (26102beed00e51b3bb1e263287028d3e4b9d85c5)
Is this change intentional? Commit description only says that this is adding a new option, not that this is changing default build behavior and automatically building without legacy wallet support if BDB is not found. Similarly, I’d also expect use_bdb value below to be default yes instead of default auto to continue requiring BDB, unless user explicitly requests to build without it, or requests to build without the wallet.
Ignore this comment if I’m misinterpreting and there is no change in default behavior, but otherwise I think it’d be better to just either restore previous behavior and require BDB unless explicitly disabled, or document somewhere (commit message or release notes) that bitcoin by default may be now built without BDB support or any wallet support if libraries aren’t detected.
This doesn’t change any behavior. I needed to do this to tell when the user explicitly does --enable-wallet. In that case, an error needs to be given when combined with --without-bdb --without-sqlite. Previously, enable_wallet would always be yes so it wasn’t possible to automatically do enable_wallet=no when --without-bdb --without-sqlite without potentially confusing the user (it would then disable the wallet if they had done --enable-wallet).
Using auto lets us default to yes, but also know if the user has set the value instead of letting it be the default.
./configure and no options bitcoin would build without legacy wallet support if BDB wasn’t found? And after this change it still does that? Or that before this change plain ./configure errored if it couldn’t find BDB and still does that? Either behavior seems fine (though second behavior seems less error prone). But if you have a commit changing default behavior it should say it’s changing default build behavior, not just say it’s adding a new option
This doesn’t change default behavior. This only effects a new behavior. The only thing this effects is the case where the user does ./configure --enable-wallet --without-bdb --without-sqlite. With this change, we throw the error wallet functionality requestsed but no BDB or SQLite support available. Without this change, we would compile without wallet support.
As before, if BDB is not found, configure would error.
bitcoin_find_bdb48.m4 doesn’t understand what “auto” means. It treats everything that isn’t “no” as “yes”
1091@@ -1072,5 +1092,9 @@ std::unique_ptr<WalletDatabase> CreateDummyWalletDatabase()
1092 /** Return object for accessing temporary in-memory database. */
1093 std::unique_ptr<WalletDatabase> CreateMockWalletDatabase()
1094 {
1095+#ifdef USE_BDB
1096 return MakeUnique<BerkeleyDatabase>(std::make_shared<BerkeleyEnvironment>(), "");
1097+#elif USE_SQLITE
1098+ return MakeUnique<SQLiteDatabase>("", "", true);
In commit “Do not compile BDB things when USE_BDB is defined” (8475409860d68e41e136129b59678fe1320cfb44)
It would be good to order commit “Fix mock SQLiteDatabases” (7b275da4608adc66ed6a7bbb1b807ca6dcb598e9) before this commit so mock databases aren’t broken between commits by this call.
Also just a note in case other reviewers were curious like me but an "" database filename will create a unnamed temporary database (https://www.sqlite.org/inmemorydb.html). ":memory:" could also be used to create a database guaranteed to be in memory and never written to disk, but "" seems most appropriate here
70@@ -68,9 +71,14 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const
71 #endif
72 argsman.AddArg("-walletrbf", strprintf("Send transactions with full-RBF opt-in enabled (RPC only, default: %u)", DEFAULT_WALLET_RBF), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
73
74+#ifdef USE_BDB
75 argsman.AddArg("-dblogsize=<n>", strprintf("Flush wallet database activity from memory to disk log every <n> megabytes (default: %u)", DEFAULT_WALLET_DBLOGSIZE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::WALLET_DEBUG_TEST);
76 argsman.AddArg("-flushwallet", strprintf("Run a thread to flush wallet periodically (default: %u)", DEFAULT_FLUSHWALLET), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::WALLET_DEBUG_TEST);
77 argsman.AddArg("-privdb", strprintf("Sets the DB_PRIVATE flag in the wallet db environment (default: %u)", DEFAULT_WALLET_PRIVDB), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::WALLET_DEBUG_TEST);
78+#else
79+ argsman.AddHiddenArgs({"-dblogsize", "-flushwallet", "-privdb"});
-flushwallet and maybe -privdb?
39@@ -40,6 +40,10 @@ CreateWalletDialog::CreateWalletDialog(QWidget* parent) :
40 ui->descriptor_checkbox->setEnabled(false);
41 ui->descriptor_checkbox->setChecked(false);
42 #endif
43+#ifndef USE_BDB
44+ ui->descriptor_checkbox->setEnabled(false);
45+ ui->descriptor_checkbox->setChecked(true);
46+#endif
1761@@ -1745,6 +1762,7 @@ echo " multiprocess = $build_multiprocess"
1762 echo " with wallet = $enable_wallet"
1763 if test "x$enable_wallet" != "xno"; then
1764 echo " with sqlite = $use_sqlite"
1765+ echo " with bdb = $use_bdb"
135 [use_sqlite=$withval],
136 [use_sqlite=auto])
137
138+AC_ARG_WITH([bdb],
139+ [AS_HELP_STRING([--without-bdb],
140+ [disable bdb wallet support (default is enabled if wallet is enabled and bdb is found)])],
Since we error when BDB is missing, the help should just be “default is enabled if wallet is enabled”
0 [disable bdb wallet support (default is enabled if wallet is enabled)])],
