Merge #18601: wallet: Refactor WalletRescanReserver to use wallet reference

MarcoFalke · MarcoFalke · commit f8102d90889a · 2020-04-19T10:32:43.000-04:00
fc289b7 wallet: Refactor WalletRescanReserver to use wallet reference (João Barbosa) Pull request description: Simple refactor to `WalletRescanReserver` to use wallet reference instead of pointer. Complements #18259. ACKs for top commit: MarcoFalke: ACK fc289b7 Tree-SHA512: b03e33f2d9df2870436aa3284137fd022dd89ea96a1b170fa27f8685ad4f986e6c4ba5975a84966c30d18430a4014d7d8740a1dff2f985c9ef8226ed18e69db9
diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp
@@ -152,7 +152,7 @@ void TestGUI(interfaces::Node& node)
         wallet->SetLastBlockProcessed(105, ::ChainActive().Tip()->GetBlockHash());
     }
     {
-        WalletRescanReserver reserver(wallet.get());
+        WalletRescanReserver reserver(*wallet);
         reserver.reserve();
         CWallet::ScanResult result = wallet->ScanForWalletTransactions(Params().GetConsensus().hashGenesisBlock, 0 /* block height */, {} /* max height */, reserver, true /* fUpdate */);
         QCOMPARE(result.status, CWallet::ScanResult::SUCCESS);
diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
@@ -130,7 +130,7 @@ UniValue importprivkey(const JSONRPCRequest& request)
 
     EnsureLegacyScriptPubKeyMan(*wallet, true);
 
-    WalletRescanReserver reserver(pwallet);
+    WalletRescanReserver reserver(*pwallet);
     bool fRescan = true;
     {
         auto locked_chain = pwallet->chain().lock();
@@ -274,7 +274,7 @@ UniValue importaddress(const JSONRPCRequest& request)
         throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled when blocks are pruned");
     }
 
-    WalletRescanReserver reserver(pwallet);
+    WalletRescanReserver reserver(*pwallet);
     if (fRescan && !reserver.reserve()) {
         throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
     }
@@ -474,7 +474,7 @@ UniValue importpubkey(const JSONRPCRequest& request)
         throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled when blocks are pruned");
     }
 
-    WalletRescanReserver reserver(pwallet);
+    WalletRescanReserver reserver(*pwallet);
     if (fRescan && !reserver.reserve()) {
         throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
     }
@@ -549,7 +549,7 @@ UniValue importwallet(const JSONRPCRequest& request)
         throw JSONRPCError(RPC_WALLET_ERROR, "Importing wallets is disabled when blocks are pruned");
     }
 
-    WalletRescanReserver reserver(pwallet);
+    WalletRescanReserver reserver(*pwallet);
     if (!reserver.reserve()) {
         throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
     }
@@ -1365,7 +1365,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
         }
     }
 
-    WalletRescanReserver reserver(pwallet);
+    WalletRescanReserver reserver(*pwallet);
     if (fRescan && !reserver.reserve()) {
         throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
     }
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
@@ -3540,7 +3540,7 @@ UniValue rescanblockchain(const JSONRPCRequest& request)
                 },
             }.Check(request);
 
-    WalletRescanReserver reserver(pwallet);
+    WalletRescanReserver reserver(*pwallet);
     if (!reserver.reserve()) {
         throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
     }
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
@@ -54,7 +54,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
             wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash());
         }
         AddKey(wallet, coinbaseKey);
-        WalletRescanReserver reserver(&wallet);
+        WalletRescanReserver reserver(wallet);
         reserver.reserve();
         CWallet::ScanResult result = wallet.ScanForWalletTransactions({} /* start_block */, 0 /* start_height */, {} /* max_height */, reserver, false /* update */);
         BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::FAILURE);
@@ -73,7 +73,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
             wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash());
         }
         AddKey(wallet, coinbaseKey);
-        WalletRescanReserver reserver(&wallet);
+        WalletRescanReserver reserver(wallet);
         reserver.reserve();
         CWallet::ScanResult result = wallet.ScanForWalletTransactions(oldTip->GetBlockHash(), oldTip->nHeight, {} /* max_height */, reserver, false /* update */);
         BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS);
@@ -96,7 +96,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
             wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash());
         }
         AddKey(wallet, coinbaseKey);
-        WalletRescanReserver reserver(&wallet);
+        WalletRescanReserver reserver(wallet);
         reserver.reserve();
         CWallet::ScanResult result = wallet.ScanForWalletTransactions(oldTip->GetBlockHash(), oldTip->nHeight, {} /* max_height */, reserver, false /* update */);
         BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::FAILURE);
@@ -118,7 +118,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
             wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash());
         }
         AddKey(wallet, coinbaseKey);
-        WalletRescanReserver reserver(&wallet);
+        WalletRescanReserver reserver(wallet);
         reserver.reserve();
         CWallet::ScanResult result = wallet.ScanForWalletTransactions(oldTip->GetBlockHash(), oldTip->nHeight, {} /* max_height */, reserver, false /* update */);
         BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::FAILURE);
@@ -463,7 +463,7 @@ class ListCoinsTestingSetup : public TestChain100Setup
         bool firstRun;
         wallet->LoadWallet(firstRun);
         AddKey(*wallet, coinbaseKey);
-        WalletRescanReserver reserver(wallet.get());
+        WalletRescanReserver reserver(*wallet);
         reserver.reserve();
         CWallet::ScanResult result = wallet->ScanForWalletTransactions(::ChainActive().Genesis()->GetBlockHash(), 0 /* start_height */, {} /* max_height */, reserver, false /* update */);
         BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS);
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -4023,7 +4023,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
         }
 
         {
-            WalletRescanReserver reserver(walletInstance.get());
+            WalletRescanReserver reserver(*walletInstance);
             if (!reserver.reserve() || (ScanResult::SUCCESS != walletInstance->ScanForWalletTransactions(locked_chain->getBlockHash(rescan_height), rescan_height, {} /* max height */, reserver, true /* update */).status)) {
                 error = _("Failed to rescan the wallet during initialization").translated;
                 return nullptr;
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
@@ -1244,35 +1244,35 @@ void MaybeResendWalletTxs();
 class WalletRescanReserver
 {
 private:
-    CWallet* m_wallet;
+    CWallet& m_wallet;
     bool m_could_reserve;
 public:
-    explicit WalletRescanReserver(CWallet* w) : m_wallet(w), m_could_reserve(false) {}
+    explicit WalletRescanReserver(CWallet& w) : m_wallet(w), m_could_reserve(false) {}
 
     bool reserve()
     {
         assert(!m_could_reserve);
-        std::lock_guard<std::mutex> lock(m_wallet->mutexScanning);
-        if (m_wallet->fScanningWallet) {
+        std::lock_guard<std::mutex> lock(m_wallet.mutexScanning);
+        if (m_wallet.fScanningWallet) {
             return false;
         }
-        m_wallet->m_scanning_start = GetTimeMillis();
-        m_wallet->m_scanning_progress = 0;
-        m_wallet->fScanningWallet = true;
+        m_wallet.m_scanning_start = GetTimeMillis();
+        m_wallet.m_scanning_progress = 0;
+        m_wallet.fScanningWallet = true;
         m_could_reserve = true;
         return true;
     }
 
     bool isReserved() const
     {
-        return (m_could_reserve && m_wallet->fScanningWallet);
+        return (m_could_reserve && m_wallet.fScanningWallet);
     }
 
     ~WalletRescanReserver()
     {
-        std::lock_guard<std::mutex> lock(m_wallet->mutexScanning);
+        std::lock_guard<std::mutex> lock(m_wallet.mutexScanning);
         if (m_could_reserve) {
-            m_wallet->fScanningWallet = false;
+            m_wallet.fScanningWallet = false;
         }
     }
 };

Original file line number	Diff line number	Diff line change
`@@ -152,7 +152,7 @@ void TestGUI(interfaces::Node& node)`
`152`	`152`	`wallet->SetLastBlockProcessed(105, ::ChainActive().Tip()->GetBlockHash());`
`153`	`153`	`}`
`154`	`154`	`{`
`155`		`- WalletRescanReserver reserver(wallet.get());`
	`155`	`+ WalletRescanReserver reserver(*wallet);`
`156`	`156`	`reserver.reserve();`
`157`	`157`	`CWallet::ScanResult result = wallet->ScanForWalletTransactions(Params().GetConsensus().hashGenesisBlock, 0 /* block height /, {} / max height /, reserver, true / fUpdate */);`
`158`	`158`	`QCOMPARE(result.status, CWallet::ScanResult::SUCCESS);`
Original file line number	Diff line number	Diff line change
`@@ -130,7 +130,7 @@ UniValue importprivkey(const JSONRPCRequest& request)`
`130`	`130`
`131`	`131`	`EnsureLegacyScriptPubKeyMan(*wallet, true);`
`132`	`132`
`133`		`- WalletRescanReserver reserver(pwallet);`
	`133`	`+ WalletRescanReserver reserver(*pwallet);`
`134`	`134`	`bool fRescan = true;`
`135`	`135`	`{`
`136`	`136`	`auto locked_chain = pwallet->chain().lock();`
`@@ -274,7 +274,7 @@ UniValue importaddress(const JSONRPCRequest& request)`
`274`	`274`	`throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled when blocks are pruned");`
`275`	`275`	`}`
`276`	`276`
`277`		`- WalletRescanReserver reserver(pwallet);`
	`277`	`+ WalletRescanReserver reserver(*pwallet);`
`278`	`278`	`if (fRescan && !reserver.reserve()) {`
`279`	`279`	`throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");`
`280`	`280`	`}`
`@@ -474,7 +474,7 @@ UniValue importpubkey(const JSONRPCRequest& request)`
`474`	`474`	`throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled when blocks are pruned");`
`475`	`475`	`}`
`476`	`476`
`477`		`- WalletRescanReserver reserver(pwallet);`
	`477`	`+ WalletRescanReserver reserver(*pwallet);`
`478`	`478`	`if (fRescan && !reserver.reserve()) {`
`479`	`479`	`throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");`
`480`	`480`	`}`
`@@ -549,7 +549,7 @@ UniValue importwallet(const JSONRPCRequest& request)`
`549`	`549`	`throw JSONRPCError(RPC_WALLET_ERROR, "Importing wallets is disabled when blocks are pruned");`
`550`	`550`	`}`
`551`	`551`
`552`		`- WalletRescanReserver reserver(pwallet);`
	`552`	`+ WalletRescanReserver reserver(*pwallet);`
`553`	`553`	`if (!reserver.reserve()) {`
`554`	`554`	`throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");`
`555`	`555`	`}`
`@@ -1365,7 +1365,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)`
`1365`	`1365`	`}`
`1366`	`1366`	`}`
`1367`	`1367`
`1368`		`- WalletRescanReserver reserver(pwallet);`
	`1368`	`+ WalletRescanReserver reserver(*pwallet);`
`1369`	`1369`	`if (fRescan && !reserver.reserve()) {`
`1370`	`1370`	`throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");`
`1371`	`1371`	`}`
Original file line number	Diff line number	Diff line change
`@@ -3540,7 +3540,7 @@ UniValue rescanblockchain(const JSONRPCRequest& request)`
`3540`	`3540`	`},`
`3541`	`3541`	`}.Check(request);`
`3542`	`3542`
`3543`		`- WalletRescanReserver reserver(pwallet);`
	`3543`	`+ WalletRescanReserver reserver(*pwallet);`
`3544`	`3544`	`if (!reserver.reserve()) {`
`3545`	`3545`	`throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");`
`3546`	`3546`	`}`
Original file line number	Diff line number	Diff line change
`@@ -4023,7 +4023,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,`
`4023`	`4023`	`}`
`4024`	`4024`
`4025`	`4025`	`{`
`4026`		`- WalletRescanReserver reserver(walletInstance.get());`
	`4026`	`+ WalletRescanReserver reserver(*walletInstance);`
`4027`	`4027`	`if (!reserver.reserve() \|\| (ScanResult::SUCCESS != walletInstance->ScanForWalletTransactions(locked_chain->getBlockHash(rescan_height), rescan_height, {} /* max height /, reserver, true / update */).status)) {`
`4028`	`4028`	`error = _("Failed to rescan the wallet during initialization").translated;`
`4029`	`4029`	`return nullptr;`
Original file line number	Diff line number	Diff line change
`@@ -1244,35 +1244,35 @@ void MaybeResendWalletTxs();`
`1244`	`1244`	`class WalletRescanReserver`
`1245`	`1245`	`{`
`1246`	`1246`	`private:`
`1247`		`- CWallet* m_wallet;`
	`1247`	`+ CWallet& m_wallet;`
`1248`	`1248`	`bool m_could_reserve;`
`1249`	`1249`	`public:`
`1250`		`- explicit WalletRescanReserver(CWallet* w) : m_wallet(w), m_could_reserve(false) {}`
	`1250`	`+ explicit WalletRescanReserver(CWallet& w) : m_wallet(w), m_could_reserve(false) {}`
`1251`	`1251`
`1252`	`1252`	`bool reserve()`
`1253`	`1253`	`{`
`1254`	`1254`	`assert(!m_could_reserve);`
`1255`		`- std::lock_guard<std::mutex> lock(m_wallet->mutexScanning);`
`1256`		`- if (m_wallet->fScanningWallet) {`
	`1255`	`+ std::lock_guard<std::mutex> lock(m_wallet.mutexScanning);`
	`1256`	`+ if (m_wallet.fScanningWallet) {`
`1257`	`1257`	`return false;`
`1258`	`1258`	`}`
`1259`		`- m_wallet->m_scanning_start = GetTimeMillis();`
`1260`		`- m_wallet->m_scanning_progress = 0;`
`1261`		`- m_wallet->fScanningWallet = true;`
	`1259`	`+ m_wallet.m_scanning_start = GetTimeMillis();`
	`1260`	`+ m_wallet.m_scanning_progress = 0;`
	`1261`	`+ m_wallet.fScanningWallet = true;`
`1262`	`1262`	`m_could_reserve = true;`
`1263`	`1263`	`return true;`
`1264`	`1264`	`}`
`1265`	`1265`
`1266`	`1266`	`bool isReserved() const`
`1267`	`1267`	`{`
`1268`		`- return (m_could_reserve && m_wallet->fScanningWallet);`
	`1268`	`+ return (m_could_reserve && m_wallet.fScanningWallet);`
`1269`	`1269`	`}`
`1270`	`1270`
`1271`	`1271`	`~WalletRescanReserver()`
`1272`	`1272`	`{`
`1273`		`- std::lock_guard<std::mutex> lock(m_wallet->mutexScanning);`
	`1273`	`+ std::lock_guard<std::mutex> lock(m_wallet.mutexScanning);`
`1274`	`1274`	`if (m_could_reserve) {`
`1275`		`- m_wallet->fScanningWallet = false;`
	`1275`	`+ m_wallet.fScanningWallet = false;`
`1276`	`1276`	`}`
`1277`	`1277`	`}`
`1278`	`1278`	`};`