Avoid opening copied wallet databases simultaneously #11476

ryanofsky · 2017-10-10T19:50:08Z

Make sure wallet databases have unique fileids. If they don't, throw an error. BDB caches do not work properly when more than one open database has the same fileid, because values written to one database may show up in reads to other databases.

Bitcoin will never create different databases with the same fileid, but users can create them by manually copying database files.

BDB caching bug was reported by @dooglus in #11429

sipa · 2017-10-10T20:26:42Z

Concept ACK

jonasschnelli · 2017-10-10T20:38:51Z

Nice clean PR!
utACK ebc7c2f

meshcollider · 2017-10-11T02:46:40Z

Concept ACK

promag · 2017-10-11T14:15:24Z

src/wallet/db.cpp

@@ -402,11 +403,41 @@ CDB::CDB(CWalletDBWrapper& dbw, const char* pszMode, bool fFlushOnCloseIn) : pdb
                            0);

            if (ret != 0) {
+                error = strprintf("CDB: Error %d, can't open database %s", ret, strFilename);


Use same format as below: CDB: Can't open database %s (open error %d)".

Didn't do this to avoid changing the existing error message.

promag · 2017-10-11T14:15:35Z

src/wallet/db.cpp

+                u_int8_t fileid[DB_FILE_ID_LEN];
+                ret = pdb->get_mpf()->get_fileid(fileid);
+                if (ret != 0) {
+                    error = strprintf("CDB: Can't open database %s (get_fileid error %d)\n", strFilename, ret);


Remove newline?

Good catch, removed in e32c2a5

promag · 2017-10-11T14:17:50Z

src/wallet/db.cpp

+                        memcmp(fileid, item_fileid, sizeof(fileid)) == 0) {
+                        const char* item_filename = nullptr;
+                        item.second->get_dbname(&item_filename, nullptr);
+                        error = strprintf("CDB: Can't open database %s (duplicates fileid %s from %s)", strFilename,


This error overrides the previous.

Should += std::endl + instead?

Fixed override in e32c2a5, but I'm not clear on what std::endl comment is referring to.

Nevermind, but the suggestion was to accumulate the errors.

promag · 2017-10-11T14:19:11Z

test/functional/multiwallet.py

@@ -29,6 +30,10 @@ def run_test(self):
        os.mkdir(os.path.join(self.options.tmpdir, 'node0', 'regtest', 'w11'))
        self.assert_start_raises_init_error(0, ['-wallet=w11'], 'Error loading wallet w11. -wallet filename must be a regular file.')

+        # should not initialize if one wallet is a copy of another
+        shutil.copyfile(self.options.tmpdir + "/node0/regtest/w2", self.options.tmpdir + "/node0/regtest/w22")


Use os.path.join?

Done in e32c2a5

promag · 2017-10-11T14:19:37Z

src/wallet/db.cpp

+                // an error. BDB caches do not work properly when more than one
+                // open database has the same fileid (values written to one
+                // database may show up in reads to other databases).
+                //


Nit, remove empty line.

It's intended to be a new paragraph.

promag · 2017-10-11T14:21:09Z

src/wallet/db.cpp

+                    error = strprintf("CDB: Can't open database %s (get_fileid error %d)\n", strFilename, ret);
+                }
+
+                for (const auto& item : env->mapDb) {


Only for when ret == 0?

Done in e32c2a5

promag · 2017-10-11T14:22:34Z

src/wallet/db.cpp

+                        error = strprintf("CDB: Can't open database %s (duplicates fileid %s from %s)", strFilename,
+                            HexStr(std::begin(item_fileid), std::end(item_fileid)),
+                            item_filename ? item_filename : "");
+                    }


Break on first duplicate?

Done in e32c2a5

laanwj · 2017-10-11T15:51:05Z

src/wallet/db.cpp

                delete pdb;
                pdb = nullptr;
                --env->mapFileUseCount[strFile];
                strFile = "";
-                throw std::runtime_error(strprintf("CDB: Error %d, can't open database %s", ret, strFilename));
+                throw std::runtime_error(error);


I guess the reason for storing the error in a string, and not just raising the exceptions at the place the error occurs, above, is to avoid duplicating this cleanup code?
It seems a bit circuitous, but I understand. An alternative would be to wrap these operations in a RAII operation.

This was one reason. The other reason was wanting the fix to be self-contained and not change existing code.

ryanofsky

Updated ebc7c2f -> e32c2a5 (pr/wid.1 -> pr/wid.2)

ryanofsky · 2017-10-11T18:38:00Z

src/wallet/db.cpp

@@ -402,11 +403,41 @@ CDB::CDB(CWalletDBWrapper& dbw, const char* pszMode, bool fFlushOnCloseIn) : pdb
                            0);

            if (ret != 0) {
+                error = strprintf("CDB: Error %d, can't open database %s", ret, strFilename);


Didn't do this to avoid changing the existing error message.

ryanofsky · 2017-10-11T18:40:09Z

src/wallet/db.cpp

+                // an error. BDB caches do not work properly when more than one
+                // open database has the same fileid (values written to one
+                // database may show up in reads to other databases).
+                //


It's intended to be a new paragraph.

ryanofsky · 2017-10-11T18:40:24Z

src/wallet/db.cpp

+                u_int8_t fileid[DB_FILE_ID_LEN];
+                ret = pdb->get_mpf()->get_fileid(fileid);
+                if (ret != 0) {
+                    error = strprintf("CDB: Can't open database %s (get_fileid error %d)\n", strFilename, ret);


Good catch, removed in e32c2a5

ryanofsky · 2017-10-11T18:40:36Z

src/wallet/db.cpp

+                    error = strprintf("CDB: Can't open database %s (get_fileid error %d)\n", strFilename, ret);
+                }
+
+                for (const auto& item : env->mapDb) {


Done in e32c2a5

ryanofsky · 2017-10-11T18:41:45Z

src/wallet/db.cpp

+                        memcmp(fileid, item_fileid, sizeof(fileid)) == 0) {
+                        const char* item_filename = nullptr;
+                        item.second->get_dbname(&item_filename, nullptr);
+                        error = strprintf("CDB: Can't open database %s (duplicates fileid %s from %s)", strFilename,


Fixed override in e32c2a5, but I'm not clear on what std::endl comment is referring to.

ryanofsky · 2017-10-11T18:41:59Z

src/wallet/db.cpp

+                        error = strprintf("CDB: Can't open database %s (duplicates fileid %s from %s)", strFilename,
+                            HexStr(std::begin(item_fileid), std::end(item_fileid)),
+                            item_filename ? item_filename : "");
+                    }


Done in e32c2a5

ryanofsky · 2017-10-11T18:43:26Z

src/wallet/db.cpp

                delete pdb;
                pdb = nullptr;
                --env->mapFileUseCount[strFile];
                strFile = "";
-                throw std::runtime_error(strprintf("CDB: Error %d, can't open database %s", ret, strFilename));
+                throw std::runtime_error(error);


This was one reason. The other reason was wanting the fix to be self-contained and not change existing code.

ryanofsky · 2017-10-11T18:43:41Z

test/functional/multiwallet.py

@@ -29,6 +30,10 @@ def run_test(self):
        os.mkdir(os.path.join(self.options.tmpdir, 'node0', 'regtest', 'w11'))
        self.assert_start_raises_init_error(0, ['-wallet=w11'], 'Error loading wallet w11. -wallet filename must be a regular file.')

+        # should not initialize if one wallet is a copy of another
+        shutil.copyfile(self.options.tmpdir + "/node0/regtest/w2", self.options.tmpdir + "/node0/regtest/w22")


Done in e32c2a5

promag · 2017-10-12T10:10:19Z

src/wallet/db.cpp

+                // copy database files.
+                for (const auto& item : env->mapDb) {
+                    u_int8_t item_fileid[DB_FILE_ID_LEN];
+                    if (item.second && item.second->get_mpf()->get_fileid(item_fileid) == 0 &&


Maybe use the ret = ... pattern like above to make easier to read?

ryanofsky · 2017-10-12T15:18:44Z

I had to make another change to avoid travis failures. For some reason the get_fileid call was failing with error 22 in accounting tests (strangely only when accounting tests were run in conjunction with other tests, not when running standalone). I fixed it by skipping the get_fileid check on in-memory databases, which actually makes sense anyway.

Updated e32c2a5 -> 4164132 (pr/wid.2 -> pr/wid.3)

jnewbery · 2017-10-12T21:18:45Z

Tested ACK 4164132

I think you could improve the code comment by saying that bdb guarantees file identification strings for different databases to be unique, and perhaps linking to the documentation (https://docs.oracle.com/cd/E17275_01/html/programmer_reference/program_copy.html).

ryanofsky · 2017-10-12T21:36:02Z

Thanks, updated comment 4164132 -> f988322 (pr/wid.3 -> pr/wid.4)

I didn't include the oracle link originally because I wasn't sure if the URL would be stable, but I realize the comment doesn't really make sense without that context, so it seems worth adding.

jnewbery · 2017-10-12T21:57:28Z

I didn't include the oracle link originally because I wasn't sure if the URL would be stable

Yes, I considered that as well. I think it's ok to add the link now. If it dies or changes, then the comment can be updated or removed. For now, it's adding useful additional context.

I can confirm that your new commit updates the comment. It also makes the following change:

-    u_int8_t fileid[DB_FILE_ID_LEN] = {0};
+    u_int8_t fileid[DB_FILE_ID_LEN];

Was that intentional?

ryanofsky · 2017-10-12T22:01:24Z

Was that intentional?

Yeah, I added the zero initialization when the get_fileid call was failing and I was considering making it optional, but it's not needed now.

jnewbery · 2017-10-13T19:09:06Z

Tested ACK f988322

Make sure wallet databases have unique fileids. If they don't, throw an error. BDB caches do not work properly when more than one open database has the same fileid, because values written to one database may show up in reads to other databases. Bitcoin will never create different databases with the same fileid, but users can create them by manually copying database files. BDB caching bug was reported by Chris Moore <dooglus@gmail.com> bitcoin#11429 Fixes bitcoin#11429

laanwj · 2017-10-19T13:07:53Z

Needs rebase for #11492, otherwise seems ready to merge.

ryanofsky · 2017-10-19T14:44:06Z

Rebased f988322 -> 478a89c (pr/wid.4 -> pr/wid.5)

laanwj · 2017-10-19T16:16:09Z

utACK 478a89c

@dooglus

478a89c Avoid opening copied wallet databases simultaneously (Russell Yanofsky) Pull request description: Make sure wallet databases have unique fileids. If they don't, throw an error. BDB caches do not work properly when more than one open database has the same fileid, because values written to one database may show up in reads to other databases. Bitcoin will never create different databases with the same fileid, but users can create them by manually copying database files. BDB caching bug was reported by @dooglus in #11429 Tree-SHA512: e7635dc81a181801f42324b72fe9e0a2a7dd00b1dcf5abcbf27fa50938eb9a1fc3065c2321326c3456c48c29ae6504353b02f3d46e6eb2f7b09e46d8fe24388d

Make sure wallet databases have unique fileids. If they don't, throw an error. BDB caches do not work properly when more than one open database has the same fileid, because values written to one database may show up in reads to other databases. Bitcoin will never create different databases with the same fileid, but users can create them by manually copying database files. BDB caching bug was reported by Chris Moore <dooglus@gmail.com> bitcoin#11429 Fixes bitcoin#11429 Github-Pull: bitcoin#11476 Rebased-From: 478a89c

@dooglus

…usly 478a89c Avoid opening copied wallet databases simultaneously (Russell Yanofsky) Pull request description: Make sure wallet databases have unique fileids. If they don't, throw an error. BDB caches do not work properly when more than one open database has the same fileid, because values written to one database may show up in reads to other databases. Bitcoin will never create different databases with the same fileid, but users can create them by manually copying database files. BDB caching bug was reported by @dooglus in bitcoin#11429 Tree-SHA512: e7635dc81a181801f42324b72fe9e0a2a7dd00b1dcf5abcbf27fa50938eb9a1fc3065c2321326c3456c48c29ae6504353b02f3d46e6eb2f7b09e46d8fe24388d

@dooglus

…usly 478a89c Avoid opening copied wallet databases simultaneously (Russell Yanofsky) Pull request description: Make sure wallet databases have unique fileids. If they don't, throw an error. BDB caches do not work properly when more than one open database has the same fileid, because values written to one database may show up in reads to other databases. Bitcoin will never create different databases with the same fileid, but users can create them by manually copying database files. BDB caching bug was reported by @dooglus in bitcoin#11429 Tree-SHA512: e7635dc81a181801f42324b72fe9e0a2a7dd00b1dcf5abcbf27fa50938eb9a1fc3065c2321326c3456c48c29ae6504353b02f3d46e6eb2f7b09e46d8fe24388d

@dooglus

…usly 478a89c Avoid opening copied wallet databases simultaneously (Russell Yanofsky) Pull request description: Make sure wallet databases have unique fileids. If they don't, throw an error. BDB caches do not work properly when more than one open database has the same fileid, because values written to one database may show up in reads to other databases. Bitcoin will never create different databases with the same fileid, but users can create them by manually copying database files. BDB caching bug was reported by @dooglus in bitcoin#11429 Tree-SHA512: e7635dc81a181801f42324b72fe9e0a2a7dd00b1dcf5abcbf27fa50938eb9a1fc3065c2321326c3456c48c29ae6504353b02f3d46e6eb2f7b09e46d8fe24388d

d5526bd Wrap dumpwallet warning and note scripts aren't dumped (MeshCollider) 3711c6a Add wallet backup text to import*, add* and dumpwallet RPCs (MeshCollider) dbda874 [Wallet] always show help-line of wallet encryption calls (Jonas Schnelli) 20c269b Avoid opening copied wallet databases simultaneously (Russell Yanofsky) e411b70 [wallet] Fix leak in CDB constructor (João Barbosa) f15aeea Change getmininginfo errors field to warnings (Andrew Chow) c04390b Unify help text for GetWarnings output in get*info RPCs (random-zebra) 1d966ce Add warnings field to getblockchaininfo (Andrew Chow) ffcd781 [Trivial] Cleanup after MOVE-ONLY commits (random-zebra) e067235 MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp (random-zebra) e947eec MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp (random-zebra) 2188c3e Move some static functions out of wallet.h/cpp (random-zebra) f49acf7 [wallet] [moveonly] Move CAffectedKeysVisitor (random-zebra) 8bd979f [wallet] Specify wallet name in wallet loading errors (random-zebra) 900bbfa Reject invalid wallet files (João Barbosa) a1f4e2a Reject duplicate wallet filenames (random-zebra) ee52c2e Fix misleading "Method not found" multiwallet errors (Russell Yanofsky) ce35e1e [Qt] Use wallet 0 in rpc console if running with multiple wallets (random-zebra) 37089d1 [tests] Update wallet_multiwallet.py functional test (random-zebra) 3955ee9 [Doc] Update release notes (random-zebra) 4fd5913 [wallet] [rpc] Add listwallets RPC (John Newbery) 1525281 [wallet] [rpc] print wallet name in getwalletinfo (John Newbery) fdf5da0 [wallet] fix comment for CWallet::Verify() (John Newbery) cf4a90b Remove factor of 3 from definition of dust. (random-zebra) a1c56fd [Policy] Introduce -dustrelayfee (random-zebra) 9fb29cc [Doc] Update multiwallet release notes (random-zebra) 379255e [Tests][Trivial] Add wallet_multiwallet.py to test_runner (random-zebra) 808fbc3 [Bugfix] consider boolean value of -zapwallettxes ParameterInteraction (random-zebra) f9141f8 [QA] Add wallet_multiwallet.py functional test (John Newbery) 2e02006 Rename -usewallet to -rpcwallet (Alex Morcos) a64440b Select wallet based on the given endpoint (Jonas Schnelli) 5683a9c Complete previous commit by moving mn stuff out of libbitcoin_wallet (random-zebra) b0fe92f Fix test_pivx circular dependency issue (random-zebra) 6cb2b92 Add wallet endpoint support to bitcoin-cli (-usewallet) (Jonas Schnelli) 7dd3916 Register wallet endpoint (Jonas Schnelli) 5bd1bd7 Properly forbid -salvagewallet and -zapwallettxes for multi wallet. (Alex Morcos) 41a7335 Remove unused variables (practicalswift) 5c3d73f Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings (Russell Yanofsky) e7cafab [Refactoring] Mimic ListCoins for sapling notes (random-zebra) 54fa122 [qt] Move some WalletModel functions into CWallet (random-zebra) 494ba64 [test] Add test for getmemoryinfo (random-zebra) 2394083 [Wallet] unset change position when there is no change on exact match (Gregory Sanders) 7d977ac Remove unused C++ code not covered by unit tests (random-zebra) 60bb4da ApproximateBestSubset should take inputs by reference, not value (Ryan Havar) 3633d75 Initialize nRelockTime (Patrick Strateman) 3a599d0 [Refactor] Return safeTx boolean in CheckTXAvailability (random-zebra) f219be9 Add safe flag to listunspent result (NicolasDorier) 0201065 Add COutput::fSafe member for safe handling of unconfirmed outputs (random-zebra) 75c8c6d Disallow copy of CReserveKeys (Gregory Sanders) 8378322 [Refactor] Replace optional reserveKey in PBF with unique pointer (random-zebra) Pull request description: I think these are all the remaining Bitcoin Core v0.15 PRs in the wallet area that we don't have yet, and are useful to us. I've grouped them here since they are all pretty small, simple, and narrow-focused (on the wallet/rpcwallet files). This changeset is based on top of - [x] #2337 as it keeps going with the multiwallet implementation, adding: - RPC endpoint support - `listwallets` RPC - wallet name in `getwalletinfo` RPC - `wallet_multiwallet.py` functional test As in some areas we are much closer to upstream, some of the commits needed adaptations (especially the functional tests). A couple of commits have been extended to include shield-related code. List of upstream PRs backported/adapted: - bitcoin#9906 : Disallow copy constructor CReserveKeys - bitcoin#9830 : Add safe flag to listunspent result - bitcoin#9993 : Initialize nRelockTime - bitcoin#10108 : ApproximateBestSubset should take inputs by reference, not value - bitcoin#10075 : Remove unused C++ code not covered by unit tests - bitcoin#10257 : Add test for getmemoryinfo - bitcoin#10295 : Move some WalletModel functions into CWallet - bitcoin#10500 : Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings - bitcoin#10522 : Remove unused variables - bitcoin#10816 : Properly forbid -salvagewallet and -zapwallettxes for multi wallet - bitcoin#10849 : Multiwallet: simplest endpoint support - bitcoin#9380 : Separate different uses of minimum fees (only dustRelayFee) - bitcoin#10817 : Redefine Dust and add a discard_rate (only first commit) - bitcoin#10883 : Rename -usewallet to -rpcwallet - bitcoin#10604 : Add listwallets RPC, include wallet name in getwalletinfo + tests - bitcoin#10893 : Avoid running multiwallet.py twice - bitcoin#10870 : Use wallet 0 in rpc console if running with multiple wallets - bitcoin#10931 : Fix misleading "method not found" multiwallet errors - bitcoin#10885 : Reject invalid wallets - bitcoin#11022 : Basic keypool topup - bitcoin#10976 : [MOVEONLY] Move some static functions out of wallet.h/cpp - bitcoin#11310 : Test listwallets RPC - bitcoin#10858 : "errors" to getblockchaininfo + unify "errors" field in get*info RPCs - bitcoin#11492 : Fix leak in CDB constructor - bitcoin#11476 : Avoid opening copied wallet databases simultaneously - bitcoin#11590 : always show help-line of wallet encryption calls - bitcoin#11289 : Add wallet backup text to import* and add* RPCs ACKs for top commit: furszy: re-re-utACK d5526bd. Fuzzbawls: ACK d5526bd Tree-SHA512: 115c4916ee212539b0a1b2cb25783ddf6753f5376de739a084191e874ed8717fff2c7cd9d744e397891f14eaa459ef2f48d675168621ef7316e81758fa6559f2

jonasschnelli added Wallet Needs backport labels Oct 10, 2017

promag reviewed Oct 11, 2017

View reviewed changes

laanwj reviewed Oct 11, 2017

View reviewed changes

ryanofsky force-pushed the pr/wid branch from ebc7c2f to e32c2a5 Compare October 11, 2017 19:00

ryanofsky commented Oct 11, 2017

View reviewed changes

promag reviewed Oct 12, 2017

View reviewed changes

ryanofsky force-pushed the pr/wid branch from e32c2a5 to 4164132 Compare October 12, 2017 15:11

laanwj added this to the 0.15.0.2 milestone Oct 12, 2017

ryanofsky force-pushed the pr/wid branch from 4164132 to f988322 Compare October 12, 2017 21:35

ryanofsky force-pushed the pr/wid branch from f988322 to 478a89c Compare October 19, 2017 14:43

laanwj merged commit 478a89c into bitcoin:master Oct 19, 2017

maflcko removed the Needs backport label Nov 1, 2017

morcos mentioned this pull request Nov 2, 2017

0.15: Backports #11592

Merged

ryanofsky mentioned this pull request Jun 8, 2020

wallet: Refactor the classes in wallet/db.{cpp/h} #18971

Closed

8 tasks

random-zebra mentioned this pull request Apr 28, 2021

[Wallet] Laggard wallet-related backports from btc 0.15 PIVX-Project/PIVX#2351

Merged

1 task

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Avoid opening copied wallet databases simultaneously #11476

Avoid opening copied wallet databases simultaneously #11476

Uh oh!

Conversation

ryanofsky commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Oct 10, 2017

Uh oh!

jonasschnelli commented Oct 10, 2017

Uh oh!

meshcollider commented Oct 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 11, 2017 • 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!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 11, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Oct 12, 2017

Uh oh!

jnewbery commented Oct 12, 2017

Uh oh!

ryanofsky commented Oct 12, 2017

Uh oh!

ryanofsky commented Oct 10, 2017 •

edited

Loading

ryanofsky Oct 11, 2017 •

edited

Loading

ryanofsky Oct 11, 2017 •

edited

Loading