Merge #12333: Make CWallet::ListCoins atomic
ClosedPublic
Actions

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

rSTAGING3979bf03baa4: Merge #12333: Make CWallet::ListCoins atomic
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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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

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

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

Harbormaster completed remote builds in B5857: Diff 8670.May 15 2019, 00:47

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

Fixed summary to reflect Fabien's suggestion.

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

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

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

In D3049#74521, @jasonbcox wrote:

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)May 31 2019, 19:17

jasonbcox accepted this revision.May 31 2019, 23:40

Fabien accepted this revision.Jun 3 2019, 06:35

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

Closed by commit rABC3979bf03baa4: Merge #12333: Make CWallet::ListCoins atomic (authored by Wladimir J. van der Laan <laanwj@gmail.com>, committed by nakihito). · Explain WhyJun 3 2019, 17:25

This revision was automatically updated to reflect the committed changes.

Nico Guiton <nico@bitframe.org> added a commit: rABC3979bf03baa4: Merge #12333: Make CWallet::ListCoins atomic.

Nico Guiton <nico@bitframe.org> added a commit: rSTAGING3979bf03baa4: Merge #12333: Make CWallet::ListCoins atomic.Sep 18 2019, 23:58