Fix lock in getVerificationProgress
AbandonedPublic
Actions

Authored by jasonbcox on Nov 25 2019, 20:10.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Summary

This fixes our sanitizer builds which are currently red on master.

This is related to the currently open Core issue here: https://github.com/bitcoin/bitcoin/issues/15994
and associated un-merged PR mentioned in the comment in GuessVerificationProgress:

// This function assumes the lock on cs_main is already held (see the
// above comment). This is a temporary check until PR15997 is backported.
// https://github.com/bitcoin/bitcoin/pull/15997
AssertLockHeld(cs_main);

However, PR15997 is currently closed, pending a different approach being reviewed in https://github.com/bitcoin/bitcoin/pull/16426

Since it's unlikely that we'll be pulling in a refactor that large any time soon, we can apply this patch to fix the build in the mean time.

Test Plan

ninja
Run TSAN, ASAN, UBSAN builds on TeamCity

Diff Detail

Repository

rABC Bitcoin ABC

Branch

fixlock

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 8279
Build 14583: Default Diff Build & Tests
Build 14582: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Nov 25 2019, 20:10

Herald added a reviewer: deadalnix. · View Herald TranscriptNov 25 2019, 20:10

jasonbcox planned changes to this revision.Nov 25 2019, 20:10

Build Bitcoin-ABC / Resource Intensive Builds / Resource Intensive Diff Testing started.

Build Bitcoin-ABC / Resource Intensive Builds / Resource Intensive Diff Testing passed.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster completed remote builds in B8279: Diff 14404.Nov 25 2019, 20:51

jasonbcox edited the summary of this revision. (Show Details)Nov 25 2019, 20:56

jasonbcox edited the test plan for this revision. (Show Details)

jasonbcox edited the summary of this revision. (Show Details)

Build Bitcoin-ABC / Diffs / Diff Testing passed.

jasonbcox requested review of this revision.Nov 25 2019, 21:12

As The GuessVerificationProgress doc says:

//! Guess how far we are in the verification process at the given block index
//! require cs_main if pindex has not been validated yet (because nChainTx might
//! be unset)

So this does not seems to be an appropriate measure. Because no description of the problem is provided, it is difficult to suggest any alternative.

This revision now requires changes to proceed.Nov 25 2019, 22:17

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

D4549