Page MenuHomePhabricator

Merge #12287: Optimise lock behaviour for GuessVerificationProgress()
ClosedPublic

Authored by jasonbcox on May 7 2019, 23:18.

Details

Summary

90ba2df11 Fix missing cs_main lock for GuessVerificationProgress() (Jonas Schnelli)

Pull request description:

`GuessVerificationProgress()` needs `cs_main` due to accessing the `pindex->nChainTx`.
This adds a `AssertLockHeld` in `GuessVerificationProgress()` and adds the missing locks in...
* `LoadChainTip()`
* `ScanForWalletTransactions()` (got missed in #11281)
* GUI, `ClientModel::getVerificationProgress()` <--- **this may have GUI performance impacts**, but could be relaxed later with a cache or something more efficient.

Tree-SHA512: 13302946571422375f32af8e396b9d2c1180f2693ea363aeba9e98c8266ddec64fe7862bfdcbb5a93a4b12165a61eec1e51e4e7d7a8515fa50879095dc163412

Backport of Core PR12287
https://github.com/bitcoin/bitcoin/pull/12287/files
Completes T642
Depends on D2979

Test Plan

make check
test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jasonbcox created this revision.May 7 2019, 23:18
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 7 2019, 23:18
Fabien requested changes to this revision.May 8 2019, 09:43

The change in qt/clientmodel.cpp is missing (moved to interfaces/node.cpp).

This revision now requires changes to proceed.May 8 2019, 09:43
jasonbcox edited the summary of this revision. (Show Details)May 9 2019, 00:15
jasonbcox requested review of this revision.May 9 2019, 00:30
In D2980#70322, @Fabien wrote:

The change in qt/clientmodel.cpp is missing (moved to interfaces/node.cpp).

I forgot to mention this: Take a look at the PR that introduced this in interfaces/node.cpp: https://github.com/bitcoin/bitcoin/pull/10244/commits/fe6f27e6ea68a139d3a98b30a53706008ef8b132
You'll notice that the changes in qt/clientmodel.cpp move from what this backport would suggest to the current implementation. Essentially, that backport was out of order and is currently in the correct state.

Fabien added a comment.May 9 2019, 06:47

This sounds strange: the PR adds a comment to state explicitly that GuessVerificationProgress requires cs_main, and a later PR discard the lock in interfaces/nodes.cpp.
Is it possible that the call from the Node interface could not trigger the behavior described in the GuessVerificationProgress comment, so it is safe to not hold the lock ?

In D2980#70690, @Fabien wrote:

This sounds strange: the PR adds a comment to state explicitly that GuessVerificationProgress requires cs_main, and a later PR discard the lock in interfaces/nodes.cpp.
Is it possible that the call from the Node interface could not trigger the behavior described in the GuessVerificationProgress comment, so it is safe to not hold the lock ?

This is a good point. The comment is very clear that the lock needs to be held where ever nChainTx is used (otherwise it may not be set before it's read).

deadalnix requested changes to this revision.May 16 2019, 16:05
deadalnix added inline comments.
src/validation.cpp
5699 ↗(On Diff #8482)

That sounds like something that would benefit from an AssertLockHeld to make sure the debug build can tell us if that comment has been ignored.

This revision now requires changes to proceed.May 16 2019, 16:05
jasonbcox updated this revision to Diff 9125.Jun 4 2019, 20:22

Rebase + added AssertLockHeld with a comment referring to the WIP fix that we can backport in the future.

Fabien requested changes to this revision.Jun 5 2019, 08:44

Intended that there is a known fix for this, why using the assertion rather than applying the fix ?

This revision now requires changes to proceed.Jun 5 2019, 08:44
In D2980#75950, @Fabien wrote:

Intended that there is a known fix for this, why using the assertion rather than applying the fix ?

Because the PR hasn't been merged into Core yet. There's no sense in pulling the fix now in case it diverges.

Alternatively, we can wait until it does get merged, but the fix has been in review for a month already and this backport is quite old (potentially delaying any other backports that build ontop of it).

jasonbcox requested review of this revision.Jun 21 2019, 22:58
Fabien accepted this revision.Jun 24 2019, 15:51

Talked offline about the pros and cons of the AssertLockHeld(). It doesn't break anything unless you are running IBD under debug, but demonstrates that there is an error and what the error is.

This is when I point out that CI dosn't run debug, or maybe it does, who knows. So I don't know if this is a giant regression or if the comment mention a fix that don't need fixing or what.

deadalnix accepted this revision.Jun 25 2019, 20:13
This revision is now accepted and ready to land.Jun 25 2019, 20:13