Merge #9227: Make nWalletDBUpdated atomic to avoid a potential race.

laanwj · laanwj · commit 02464da5e4aa · 2017-02-06T16:14:17.000+01:00
d63ff62 Make nWalletDBUpdated atomic to avoid a potential race. (Patrick Strateman)
diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp
@@ -24,9 +24,6 @@
 using namespace std;
 
 
-unsigned int nWalletDBUpdated;
-
-
 //
 // CDB
 //
diff --git a/src/wallet/db.h b/src/wallet/db.h
@@ -23,8 +23,6 @@
 static const unsigned int DEFAULT_WALLET_DBLOGSIZE = 100;
 static const bool DEFAULT_WALLET_PRIVDB = true;
 
-extern unsigned int nWalletDBUpdated;
-
 class CDBEnv
 {
 private:
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -3691,7 +3691,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
         walletInstance->ScanForWalletTransactions(pindexRescan, true);
         LogPrintf(" rescan      %15dms\n", GetTimeMillis() - nStart);
         walletInstance->SetBestChain(chainActive.GetLocator());
-        nWalletDBUpdated++;
+        CWalletDB::IncrementUpdateCounter();
 
         // Restore wallet transaction metadata after -zapwallettxes=1
         if (GetBoolArg("-zapwallettxes", false) && GetArg("-zapwallettxes", "1") != "2")
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
@@ -15,6 +15,8 @@
 #include "utiltime.h"
 #include "wallet/wallet.h"
 
+#include <atomic>
+
 #include <boost/version.hpp>
 #include <boost/filesystem.hpp>
 #include <boost/foreach.hpp>
@@ -24,51 +26,53 @@ using namespace std;
 
 static uint64_t nAccountingEntryNumber = 0;
 
+static std::atomic<unsigned int> nWalletDBUpdateCounter;
+
 //
 // CWalletDB
 //
 
 bool CWalletDB::WriteName(const string& strAddress, const string& strName)
 {
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
     return Write(make_pair(string("name"), strAddress), strName);
 }
 
 bool CWalletDB::EraseName(const string& strAddress)
 {
     // This should only be used for sending addresses, never for receiving addresses,
     // receiving addresses must always have an address book entry if they're not change return.
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
     return Erase(make_pair(string("name"), strAddress));
 }
 
 bool CWalletDB::WritePurpose(const string& strAddress, const string& strPurpose)
 {
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
     return Write(make_pair(string("purpose"), strAddress), strPurpose);
 }
 
 bool CWalletDB::ErasePurpose(const string& strPurpose)
 {
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
     return Erase(make_pair(string("purpose"), strPurpose));
 }
 
 bool CWalletDB::WriteTx(const CWalletTx& wtx)
 {
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
     return Write(std::make_pair(std::string("tx"), wtx.GetHash()), wtx);
 }
 
 bool CWalletDB::EraseTx(uint256 hash)
 {
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
     return Erase(std::make_pair(std::string("tx"), hash));
 }
 
 bool CWalletDB::WriteKey(const CPubKey& vchPubKey, const CPrivKey& vchPrivKey, const CKeyMetadata& keyMeta)
 {
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
 
     if (!Write(std::make_pair(std::string("keymeta"), vchPubKey),
                keyMeta, false))
@@ -88,7 +92,7 @@ bool CWalletDB::WriteCryptedKey(const CPubKey& vchPubKey,
                                 const CKeyMetadata &keyMeta)
 {
     const bool fEraseUnencryptedKey = true;
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
 
     if (!Write(std::make_pair(std::string("keymeta"), vchPubKey),
             keyMeta))
@@ -106,31 +110,31 @@ bool CWalletDB::WriteCryptedKey(const CPubKey& vchPubKey,
 
 bool CWalletDB::WriteMasterKey(unsigned int nID, const CMasterKey& kMasterKey)
 {
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
     return Write(std::make_pair(std::string("mkey"), nID), kMasterKey, true);
 }
 
 bool CWalletDB::WriteCScript(const uint160& hash, const CScript& redeemScript)
 {
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
     return Write(std::make_pair(std::string("cscript"), hash), *(const CScriptBase*)(&redeemScript), false);
 }
 
 bool CWalletDB::WriteWatchOnly(const CScript &dest)
 {
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
     return Write(std::make_pair(std::string("watchs"), *(const CScriptBase*)(&dest)), '1');
 }
 
 bool CWalletDB::EraseWatchOnly(const CScript &dest)
 {
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
     return Erase(std::make_pair(std::string("watchs"), *(const CScriptBase*)(&dest)));
 }
 
 bool CWalletDB::WriteBestBlock(const CBlockLocator& locator)
 {
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
     Write(std::string("bestblock"), CBlockLocator()); // Write empty block locator so versions that require a merkle branch automatically rescan
     return Write(std::string("bestblock_nomerkle"), locator);
 }
@@ -143,13 +147,13 @@ bool CWalletDB::ReadBestBlock(CBlockLocator& locator)
 
 bool CWalletDB::WriteOrderPosNext(int64_t nOrderPosNext)
 {
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
     return Write(std::string("orderposnext"), nOrderPosNext);
 }
 
 bool CWalletDB::WriteDefaultKey(const CPubKey& vchPubKey)
 {
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
     return Write(std::string("defaultkey"), vchPubKey);
 }
 
@@ -160,13 +164,13 @@ bool CWalletDB::ReadPool(int64_t nPool, CKeyPool& keypool)
 
 bool CWalletDB::WritePool(int64_t nPool, const CKeyPool& keypool)
 {
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
     return Write(std::make_pair(std::string("pool"), nPool), keypool);
 }
 
 bool CWalletDB::ErasePool(int64_t nPool)
 {
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
     return Erase(std::make_pair(std::string("pool"), nPool));
 }
 
@@ -780,20 +784,20 @@ void ThreadFlushWalletDB()
     if (!GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET))
         return;
 
-    unsigned int nLastSeen = nWalletDBUpdated;
-    unsigned int nLastFlushed = nWalletDBUpdated;
+    unsigned int nLastSeen = CWalletDB::GetUpdateCounter();
+    unsigned int nLastFlushed = CWalletDB::GetUpdateCounter();
     int64_t nLastWalletUpdate = GetTime();
     while (true)
     {
         MilliSleep(500);
 
-        if (nLastSeen != nWalletDBUpdated)
+        if (nLastSeen != CWalletDB::GetUpdateCounter())
         {
-            nLastSeen = nWalletDBUpdated;
+            nLastSeen = CWalletDB::GetUpdateCounter();
             nLastWalletUpdate = GetTime();
         }
 
-        if (nLastFlushed != nWalletDBUpdated && GetTime() - nLastWalletUpdate >= 2)
+        if (nLastFlushed != CWalletDB::GetUpdateCounter() && GetTime() - nLastWalletUpdate >= 2)
         {
             TRY_LOCK(bitdb.cs_db,lockDb);
             if (lockDb)
@@ -815,7 +819,7 @@ void ThreadFlushWalletDB()
                     if (_mi != bitdb.mapFileUseCount.end())
                     {
                         LogPrint("db", "Flushing %s\n", strFile);
-                        nLastFlushed = nWalletDBUpdated;
+                        nLastFlushed = CWalletDB::GetUpdateCounter();
                         int64_t nStart = GetTimeMillis();
 
                         // Flush wallet file so it's self contained
@@ -922,19 +926,29 @@ bool CWalletDB::Recover(CDBEnv& dbenv, const std::string& filename)
 
 bool CWalletDB::WriteDestData(const std::string &address, const std::string &key, const std::string &value)
 {
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
     return Write(std::make_pair(std::string("destdata"), std::make_pair(address, key)), value);
 }
 
 bool CWalletDB::EraseDestData(const std::string &address, const std::string &key)
 {
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
     return Erase(std::make_pair(std::string("destdata"), std::make_pair(address, key)));
 }
 
 
 bool CWalletDB::WriteHDChain(const CHDChain& chain)
 {
-    nWalletDBUpdated++;
+    nWalletDBUpdateCounter++;
     return Write(std::string("hdchain"), chain);
 }
+
+void CWalletDB::IncrementUpdateCounter()
+{
+    nWalletDBUpdateCounter++;
+}
+
+unsigned int CWalletDB::GetUpdateCounter()
+{
+    return nWalletDBUpdateCounter;
+}
diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h
@@ -176,10 +176,11 @@ class CWalletDB : public CDB
     //! write the hdchain model (external chain child index counter)
     bool WriteHDChain(const CHDChain& chain);
 
+    static void IncrementUpdateCounter();
+    static unsigned int GetUpdateCounter();
 private:
     CWalletDB(const CWalletDB&);
     void operator=(const CWalletDB&);
-
 };
 
 void ThreadFlushWalletDB();

Original file line number	Diff line number	Diff line change
`@@ -23,8 +23,6 @@`
`23`	`23`	`static const unsigned int DEFAULT_WALLET_DBLOGSIZE = 100;`
`24`	`24`	`static const bool DEFAULT_WALLET_PRIVDB = true;`
`25`	`25`
`26`		`-extern unsigned int nWalletDBUpdated;`
`27`		`-`
`28`	`26`	`class CDBEnv`
`29`	`27`	`{`
`30`	`28`	`private:`
Original file line number	Diff line number	Diff line change
`@@ -15,6 +15,8 @@`
`15`	`15`	`#include "utiltime.h"`
`16`	`16`	`#include "wallet/wallet.h"`
`17`	`17`
	`18`	`+#include <atomic>`
	`19`	`+`
`18`	`20`	`#include <boost/version.hpp>`
`19`	`21`	`#include <boost/filesystem.hpp>`
`20`	`22`	`#include <boost/foreach.hpp>`
`@@ -24,51 +26,53 @@ using namespace std;`
`24`	`26`
`25`	`27`	`static uint64_t nAccountingEntryNumber = 0;`
`26`	`28`
	`29`	`+static std::atomic<unsigned int> nWalletDBUpdateCounter;`
	`30`	`+`
`27`	`31`	`//`
`28`	`32`	`// CWalletDB`
`29`	`33`	`//`
`30`	`34`
`31`	`35`	`bool CWalletDB::WriteName(const string& strAddress, const string& strName)`
`32`	`36`	`{`
`33`		`- nWalletDBUpdated++;`
	`37`	`+ nWalletDBUpdateCounter++;`
`34`	`38`	`return Write(make_pair(string("name"), strAddress), strName);`
`35`	`39`	`}`
`36`	`40`
`37`	`41`	`bool CWalletDB::EraseName(const string& strAddress)`
`38`	`42`	`{`
`39`	`43`	`// This should only be used for sending addresses, never for receiving addresses,`
`40`	`44`	`// receiving addresses must always have an address book entry if they're not change return.`
`41`		`- nWalletDBUpdated++;`
	`45`	`+ nWalletDBUpdateCounter++;`
`42`	`46`	`return Erase(make_pair(string("name"), strAddress));`
`43`	`47`	`}`
`44`	`48`
`45`	`49`	`bool CWalletDB::WritePurpose(const string& strAddress, const string& strPurpose)`
`46`	`50`	`{`
`47`		`- nWalletDBUpdated++;`
	`51`	`+ nWalletDBUpdateCounter++;`
`48`	`52`	`return Write(make_pair(string("purpose"), strAddress), strPurpose);`
`49`	`53`	`}`
`50`	`54`
`51`	`55`	`bool CWalletDB::ErasePurpose(const string& strPurpose)`
`52`	`56`	`{`
`53`		`- nWalletDBUpdated++;`
	`57`	`+ nWalletDBUpdateCounter++;`
`54`	`58`	`return Erase(make_pair(string("purpose"), strPurpose));`
`55`	`59`	`}`
`56`	`60`
`57`	`61`	`bool CWalletDB::WriteTx(const CWalletTx& wtx)`
`58`	`62`	`{`
`59`		`- nWalletDBUpdated++;`
	`63`	`+ nWalletDBUpdateCounter++;`
`60`	`64`	`return Write(std::make_pair(std::string("tx"), wtx.GetHash()), wtx);`
`61`	`65`	`}`
`62`	`66`
`63`	`67`	`bool CWalletDB::EraseTx(uint256 hash)`
`64`	`68`	`{`
`65`		`- nWalletDBUpdated++;`
	`69`	`+ nWalletDBUpdateCounter++;`
`66`	`70`	`return Erase(std::make_pair(std::string("tx"), hash));`
`67`	`71`	`}`
`68`	`72`
`69`	`73`	`bool CWalletDB::WriteKey(const CPubKey& vchPubKey, const CPrivKey& vchPrivKey, const CKeyMetadata& keyMeta)`
`70`	`74`	`{`
`71`		`- nWalletDBUpdated++;`
	`75`	`+ nWalletDBUpdateCounter++;`
`72`	`76`
`73`	`77`	`if (!Write(std::make_pair(std::string("keymeta"), vchPubKey),`
`74`	`78`	`keyMeta, false))`
`@@ -88,7 +92,7 @@ bool CWalletDB::WriteCryptedKey(const CPubKey& vchPubKey,`
`88`	`92`	`const CKeyMetadata &keyMeta)`
`89`	`93`	`{`
`90`	`94`	`const bool fEraseUnencryptedKey = true;`
`91`		`- nWalletDBUpdated++;`
	`95`	`+ nWalletDBUpdateCounter++;`
`92`	`96`
`93`	`97`	`if (!Write(std::make_pair(std::string("keymeta"), vchPubKey),`
`94`	`98`	`keyMeta))`
`@@ -106,31 +110,31 @@ bool CWalletDB::WriteCryptedKey(const CPubKey& vchPubKey,`
`106`	`110`
`107`	`111`	`bool CWalletDB::WriteMasterKey(unsigned int nID, const CMasterKey& kMasterKey)`
`108`	`112`	`{`
`109`		`- nWalletDBUpdated++;`
	`113`	`+ nWalletDBUpdateCounter++;`
`110`	`114`	`return Write(std::make_pair(std::string("mkey"), nID), kMasterKey, true);`
`111`	`115`	`}`
`112`	`116`
`113`	`117`	`bool CWalletDB::WriteCScript(const uint160& hash, const CScript& redeemScript)`
`114`	`118`	`{`
`115`		`- nWalletDBUpdated++;`
	`119`	`+ nWalletDBUpdateCounter++;`
`116`	`120`	`return Write(std::make_pair(std::string("cscript"), hash), (const CScriptBase)(&redeemScript), false);`
`117`	`121`	`}`
`118`	`122`
`119`	`123`	`bool CWalletDB::WriteWatchOnly(const CScript &dest)`
`120`	`124`	`{`
`121`		`- nWalletDBUpdated++;`
	`125`	`+ nWalletDBUpdateCounter++;`
`122`	`126`	`return Write(std::make_pair(std::string("watchs"), (const CScriptBase)(&dest)), '1');`
`123`	`127`	`}`
`124`	`128`
`125`	`129`	`bool CWalletDB::EraseWatchOnly(const CScript &dest)`
`126`	`130`	`{`
`127`		`- nWalletDBUpdated++;`
	`131`	`+ nWalletDBUpdateCounter++;`
`128`	`132`	`return Erase(std::make_pair(std::string("watchs"), (const CScriptBase)(&dest)));`
`129`	`133`	`}`
`130`	`134`
`131`	`135`	`bool CWalletDB::WriteBestBlock(const CBlockLocator& locator)`
`132`	`136`	`{`
`133`		`- nWalletDBUpdated++;`
	`137`	`+ nWalletDBUpdateCounter++;`
`134`	`138`	`Write(std::string("bestblock"), CBlockLocator()); // Write empty block locator so versions that require a merkle branch automatically rescan`
`135`	`139`	`return Write(std::string("bestblock_nomerkle"), locator);`
`136`	`140`	`}`
`@@ -143,13 +147,13 @@ bool CWalletDB::ReadBestBlock(CBlockLocator& locator)`
`143`	`147`
`144`	`148`	`bool CWalletDB::WriteOrderPosNext(int64_t nOrderPosNext)`
`145`	`149`	`{`
`146`		`- nWalletDBUpdated++;`
	`150`	`+ nWalletDBUpdateCounter++;`
`147`	`151`	`return Write(std::string("orderposnext"), nOrderPosNext);`
`148`	`152`	`}`
`149`	`153`
`150`	`154`	`bool CWalletDB::WriteDefaultKey(const CPubKey& vchPubKey)`
`151`	`155`	`{`
`152`		`- nWalletDBUpdated++;`
	`156`	`+ nWalletDBUpdateCounter++;`
`153`	`157`	`return Write(std::string("defaultkey"), vchPubKey);`
`154`	`158`	`}`
`155`	`159`
`@@ -160,13 +164,13 @@ bool CWalletDB::ReadPool(int64_t nPool, CKeyPool& keypool)`
`160`	`164`
`161`	`165`	`bool CWalletDB::WritePool(int64_t nPool, const CKeyPool& keypool)`
`162`	`166`	`{`
`163`		`- nWalletDBUpdated++;`
	`167`	`+ nWalletDBUpdateCounter++;`
`164`	`168`	`return Write(std::make_pair(std::string("pool"), nPool), keypool);`
`165`	`169`	`}`
`166`	`170`
`167`	`171`	`bool CWalletDB::ErasePool(int64_t nPool)`
`168`	`172`	`{`
`169`		`- nWalletDBUpdated++;`
	`173`	`+ nWalletDBUpdateCounter++;`
`170`	`174`	`return Erase(std::make_pair(std::string("pool"), nPool));`
`171`	`175`	`}`
`172`	`176`
`@@ -780,20 +784,20 @@ void ThreadFlushWalletDB()`
`780`	`784`	`if (!GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET))`
`781`	`785`	`return;`
`782`	`786`
`783`		`- unsigned int nLastSeen = nWalletDBUpdated;`
`784`		`- unsigned int nLastFlushed = nWalletDBUpdated;`
	`787`	`+ unsigned int nLastSeen = CWalletDB::GetUpdateCounter();`
	`788`	`+ unsigned int nLastFlushed = CWalletDB::GetUpdateCounter();`
`785`	`789`	`int64_t nLastWalletUpdate = GetTime();`
`786`	`790`	`while (true)`
`787`	`791`	`{`
`788`	`792`	`MilliSleep(500);`
`789`	`793`
`790`		`- if (nLastSeen != nWalletDBUpdated)`
	`794`	`+ if (nLastSeen != CWalletDB::GetUpdateCounter())`
`791`	`795`	`{`
`792`		`- nLastSeen = nWalletDBUpdated;`
	`796`	`+ nLastSeen = CWalletDB::GetUpdateCounter();`
`793`	`797`	`nLastWalletUpdate = GetTime();`
`794`	`798`	`}`
`795`	`799`
`796`		`- if (nLastFlushed != nWalletDBUpdated && GetTime() - nLastWalletUpdate >= 2)`
	`800`	`+ if (nLastFlushed != CWalletDB::GetUpdateCounter() && GetTime() - nLastWalletUpdate >= 2)`
`797`	`801`	`{`
`798`	`802`	`TRY_LOCK(bitdb.cs_db,lockDb);`
`799`	`803`	`if (lockDb)`
`@@ -815,7 +819,7 @@ void ThreadFlushWalletDB()`
`815`	`819`	`if (_mi != bitdb.mapFileUseCount.end())`
`816`	`820`	`{`
`817`	`821`	`LogPrint("db", "Flushing %s\n", strFile);`
`818`		`- nLastFlushed = nWalletDBUpdated;`
	`822`	`+ nLastFlushed = CWalletDB::GetUpdateCounter();`
`819`	`823`	`int64_t nStart = GetTimeMillis();`
`820`	`824`
`821`	`825`	`// Flush wallet file so it's self contained`
`@@ -922,19 +926,29 @@ bool CWalletDB::Recover(CDBEnv& dbenv, const std::string& filename)`
`922`	`926`
`923`	`927`	`bool CWalletDB::WriteDestData(const std::string &address, const std::string &key, const std::string &value)`
`924`	`928`	`{`
`925`		`- nWalletDBUpdated++;`
	`929`	`+ nWalletDBUpdateCounter++;`
`926`	`930`	`return Write(std::make_pair(std::string("destdata"), std::make_pair(address, key)), value);`
`927`	`931`	`}`
`928`	`932`
`929`	`933`	`bool CWalletDB::EraseDestData(const std::string &address, const std::string &key)`
`930`	`934`	`{`
`931`		`- nWalletDBUpdated++;`
	`935`	`+ nWalletDBUpdateCounter++;`
`932`	`936`	`return Erase(std::make_pair(std::string("destdata"), std::make_pair(address, key)));`
`933`	`937`	`}`
`934`	`938`
`935`	`939`
`936`	`940`	`bool CWalletDB::WriteHDChain(const CHDChain& chain)`
`937`	`941`	`{`
`938`		`- nWalletDBUpdated++;`
	`942`	`+ nWalletDBUpdateCounter++;`
`939`	`943`	`return Write(std::string("hdchain"), chain);`
`940`	`944`	`}`
	`945`	`+`
	`946`	`+void CWalletDB::IncrementUpdateCounter()`
	`947`	`+{`
	`948`	`+ nWalletDBUpdateCounter++;`
	`949`	`+}`
	`950`	`+`
	`951`	`+unsigned int CWalletDB::GetUpdateCounter()`
	`952`	`+{`
	`953`	`+ return nWalletDBUpdateCounter;`
	`954`	`+}`