Paths

Table of Contentst

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

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

Details

Reviewers

deadalnix
Fabien

Group Reviewers

Restricted Project

Commits

rSTAGING6feaf4cb627d: Merge #12287: Optimise lock behaviour for GuessVerificationProgress()
rABC6feaf4cb627d: Merge #12287: Optimise lock behaviour for GuessVerificationProgress()

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 Passed

Unit

No Test Coverage

Build Status

Buildable 6141
Build 10330: Bitcoin ABC Buildbot (legacy)
Build 10329: arc lint + arc unit

Event Timeline

jasonbcox created this revision.May 7 2019, 23:18

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 7 2019, 23:18

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

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

Harbormaster completed remote builds in B5743: Diff 8482.May 7 2019, 23:32

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

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.

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

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

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

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

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

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

Harbormaster completed remote builds in B6141: Diff 9125.Jun 4 2019, 20:37

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

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

Closed by commit rABC6feaf4cb627d: Merge #12287: Optimise lock behaviour for GuessVerificationProgress() (authored by Jonas Schnelli <dev@jonasschnelli.ch>, committed by jasonbcox). · Explain WhyJun 27 2019, 16:37

This revision was automatically updated to reflect the committed changes.

jasonbcox added a commit: rABC6feaf4cb627d: Merge #12287: Optimise lock behaviour for GuessVerificationProgress().

jasonbcox added a commit: rSTAGING6feaf4cb627d: Merge #12287: Optimise lock behaviour for GuessVerificationProgress().Sep 19 2019, 00:38

nakihito mentioned this in D4516: Merge #12639: Reduce cs_main lock and avoid extra lookups of mapAddressBook in listunspent RPC.Nov 25 2019, 19:29