Page MenuHomePhabricator

Merge #11039: Avoid second mapWallet lookup
ClosedPublic

Authored by jasonbcox on Thu, Mar 14, 18:37.

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Project
Commits
rABC812ae90b6cee: Merge #11039: Avoid second mapWallet lookup
Summary

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

Backport of Core PR 11039
https://github.com/bitcoin/bitcoin/pull/11039/files
Completes T550
Depends on D2689

Test Plan

ninja check
test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jasonbcox created this revision.Thu, Mar 14, 18:37
Herald added a reviewer: Restricted Project. · View Herald TranscriptThu, Mar 14, 18:37
Herald added a subscriber: schancel. · View Herald Transcript
jasonbcox added inline comments.Thu, Mar 14, 18:39
src/wallet/wallet.cpp
1157 ↗(On Diff #7707)

Removed the auto from here and renamed some of the it to it2 to prevent shadow warnings.

See https://github.com/bitcoin/bitcoin/pull/11039#issuecomment-324238084 Core still hasn't fixed this.

1191 ↗(On Diff #7707)

See comment above

1258 ↗(On Diff #7707)

See comment above

Fabien requested changes to this revision.Fri, Mar 15, 12:44
Fabien added inline comments.
src/wallet/wallet.cpp
1157 ↗(On Diff #7707)

Why is this case different so that you can reuse the it variable here but not below ?

1276 ↗(On Diff #7707)

Restore

This revision now requires changes to proceed.Fri, Mar 15, 12:44
jasonbcox added inline comments.Fri, Mar 15, 16:24
src/wallet/wallet.cpp
1157 ↗(On Diff #7707)

This line is guaranteed to run in this context for every loop, while the later is not. If you think it would be safer, I can rename this one as well to make it 100% clear. I just didn't really like the idea of having it, it2, and it3... Giving them "better" names may actually complicate merging backports in the future since it won't be clear if something was missed (imo).

1276 ↗(On Diff #7707)

This actually matches the code in Core. Seeing as it meets our linting rules and reduces merge conflicts, I think it's best to keep this.

Fabien accepted this revision.Fri, Mar 15, 17:01
Fabien added inline comments.
src/wallet/wallet.cpp
1157 ↗(On Diff #7707)

OK, thanks for making it clear. I think you can leave it as is.

1276 ↗(On Diff #7707)

I'll leave that up to you intended that this is only comments, and both cases have pros and cons.

This revision is now accepted and ready to land.Fri, Mar 15, 17:01
Closed by commit rABC812ae90b6cee: Merge #11039: Avoid second mapWallet lookup (authored by Wladimir J. van der Laan <laanwj@gmail.com>, committed by jasonbcox). · Explain WhySat, Mar 16, 15:46
This revision was automatically updated to reflect the committed changes.