wallet: Handle duplicate fileid exception by promag · Pull Request #16923 · bitcoin/bitcoin

promag · 2019-09-20T14:55:10Z

Handle the duplicate fileid exception thrown at CheckUniqueFileid in tow cases:

when duplicate wallets are set on the command line - catch in LoadWallets;
when a duplicate wallet is loaded dynamically - catch in LoadWallet.

promag · 2019-09-20T14:56:01Z

To backport?

cvengler · 2019-09-20T16:28:03Z

I don't know if that's a thing here but there is no function at the end which returns something (In practice it is). Probably some compilers will give a warning or errer so you might should add a return false; at the end

jonatack

Thanks @promag for working on this.

Note: the PR description should be updated to suggest reviewing with git show -w 8b8964e

Functional test wallet_multiwallet.py --usecli is failing for me with AssertionError: [node 0] Expected message "BerkeleyBatch: Can't open database w8_copy \(duplicates fileid \w+ from w8\)" does not partially match stderr: ""

Here are the results at 8b8964e of loading 2 identical wallets in ~/.bitcoin/testnet3/wallets/ named t7 and t8:

$ ./src/qt/bitcoin-qt -debug -testnet
terminate called after throwing an instance of 'std::runtime_error'
  what():  BerkeleyBatch: Can't open database wallet.dat (duplicates fileid 99059e0100fe00005cbe4ae945270e0000000000 from wallet.dat)
Aborted

$ ./src/bitcoind -testnet -wallet=t7.dat -wallet=t8.dat
2019-09-20T17:55:40Z Bitcoin Core version v0.18.99.0-8b8964e872 (debug build)
...
2019-09-20T17:55:40Z Using data directory /home/jon/.bitcoin/testnet3
2019-09-20T17:55:40Z Using wallet directory /home/jon/.bitcoin/testnet3/wallets
2019-09-20T17:55:40Z init message: Verifying wallet(s)...
2019-09-20T17:55:40Z Using BerkeleyDB version Berkeley DB 4.8.30: (April  9, 2010)
2019-09-20T17:55:40Z Using wallet /home/jon/.bitcoin/testnet3/wallets/t7.dat
2019-09-20T17:55:40Z BerkeleyEnvironment::Open: LogDir=/home/jon/.bitcoin/testnet3/wallets/t7.dat/database
                      ErrorFile=/home/jon/.bitcoin/testnet3/wallets/t7.dat/db.log
2019-09-20T17:55:40Z scheduler thread start
2019-09-20T17:55:40Z Using BerkeleyDB version Berkeley DB 4.8.30: (April  9, 2010)
2019-09-20T17:55:40Z Using wallet /home/jon/.bitcoin/testnet3/wallets/t8.dat
2019-09-20T17:55:40Z BerkeleyEnvironment::Open: LogDir=/home/jon/.bitcoin/testnet3/wallets/t8.dat/database
                      ErrorFile=/home/jon/.bitcoin/testnet3/wallets/t8.dat/db.log

DrahtBot · 2019-10-07T15:02:06Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

gui: Bilingual GUI error messages #16224 (gui: Bilingual GUI error messages 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.

hebasto · 2019-11-20T10:03:45Z

Dynamically loading a duplicate wallet doesn't result in the same problem.

That's not the case, unfortunately. Tested on Linux Mint 19.2:

2019-11-20T09:42:58Z [] Using BerkeleyDB version Berkeley DB 4.8.30: (April  9, 2010)
2019-11-20T09:42:58Z [] Using wallet /home/hebasto/.bitcoin/testnet3/wallets/w20191015
2019-11-20T09:42:58Z [] BerkeleyEnvironment::Open: LogDir=/home/hebasto/.bitcoin/testnet3/wallets/w20191015/database ErrorFile=/home/hebasto/.bitcoin/testnet3/wallets/w20191015/db.log
2019-11-20T09:42:58Z [] init message: Loading wallet...
2019-11-20T09:42:58Z [] BerkeleyEnvironment::Open: LogDir=/home/hebasto/.bitcoin/testnet3/wallets/w20191015/database ErrorFile=/home/hebasto/.bitcoin/testnet3/wallets/w20191015/db.log
2019-11-20T09:42:58Z [] [w20191015] Wallet File Version = 169900
2019-11-20T09:42:58Z [] [w20191015] Keys: 2003 plaintext, 0 encrypted, 2003 w/ metadata, 2003 total. Unknown wallet records: 0
2019-11-20T09:42:58Z [] [w20191015] Wallet completed loading in              29ms
...
2019-11-20T09:43:16Z [] Using BerkeleyDB version Berkeley DB 4.8.30: (April  9, 2010)
2019-11-20T09:43:16Z [] Using wallet /home/hebasto/.bitcoin/testnet3/wallets/w20191015 (copy)
2019-11-20T09:43:16Z [] BerkeleyEnvironment::Open: LogDir=/home/hebasto/.bitcoin/testnet3/wallets/w20191015 (copy)/database ErrorFile=/home/hebasto/.bitcoin/testnet3/wallets/w20191015 (copy)/db.log
2019-11-20T09:43:16Z [] init message: Loading wallet...
2019-11-20T09:43:16Z [] BerkeleyEnvironment::Open: LogDir=/home/hebasto/.bitcoin/testnet3/wallets/w20191015 (copy)/database ErrorFile=/home/hebasto/.bitcoin/testnet3/wallets/w20191015 (copy)/db.log
terminate called after throwing an instance of 'std::runtime_error'
  what():  BerkeleyBatch: Can't open database wallet.dat (duplicates fileid f78d2600020800008513f6218301020000000000 from wallet.dat)
Aborted (core dumped)

promag · 2020-01-09T15:45:19Z

I don't know if that's a thing here but there is no function at the end which returns something (In practice it is). Probably some compilers will give a warning or errer so you might should add a return false; at the end

@emilengler I think you mean in LoadWallets, note that all branches have a return value.

promag · 2020-01-09T15:47:56Z

@hebasto IIRC I tried with no success to hit that, prolly I've done something wrong.

hebasto · 2020-01-09T17:01:39Z

@promag

@hebasto IIRC I tried with no success to hit that, prolly I've done something wrong.

I can confirm that the fix does not work, unfortunately.

Steps to reproduce:

prepare a copy of a wallet:

$ ls ~/.bitcoin/testnet3/wallets/ | grep w20191015
w20191015
w20191015 (copy)

load wallets one by one

on master:

$ ./src/qt/bitcoin-qt -testnet
terminate called after throwing an instance of 'std::runtime_error'
  what():  BerkeleyBatch: Can't open database wallet.dat (duplicates fileid f78d2600020800008513f6218301020000000000 from wallet.dat)
Aborted (core dumped)

with this pr:

$ ./src/qt/bitcoin-qt -testnet
terminate called after throwing an instance of 'std::runtime_error'
  what():  BerkeleyBatch: Can't open database wallet.dat (duplicates fileid f78d2600020800008513f6218301020000000000 from wallet.dat)
Aborted (core dumped)

Literally, I did steps as described in #16776 (comment)

hebasto · 2020-03-27T21:13:04Z

@promag are you still working on this pr?

promag · 2020-03-28T11:08:46Z

@hebasto updated OP, thanks for the remind!

hebasto

Tested 4deb8fa32e4f40b2f63ac8aa5b6fff76101af40c on Linux Mint 19.3:

i.e., the exception is handled, but the client keeps trying to load the duplicated wallet forever.

promag · 2020-03-28T18:30:10Z

i.e., the exception is handled, but the client keeps trying to load the duplicated wallet forever.

Forever how?

hebasto · 2020-03-28T18:33:36Z

i.e., the exception is handled, but the client keeps trying to load the duplicated wallet forever.

Forever how?

Run test again: it lasts 2 minutes and keeps going...

promag · 2020-03-30T01:00:31Z

@hebasto I fail to reproduce your case on osx. Does the progress go away when you press "OK" on the "Open wallet failed" dialog? On osx the error dialog shows on top.

hebasto · 2020-03-30T07:33:14Z

@promag

Does the progress go away when you press "OK" on the "Open wallet failed" dialog?

No.

On osx the error dialog shows on top.

On Linux Mint 19.3 the sequence of events is as follows:

window "Open wallet failed" appears
4 sec timeout (QProgressDialog.minimumDuration)
window "Opening Wallet ..." appears

hebasto · 2020-03-30T11:52:56Z

ACK a7f81a9f98113a92de515bd13e59052885b42541, tested on Linux Mint 19.3.

promag · 2020-03-30T12:03:09Z

@hebasto improved last commit, no longer wip.

hebasto

ACK 68e0ff0e1f530c942721aab49cf67ffc07104628

@promag your last changes were on my tongue ;)

nit: #include <cassert>

jonatack

ACK 68e0ff0e1 tested rebased on master 5f9cd62

(origin/pr/16923) $ uname -a
Linux 4.19.0-5-amd64 #1 SMP Debian 4.19.37-5+deb10u2 (2019-08-08) x86_64 GNU/Linux
(origin/pr/16923) $ ./src/bitcoind -testnet -wallet=t8-copy.dat -wallet=t8.dat
...
2020-03-30T13:04:17Z Using wallet directory /home/jon/.bitcoin/testnet3/wallets
2020-03-30T13:04:17Z init message: Verifying wallet(s)...
2020-03-30T13:04:17Z Using BerkeleyDB version Berkeley DB 4.8.30: (April  9, 2010)
2020-03-30T13:04:17Z Using wallet /home/jon/.bitcoin/testnet3/wallets/t8-copy.dat
2020-03-30T13:04:17Z BerkeleyEnvironment::Open:
    LogDir=/home/jon/.bitcoin/testnet3/wallets/t8-copy.dat/database
    ErrorFile=/home/jon/.bitcoin/testnet3/wallets/t8-copy.dat/db.log
2020-03-30T13:04:17Z Using BerkeleyDB version Berkeley DB 4.8.30: (April  9, 2010)
2020-03-30T13:04:17Z Using wallet /home/jon/.bitcoin/testnet3/wallets/t8.dat
2020-03-30T13:04:17Z BerkeleyEnvironment::Open:
    LogDir=/home/jon/.bitcoin/testnet3/wallets/t8.dat/database
    ErrorFile=/home/jon/.bitcoin/testnet3/wallets/t8.dat/db.log

Attempting to load wallet t8-copy after loading wallet t8:

In a follow-up, it might be helpful if the error message stated the conflicting wallet names.

promag · 2020-03-30T16:51:34Z

In a follow-up, it might be helpful if the error message stated the conflicting wallet names.

But It says :) yeah display the wallet name, not the underlying filename which is useless for directory based wallets.

promag · 2020-03-31T13:25:18Z

Rebased after merging #18338.

jonatack · 2020-03-31T15:30:40Z

Re-ACK 9eefc6e no change since last review 68e0ff0e1f530c942721aab49cf67ffc07104628

jonatack · 2020-03-31T17:37:03Z

Tested on macOS 10.14.6 as well (preceding test was on Debian).

hebasto · 2020-03-31T21:10:41Z

re-ACK 9eefc6e

9eefc6e gui: Delete progress dialog instead of hidding it (João Barbosa) ee9e88b wallet: Handle duplicate fileid exception (João Barbosa) Pull request description: Handle the duplicate fileid exception thrown at `CheckUniqueFileid` in tow cases: - when duplicate wallets are set on the command line - catch in `LoadWallets`; - when a duplicate wallet is loaded dynamically - catch in `LoadWallet`. Fixes bitcoin#16776. ACKs for top commit: jonatack: Re-ACK 9eefc6e no change since last review 68e0ff0e1f530c942721aab49cf67ffc07104628 hebasto: re-ACK 9eefc6e Tree-SHA512: 46e3c1cd6708b54e2d1c4973a74c8d5428822e04cecbc147cf200eb034efa385e867bd749c7c639020e83c9813fae8fed64a851bdd99abf60c33b07e0363f5d5

Summary: Loading two identical wallet files caused a crash > Handle the duplicate fileid exception thrown at CheckUniqueFileid in tow cases: > > - when duplicate wallets are set on the command line - catch in LoadWallets; > - when a duplicate wallet is loaded dynamically - catch in LoadWallet. This is a backport of Core [[bitcoin/bitcoin#16923 | PR16923]] Test Plan: `ninja all check-all` In bitcoin-qt, backup the currently open wallet to a file named wallet.dat, then in the console window type `loadwallet /path/to/wallet/`. Check that the program does not crash and you get a useful error message dialog. Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8898

promag force-pushed the 2019-09-fix-duplicate-exception branch from 1c55c12 to 8b8964e Compare September 20, 2019 15:15

DrahtBot added the Wallet label Sep 20, 2019

jonatack reviewed Sep 20, 2019

View reviewed changes

promag force-pushed the 2019-09-fix-duplicate-exception branch from 01717f5 to fd576c4 Compare September 29, 2019 22:17

laanwj added the Bug label Sep 30, 2019

DrahtBot added the Needs rebase label Oct 21, 2019

promag force-pushed the 2019-09-fix-duplicate-exception branch from fd576c4 to e06614b Compare November 3, 2019 23:49

DrahtBot removed the Needs rebase label Nov 4, 2019

DrahtBot mentioned this pull request Feb 11, 2020

gui: Bilingual GUI error messages #16224

Merged

brakmic mentioned this pull request Mar 19, 2020

wallet: Duplicate wallets crash bitcoin core #16776

Closed

promag force-pushed the 2019-09-fix-duplicate-exception branch from e06614b to 635f509 Compare March 28, 2020 02:15

DrahtBot mentioned this pull request Mar 28, 2020

Use shared pointers only in validation interface #18354

Closed

promag changed the title ~~wallet: Fix duplicates fileid exception on start~~ wallet: Handle duplicates fileid exception Mar 28, 2020

promag changed the title ~~wallet: Handle duplicates fileid exception~~ wallet: Handle duplicate fileid exception Mar 28, 2020

promag force-pushed the 2019-09-fix-duplicate-exception branch from 635f509 to 4deb8fa Compare March 28, 2020 11:06

This was referenced Mar 28, 2020

Replace -upgradewallet startup option with upgradewallet RPC #15761

Merged

Multiprocess bitcoin #10102

Draft

hebasto reviewed Mar 28, 2020

View reviewed changes

DrahtBot mentioned this pull request Mar 29, 2020

Fix wallet unload race condition #18338

Merged

DrahtBot mentioned this pull request Mar 30, 2020

Build: Move wallet RPCs to their own libbitcoin_walletrpcs module #18469

Closed

promag force-pushed the 2019-09-fix-duplicate-exception branch from a7f81a9 to 68e0ff0 Compare March 30, 2020 12:02

hebasto approved these changes Mar 30, 2020

View reviewed changes

jonatack reviewed Mar 30, 2020

View reviewed changes

promag added 2 commits March 31, 2020 14:23

wallet: Handle duplicate fileid exception

ee9e88b

gui: Delete progress dialog instead of hidding it

9eefc6e

promag force-pushed the 2019-09-fix-duplicate-exception branch from 68e0ff0 to 9eefc6e Compare March 31, 2020 13:23

laanwj added this to the 0.20.0 milestone Mar 31, 2020

laanwj merged commit 6bdd515 into bitcoin:master Apr 2, 2020

promag deleted the 2019-09-fix-duplicate-exception branch April 2, 2020 13:10

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

promag commented Sep 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Sep 20, 2019

Uh oh!

cvengler commented Sep 20, 2019

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

hebasto commented Nov 20, 2019

Uh oh!

promag commented Jan 9, 2020

Uh oh!

promag commented Jan 9, 2020

Uh oh!

hebasto commented Jan 9, 2020

Uh oh!

hebasto commented Mar 27, 2020

Uh oh!

promag commented Mar 28, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Mar 28, 2020

Uh oh!

hebasto commented Mar 28, 2020

Uh oh!

promag commented Mar 30, 2020

Uh oh!

hebasto commented Mar 30, 2020

Uh oh!

hebasto commented Mar 30, 2020

Uh oh!

promag commented Mar 30, 2020

Uh oh!

hebasto 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!

jonatack 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!

promag commented Mar 30, 2020

Uh oh!

promag commented Mar 31, 2020

Uh oh!

jonatack commented Mar 31, 2020

Uh oh!

jonatack commented Mar 31, 2020

Uh oh!

hebasto commented Mar 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

promag commented Sep 20, 2019 •

edited

Loading

DrahtBot commented Oct 7, 2019 •

edited

Loading

hebasto left a comment •

edited

Loading

jonatack left a comment •

edited

Loading