Avoid second mapWallet lookup by promag · Pull Request #11039 · bitcoin/bitcoin

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

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

Seems pointlessly verbose.

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

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

src/wallet/wallet.cpp

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

Conversation

promag commented Aug 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Aug 14, 2017

Uh oh!

promag commented Aug 14, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Aug 14, 2017

Choose a reason for hiding this comment

Uh oh!

promag Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Aug 14, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Aug 14, 2017

Choose a reason for hiding this comment

Uh oh!

promag Aug 14, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Aug 14, 2017

Choose a reason for hiding this comment

Uh oh!

promag Aug 14, 2017

Choose a reason for hiding this comment

Uh oh!

promag commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr Aug 18, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj commented Aug 18, 2017

Uh oh!

paveljanik commented Aug 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

promag commented Aug 13, 2017 •

edited

Loading

promag Aug 14, 2017 •

edited

Loading

promag commented Aug 14, 2017 •

edited

Loading