wallet: Make BDB support optional #20202

achow101 · 2020-10-20T16:31:12Z

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.

hebasto · 2020-10-20T16:39:50Z

Concept ACK.

build-aux/m4/bitcoin_find_bdb48.m4

configure.ac

src/wallet/init.cpp

src/wallet/salvage.cpp

luke-jr · 2020-10-20T16:46:30Z

src/wallet/walletdb.cpp

For a followup PR...

Should we be testing with both mock dbs when bdb+sqlite are built?

src/qt/createwalletdialog.cpp

practicalswift · 2020-10-20T19:18:01Z

Concept ACK

DrahtBot · 2020-10-21T01:24:38Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: List all wallets in non-SQLite and non-BDB builds #20275 (wallet: List SQLite wallets in non-SQLite builds by ryanofsky)
wallet, refactor: Include headers instead of function declarations #20206 (wallet, refactor: Include headers instead of function declarations by hebasto)
build: pkg-config related cleanup #20201 (build: pkg-config related cleanup by hebasto)
wallettool: Add dump and createfromdump commands #19137 (wallettool: Add dump and createfromdump commands by achow101)
refactor: Remove CAddressBookData::destdata #18608 (refactor: Remove CAddressBookData::destdata by ryanofsky)
External signer support - Wallet Box edition #16546 (External signer support - Wallet Box edition by Sjors)
Install icon & .desktop file to XDG data #15068 (Install icon & .desktop file to XDG data by luke-jr)

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.

Sjors

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.

Sjors · 2020-10-23T11:51:22Z

configure.ac

bb93bf607b9d83218cff562e54f6bd00505c3ec7: nit: move = to the right a bit:

with wallet = yes with sqlite = yes with bdb = no

achow101 · 2020-10-23T16:56:22Z

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.

luke-jr · 2020-10-24T19:38:53Z

IMO PRs shouldn't be merged with tests broken...

maflcko · 2020-10-25T07:33:04Z

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]

achow101 · 2020-10-26T21:09:19Z

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.

laanwj · 2020-11-23T09:29:09Z

Code review ACK d52f502

Sjors · 2020-11-23T10:15:08Z

This is missing a line in config.ini.in (@USE_BDB_TRUE@USE_BDB=true) and an is_bdb_compiled helper for functional tests. Done in a followup #20458

b87caf1 test: add is_bdb_compiled helper (Sjors Provoost) Pull request description: Followup for #20202, needed by #16546. Allow the functional test suite to skip tests that require BDB, as well as introduce specific logic to handle whether BDB support is enabled or not. It follows the same pattern as `skip_if_no_sqlite` and `is_sqlite_compiled`. ACKs for top commit: laanwj: Code review ACK b87caf1 Tree-SHA512: e84fb22e017b28f0f75d17e5368fcba22a893484b31b12082cfe9354e6fbd566daf34b3b82f7deb7205b2061c9c61538e402df000e2f05428affae6dbea05c5e

RonSherfey

How can I be sure which wallet to add?

Github-Pull: bitcoin#20202 Rebased-From: b33af48

Github-Pull: bitcoin#20202 Rebased-From: a58b719

Github-Pull: bitcoin#20202 Rebased-From: 6ebc41b

Github-Pull: bitcoin#20202 Rebased-From: 71e40b3

Github-Pull: bitcoin#20202 Rebased-From: ee47f11

Github-Pull: bitcoin#20202 Rebased-From: 99309ab

Github-Pull: bitcoin#20202 Rebased-From: d52f502

luke-jr suggested changes Oct 20, 2020

View reviewed changes

achow101 force-pushed the opt-sqlite-bdb branch from 28457c4 to e4a48df Compare October 20, 2020 17:46

luke-jr reviewed Oct 20, 2020

View reviewed changes

src/qt/createwalletdialog.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the opt-sqlite-bdb branch from e4a48df to bb93bf6 Compare October 20, 2020 18:06

DrahtBot added Build system Docs GUI RPC/REST/ZMQ Wallet labels Oct 20, 2020

DrahtBot mentioned this pull request Oct 21, 2020

wallet, refactor: Include headers instead of function declarations #20206

Closed

Sjors reviewed Oct 23, 2020

View reviewed changes

maflcko removed Docs GUI RPC/REST/ZMQ labels Oct 25, 2020

achow101 force-pushed the opt-sqlite-bdb branch 2 times, most recently from d23454f to 7f518a7 Compare October 26, 2020 21:10

achow101 force-pushed the opt-sqlite-bdb branch from 7b275da to ba0f769 Compare November 13, 2020 17:10

DrahtBot added the Needs rebase label Nov 17, 2020

achow101 force-pushed the opt-sqlite-bdb branch from ba0f769 to d972780 Compare November 17, 2020 18:19

DrahtBot added Needs rebase and removed Needs rebase labels Nov 17, 2020

achow101 added 7 commits November 18, 2020 11:55

Include wallet/bdb.h where it is actually being used

b33af48

Do not compile BDB things when USE_BDB is defined

a58b719

Enforce salvage is only for BDB wallets

6ebc41b

RPC: Require descriptors=True for createwallet when BDB is not compiled

71e40b3

GUI: Force descriptor wallets when BDB is not compiled

ee47f11

Allow disabling BDB in configure with --without-bdb

99309ab

Fix mock SQLiteDatabases

d52f502

achow101 force-pushed the opt-sqlite-bdb branch from d972780 to d52f502 Compare November 18, 2020 16:56

DrahtBot removed the Needs rebase label Nov 18, 2020

laanwj merged commit 86bf3ae into bitcoin:master Nov 23, 2020

Sjors mentioned this pull request Nov 23, 2020

test: add is_bdb_compiled helper #20458

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 23, 2020

Merge bitcoin#20202: wallet: Make BDB support optional

43d1fbb

jonasschnelli mentioned this pull request Nov 24, 2020

Don't set BDB flags when configuring without #20478

Merged

RonSherfey reviewed Feb 19, 2021

View reviewed changes

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

Include wallet/bdb.h where it is actually being used

1537dde

Github-Pull: bitcoin#20202 Rebased-From: b33af48

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

Do not compile BDB things when USE_BDB is defined

74c50e8

Github-Pull: bitcoin#20202 Rebased-From: a58b719

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

Enforce salvage is only for BDB wallets

7b7e486

Github-Pull: bitcoin#20202 Rebased-From: 6ebc41b

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

RPC: Require descriptors=True for createwallet when BDB is not compiled

950ac8b

Github-Pull: bitcoin#20202 Rebased-From: 71e40b3

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

GUI: Force descriptor wallets when BDB is not compiled

bc4a2d2

Github-Pull: bitcoin#20202 Rebased-From: ee47f11

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

Allow disabling BDB in configure with --without-bdb

c8694ff

Github-Pull: bitcoin#20202 Rebased-From: 99309ab

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

Fix mock SQLiteDatabases

e0e1484

Github-Pull: bitcoin#20202 Rebased-From: d52f502

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

wallet: Make BDB support optional #20202

wallet: Make BDB support optional #20202

Uh oh!

Conversation

achow101 commented Oct 20, 2020

Uh oh!

hebasto commented Oct 20, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

luke-jr Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

practicalswift commented Oct 20, 2020

Uh oh!

DrahtBot commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Sjors left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 commented Oct 23, 2020

Uh oh!

luke-jr commented Oct 24, 2020

Uh oh!

maflcko commented Oct 25, 2020

Uh oh!

achow101 commented Oct 26, 2020

Uh oh!

laanwj commented Nov 23, 2020

Uh oh!

Sjors commented Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RonSherfey left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

DrahtBot commented Oct 21, 2020 •

edited

Loading

Sjors left a comment •

edited

Loading

Sjors commented Nov 23, 2020 •

edited

Loading