Merge #11039: Avoid second mapWallet lookup

8f2f1e0 wallet: Avoid second mapWallet lookup (João Barbosa)

Pull request description:

  All calls to `mapWallet.count()` have the intent to detect if a `txid` exists and most are followed by a second lookup to retrieve the `CWalletTx`.

  This PR replaces all `mapWallet.count()` calls with `mapWallet.find()` to avoid the second lookup.

Tree-SHA512: 96b7de7f5520ebf789a1aec1949a4e9c74e13683869cee012f717e5be8e51097d068e2347a36e89097c9a89f1ed1a1529db71760dac9b572e36a3e9ac1155f29

src/qt/walletmodel.cpp

@@ -577,10 +577,11 @@ void WalletModel::getOutputs(const std::vector<COutPoint>& vOutpoints, std::vect
     LOCK2(cs_main, wallet->cs_wallet);
     for (const COutPoint& outpoint : vOutpoints)
     {
-        if (!wallet->mapWallet.count(outpoint.hash)) continue;
+        auto it = wallet->mapWallet.find(outpoint.hash);
-        int nDepth = wallet->mapWallet[outpoint.hash].GetDepthInMainChain();
+        if (it == wallet->mapWallet.end()) continue;
+        int nDepth = it->second.GetDepthInMainChain();
         if (nDepth < 0) continue;
-        COutput out(&wallet->mapWallet[outpoint.hash], outpoint.n, nDepth, true /* spendable */, true /* solvable */, true /* safe */);
+        COutput out(&it->second, outpoint.n, nDepth, true /* spendable */, true /* solvable */, true /* safe */);
         vOutputs.push_back(out);
     }
 }

src/wallet/feebumper.cpp

@@ -76,12 +76,12 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoin
     vErrors.clear();
     bumpedTxid.SetNull();
     AssertLockHeld(pWallet->cs_wallet);
-    if (!pWallet->mapWallet.count(txid)) {
+    auto it = pWallet->mapWallet.find(txid);
+    if (it == pWallet->mapWallet.end()) {
         vErrors.push_back("Invalid or non-wallet transaction id");
         currentResult = BumpFeeResult::INVALID_ADDRESS_OR_KEY;
         return;
     }
-    auto it = pWallet->mapWallet.find(txid);
     const CWalletTx& wtx = it->second;
 
     if (!preconditionChecks(pWallet, wtx)) {
@@ -241,12 +241,13 @@ bool CFeeBumper::commit(CWallet *pWallet)
     if (!vErrors.empty() || currentResult != BumpFeeResult::OK) {
         return false;
     }
-    if (txid.IsNull() || !pWallet->mapWallet.count(txid)) {
+    auto it = txid.IsNull() ? pWallet->mapWallet.end() : pWallet->mapWallet.find(txid);
+    if (it == pWallet->mapWallet.end()) {
         vErrors.push_back("Invalid or non-wallet transaction id");
         currentResult = BumpFeeResult::MISC_ERROR;
         return false;
     }
-    CWalletTx& oldWtx = pWallet->mapWallet[txid];
+    CWalletTx& oldWtx = it->second;
 
     // make sure the transaction still has no descendants and hasn't been mined in the meantime
     if (!preconditionChecks(pWallet, oldWtx)) {

src/wallet/rpcwallet.cpp

@@ -1874,10 +1874,11 @@ UniValue listsinceblock(const JSONRPCRequest& request)
             throw JSONRPCError(RPC_INTERNAL_ERROR, "Can't read block from disk");
         }
         for (const CTransactionRef& tx : block.vtx) {
-            if (pwallet->mapWallet.count(tx->GetHash()) > 0) {
+            auto it = pwallet->mapWallet.find(tx->GetHash());
+            if (it != pwallet->mapWallet.end()) {
                 // We want all transactions regardless of confirmation count to appear here,
                 // even negative confirmation ones, hence the big negative.
-                ListTransactions(pwallet, pwallet->mapWallet[tx->GetHash()], "*", -100000000, true, removed, filter);
+                ListTransactions(pwallet, it->second, "*", -100000000, true, removed, filter);
             }
         }
         paltindex = paltindex->pprev;
@@ -1957,10 +1958,11 @@ UniValue gettransaction(const JSONRPCRequest& request)
             filter = filter | ISMINE_WATCH_ONLY;
 
     UniValue entry(UniValue::VOBJ);
-    if (!pwallet->mapWallet.count(hash)) {
+    auto it = pwallet->mapWallet.find(hash);
+    if (it == pwallet->mapWallet.end()) {
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid or non-wallet transaction id");
     }
-    const CWalletTx& wtx = pwallet->mapWallet[hash];
+    const CWalletTx& wtx = it->second;
 
     CAmount nCredit = wtx.GetCredit(filter);
     CAmount nDebit = wtx.GetDebit(filter);

src/wallet/wallet.cpp

@@ -623,8 +623,9 @@ void CWallet::AddToSpends(const COutPoint& outpoint, const uint256& wtxid)
 
 void CWallet::AddToSpends(const uint256& wtxid)
 {
-    assert(mapWallet.count(wtxid));
+    auto it = mapWallet.find(wtxid);
-    CWalletTx& thisTx = mapWallet[wtxid];
+    assert(it != mapWallet.end());
+    CWalletTx& thisTx = it->second;
     if (thisTx.IsCoinBase()) // Coinbases don't spend anything!
         return;
 
@@ -1007,8 +1008,9 @@ bool CWallet::LoadToWallet(const CWalletTx& wtxIn)
     wtxOrdered.insert(std::make_pair(wtx.nOrderPos, TxPair(&wtx, nullptr)));
     AddToSpends(hash);
     for (const CTxIn& txin : wtx.tx->vin) {
-        if (mapWallet.count(txin.prevout.hash)) {
+        auto it = mapWallet.find(txin.prevout.hash);
-            CWalletTx& prevtx = mapWallet[txin.prevout.hash];
+        if (it != mapWallet.end()) {
+            CWalletTx& prevtx = it->second;
             if (prevtx.nIndex == -1 && !prevtx.hashUnset()) {
                 MarkConflicted(prevtx.hashBlock, wtx.GetHash());
             }
@@ -1107,8 +1109,9 @@ bool CWallet::AbandonTransaction(const uint256& hashTx)
     std::set<uint256> done;
 
     // Can't mark abandoned if confirmed or in mempool
-    assert(mapWallet.count(hashTx));
+    auto it = mapWallet.find(hashTx);
-    CWalletTx& origtx = mapWallet[hashTx];
+    assert(it != mapWallet.end());
+    CWalletTx& origtx = it->second;
     if (origtx.GetDepthInMainChain() > 0 || origtx.InMempool()) {
         return false;
     }
@@ -1119,8 +1122,9 @@ bool CWallet::AbandonTransaction(const uint256& hashTx)
         uint256 now = *todo.begin();
         todo.erase(now);
         done.insert(now);
-        assert(mapWallet.count(now));
+        auto it = mapWallet.find(now);
-        CWalletTx& wtx = mapWallet[now];
+        assert(it != mapWallet.end());
+        CWalletTx& wtx = it->second;
         int currentconfirm = wtx.GetDepthInMainChain();
         // If the orig tx was not in block, none of its spends can be
         assert(currentconfirm <= 0);
@@ -1145,8 +1149,10 @@ bool CWallet::AbandonTransaction(const uint256& hashTx)
             // available of the outputs it spends. So force those to be recomputed
             for (const CTxIn& txin : wtx.tx->vin)
             {
-                if (mapWallet.count(txin.prevout.hash))
+                auto it = mapWallet.find(txin.prevout.hash);
-                    mapWallet[txin.prevout.hash].MarkDirty();
+                if (it != mapWallet.end()) {
+                    it->second.MarkDirty();
+                }
             }
         }
     }
@@ -1184,8 +1190,9 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx)
         uint256 now = *todo.begin();
         todo.erase(now);
         done.insert(now);
-        assert(mapWallet.count(now));
+        auto it = mapWallet.find(now);
-        CWalletTx& wtx = mapWallet[now];
+        assert(it != mapWallet.end());
+        CWalletTx& wtx = it->second;
         int currentconfirm = wtx.GetDepthInMainChain();
         if (conflictconfirms < currentconfirm) {
             // Block is 'more conflicted' than current confirm; update.
@@ -1204,10 +1211,11 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx)
             }
             // If a transaction changes 'conflicted' state, that changes the balance
             // available of the outputs it spends. So force those to be recomputed
-            for (const CTxIn& txin : wtx.tx->vin)
+            for (const CTxIn& txin : wtx.tx->vin) {
-            {
+                auto it = mapWallet.find(txin.prevout.hash);
-                if (mapWallet.count(txin.prevout.hash))
+                if (it != mapWallet.end()) {
-                    mapWallet[txin.prevout.hash].MarkDirty();
+                    it->second.MarkDirty();
+                }
             }
         }
     }
@@ -1222,10 +1230,11 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, const CBlockIndex *pin
     // If a transaction changes 'conflicted' state, that changes the balance
     // available of the outputs it spends. So force those to be
     // recomputed, also:
-    for (const CTxIn& txin : tx.vin)
+    for (const CTxIn& txin : tx.vin) {
-    {
+        auto it = mapWallet.find(txin.prevout.hash);
-        if (mapWallet.count(txin.prevout.hash))
+        if (it != mapWallet.end()) {
-            mapWallet[txin.prevout.hash].MarkDirty();
+            it->second.MarkDirty();
+        }
     }
 }