Page MenuHomePhabricator

Merge #11039: Avoid second mapWallet lookup
ClosedPublic

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

Details

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.Mar 15 2019, 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.Mar 15 2019, 12:44
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 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.Mar 15 2019, 17:01
This revision was automatically updated to reflect the committed changes.