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.
Concept ACK.
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)])
Nit: s/functionality/support/
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
If --enable-wallet is explicitly specified, this should AC_MSG_ERROR
Done
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);
What does this do without BDB?
Nothing, so I've also disabled it.
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
Prefer to not compile the file at all...
Done
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);
What happened to setChecked(false)?
Concept ACK
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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:
with wallet = yes
with sqlite = yes
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.
IMO PRs shouldn't be merged with tests broken...
C:\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]
LINK : fatal error LNK1257: code generation failed [C:\projects\bitcoin\build_msvc\test_bitcoin-qt\test_bitcoin-qt.vcxproj]
db_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]
C:\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.
Ok, true, most behaviour shouldn't change though :)
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:
options.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
This was done to have reasonable error messages. Can be cleaned up in a followup.
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.
Are you saying before this change with 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.
Thanks. Apparently this works because 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
Code review ACK 7b275da4608adc66ed6a7bbb1b807ca6dcb598e9. Comments are minor and could be ignored or addressed later
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"});
Also needed for -flushwallet and maybe -privdb?
Not quite sure what you're asking.
Me either, I think I somehow saw a different line here...
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
Since descriptor wallets are still experimental, I think it might be better to leave the checkbox alone (enabled + default off) but make the button give an error until the user explicitly checks it?
I think that UX would be frustrating to use because there's only one true option in that case, but you're pretending there's 2. Users will still get the experimental warning when the wallet is created.
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"
Should we list bdb first since it's older?
I don't think it matters.
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"
[disable bdb wallet support (default is enabled if wallet is enabled)])],
Done.
Code review ACK d52f502b1ea1cafa7d58c5517f01dba26ecb7269
How can I be sure which wallet to add?