Page MenuHomePhabricator

Merge #12287: Optimise lock behaviour for GuessVerificationProgress()
Needs RevisionPublic

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

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Project
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
Branch
pr12287
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5743
Build 9548: Bitcoin ABC Teamcity Staging
Build 9547: arc lint + arc unit

Event Timeline

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

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

This revision now requires changes to proceed.Wed, May 8, 09:43
jasonbcox edited the summary of this revision. (Show Details)Thu, May 9, 00:15
jasonbcox requested review of this revision.Thu, May 9, 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.Thu, May 9, 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.Thu, May 16, 16:05
deadalnix added inline comments.
src/validation.cpp
5699

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.Thu, May 16, 16:05