As a new requirement, sqlite support should be optional. This PR aims to be only minimum/blocker changes for 0.21.
Potential follow-up PRs after this:
127@@ -128,6 +128,12 @@ AC_ARG_ENABLE([wallet],
128 [enable_wallet=$enableval],
129 [enable_wallet=yes])
130
131+AC_ARG_WITH([sqlite],
132+ [AS_HELP_STRING([--without-sqlite],
133+ [disable sqlite wallet support (default is yes if wallet is enabled and sqlite is found)])],
(default is no? Or less ambiguous: enabled
Concept ACK.
Sqlite is already optional in depends using NO_SQLITE=1, but configure will still look for it (without this PR).
I prefer to disable the creation of descriptor wallets if this flag is used (createwallet RPC and gui). That gives us the flexibility to use native Sqlite stuff for those wallets in the future, without having to worry about BDB upgrade paths.
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.
Make BDB support optional
Making bdb optional would be more interesting looking forward. But no problem with this either, concept ACK.
Improved configure help per request, and added nicer behaviour in RPC/GUI around descriptor wallet stuff.
(A future PR can remove descriptor wallet code entirely if desired.)
7@@ -8,7 +8,11 @@
8 #include <util/system.h>
9
10 bool ExistsBerkeleyDatabase(const fs::path& path);
11+#ifdef USE_SQLITE
1013@@ -1012,6 +1014,7 @@ std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const Databas
1014 if (ExistsBerkeleyDatabase(path)) {
1015 format = DatabaseFormat::BERKELEY;
1016 }
1017+#ifdef USE_SQLITE
ExistsSQLiteDatabase is false.
ExistsSQLiteDatabase is defined in wallet/sqlite.h.
1056 if (format && format == DatabaseFormat::SQLITE) {
1057 return MakeSQLiteDatabase(path, options, status, error);
1058 }
1059+#endif
1060
1061 return MakeBerkeleyDatabase(path, options, status, error);
format == DatabaseFormat::BERKELEY.
assert(format != DatabaseFormat::SQLITE); in an #else
378 wallet/walletutil.cpp \
379 wallet/coinselection.cpp \
380 $(BITCOIN_CORE_H)
381
382+if USE_SQLITE
383+libbitcoin_wallet_a_SOURCES += wallet/sqlite.cpp
wallet/sqlite.h be included here too?
sqlite3.h header would then fail to compile.
33@@ -34,6 +34,11 @@ 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);
setChecked(false). When descriptor wallets become the default and this is default checked, that will be necessary.
127@@ -128,6 +128,12 @@ AC_ARG_ENABLE([wallet],
128 [enable_wallet=$enableval],
129 [enable_wallet=yes])
130
131+AC_ARG_WITH([sqlite],
132+ [AS_HELP_STRING([--without-sqlite],
133+ [disable sqlite wallet support (default is enabled if wallet is enabled and sqlite is found)])],
5c19bd0f4b893a91c02e652ac8f45ef0ccdeca1c
Mind avoiding negative form in help string:
0 [AS_HELP_STRING([--with-sqlite=yes|no|auto],
1 [enable sqlite wallet support (default: auto, i.e., enabled if wallet is enabled and sqlite is found)])],
?
1234+ if test "x$use_sqlite" != "xno"; then
1235+ PKG_CHECK_MODULES([SQLITE], [sqlite3 >= 3.7.17], [have_sqlite=yes], [have_sqlite=no])
1236+ fi
1237+ AC_MSG_CHECKING([whether to build wallet with support for sqlite])
1238+ if test "x$use_sqlite" = "xno"; then
1239+ use_sqlite=no
5c19bd0f4b893a91c02e652ac8f45ef0ccdeca1c
is this a tautology?
355@@ -356,7 +356,7 @@ endif
356
357 # wallet: shared between bitcoind and bitcoin-qt, but only linked
358 # when wallet enabled
359-libbitcoin_wallet_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)
360+libbitcoin_wallet_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(SQLITE_CFLAGS)
5c19bd0f4b893a91c02e652ac8f45ef0ccdeca1c
Nice!
During reviewing #19077 I’ve noticed that the SQLITE_CFLAGS is empty, and I was not worried about adding it to the libbitcoin_wallet_a_CPPFLAGS. But consistency is good, it is not guaranteed the SQLITE_CFLAGS will be empty always :)
Would detecting an sqlite wallet (look for the sqlite magic) make sense to give more specific errors in case someone tries to load a sqlite wallet with a non-sqlite-supporting bitcoind/qt? (could be a follow up PR is complicated).
Current error in the deamon…
02020-10-20T14:17:40Z Error: "/tmp/dummy/regtest/wallets/test/wallet.dat" corrupt. Try using the wallet tool bitcoin-wallet to salvage or restoring a backup.
1Error: "/tmp/dummy/regtest/wallets/test/wallet.dat" corrupt. Try using the wallet tool bitcoin-wallet to salvage or restoring a backup.
and the GUI:
ACK bbb42a68961a8ae1e4ef3221ffb1d5ff7272b075, tested on Linux Mint 20 (x86_64, Qt 5.12.8).
Also tested with uninstalled libsqlite3-dev package.
Labels
Wallet
Build system
Milestone
0.21.0