build: Allow BDB between 4.8 and 5.3 without --with-incompatible-bdb

achow101 commented at 8:36 pm on May 4, 2020: member

As noted in #18844 (comment), BDB versions 4.7 through 5.3 inclusive use the same database file format, so the wallet.dat files are compatible between these versions. The thing that is not compatible is the database transaction logs, but those are cleaned up on a clean wallet close. As such, allow people to build with those versions of BDB without needing to pass --with-incompatible-bdb. The docs have been updated to reflect this change. They now no longer mention that BDB 4.8 is required. The caveat about the transaction logs is mentioned.

This does not change the version of BDB used in the depends system, so we will still ship with BDB 4.8.

The search order for BDB was also changed to prefer system BDB and newer versions of BDB in general.

One issue I noticed while testing this was that it configure would pick up my system BDB instead of the depends BDB. It’s possible that this could effect gitian builds, which would be a bug. Someone more familiar with our build system should check that.

DrahtBot added the label Build system on May 4, 2020

DrahtBot added the label Docs on May 4, 2020

DrahtBot added the label Tests on May 4, 2020

in doc/build-unix.md:89 in 51c215b86a outdated

89-BerkeleyDB 5.1 or later. This will break binary wallet compatibility with the distributed executables, which
90-are based on BerkeleyDB 4.8. If you do not care about wallet compatibility,
91-pass `--with-incompatible-bdb` to configure.
92+Ubuntu and Debian have their own `libdb-dev` and `libdb++-dev` packages, which install BerkeleyDB 5.3.
93+
94+If you use BerkeleyDB 6.0 or later, the database file will be incompatible with the distributed executables.

luke-jr commented at 10:10 pm on May 4, 2020:

Add …" and you will be legally required to publish the source code for your node, and modify it to inform peers and RPC clients where to download it."

Or maybe just say 6.0 isn’t supported (and reject it in configure?).

luke-jr commented at 10:11 pm on May 4, 2020:

(This is due to BDB 6.0+ being only available under the AGPL-3 license)

achow101 commented at 0:08 am on May 5, 2020:

That’s a good point. I’ve dropped --with--incompatible-bdb and just made 6.0+ not a valid option.

MarcoFalke removed the label Docs on May 4, 2020

MarcoFalke removed the label Tests on May 4, 2020

in doc/dependencies.md:8 in 51c215b86a outdated

4@@ -5,7 +5,7 @@ These are the dependencies currently used by Bitcoin Core. You can find instruct
5 
6 | Dependency | Version used | Minimum required | CVEs | Shared | [Bundled Qt library](https://doc.qt.io/qt-5/configure-options.html#third-party-libraries) |
7 | --- | --- | --- | --- | --- | --- |
8-| Berkeley DB | [4.8.30](https://www.oracle.com/technetwork/database/database-technologies/berkeleydb/downloads/index.html) | 4.8.x | No |  |  |
9+| Berkeley DB | [4.8.30](https://www.oracle.com/technetwork/database/database-technologies/berkeleydb/downloads/index.html) | 4.7.x | No |  |  |

MarcoFalke commented at 10:19 pm on May 4, 2020:

Any reason to lower the minimum? I am not aware of a single user or even operating system that provides this version. The only versions that we have been testing in the past are 4.8, 5.1 and 5.3

sipa commented at 10:24 pm on May 4, 2020:

I am not aware of a single user or even operating system that provides this version.

You mean you don’t want to retain compatibility with Bitcoin Core releases before 0.4.0 (when the BDB dependency was changed from 4.7 to 4.8)? :p

achow101 commented at 11:13 pm on May 4, 2020:

It was done just for completeness since 4.7 does technically work.

achow101 commented at 0:08 am on May 5, 2020:

I’ve set it back to 4.8 as the minimum.

Sjors commented at 1:27 pm on May 8, 2020:

Thanks, that seems safer given that Travis only lets us test new versions of BDB against the 4.8 hardcode in previous releases, and I don’t see us adding 0.4.0 to the previous release test suite (though that would be cool).

meshcollider commented at 11:42 pm on May 4, 2020: contributor

Concept ACK

MarcoFalke added the label Needs gitian build on May 4, 2020

MarcoFalke added the label Needs Guix build on May 4, 2020

achow101 force-pushed on May 5, 2020

MarcoFalke removed the label Needs Guix build on May 5, 2020

MarcoFalke removed the label Needs gitian build on May 5, 2020

DrahtBot commented at 2:40 pm on May 5, 2020: member

Guix builds

File	commit e727c2bdcab3660297f452c76c6f877038015c02(master)	commit df33f7052fa6dd032ca01e97c5366afec3303c56(master and this pull)
bitcoin-0.20.99-aarch64-linux-gnu-debug.tar.gz	`c399df47e9ec0e56...`	`8bee652478d1f571...`
bitcoin-0.20.99-aarch64-linux-gnu.tar.gz	`44aa4050694c96d7...`	`5d293a9b39694e4f...`
bitcoin-0.20.99-arm-linux-gnueabihf-debug.tar.gz	`9555f2ad0ac8f751...`	`ebe6b344a3f3dcfd...`
bitcoin-0.20.99-arm-linux-gnueabihf.tar.gz	`a37230b57c5e499f...`	`f5d7f28079a6bc1e...`
bitcoin-0.20.99-riscv64-linux-gnu-debug.tar.gz	`39606a20b108dc67...`	`599f35117cb2c444...`
bitcoin-0.20.99-riscv64-linux-gnu.tar.gz	`d06065d714dfdb25...`	`2c980abb89561ce0...`
bitcoin-0.20.99-win-unsigned.tar.gz	`5c104b1cd366503d...`	`7b097a9bfa54722a...`
bitcoin-0.20.99-win64-debug.zip	`11d40ebfb3b7be95...`	`7cdea93f4d1b3f73...`
bitcoin-0.20.99-win64-setup-unsigned.exe	`4d00a16783cad5b0...`	`4d00a16783cad5b0...`
bitcoin-0.20.99-win64.zip	`0e6a98356677bc90...`	`e4ea46211b358d2b...`
bitcoin-0.20.99-x86_64-linux-gnu-debug.tar.gz	`153b88575e3fd0cb...`	`1eb0089b3c53211b...`
bitcoin-0.20.99-x86_64-linux-gnu.tar.gz	`8c344dc2d78153d4...`	`f05b5c3462befc61...`
bitcoin-0.20.99.tar.gz	`10a74457cc435f3e...`	`e8c680c33f441bf1...`
guix_build.log	`73f2f1b3a47d6517...`	`de159edd3c4bc9d0...`
guix_build.log.diff		`27ea66b4691c3ccd...`

practicalswift commented at 2:52 pm on May 5, 2020: contributor

Super strong concept ACK

As contributors we’re used to passing --with-incompatible-bdb, but for a new user compiling Bitcoin Core from scratch the first time it is really unintuitive that the standard ./configure && make routine is expected to fail (for nearly all default configs) :)

in doc/build-unix.md:167 in 0599cf835f outdated

170-```shell
171-./contrib/install_db4.sh `pwd`
172-```
173 
174-from the root of the repository.
175+BerkeleyDB 4.7 through 5.3 (inclusive) can be used. Most linux distributions ship BerkeleyDB 5.3.

jonatack commented at 6:49 pm on May 5, 2020:

nit: s/linux/Linux/

achow101 commented at 3:44 pm on May 7, 2020:

Done

in doc/build-unix.md:85 in 0599cf835f outdated

81@@ -82,12 +82,11 @@ Now, you can either build from self-compiled [depends](/depends/README.md) or in
82 
83     sudo apt-get install libevent-dev libboost-system-dev libboost-filesystem-dev libboost-test-dev libboost-thread-dev
84 
85-BerkeleyDB is required for the wallet.
86+BerkeleyDB is required for the wallet. Releases ship with BerkeleyDB 4.8, but any version from 4.7 to 5.3 can be used with minimal compatibility issues.

jonatack commented at 6:52 pm on May 5, 2020:

s/Releases/Bitcoin Core releases/ (to avoid confusion with BDB releases)

achow101 commented at 3:44 pm on May 7, 2020:

Done

in doc/build-unix.md:89 in 0599cf835f outdated

89-BerkeleyDB 5.1 or later. This will break binary wallet compatibility with the distributed executables, which
90-are based on BerkeleyDB 4.8. If you do not care about wallet compatibility,
91-pass `--with-incompatible-bdb` to configure.
92+Ubuntu and Debian have their own `libdb-dev` and `libdb++-dev` packages, which install BerkeleyDB 5.3.
93+
94+BerkeleyDB 6.0 and later are not supported as they use an incompatible license and the database file format is not backwards compatible with the distributed executables.

jonatack commented at 6:55 pm on May 5, 2020:

suggestion: s/6.0/versions 6.0/

achow101 commented at 3:44 pm on May 7, 2020:

Done

in doc/build-unix.md:169 in 0599cf835f outdated

172-```
173 
174-from the root of the repository.
175+BerkeleyDB 4.7 through 5.3 (inclusive) can be used. Most linux distributions ship BerkeleyDB 5.3.
176+The `wallet.dat` files will be compatible as the database file format does not change within these versions.
177+However the database transaction log files do have incompatible formats. But these log files are only relevant in unclean wallet closes.

jonatack commented at 6:58 pm on May 5, 2020:

suggestion:

0-However the database transaction log files do have incompatible formats. But these log files are only relevant in unclean wallet closes.
1+The database transaction log files do have incompatible formats, but these log
2+files are only relevant if the wallet closes uncleanly.

achow101 commented at 3:45 pm on May 7, 2020:

Done

in doc/build-unix.md:171 in 0599cf835f outdated

174-from the root of the repository.
175+BerkeleyDB 4.7 through 5.3 (inclusive) can be used. Most linux distributions ship BerkeleyDB 5.3.
176+The `wallet.dat` files will be compatible as the database file format does not change within these versions.
177+However the database transaction log files do have incompatible formats. But these log files are only relevant in unclean wallet closes.
178+If a wallet is cleanly closed, the log files will be removed. If it is not cleanly closed, then a
179+build using the same version of BDB will need to be used to open and close that wallet to guarantee that

jonatack commented at 6:59 pm on May 5, 2020:

s/BDB/BerkeleyDB/

ryanofsky commented at 12:54 pm on May 7, 2020:

If it is not cleanly closed, then a build using the same version of BDB will need to be used to open and close that wallet to guarantee that

What exactly happens in this case? Is there a clear error aborting the wallet load and saying there’s a version discrepancy? Or is the wallet allowed to load with incomplete data? If it’s the first, then this PR seems good. If it’s the second, then allowing a case of data loss to avoid the need to type --with-incompatible-bdb doesn’t seem like a too appealing tradeoff.

Another thing to think about (I don’t this is a big concern if error handling above is adequate) is that “these log files are only relevant in unclean wallet closes” happens to be true right now. But it wasn’t always true in previous releases, and it may not be true again in the future if we want to do something to improve large wallet performance without sacrificing data integrity.

But my main question is just whether there’s an error or data loss a when 4.8 wallet software tries to open a wallet with 5.3 logs

achow101 commented at 3:42 pm on May 7, 2020:

Or is the wallet allowed to load with incomplete data?

The wallet is allowed to load. The logs are moved to a backup directory and an error is logged to both the debug.log and db.log. Unless the user is actively monitoring one of those files, they wouldn’t know that there is an error and potential data loss.

achow101 commented at 3:45 pm on May 7, 2020:

Done

jonatack commented at 8:23 pm on May 5, 2020: member

Concept ACK. A few nits below if you retouch. Testing it now.

MarcoFalke renamed this:
~~build: Allow BDB between 4.7 and 5.3 without --with-incompatible-bdb~~
build: Allow BDB between 4.8 and 5.3 without --with-incompatible-bdb
on May 5, 2020

achow101 force-pushed on May 7, 2020

in doc/build-unix.md:172 in 67e493abcd outdated

175+BerkeleyDB 4.7 through 5.3 (inclusive) can be used. Most Linux distributions ship BerkeleyDB 5.3.
176+The `wallet.dat` files will be compatible as the database file format does not change within these versions.
177+The database transaction log files do have incompatible formats, but these log files are only relevant if the wallet closes uncleanly.
178+If a wallet is cleanly closed, the log files will be removed. If it is not cleanly closed, then a
179+build using the same version of BerkeleyDB will need to be used to open and close that wallet to guarantee that
180+no data is lost.

ryanofsky commented at 4:48 pm on May 7, 2020:

Part after “to guarantee that no data is lost” seems misleading and not specific enough when there’s not a good indication of lost data. Would change to “If not cleanly closed, then builds with the newer versions of BerkeleyDB will recover using all available log data, but builds with older versions of BerkeleyDB will open the wallet potentially not using all of the recovery data.”

re: #18870 (review)

Or is the wallet allowed to load with incomplete data?

The wallet is allowed to load. The logs are moved to a backup directory and an error is logged to both the debug.log and db.log. Unless the user is actively monitoring one of those files, they wouldn’t know that there is an error and potential data loss.

Ouch. With or without this PR, it seems like dangerous to just throw away logs from future versions like that. I think separately we should implement a fix to prevent this type of data loss, even it is something really cheesy like writing a wallet.db.ver file each time we load a wallet with the current Berkeley DB version number, and refusing to load any wallet when (1) wallet.db.ver is present and (2) leftover database/log.?????????? files are present and (3) wallet.db.ver is greater than the current berkeley db version. Error message for refusing to load could be something like “Wallet [name] not shut down cleanly, and was last loaded using a newer version of Berkeley DB. Wallet software built with Berkeley DB version [version] or later is required to recover this wallet.”

All of this makes me less enthusiastic than other reviewers are about this PR. Simplifying the build a little vs creating a new scenario for data loss does not seem like an appealing tradeoff to me, especially when we may want to rely on logs more in the future to deal with wallet performance problems like #18886 / #18904. But it’s not a crazy tradeoff and people do love simple build steps, so there’s that.

achow101 commented at 5:50 pm on May 7, 2020:

I think we could just remove the behavior where we remove the log files and retry, especially if we are getting a DB_RUNRECOVERY error, which, as the name implies, means that BDB thinks we should be doing recovery procedures. This is the error that it returns when the wrong log version is found.

ryanofsky commented at 6:12 pm on May 7, 2020:

I think we could just remove the behavior where we remove the log files and retry, especially if we are getting a DB_RUNRECOVERY error, which, as the name implies, means that BDB thinks we should be doing recovery procedures. This is the error that it returns when the wrong log version is found.

Yes, I might have been too pessimistic about how often the recovery case would happen. That idea seems simpler and better than my suggestion if it works and doesn’t cause headaches for users needing to do manual recoveries. If it does cause headaches, something like the wallet.db.ver idea could always be added later to restore automatic recovery when there isn’t a risk from version mismatch

Sjors commented at 2:09 pm on May 8, 2020: member

Tested 67e493abcd3937ac590fe6515a9ebf85e48c8bcc on macOS 10.15.4, including the backwards compatibility functional tests (though that’s uninteresting on macOS, see below).

I managed to build with berkeley-db@4 via homebrew (DB 4.8.30: (April 9, 2010)

The Homebrew package for berkeley-db is at version 18. That’s not in your whitelist, but it did work previously.

When both berkeley-db@4 and berkeley-db are present, it prefers the older version. That is the behavior on master as well. In any case, it’s fine, because nobody installs berkeley-db@4 just for the fun of it.

But when only berkeley-db is present I can no longer build (unless I use depends). Previously ./configure would fail and tell you to run with --with-incompatible-bdb. That would compile with some warnings, and version 18 indeed isn’ backwards compatible (Wallet file verification failed: wallet.dat corrupt, salvage failed (-4)). But it works.

We still need to keep --with-incompatible-bdb around for these newer versions. If some lawyer doesn’t like it, well…

ryanofsky commented at 2:43 pm on May 8, 2020: member

re: #18870#issue-413171800

One issue I noticed while testing this was that it configure would pick up my system BDB instead of the depends BDB. It’s possible that this could effect gitian builds, which would be a bug. Someone more familiar with our build system should check that.

Took this up in #18915

re: #18870 (comment)

We still need to keep --with-incompatible-bdb around for these newer versions. If some lawyer doesn’t like it, well…

I was confused about this. It seems like there’s a real aversion to having to type --with-incompatible-bdb, and if your bdb system version is 5.3 instead of 4.8, then you no longer have to type --with-incompatible-bdb, and so hurray for this PR.

But if next year your system drops 5.3 and goes to 6, then aren’t you back in the same boat again? If the real goal of this PR is to get rid of the --with-incompatible-bdb option and dreaded “Found Berkeley DB other than 4.8, required for portable wallets” error, then it seems like there are other ways to do this. Like replace the --with-incompatible-bdb configure option with a with_incompatible_bdb bitcoin.conf setting. Or in >4.8 bdb builds of bitcoin, don’t create empty wallets by default, and ask for a GUI confirmation or confirmational RPC arguments when creating a wallet or loading one for the first time.

achow101 commented at 4:26 pm on May 8, 2020: member

But if next year your system drops 5.3 and goes to 6, then aren’t you back in the same boat again?

Yes, but that’s unlikely to happen. 5.3 is already 7 years old and not maintained by Oracle anymore. Distros haven’t moved to 6.0+ because of the license issues. What’s more likely to happen is that BDB gets dropped by distros entirely in which case none of this matters.

If the real goal of this PR is to get rid of the --with-incompatible-bdb option and dreaded “Found Berkeley DB other than 4.8, required for portable wallets” error, then it seems like there are other ways to do this. Like replace the --with-incompatible-bdb configure option with a with_incompatible_bdb bitcoin.conf setting. Or in >4.8 bdb builds of bitcoin, don’t create empty wallets by default, and ask for a GUI confirmation or confirmational RPC arguments when creating a wallet or loading one for the first time.

I don’t think a build related option should end up as a runtime option.

Sjors commented at 4:27 pm on May 8, 2020: member

But if next year your system drops 5.3 and goes to 6, then aren’t you back in the same boat again?

Yes.

If the real goal of this PR is to get rid of the –with-incompatible-bdb option and dreaded “Found Berkeley DB other than 4.8, required for portable wallets” error, then it seems like there are other ways to do this

I think it’s a good first step to allow a wider range of versions, which this PR offers.

I don’t think we should restrict ourselves by licensing, when it comes to what versions users compile on their own computer. Only by backwards compatibility.

Moving forward there’s also the option of using SQLite3, see #18916.

I don’t think a build related option should end up as a runtime option.

Agreed, we can always fail to load if we can’t read a format.

luke-jr commented at 5:34 pm on May 8, 2020: member

I don’t think we should restrict ourselves by licensing, when it comes to what versions users compile on their own computer. Only by backwards compatibility.

AGPL isn’t like most open source licenses (which only restrict distribution). AGPL restricts end users too. Running Bitcoin Core compiled with bdb 6 would automatically be infringing on the bdb 6 license. To legally run this combination, you would need to modify Core to advertise and share its own code to peers.

Moving forward there’s also the option of using SQLite3, see #18916.

SQLite (a relational database) doesn’t do the same thing as BDB (a key/value store). Some years ago, @jonasschnelli IIRC was working on a logdb to replace bdb…

jonatack commented at 5:35 pm on May 8, 2020: member

Am I perhaps the only person who never uses --with-incompatible-bdb? My limited data set of experiences with this, is that people stumble across the compatibility thing on first install, and then either go with passing that flag or they use the 4.8 install script in /contrib, which I finds works well and is quick, reliable and convenient.

I went with v4.8 on first install to be on the safe side… After that I added bash aliases shortcuts for the various flags and builds, and have been using them since. Perhaps that isn’t an optimal way to build? For now I don’t think I’d find using other versions of BDB to be more convenient. With or without the flag, it changes little for me anyway once it’s abstracted away to aliases. So I see this PR as mainly of use for first-time installers and am likely a minority in that view.

luke-jr commented at 5:37 pm on May 8, 2020: member

@jonatack You’re definitely not alone. Ubuntu users can easily get bdb4.8 from the PPAs. It’s a main package in Gentoo. And like you said, we have it easy to install via depends.

Sjors commented at 6:16 pm on May 8, 2020: member

AGPL isn’t like most open source licenses (which only restrict distribution). AGPL restricts end users too. Running Bitcoin Core compiled with bdb 6 would automatically be infringing on the bdb 6 license. To legally run this combination, you would need to modify Core to advertise and share its own code to peers.

That is insane. But we already enable this with --with-incompatible-bdb.

Am I perhaps the only person who never uses --with-incompatible-bdb?

I used to use that option a lot on macOS, because the berkeley-db@4 package wasn’t always reliable (#15819, #14327 (comment)), and I have no need to open wallets in an older version.

I’m also a bit paranoid about ancient versions of libraries that are maintained for the sole purpose of Bitcoin Core users. Maybe others have the same licensing concerns, but why wouldn’t they use BDB 5?

I never tried the the 4.8 install script in /contrib. macOS updates are famous for breaking stuff, so the more install options and supported versions, the better.

The default version used by Homebrew is 18 from Oracle, see this comment: https://github.com/Homebrew/homebrew-core/blob/master/Formula/berkeley-db.rb#L4

So I suppose there’s good reasons to encourage users to stick to the old versions, but we shouldn’t force them.

jonatack commented at 6:30 pm on May 8, 2020: member

Thanks @luke-jr and @Sjors. It’s interesting for me to hear other user experiences.

tryphe commented at 7:55 pm on May 19, 2020: contributor

Concept ACK.

I’m really liking this idea as there’s a lot of *nix distros with repository versions in this range that aren’t exactly 4.8. If someone tries to make wallet backups with the same version but switches distros or distro versions, this will make it a bit easier on them as far as not needing to build a specific BDB.

On the other hand, I think we need to be extra careful because it’s possible that while the format is the same between versions, there could be other minor caveats in the BDB code that screw this compatibility up some how. The format is the same, but is the data handled exactly the same by BDB? I’m more curious about this.

Maybe we should only succeed if they have one of the major versions that are common in repos, rather than any version. (or maybe it’s already this way, but it’s not clear to me)

DrahtBot commented at 9:23 pm on May 27, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19077 (wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101)
#16546 (External signer support - Wallet Box edition by Sjors)
#15382 (util: add RunCommandParseJSON by Sjors)

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.

DrahtBot added the label Needs rebase on Jun 19, 2020

build: Use system bdb and only have --with-incompatible-bdb for 6.0+

Prefer system provided BDB but still search subdirs as done previously.
Prefer higher versions over lower version.

Expands the range of allowed BDB versions to 4.7 - 5.3 inclusive.
--with-incompatible-bdb will need to be used for 6.0+.

4015ad0f17

docs: Remove mention of requiring BDB 4.8 and mention BDB log caveat

We don't require BDB 4.8 anymore. Mention what versions are supported
and the warning about log files being incompatible.

e4793c482b

travis: Remove --with-incompatible-db from environments with system libs

System libs are now compatible versions of bdb, so
--with-incompatible-bdb is no longer needed for those.

38d01dec4c

achow101 force-pushed on Jun 19, 2020

DrahtBot removed the label Needs rebase on Jun 19, 2020

achow101 commented at 3:28 pm on July 10, 2020: member

This seems to still be a bit problematic because of those transaction logs. So closing for now.

achow101 closed this on Jul 10, 2020

MarcoFalke referenced this in commit c7007babb7 on Jul 22, 2020

sidhujag referenced this in commit 39bd69b200 on Jul 24, 2020

DrahtBot locked this on Feb 15, 2022

build: Allow BDB between 4.8 and 5.3 without –with-incompatible-bdb #18870

Guix builds

Conflicts