Avoid second mapWallet lookup #11039

promag · 2017-08-13T14:19:59Z

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.

promag

Missing mapWallet[] cases.

jonasschnelli · 2017-08-14T10:33:22Z

General Concept ACK. I guess extending to cover mapWallet[] would make sense?

promag · 2017-08-14T10:34:16Z

Yes already on it.

ryanofsky

utACK 8e1b843103235d5ded84af5d25ee1530da448d53

I guess extending to cover mapWallet[] would make sense?

Not sure specifically what is meant here, but #9680 replaces [] lookups with at() lookups to avoid inadvertently adding entries to the map when the txid doesn't exist.

ryanofsky · 2017-08-14T18:46:18Z

src/wallet/feebumper.cpp

@@ -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 = pWallet->mapWallet.find(txid);
+    if (txid.IsNull() || it == pWallet->mapWallet.end()) {


Maybe something for a followup commit, but I don't understand the txid IsNull check. If it's expected that there could be a null txid in mapWallet, there should be a comment explaining why that would happen, otherwise the check should be dropped or replaced with an assert or other error indicating that wallet state is corrupted.

@ryanofsky I guess it's to prevent an unnecessary count() in the old code. What about:

auto it = txid.IsNull() ? pWallet->mapWallet.end() : pWallet->mapWallet.find(txid); if (it == pWallet->mapWallet.end()) {

Thread #11039 (comment)

Good idea. I guess if the purpose of the IsNull call was to serve as a minor optimization, the intent is at still discernible, though it's also still unclear. And your new change would match previous behavior more closely, so that's good too.

I think ideally IsNull check would be dropped or actually explained, but that seems like something for a followup. Your new change would be good to add to this PR.

ryanofsky · 2017-08-14T18:47:41Z

src/wallet/rpcwallet.cpp

@@ -2011,7 +2013,7 @@ UniValue abandontransaction(const JSONRPCRequest& request)
    uint256 hash;
    hash.SetHex(request.params[0].get_str());

-    if (!pwallet->mapWallet.count(hash)) {
+    if (pwallet->mapWallet.find(hash) == pwallet->mapWallet.end()) {


Seems pointlessly verbose.

Yeah, and there is no second lookup here so..

ryanofsky · 2017-08-14T18:48:59Z

src/wallet/wallet.cpp

@@ -1017,7 +1019,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockI
            }
        }

-        bool fExisted = mapWallet.count(tx.GetHash()) != 0;
+        bool fExisted = mapWallet.find(tx.GetHash()) != mapWallet.end();


Also unnecessarily verbose.

Yeah, and there is no second lookup here so..

promag · 2017-08-14T21:44:54Z

@jonasschnelli leaving mapWallet[] cases out because of @ryanofsky suggestion above.

ryanofsky

utACK 8f2f1e0. Only changes since previous are what was discussed in comments.

luke-jr

utACK, with nit

luke-jr · 2017-08-18T14:09:46Z

src/wallet/rpcwallet.cpp

        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid or non-wallet transaction id");
    }
-    const CWalletTx& wtx = pwallet->mapWallet[hash];
+    const CWalletTx& wtx = it->second;


IMO also use this in the other cases you're using it->second.

laanwj · 2017-08-18T14:26:36Z

utACK 8f2f1e0

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

paveljanik · 2017-08-23T06:51:19Z

This brought three new -Wshadow warnings:

  CXX      wallet/libbitcoin_wallet_a-wallet.o
wallet/wallet.cpp:1125:14: warning: declaration shadows a local variable [-Wshadow]
        auto it = mapWallet.find(now);
             ^
wallet/wallet.cpp:1112:10: note: previous declaration is here
    auto it = mapWallet.find(hashTx);
         ^
wallet/wallet.cpp:1152:22: warning: declaration shadows a local variable [-Wshadow]
                auto it = mapWallet.find(txin.prevout.hash);
                     ^
wallet/wallet.cpp:1125:14: note: previous declaration is here
        auto it = mapWallet.find(now);
             ^
wallet/wallet.cpp:1215:22: warning: declaration shadows a local variable [-Wshadow]
                auto it = mapWallet.find(txin.prevout.hash);
                     ^
wallet/wallet.cpp:1193:14: note: previous declaration is here
        auto it = mapWallet.find(now);
             ^
3 warnings generated.

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

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 More of 11039

promag commented Aug 13, 2017

View reviewed changes

fanquake added the Wallet label Aug 14, 2017

jonasschnelli added the Refactoring label Aug 14, 2017

ryanofsky mentioned this pull request Aug 14, 2017

Unify CWalletTx construction #9680

Merged

ryanofsky reviewed Aug 14, 2017

View reviewed changes

promag force-pushed the 2017-08-avoid-second-mapwallet-lookup branch from 8e1b843 to f7e13ef Compare August 14, 2017 21:32

wallet: Avoid second mapWallet lookup

8f2f1e0

promag force-pushed the 2017-08-avoid-second-mapwallet-lookup branch from f7e13ef to 8f2f1e0 Compare August 14, 2017 22:10

ryanofsky reviewed Aug 16, 2017

View reviewed changes

luke-jr approved these changes Aug 18, 2017

View reviewed changes

laanwj merged commit 8f2f1e0 into bitcoin:master Aug 18, 2017

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid second mapWallet lookup #11039

Avoid second mapWallet lookup #11039

promag commented Aug 13, 2017 •

edited

promag left a comment

jonasschnelli commented Aug 14, 2017

promag commented Aug 14, 2017

ryanofsky left a comment

ryanofsky Aug 14, 2017

promag Aug 14, 2017 •

edited

ryanofsky Aug 14, 2017

ryanofsky Aug 14, 2017

promag Aug 14, 2017

ryanofsky Aug 14, 2017

promag Aug 14, 2017

promag commented Aug 14, 2017 •

edited

ryanofsky left a comment

luke-jr left a comment

luke-jr Aug 18, 2017

laanwj commented Aug 18, 2017

paveljanik commented Aug 23, 2017

Avoid second mapWallet lookup #11039

Avoid second mapWallet lookup #11039

Conversation

promag commented Aug 13, 2017 • edited

promag left a comment

Choose a reason for hiding this comment

jonasschnelli commented Aug 14, 2017

promag commented Aug 14, 2017

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Aug 14, 2017

Choose a reason for hiding this comment

promag Aug 14, 2017 • edited

Choose a reason for hiding this comment

ryanofsky Aug 14, 2017

Choose a reason for hiding this comment

ryanofsky Aug 14, 2017

Choose a reason for hiding this comment

promag Aug 14, 2017

Choose a reason for hiding this comment

ryanofsky Aug 14, 2017

Choose a reason for hiding this comment

promag Aug 14, 2017

Choose a reason for hiding this comment

promag commented Aug 14, 2017 • edited

ryanofsky left a comment

Choose a reason for hiding this comment

luke-jr left a comment

Choose a reason for hiding this comment

luke-jr Aug 18, 2017

Choose a reason for hiding this comment

laanwj commented Aug 18, 2017

paveljanik commented Aug 23, 2017

promag commented Aug 13, 2017 •

edited

promag Aug 14, 2017 •

edited

promag commented Aug 14, 2017 •

edited