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 https://github.com/bitcoin/bitcoin/issues/11429
402@@ -402,11 +403,41 @@ CDB::CDB(CWalletDBWrapper& dbw, const char* pszMode, bool fFlushOnCloseIn) : pdb
403 0);
404
405 if (ret != 0) {
406+ error = strprintf("CDB: Error %d, can't open database %s", ret, strFilename);
CDB: Can't open database %s (open error %d)".
414+ // fileid, but this error can be triggered if users manually
415+ // copy database files.
416+ u_int8_t fileid[DB_FILE_ID_LEN];
417+ ret = pdb->get_mpf()->get_fileid(fileid);
418+ if (ret != 0) {
419+ error = strprintf("CDB: Can't open database %s (get_fileid error %d)\n", strFilename, ret);
423+ u_int8_t item_fileid[DB_FILE_ID_LEN];
424+ if (item.second && item.second->get_mpf()->get_fileid(item_fileid) == 0 &&
425+ memcmp(fileid, item_fileid, sizeof(fileid)) == 0) {
426+ const char* item_filename = nullptr;
427+ item.second->get_dbname(&item_filename, nullptr);
428+ error = strprintf("CDB: Can't open database %s (duplicates fileid %s from %s)", strFilename,
+= std::endl + instead?
29@@ -29,6 +30,10 @@ def run_test(self):
30 os.mkdir(os.path.join(self.options.tmpdir, 'node0', 'regtest', 'w11'))
31 self.assert_start_raises_init_error(0, ['-wallet=w11'], 'Error loading wallet w11. -wallet filename must be a regular file.')
32
33+ # should not initialize if one wallet is a copy of another
34+ shutil.copyfile(self.options.tmpdir + "/node0/regtest/w2", self.options.tmpdir + "/node0/regtest/w22")
os.path.join?
407+ } else {
408+ // Make sure database has a unique fileid. If it doesn't, throw
409+ // an error. BDB caches do not work properly when more than one
410+ // open database has the same fileid (values written to one
411+ // database may show up in reads to other databases).
412+ //
417+ ret = pdb->get_mpf()->get_fileid(fileid);
418+ if (ret != 0) {
419+ error = strprintf("CDB: Can't open database %s (get_fileid error %d)\n", strFilename, ret);
420+ }
421+
422+ for (const auto& item : env->mapDb) {
for when ret == 0?
426+ const char* item_filename = nullptr;
427+ item.second->get_dbname(&item_filename, nullptr);
428+ error = strprintf("CDB: Can't open database %s (duplicates fileid %s from %s)", strFilename,
429+ HexStr(std::begin(item_fileid), std::end(item_fileid)),
430+ item_filename ? item_filename : "");
431+ }
436 delete pdb;
437 pdb = nullptr;
438 --env->mapFileUseCount[strFile];
439 strFile = "";
440- throw std::runtime_error(strprintf("CDB: Error %d, can't open database %s", ret, strFilename));
441+ throw std::runtime_error(error);
423+ // Bitcoin will never create different databases with the same
424+ // fileid, but this error can be triggered if users manually
425+ // copy database files.
426+ for (const auto& item : env->mapDb) {
427+ u_int8_t item_fileid[DB_FILE_ID_LEN];
428+ if (item.second && item.second->get_mpf()->get_fileid(item_fileid) == 0 &&
ret = ... pattern like above to make easier to read?
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 e32c2a51b757ab764ec5dbe8bf04b3fc4aa3cb5f -> 4164132f6b475d3a3b4f1492f22f515cdb98fcd8 (pr/wid.2 -> pr/wid.3)
Tested ACK 4164132f6b475d3a3b4f1492f22f515cdb98fcd8
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).
Thanks, updated comment 4164132f6b475d3a3b4f1492f22f515cdb98fcd8 -> f98832222c17c25814b9a18859e72f5041dfb55d (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.
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:
0- u_int8_t fileid[DB_FILE_ID_LEN] = {0};
1+ u_int8_t fileid[DB_FILE_ID_LEN];
Was that intentional?
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.
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>
https://github.com/bitcoin/bitcoin/issues/11429
Fixes #11429