The implementation in current master can not detect if the file ID is duplicate with flushed BerkeleyEnvironment. This PR would store the file ID in a global variable g_fileids and release it when the BerkeleyDatabase close. So it won’t have to rely on a Db*.
Fix #14304
20@@ -20,6 +21,12 @@
21 #include <boost/thread.hpp>
22
23 namespace {
24+
25+struct BerkeleyFileid {
26+ u_int8_t value[DB_FILE_ID_LEN];
27+};
28+std::unordered_map<std::string, BerkeleyFileid> g_fileids;
I think it’d be more correct to add a std::unordered_map<std::string, BerkeleyFileid> m_fileids member in BerkeleyEnvironment member instead of a g_fileids global because there can be duplicate filenames if the files are in different directories.
Also, making this a member would be consistent with existing mapFileUseCount mapDb members there which are also maps indexed by filename. In the future it could be nice to consolidate the 3 maps.
Great catch, I was surprised to see this bug. It seems like the problem here is that BerkeleyEnvironment::Flush actually fully unloads databases, so the check for duplicate fileids when opening a new database doesn’t work at all after flushing.
I wonder if you could make a python test case out of the steps in #14304.
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.
48- if (item.second && item.second->get_mpf()->get_fileid(item_fileid) == 0 &&
49- memcmp(fileid, item_fileid, sizeof(fileid)) == 0) {
50- const char* item_filename = nullptr;
51- item.second->get_dbname(&item_filename, nullptr);
52+ for (const auto& item : env.m_fileids) {
53+ if (item.first != filename && memcmp(fileid.value, item.second.value, sizeof(fileid)) == 0) {
memcmp to BerkeleyFileid::operator==?
56- item_filename ? item_filename : "(unknown database)"));
57+ HexStr(std::begin(item.second.value), std::end(item.second.value)), item.first));
58 }
59 }
60+
61+ env.m_fileids.emplace(filename, fileid);
CheckUniqueFileid is called on all BerkeleyEnvironment instances, this will incorrectly add the filename to unrelated BerkeleyEnvironment instances that don’t actually contain filename. I’d suggest dropping this line, adding a BerkeleyFileid& fileid output parameter to CheckUniqueFileid, and passing in this->env->m_fileids[strFilename] where CheckUniqueFileid is called in BerkeleyBatch::BerkeleyBatch.
88@@ -89,7 +89,7 @@ bool FileLock::TryLock()
89 return false;
90 }
91 _OVERLAPPED overlapped = {0};
92- if (!LockFileEx(hFile, LOCKFILE_EXCLUSIVE_LOCK | LOCKFILE_FAIL_IMMEDIATELY, 0, 0, 0, &overlapped)) {
93+ if (!LockFileEx(hFile, LOCKFILE_EXCLUSIVE_LOCK | LOCKFILE_FAIL_IMMEDIATELY, 0, -1, -1, &overlapped)) {
In commit “util: Fix broken Windows file lock”
Can you describe how the lock is broken in the commit message?
That was introduced by #13862. I assume that it could work with length 0 if it is a exclusive lock, but it doesn’t. I would have to specify -1, -1 to make it work.
The boost implementation:
825@@ -826,6 +826,8 @@ void BerkeleyDatabase::Flush(bool shutdown)
826 LOCK(cs_db);
env would be a wild pointer after the first call.
Thread #14320 (review)
Good catch indeed. The bug was not introduced in 0b82bac but in a769461?
0b82bac76d0f842bd2294a290388536951fbc576 added the call to erase the environment too early and a769461d5e37ddcb771ae836254fdc69177a28c4 just moved the call, IIUC.
502@@ -503,8 +503,8 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo
503 // be implemented, so no equality checks are needed at all. (Newer
504 // versions of BDB have an set_lk_exclusive method for this
505 // purpose, but the older version we use does not.)
506- for (const auto& env : g_dbenvs) {
507- CheckUniqueFileid(env.second, strFilename, *pdb_temp);
508+ for (auto& env : g_dbenvs) {
auto& env to const auto& env here.
47- if (item.second && item.second->get_mpf()->get_fileid(item_fileid) == 0 &&
48- memcmp(fileid, item_fileid, sizeof(fileid)) == 0) {
49- const char* item_filename = nullptr;
50- item.second->get_dbname(&item_filename, nullptr);
51+ for (const auto& item : env.m_fileids) {
52+ if (item.first != filename && fileid == item.second) {
I think item.first != filename condition makes the check too loose again because the same filename can exist in different directories (and we’d expect pretty much all wallet directories to have a wallet.dat file).
Perhaps change condition to: if (item.second == fileid && !(&item.second == &fileid))
50@@ -56,6 +51,11 @@ CCriticalSection cs_db;
51 std::map<std::string, BerkeleyEnvironment> g_dbenvs GUARDED_BY(cs_db); //!< Map from directory name to open db environment.
52 } // namespace
53
54+bool BerkeleyFileid::operator==(const BerkeleyFileid& b) const
55+{
56+ return memcmp(this, &b, sizeof(BerkeleyFileid)) == 0;
It might be safer if memcmp compared just the value field rather than the whole object, since it seems possible the compiler could pad the objects. Maybe:
0return memcmp(this->field, b.field, sizeof(this->field)) == 0;
825@@ -826,6 +826,8 @@ void BerkeleyDatabase::Flush(bool shutdown)
826 LOCK(cs_db);
827 g_dbenvs.erase(env->Directory().string());
828 env = nullptr;
829+ } else {
m_fileds.erase if shutdown is true? It seems ok to only erase if shutdown is false, but if there is a specific reason for not erasing when shutting down, it would be useful to note in a code comment.
g_dbenvs.erase called, the map doesn’t exist anymore.
Is this for backport?
Could be backported, but I think the benefit would be very minimal. I think you have to literally copy a wallet database file and use the loadwallet RPC in order to trigger the missing error check that this PR restores.
| Coverage | Change (pull 14320) | Reference (master) |
|---|---|---|
| Lines | +0.0019 % | 87.0361 % |
| Functions | -0.0049 % | 84.1130 % |
| Branches | -0.0008 % | 51.5451 % |
74 wallet_names.remove('w7_symlink')
75+ wallet_exclude = {'w7'}
76 extra_args = ['-wallet={}'.format(n) for n in wallet_names]
77 self.start_node(0, extra_args)
78- assert_equal(set(map(lambda w: w['name'], self.nodes[0].listwalletdir()['wallets'])), set(['', 'w3', 'w2', 'sub/w5', 'w7', 'w7', 'w1', 'w8', 'w']))
79+ assert_equal(set(map(lambda w: w['name'], self.nodes[0].listwalletdir()['wallets'])), set(['', 'w3', 'w2', os.path.join('sub', 'w5'), 'w7', 'w7', 'w1', 'w8', 'w']) - wallet_exclude)
‘w7’, ‘w7’
Code precedes this PR, but I think listing w7 twice has no effect.
825@@ -826,6 +826,10 @@ void BerkeleyDatabase::Flush(bool shutdown)
826 LOCK(cs_db);
827 g_dbenvs.erase(env->Directory().string());
828 env = nullptr;
829+ } else {
830+ // To avoid accessing a map that has already deconstructed, do not call this when shutdown is true. g_dbenvs.erase would also erase this value.
831+ // TODO: get rid of wild pointers
832+ env->m_fileids.erase(strFile);
It might not be clear what “wild pointers” refers to in this todo (laanwj asked about it here: #14531 (review)). Wild pointers could mean either dead pointers or raw pointers, but I think the TODO is referring to the bug described here: #14320 (review). Maybe could be expanded as:
0// TODO: To avoid g_dbenvs.erase erasing the environment prematurely after the
1// first database shutdown when multiple databases are open in the same
2// environment, should replace raw database `env` pointers with shared or weak
3// pointers, or else separate the database and environment shutdowns so
4// environments can be shut down after databases.
24 #include <db_cxx.h>
25
26 static const unsigned int DEFAULT_WALLET_DBLOGSIZE = 100;
27 static const bool DEFAULT_WALLET_PRIVDB = true;
28
29+struct BerkeleyFileid {
ef16fc5 -> 4ea7732
