Merge #11039: Avoid second mapWallet lookup
ClosedPublic
Actions

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

Details

Reviewers

deadalnix
Fabien

Group Reviewers

Restricted Project

Commits

rSTAGING812ae90b6cee: Merge #11039: Avoid second mapWallet lookup
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

Branch

pr11039

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 5220
Build 8503: Bitcoin ABC Buildbot (legacy)
Build 8502: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Mar 14 2019, 18:37

Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 14 2019, 18:37

Herald added a subscriber: schancel. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

jasonbcox added inline comments.Mar 14 2019, 18:39

src/wallet/wallet.cpp
1157	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	See comment above
1258	See comment above

jasonbcox edited the summary of this revision. (Show Details)Mar 14 2019, 18:39

jasonbcox added a parent revision: D2689: Merge #9622: [rpc] listsinceblock should include lost transactions when parameter is a reorg'd block.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5220: Diff 7707.Mar 14 2019, 18:52

Fabien requested changes to this revision.Mar 15 2019, 12:44

Fabien added inline comments.

src/wallet/wallet.cpp
1157	Why is this case different so that you can reuse the `it` variable here but not below ?
1276	Restore

This revision now requires changes to proceed.Mar 15 2019, 12:44

jasonbcox added inline comments.Mar 15 2019, 16:24

src/wallet/wallet.cpp
1157	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	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.Mar 15 2019, 17:01

Fabien added inline comments.

src/wallet/wallet.cpp
1157	OK, thanks for making it clear. I think you can leave it as is.
1276	I'll leave that up to you intended that this is only comments, and both cases have pros and cons.