Page MenuHomePhabricator

Merge #11039: Avoid second mapWallet lookup

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



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
Completes T550
Depends on D2689

Test Plan

ninja check

Diff Detail

rABC Bitcoin ABC
Lint Not Applicable
Tests Not Applicable

Event Timeline

1157 ↗(On Diff #7707)

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

See 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.
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)


This revision now requires changes to proceed.Mar 15 2019, 12:44
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.
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.