Fix bugs in various BDB wallet edge cases #242
Conversation
…point and lsn_reset
…atabase" subdirectory Otherwise, there's a race condition if another program/instance opens a wallet before we delete it
…tdown It's hypothetically possible, albeit difficult to arrange, that other non-loaded wallets are still relying on it
…vironment::Close() This reverts commit 8a06c8cf55f67934a4d86732217e38e42ac5198e.
| if (dbenv->txn_checkpoint(0, 0, 0) != 0) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
May be worth adding extra logging for debugging the underlying BDB error code.
int ret = dbenv->txn_checkpoint(0, 0, 0);
if (ret != 0) {
LogPrintLevel(BCLog::WALLETDB, BCLog::Level::Error, "txn_checkpoint failed: %s\n", DbEnv::strerror(ret));
return false;
}
| if (dbenv->txn_checkpoint(0, 0, 0) != 0) { | ||
| LogPrintLevel(BCLog::WALLETDB, BCLog::Level::Error, "%s: %s checkpoint FAILED\n", __func__, strFile); | ||
| no_dbs_accessed = false; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
When txn_checkpoint fails, the code sets no_dbs_accessed = false and continues to the next file. However, on line 646, nRefCount is set to -1 unconditionally after the loop body.
Should line 646 be inside the success path, or is the current behavior intentional? nRefCount is local and doesn't affect the actual db_it.second.get().m_refcount
There was a problem hiding this comment.
It appears line 646 has always been a no-op. IMO it should be changing db_it.second.get().m_refcount to -1 only on success. Added a commit to fix it.
| if (fShutdown) { | ||
| char** listp; | ||
| if (no_dbs_accessed) { | ||
| dbenv->log_archive(&listp, DB_ARCH_REMOVE); |
There was a problem hiding this comment.
If log_archive fails, should we still call Close()?
There was a problem hiding this comment.
I believe so. log_archive only deletes old/unnecessary log files, so if it fails, it's no big deal.
| err = strprintf(_("Error initializing wallet database environment %s!"), fs::quoted(fs::PathToString(Directory()))); | ||
| if (ret == DB_RUNRECOVERY) { | ||
| 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"); | ||
| err += Untranslated(" ") + _("This error could occur if this wallet was last loaded using a build with a newer version of Berkeley DB."); |
There was a problem hiding this comment.
"was not shutdown cleanly and" is it necessary to remove this?
There was a problem hiding this comment.
I am uncertain that a clean shutdown will guarantee compatibility with different BDB versions, after these changes.
… BerkeleyEnvironment::Flush
2 participants
No description provided.