build: Allow BDB between 4.8 and 5.3 without --with-incompatible-bdb #18870
Conversation
doc/build-unix.md
Outdated
| pass `--with-incompatible-bdb` to configure. | ||
| Ubuntu and Debian have their own `libdb-dev` and `libdb++-dev` packages, which install BerkeleyDB 5.3. | ||
|
|
||
| If you use BerkeleyDB 6.0 or later, the database file will be incompatible with the distributed executables. |
There was a problem hiding this comment.
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?).
There was a problem hiding this comment.
(This is due to BDB 6.0+ being only available under the AGPL-3 license)
There was a problem hiding this comment.
That's a good point. I've dropped --with--incompatible-bdb and just made 6.0+ not a valid option.
doc/dependencies.md
Outdated
| @@ -5,7 +5,7 @@ These are the dependencies currently used by Bitcoin Core. You can find instruct | |||
|
|
|||
| | Dependency | Version used | Minimum required | CVEs | Shared | [Bundled Qt library](https://doc.qt.io/qt-5/configure-options.html#third-party-libraries) | | |||
| | --- | --- | --- | --- | --- | --- | | |||
| | Berkeley DB | [4.8.30](https://www.oracle.com/technetwork/database/database-technologies/berkeleydb/downloads/index.html) | 4.8.x | No | | | | |||
| | Berkeley DB | [4.8.30](https://www.oracle.com/technetwork/database/database-technologies/berkeleydb/downloads/index.html) | 4.7.x | No | | | | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
It was done just for completeness since 4.7 does technically work.
There was a problem hiding this comment.
I've set it back to 4.8 as the minimum.
There was a problem hiding this comment.
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).
|
Concept ACK |
Guix builds
|
|
Super strong concept ACK As contributors we're used to passing |
doc/build-unix.md
Outdated
| If a wallet is cleanly closed, the log files will be removed. 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| If a wallet is cleanly closed, the log files will be removed. If it is not cleanly closed, then a | ||
| build using the same version of BerkeleyDB will need to be used to open and close that wallet to guarantee that | ||
| no data is lost. |
There was a problem hiding this comment.
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 (comment)
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think we could just remove the behavior where we remove the log files and retry, especially if we are getting a
DB_RUNRECOVERYerror, 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
|
Tested 67e493a on macOS 10.15.4, including the backwards compatibility functional tests (though that's uninteresting on macOS, see below). I managed to build with The Homebrew package for When both But when only We still need to keep |
|
re: #18870 (comment)
Took this up in #18915 re: #18870 (comment)
I was confused about this. It seems like there's a real aversion to having to type 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 |
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.
I don't think a build related option should end up as a runtime option. |
Yes.
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.
Agreed, we can always fail to load if we can't read a format. |
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.
SQLite (a relational database) doesn't do the same thing as BDB (a key/value store). |
|
Am I perhaps the only person who never uses 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. |
|
@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. |
That is insane. But we already enable this with
I used to use that option a lot on macOS, because the 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 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. |
|
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) |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
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+.
We don't require BDB 4.8 anymore. Mention what versions are supported and the warning about log files being incompatible.
System libs are now compatible versions of bdb, so --with-incompatible-bdb is no longer needed for those.
67e493a to
38d01de
Compare
|
This seems to still be a bit problematic because of those transaction logs. So closing for now. |
…stead error d0ea9ba walletdb: Don't remove database transaction logs and instead error (Andrew Chow) Pull request description: Instead of removing the database transaction logs and retrying the wallet loading, just return an error message to the user. Additionally, speciically for DB_RUNRECOVERY, notify the user that this could be due to different BDB versions. Kind of implements the suggestion from bitcoin/bitcoin#18870 (comment) ACKs for top commit: Sjors: re-utACK d0ea9ba ryanofsky: Code review ACK d0ea9ba. Only changes since last review are rebase and expanding error and commit messages. Tree-SHA512: f6e67dc70f58188742a5c8af7cdc63a2b58779aa0d26ae7f1e75805a239f1a342433860e5a238d6577fae5ab04b9d15e7f11c55b867065dfd13781a6a62e4958
… and instead error d0ea9ba walletdb: Don't remove database transaction logs and instead error (Andrew Chow) Pull request description: Instead of removing the database transaction logs and retrying the wallet loading, just return an error message to the user. Additionally, speciically for DB_RUNRECOVERY, notify the user that this could be due to different BDB versions. Kind of implements the suggestion from bitcoin#18870 (comment) ACKs for top commit: Sjors: re-utACK d0ea9ba ryanofsky: Code review ACK d0ea9ba. Only changes since last review are rebase and expanding error and commit messages. Tree-SHA512: f6e67dc70f58188742a5c8af7cdc63a2b58779aa0d26ae7f1e75805a239f1a342433860e5a238d6577fae5ab04b9d15e7f11c55b867065dfd13781a6a62e4958
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.