Avoid second mapWallet lookup #11039

promag commented at 2:19 PM on August 13, 2017: member

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 commented at 4:35 PM on August 13, 2017: member

Missing mapWallet[] cases.

fanquake added the label Wallet on Aug 14, 2017

jonasschnelli added the label Refactoring on Aug 14, 2017

jonasschnelli commented at 10:33 AM on August 14, 2017: contributor

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

promag commented at 10:34 AM on August 14, 2017: member

Yes already on it.

in src/wallet/feebumper.cpp:245 in 8e1b843103 outdated

 240 | @@ -241,12 +241,13 @@ bool CFeeBumper::commit(CWallet *pWallet)
 241 |      if (!vErrors.empty() || currentResult != BumpFeeResult::OK) {
 242 |          return false;
 243 |      }
 244 | -    if (txid.IsNull() || !pWallet->mapWallet.count(txid)) {
 245 | +    auto it = pWallet->mapWallet.find(txid);
 246 | +    if (txid.IsNull() || it == pWallet->mapWallet.end()) {

ryanofsky commented at 6:46 PM on August 14, 2017:

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.

promag commented at 9:42 PM on August 14, 2017:

@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()) {

ryanofsky commented at 9:56 PM on August 14, 2017:

Thread #11039 (review)

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.

in src/wallet/rpcwallet.cpp:2016 in 8e1b843103 outdated

2012 | @@ -2011,7 +2013,7 @@ UniValue abandontransaction(const JSONRPCRequest& request)
2013 |      uint256 hash;
2014 |      hash.SetHex(request.params[0].get_str());
2015 |  
2016 | -    if (!pwallet->mapWallet.count(hash)) {
2017 | +    if (pwallet->mapWallet.find(hash) == pwallet->mapWallet.end()) {

ryanofsky commented at 6:47 PM on August 14, 2017:

Seems pointlessly verbose.

promag commented at 9:12 PM on August 14, 2017:

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

in src/wallet/wallet.cpp:1022 in 8e1b843103 outdated

1018 | @@ -1017,7 +1019,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockI
1019 |              }
1020 |          }
1021 |  
1022 | -        bool fExisted = mapWallet.count(tx.GetHash()) != 0;
1023 | +        bool fExisted = mapWallet.find(tx.GetHash()) != mapWallet.end();

ryanofsky commented at 6:48 PM on August 14, 2017:

Also unnecessarily verbose.

promag commented at 9:12 PM on August 14, 2017:

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

ryanofsky commented at 6:56 PM on August 14, 2017: member

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.

promag force-pushed on Aug 14, 2017

promag commented at 9:44 PM on August 14, 2017: member

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

wallet: Avoid second mapWallet lookup 8f2f1e0458

promag force-pushed on Aug 14, 2017

ryanofsky commented at 6:44 AM on August 16, 2017: member

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

in src/wallet/rpcwallet.cpp:1965 in 8f2f1e0458

1962 | +    auto it = pwallet->mapWallet.find(hash);
1963 | +    if (it == pwallet->mapWallet.end()) {
1964 |          throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid or non-wallet transaction id");
1965 |      }
1966 | -    const CWalletTx& wtx = pwallet->mapWallet[hash];
1967 | +    const CWalletTx& wtx = it->second;

luke-jr commented at 2:09 PM on August 18, 2017:

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

luke-jr approved

luke-jr commented at 2:13 PM on August 18, 2017: member

utACK, with nit

laanwj commented at 2:26 PM on August 18, 2017: member

utACK 8f2f1e0

laanwj merged this on Aug 18, 2017

laanwj closed this on Aug 18, 2017

laanwj referenced this in commit fc51565cbd on Aug 18, 2017

paveljanik commented at 6:51 AM on August 23, 2017: contributor

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.

PastaPastaPasta referenced this in commit 71215d0c1d on Sep 19, 2019

PastaPastaPasta referenced this in commit 783e321089 on Sep 23, 2019

PastaPastaPasta referenced this in commit c80a7bee84 on Sep 24, 2019

PastaPastaPasta referenced this in commit 3069578075 on Nov 19, 2019

PastaPastaPasta referenced this in commit bc2bed2e47 on Nov 21, 2019

PastaPastaPasta referenced this in commit d6d7a934f6 on Dec 9, 2019

PastaPastaPasta referenced this in commit 9960a2c7a1 on Jan 1, 2020

PastaPastaPasta referenced this in commit 45379c490d on Jan 2, 2020

PastaPastaPasta referenced this in commit 731accf6ed on Jan 2, 2020

PastaPastaPasta referenced this in commit a6342643a3 on Jan 2, 2020

PastaPastaPasta referenced this in commit fcdc80b080 on Jan 2, 2020

PastaPastaPasta referenced this in commit 4a8349e93f on Jan 3, 2020

PastaPastaPasta referenced this in commit 4f18b65d89 on Jan 10, 2020

ckti referenced this in commit b78c659665 on Mar 28, 2021

DrahtBot locked this on Sep 8, 2021