Page MenuHomePhabricator

Merge #12333: Make CWallet::ListCoins atomic
ClosedPublic

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

Details

Reviewers
jasonbcox
deadalnix
Fabien
markblundeberg
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC3979bf03baa4: Merge #12333: Make CWallet::ListCoins atomic
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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 5857
Build 9774: Bitcoin ABC Teamcity Staging
Build 9773: arc lint + arc unit

Event Timeline

nakihito created this revision.May 15 2019, 00:32
Owners added a reviewer: Restricted Owners Package.May 15 2019, 00:32
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 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)May 20 2019, 17:50
nakihito requested review of this revision.

Fixed summary to reflect Fabien's suggestion.

Fabien requested changes to this revision.Tue, May 21, 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.Tue, May 21, 10:45
nakihito edited the test plan for this revision. (Show Details)Tue, May 28, 19:17
nakihito requested review of this revision.

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

jasonbcox requested changes to this revision.Tue, May 28, 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.Tue, May 28, 22:12
nakihito requested review of this revision.EditedFri, May 31, 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

nakihito edited the summary of this revision. (Show Details)Fri, May 31, 19:17
jasonbcox accepted this revision.Fri, May 31, 23:40
Fabien accepted this revision.Mon, Jun 3, 06:35
This revision is now accepted and ready to land.Mon, Jun 3, 06:35