Require that sqlite is available in order to compile the wallet. Removes instances of USE_SQLITE since it is no longer possible to not have sqlite available.
The NO_SQLITE option is dropped from depends.
This is another step towards dropping the legacy wallet, extracted from #31250.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31961.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | m3dwards, hebasto, davidgumberg |
| Concept ACK | jonatack, rkrux |
| Stale ACK | achow101, furszy, brunoerg, maflcko |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
bitcoin-chainstate.exe to bitcoinkernel.dll on Windows by hebasto)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.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/37949959629
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
189eb8c in #31250](https://github.com/bitcoin/bitcoin/pull/31250/commits/189eb8c02b5b81fe3239df34cd9ef48871e2e5de)
7@@ -8,9 +8,10 @@ target_sources(fuzz
8 coinselection.cpp
9 crypter.cpp
10 fees.cpp
11- $<$<BOOL:${USE_SQLITE}>:${CMAKE_CURRENT_LIST_DIR}/notifications.cpp>
12- $<$<BOOL:${USE_SQLITE}>:${CMAKE_CURRENT_LIST_DIR}/scriptpubkeyman.cpp>
13+ notifications.cpp
14+ scriptpubkeyman.cpp
15 spend.cpp
16+ scriptpubkeyman.cpp
There are a few leftovers. Genereally, when removing stuff, you can use git grep to find leftovers: git grep WITH_SQLITE. This also makes me wonder why CI passed. I would have expected for depends builds a warning:
0CMake Warning:
1 Manually-specified variables were not used by the project:
2 WITH_SQLITE
Otherwise:
review ACK 26b588534f4f9ec15a8a10a423a5ba120c290428 🏜
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 26b588534f4f9ec15a8a10a423a5ba120c290428 🏜
3z75OOe3pHhQhnLx+3fUpPrOCOg0KngxTHqyR73Yfm3HOmia6Ex9XuuxN+fT10x/FcOV8xOBfBhuYK22MBnowDA==
7@@ -8,8 +8,7 @@ target_sources(fuzz
8 coinselection.cpp
9 crypter.cpp
10 fees.cpp
11- $<$<BOOL:${USE_SQLITE}>:${CMAKE_CURRENT_LIST_DIR}/notifications.cpp>
12- $<$<BOOL:${USE_SQLITE}>:${CMAKE_CURRENT_LIST_DIR}/scriptpubkeyman.cpp>
13+ notifications.cpp
14 spend.cpp
15 wallet_bdb_parser.cpp
16 )
now you removed both. Neither 2, nor 0 are correct. It would be good to have 1.
after you push a commit to github, you can review the diff yourself. This should help to spot obvious errors
commit bafba0658c33e88664326f0b94a170f55bf7b5bd is wrong 🏃
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: commit bafba0658c33e88664326f0b94a170f55bf7b5bd is wrong 🏃
3ocigj6g1m6WwMWXkk2SejazINqjyzq2RfdL1XdRULy9jkKlNNWH2A8yZQsONssjlwtN46BpdeR3jMlInu5c+Dg==
32@@ -33,7 +33,6 @@ def set_test_params(self):
33
34 def skip_test_if_missing_module(self):
35 self.skip_if_no_wallet()
36- self.skip_if_no_sqlite()
37 self.skip_if_no_py_sqlite3()
Should this skip_if_no_py_sqlite3() call be removed as well? Right now, the wallet_descriptor.py test will be skipped if the python sqlite3 package is unavailable. I would assume that if the wallet is enabled (for which skip_if_no_wallet() 2 lines above is sufficient), I would want this test to run all the time instead of being skipped conditionally.
What do others think?
True.
My point was if the wallet is enabled, I would prefer to have this test run always for which this python package should be installed as well. But I now realise it is a developer’s POV.
Not removing this call would be acceptable if we assume not everyone who compile the wallet would want to run the functional tests, which I guess could be a big enough group.
if we assume not everyone who compile the wallet would want to run the functional tests
Probably the vast majority.
src/wallet/test/fuzz/CMakeLists.txt
NO_SQLITE=1 makes no sense now, doesn’t it?
Building depends with
NO_SQLITE=1makes no sense now, doesn’t it?
Makes sense if you don’t want the wallet?
Building depends with
NO_SQLITE=1makes no sense now, doesn’t it?Makes sense if you don’t want the wallet?
0$ make -C depends NO_SQLITE=1
1$ cmake -B build --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake
2-- The CXX compiler identification is GNU 14.2.0
3-- Detecting CXX compiler ABI info
4-- Detecting CXX compiler ABI info - done
5-- Check for working CXX compiler: /usr/bin/g++ - skipped
6-- Detecting CXX compile features
7-- Detecting CXX compile features - done
8-- Setting build type to "RelWithDebInfo" as none was specified
9-- Performing Test CXX_SUPPORTS__WERROR
10-- Performing Test CXX_SUPPORTS__WERROR - Success
11-- Performing Test CXX_SUPPORTS__G3
12-- Performing Test CXX_SUPPORTS__G3 - Success
13-- Performing Test LINKER_SUPPORTS__G3
14-- Performing Test LINKER_SUPPORTS__G3 - Success
15-- Performing Test CXX_SUPPORTS__FTRAPV
16-- Performing Test CXX_SUPPORTS__FTRAPV - Success
17-- Performing Test LINKER_SUPPORTS__FTRAPV
18-- Performing Test LINKER_SUPPORTS__FTRAPV - Success
19CMake Error at /usr/share/cmake-3.30/Modules/FindPackageHandleStandardArgs.cmake:233 (message):
20 Could NOT find SQLite3 (missing: SQLite3_INCLUDE_DIR SQLite3_LIBRARY)
21 (Required is at least version "3.7.17")
22Call Stack (most recent call first):
23 /usr/share/cmake-3.30/Modules/FindPackageHandleStandardArgs.cmake:603 (_FPHSA_FAILURE_MESSAGE)
24 /usr/share/cmake-3.30/Modules/FindSQLite3.cmake:66 (find_package_handle_standard_args)
25 CMakeLists.txt:104 (find_package)
26
27
28-- Configuring incomplete, errors occurred!
UPDATE: If I don’t want the wallet, I’d use NO_WALLET=1, right?
I could change depends here, but maybe it’s easier to do that in the final PR that drops BDB itself?
Changes to depends shouldn’t be big. Otherwise, this statement from the PR description:
… it is no longer possible to not have sqlite available.
does not always hold (see a counter-example in #31961 (comment)).
but maybe it’s easier to do that in the final PR that drops BDB itself?
I don’t see why, and it just leaves things broken/confusing in the interim. If we are going to move forward with this PR, the NO_SQLITE depends option should be removed at the same time.
No strong opinion about depends (follow-up or here), otherwise:
review ACK ac6cde731314d981391bc313c98d671c68211d33 🐥
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK ac6cde731314d981391bc313c98d671c68211d33 🐥
3YFBdupE6IpnooWx+yEIosPIsIuWjfHGldFwOz4f6Fk1WUyjq1TikqGtqynFbWDQwsdgUk7MC3GyqNO8G+z+RBA==
I initially considered dropping the NO_WALLET option from depends. Most other depends packages refer to the library name rather than the feature, so this seemed better than dropping NO_SQLITE.
However it could break automations (and habits) of anyone who tries to build the node without wallet support. So I ended up dropping NO_SQLITE and keeping NO_WALLET.
Also rebased after #31161 just in case.
Concept ACK 47e5ed3a5ba21049a9a8f3c4c44023e82e6ab88e
I’ve checked that there are no more instances present of the following make options, and WITH_SQLITE doesn’t appear anymore in the output of ccmake build.
0➜ bitcoin git:(2025/02/sqlite) ✗ git grep "USE_SQLITE"
1➜ bitcoin git:(2025/02/sqlite) ✗ git grep "WITH_SQLITE"
2➜ bitcoin git:(2025/02/sqlite) ✗ git grep "NO_SQLITE"
Since the PR is rebased after the #31161 changes, I rm-rf’ed my build directory before configuring and building core and wallet again. I’m able to run bitcoind and bitcoin-cli successfully from the new ./build/bin directory.
I also checked by explicitly passing WITH_SQLITE=ON again while configuring and noticed a warning like below.
0➜ bitcoin git:(2025/02/sqlite) ✗ cmake -B build -DWITH_SQLITE=ON -DWITH_BDB=OFF -LH
1
2-- Generating done (0.8s)
3CMake Warning:
4 Manually-specified variables were not used by the project:
5
6 WITH_SQLITE
However, the NO_WALLET=1 option (used like below) seemed to have no effect and I was able to use the wallet still. PLMK if I used this option incorrectly. It was only after I used ENABLE_WALLET=OFF that I was not able to use the wallet RPCs and instead received the Method not found error via the CLI.
0cmake -B build -DNO_WALLET=1 -LH
Edit: I think this^ is not the right way because it is not a CMake build option, trying like below instead.
0 make -C depends NO_WALLET=1
Edit 2: The NO_WALLET works fine as well when used while building depends like below:
0make -C depends NO_WALLET=1 NO_QT=1 NO_QR=1
1cmake -B build --toolchain depends/x86_64-apple-darwin24.3.0/toolchain.cmake
2cmake --build build -j 13
However, the
NO_WALLET=1option (used like below) seemed to have no effect and I was able to use the wallet still. PLMK if I used this option incorrectly. It was only after I usedENABLE_WALLET=OFFthat I was not able to use the wallet RPCs.0cmake -B build -DNO_WALLET=1 -LH
NO_WALLET=1 is dedicated to be used when building depends: make -C depends NO_WALLET=1.
Approach ACK 47e5ed3a5ba21049a9a8f3c4c44023e82e6ab88e.
Here are two suggestions:
0--- a/src/wallet/CMakeLists.txt
1+++ b/src/wallet/CMakeLists.txt
2@@ -28,6 +28,7 @@ add_library(bitcoin_wallet STATIC EXCLUDE_FROM_ALL
3 rpc/wallet.cpp
4 scriptpubkeyman.cpp
5 spend.cpp
6+ sqlite.cpp
7 transaction.cpp
8 wallet.cpp
9 walletdb.cpp
10@@ -37,17 +38,13 @@ target_link_libraries(bitcoin_wallet
11 PRIVATE
12 core_interface
13 bitcoin_common
14+ $<TARGET_NAME_IF_EXISTS:unofficial::sqlite3::sqlite3>
15+ $<TARGET_NAME_IF_EXISTS:SQLite::SQLite3>
16 univalue
17 Boost::headers
18 $<TARGET_NAME_IF_EXISTS:USDT::headers>
19 )
20
21-target_sources(bitcoin_wallet PRIVATE sqlite.cpp)
22-target_link_libraries(bitcoin_wallet
23- PRIVATE
24- $<TARGET_NAME_IF_EXISTS:unofficial::sqlite3::sqlite3>
25- $<TARGET_NAME_IF_EXISTS:SQLite::SQLite3>
26-)
27 if(USE_BDB)
28 target_sources(bitcoin_wallet PRIVATE bdb.cpp salvage.cpp)
29 target_link_libraries(bitcoin_wallet PUBLIC BerkeleyDB::BerkeleyDB)
vcpkg.json as well: 0--- a/vcpkg.json
1+++ b/vcpkg.json
2@@ -11,6 +11,7 @@
3 "qt5",
4 "tests",
5 "wallet",
6+ "berkeleydb",
7 "zeromq"
8 ],
9 "features": {
10@@ -28,12 +29,6 @@
11 "libqrencode"
12 ]
13 },
14- "sqlite": {
15- "description": "Enable SQLite wallet support",
16- "dependencies": [
17- "sqlite3"
18- ]
19- },
20 "tests": {
21 "description": "Build test_bitcoin.exe executable",
22 "dependencies": [
23@@ -41,9 +36,8 @@
24 ]
25 },
26 "wallet": {
27- "description": "Enable wallet",
28+ "description": "Enable wallet (SQLite)",
29 "dependencies": [
30- "berkeleydb",
31 "sqlite3"
32 ]
33 },
Taken both suggestions.
@hebasto native fuzz build is unhappy
0--- a/.github/workflows/ci.yml
1+++ b/.github/workflows/ci.yml
2@@ -171,7 +171,7 @@ jobs:
3 generate-options: '-DBUILD_GUI=ON -DWITH_BDB=ON -DWITH_ZMQ=ON -DBUILD_BENCH=ON -DWERROR=ON'
4 job-name: 'Win64 native, VS 2022'
5 - job-type: fuzz
6- generate-options: '-DVCPKG_MANIFEST_NO_DEFAULT_FEATURES=ON -DVCPKG_MANIFEST_FEATURES="sqlite" -DBUILD_GUI=OFF -DBUILD_FOR_FUZZING=ON -DWERROR=ON'
7+ generate-options: '-DVCPKG_MANIFEST_NO_DEFAULT_FEATURES=ON -DVCPKG_MANIFEST_FEATURES="wallet" -DBUILD_GUI=OFF -DBUILD_FOR_FUZZING=ON -DWERROR=ON'
8 job-name: 'Win64 native fuzz, VS 2022'
9
10 steps:
136-if("${wallet_packages}" STREQUAL "" OR "${sqlite_packages}" STREQUAL "")
137- set(WITH_SQLITE OFF CACHE BOOL "")
138-else()
139- set(WITH_SQLITE ON CACHE BOOL "")
140-endif()
141-
A corresponding change in Makefile may be done:
0--- a/depends/Makefile
1+++ b/depends/Makefile
2@@ -233,7 +233,6 @@ $(host_prefix)/toolchain.cmake : toolchain.cmake.in $(host_prefix)/.stamp_$(fina
3 -e 's|@zmq_packages@|$(zmq_packages_)|' \
4 -e 's|@wallet_packages@|$(wallet_packages_)|' \
5 -e 's|@bdb_packages@|$(bdb_packages_)|' \
6- -e 's|@sqlite_packages@|$(sqlite_packages_)|' \
7 -e 's|@usdt_packages@|$(usdt_packages_)|' \
8 -e 's|@no_harden@|$(NO_HARDEN)|' \
9 -e 's|@multiprocess@|$(MULTIPROCESS)|' \
159@@ -161,7 +160,7 @@ qrencode_packages_$(NO_QR) = $(qrencode_$(host_os)_packages)
160 qt_packages_$(NO_QT) = $(qt_packages) $(qt_$(host_os)_packages) $(qt_$(host_arch)_$(host_os)_packages) $(qrencode_packages_)
161
162 bdb_packages_$(NO_BDB) = $(bdb_packages)
163-sqlite_packages_$(NO_SQLITE) = $(sqlite_packages)
164+sqlite_packages_$(NO_WALLET) = $(sqlite_packages)
165 wallet_packages_$(NO_WALLET) = $(bdb_packages_) $(sqlite_packages_)
This can be simplified:
0wallet_packages_$(NO_WALLET) = $(bdb_packages_) $(sqlite_packages)
ACK 564c54f3371b6faf50be2c7e7b6280357c90e62c.
Left a couple of nits and the following notes.
For some BSD systems, we document how to build BDB in depends. Now, it builds sqlite along with bdb. I don’t believe it’s critical, especially with BDB being removed soon.
re comment:
I initially considered dropping the
NO_WALLEToption from depends. Most other depends packages refer to the library name rather than the feature, so this seemed better than droppingNO_SQLITE.
This alignes with the design as it was before sqlite was introduced in depends. However, in general, depends shouldn’t be aware of the resulting software features.
Maybe @ryanofsky could provide more feedback on this?
Require that sqlite is available in order to compile the wallet. Removes
instances of USE_SQLITE since it is no longer possible to not have
sqlite available.
The NO_SQLITE option is dropped from depends.
Co-authored-by: Ava Chow <github@achow101.com>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Built with and without depends and all was working
I initially considered dropping the
NO_WALLEToption from depends. Most other depends packages refer to the library name rather than the feature, so this seemed better than droppingNO_SQLITE.However it could break automations (and habits) of anyone who tries to build the node without wallet support. So I ended up dropping
NO_SQLITEand keepingNO_WALLET.
As someone who doesn’t have those habits my vote would have been for dropping NO_WALLET. I think of depends in terms of libraries, not bitcoin functionality.
I assume this check in src/leveldb/CMakeLists.txt still makes sense rather than checking for ENABLE_WALLET?
0 check_library_exists(sqlite3 sqlite3_open "" HAVE_SQLITE3)
1 if(HAVE_SQLITE3)
2 leveldb_benchmark("benchmarks/db_bench_sqlite3.cc")
3 target_link_libraries(db_bench_sqlite3 sqlite3)
4 endif(HAVE_SQLITE3)
NO_WALLET option for depends has existed since at least v0.11: https://github.com/bitcoin/bitcoin/tree/v0.11.0/depends
@m3dwards src/leveldb is a subtree, so the code there is not aware of Bitcoin Core concepts like a wallet.
