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 #18870 (review)
Concept ACK c1029c6ff3e58a64d0b07515f92938a717a42a02. This removes database loading with no logs behavior implemented in #2558 1859aafef0a273e27e886646e36140cc5e375ee1, which hopefully should be unnecessary now that the wallet flushes very frequently.
This change should reduce chances for data loss and provide better error feedback if there’s an unsuccessful attempt to load newer bdb logs with an older version of bdb. It also simplifies the wallet opening code.
It’s possible this PR could cause new errors for users who weren’t seeing errors before. If this happens we could add an easier to use manual recovery option, or implement a middle-ground automatic approach like #18870 (review) that will retry loading the wallet without log files as long as the log files came from a compatible bdb version.
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.
Some travis failures here. One in rpc_rawtransaction appears to be spurious https://travis-ci.org/github/bitcoin/bitcoin/jobs/684393890#L5036:
02020-05-07T18:54:10.222000Z TestFramework (ERROR): Unexpected exception caught during testing
1Traceback (most recent call last):
2 File "/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 114, in main
3 self.run_test()
4 File "/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/rpc_rawtransaction.py", line 299, in run_test
5 vout = next(o for o in rawTx['vout'] if o['value'] == Decimal('2.20000000'))
6StopIteration
Other two failures in wallet_tool test seem to be the result of changed error strings https://travis-ci.org/github/bitcoin/bitcoin/jobs/684393889#L3118 and https://travis-ci.org/github/bitcoin/bitcoin/jobs/684393892#L4350:
0AssertionError: not(Error loading wallet.dat. Is wallet being used by other process? == Error initializing wallet database environment "/home/travis/build/bitcoin/bitcoin/ci/scratch/test_runner/test_runner_₿_🏃_20200507_184719/tool_wallet_117/node0/regtest/wallets"!
0 File "/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/tool_wallet.py", line 39, in assert_raises_tool_error
1 assert_equal(stderr.strip(), error)
Code review ACK 38ac29c1e8e862669c2c1fe8d9ddbced8fb53392. Changes since last review were removing bdb error strings from error messages to deal with failing travis tests and adding new python test (thanks). It’d be nice if we could include detailed bdb error messages and fix the broken travis tests to be less sensitive, but no need for that here.
Travis is failing again currently because test_runner test list is out of date. Errors are “WARNING! The following scripts are not being run: [‘wallet_db.py’]. Check the test lists in test_runner.py.” https://travis-ci.org/github/bitcoin/bitcoin/jobs/688954958#L3332
Travis is failing again currently because test_runner test list is o
Fixed
./test/sanitizer_suppressions/lsan
Apparently the new wallet_db.py test exposes a memory leak that happens when BerkeleyEnvironment::Open fails: https://travis-ci.org/github/bitcoin/bitcoin/jobs/689013660#L4659
I didn’t look closely, but in case the leak does not seem to be something obvious, I’d forgive you if you just want to drop the test here and pretend the leak isn’t happening. (I’m assuming it is pre-existing.) I can follow up with the test in another PR.
230- }
231- } else {
232- return false;
233+ err = strprintf(_("Error initializing wallet database environment %s!"), Directory());
234+ if (ret == DB_RUNRECOVERY) {
235+ err = strprintf(_("Error initializing wallet database environment %s! This error could occur if this wallet was not shutdown cleanly and was last loaded using a build with a newver version of Berkeley DB. If so, please use the software that last loaded this wallet"), Directory());
0 err += Untranslated(" ") + _("This error could occur if this wallet was not shutdown cleanly and was last loaded using a build with a newer version of Berkeley DB. If so, please use the software that last loaded this wallet");
There is a typo in the translation message. Also, this can be written shorter by just appending the more precise reason. (You might have to rebase, to get the + operators)
205- }
206- } else {
207- return false;
208+ err = strprintf(_("Error initializing wallet database environment %s!"), Directory());
209+ if (ret == DB_RUNRECOVERY) {
210+ err += Untranslated(" ") + _("This error could occur if this wallet was not shutdown cleanly and was last loaded using a build with a newver version of Berkeley DB. If so, please use the software that last loaded this wallet");
newver typo
tACK c2faa34 on macOS 10.15.5
I tested a few scenario’s. First I created a fresh wallet with bdb 18.
It closed it normally, recompiled with bdb4.8 and loaded it. That fails with Wallet file verification fails. wallet.dat corrupt. Try using the wallet tool bitcoin-wallet to salvage or restoring a backup.. Same error with or without this commit; bdb 18 is probably just too new.
While the bdb18 wallet was open, I copied the database directory. I then closed Bitcoin QT and copied the directory back. I then opened it, without this PR, and it renamed the directory to .bak. Same error as above.
Same as (2), but with this PR, now I get the new error message.
The commit message could be more clear that this error is only triggered for users who manually compiled with a newer BDB version.
Nit: speciically typo in commit message
Same error with or without this commit; bdb 18 is probably just too new.
It is. The best version to test with is probably 5.x as it will produce compatible/identical wallet.dat files but incompatible logs.
utACK c2faa3432241d15f8d0a9968ba46cc0eef7fe21f
With three ACKs this can go in despite the annoyingly obvious typo ;)
EDIT: there is a build error so you can fix the typo now
0wallet/salvage.cpp: In function ‘bool RecoverDatabaseFile(const boost::filesystem::path&)’:
1wallet/salvage.cpp:23:36: error: no matching function for call to ‘BerkeleyEnvironment::Open(bool)’
2 if (!env->Open(true /* retry */)) {
Instead of removing the database transaction logs and retrying the
wallet loading, just return an error message to the user. Additionally,
specifically for DB_RUNRECOVERY, notify the user that this could be due
to different BDB versions. This error is pretty much only caused by
compiling with a newer version of BDB and then trying to open the wallet
with a version compiled with an older version of BDB.
Tested:
0$ ./src/bitcoin-cli -regtest getnewaddress
12020-07-22T06:50:11Z [default wallet] keypool reserve 2
22020-07-22T06:50:11Z [default wallet] keypool keep 2
3bitcoind: wallet/rpcwallet.cpp:278: UniValue getnewaddress(const JSONRPCRequest&): Assertion `false' failed.
4error: Could not connect to the server 127.0.0.1:18443 (error code 1 - "EOF reached")
5
6Make sure the bitcoind server is running and that you are connecting to the correct RPC port.
7[2]- Aborted (core dumped) ./src/bitcoind -regtest
8
9
10
11$ ls ~/.bitcoin/regtest/wallets/
12database db.log wallet.dat
13
14
15$ ./src/bitcoind-with-older-bdb -regtest
16...
172020-07-22T06:55:42Z Using wallet directory /root/.bitcoin/regtest/wallets
182020-07-22T06:55:42Z init message: Verifying wallet(s)...
192020-07-22T06:55:42Z Using BerkeleyDB version Berkeley DB 4.8.30: (April 9, 2010)
202020-07-22T06:55:42Z Using wallet /root/.bitcoin/regtest/wallets/wallet.dat
212020-07-22T06:55:42Z BerkeleyEnvironment::Open: LogDir=/root/.bitcoin/regtest/wallets/database ErrorFile=/root/.bitcoin/regtest/wallets/db.log
222020-07-22T06:55:42Z BerkeleyEnvironment::Open: Error -30974 opening database environment: DB_RUNRECOVERY: Fatal error, run database recovery
232020-07-22T06:55:42Z Error: Error initializing wallet database environment "/root/.bitcoin/regtest/wallets"! This error could occur if this wallet was not shutdown cleanly and was last loaded using a build with a newer version of Berkeley DB. If so, please use the software that last loaded this wallet
24Error: Error initializing wallet database environment "/root/.bitcoin/regtest/wallets"! This error could occur if this wallet was not shutdown cleanly and was last loaded using a build with a newer version of Berkeley DB. If so, please use the software that last loaded this wallet
252020-07-22T06:55:42Z Shutdown: In progress...
262020-07-22T06:55:42Z scheduler thread exit
272020-07-22T06:55:42Z Shutdown: done
329@@ -342,7 +330,8 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo
330
331 {
332 LOCK(cs_db);
333- if (!env->Open(false /* retry */))
334+ bilingual_str open_err;
335+ if (!env->Open(open_err))
style-nit: Could upgrade to the new style while touching the line
0 if (!env->Open(open_err)) {