Page MenuHomePhabricator

Merge #12333: Make CWallet::ListCoins atomic
ClosedPublic

Authored by nakihito on May 15 2019, 00:32.

Details

Summary

1beea7a [wallet] Make CWallet::ListCoins atomic (João Barbosa)

Pull request description:

Fix a potencial race in `CWallet::ListCoins`.

Replaces `cs_main` and `cs_wallet` locks by assertions in `CWallet::AvailableCoins`.

Tree-SHA512: 09109f44a08b4b53f7605d950ab506d3f748490ab9aed474aa200e93f7b0b9f96f9bf60abe1c5f658240fd13d9e3267c0dd43fd3c1695d82384198ce1da8109f

Backport of Core PR12333
https://github.com/bitcoin/bitcoin/pull/12333/

Note: 2f960b5 [wallet] Indent only change of CWallet::AvailableCoins (João Barbosa) was skipped because our linter made the changes unnecessary. I also verified that cherry-picking this commit resulted in no change to the code.

Test Plan
../configure --enable-debug
make check
make check should not throw any locking errors.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR12333
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5857
Build 9774: Bitcoin ABC Buildbot (legacy)
Build 9773: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.May 15 2019, 00:32
Fabien requested changes to this revision.May 20 2019, 07:15

Please update the summary: only commit 1beea7a [wallet] Make CWallet::ListCoins atomic (João Barbosa) is ported here as the other one does not apply thanks to the linter.
It will make review much easier.

This revision now requires changes to proceed.May 20 2019, 07:15
nakihito edited the summary of this revision. (Show Details)

Fixed summary to reflect Fabien's suggestion.

Fabien requested changes to this revision.May 21 2019, 10:45

You should run the tests with debug enabled when modifying locking behavior.
Add ../configure --enable-debug before make check in your test plan.

This revision now requires changes to proceed.May 21 2019, 10:45
nakihito edited the test plan for this revision. (Show Details)

Ran unit tests under ../configure --enable-debug and changed test plan to reflect this.

jasonbcox requested changes to this revision.May 28 2019, 22:12

I know there's a note regarding intendation for the second commit, but that commit also removes brackets for scoping. IIRC these were removed by ABC in the past, but please verify this and that no prior backports are missed.

This revision now requires changes to proceed.May 28 2019, 22:12
nakihito requested review of this revision.EditedMay 31 2019, 19:16

I know there's a note regarding intendation for the second commit, but that commit also removes brackets for scoping. IIRC these were removed by ABC in the past, but please verify this and that no prior backports are missed.

I tried cherry-pick the other commit as well, but once merge conflicts were fixed, there were no changes to commit. I will update the summary to reflect this more specifically.

Edit: I went through the second commit again to verify the was no missing or out of order backport. I only see one scope that was present in Core's pre-PR, but absent in our. Looks like it was taken out here: https://reviews.bitcoinabc.org/D301

This revision is now accepted and ready to land.Jun 3 2019, 06:35