fix: GuessVerificationProgress requires cs

promag commented at 8:55 pm on May 9, 2019: member

All calls to GuessVerificationProgress, except the ones touched in this change, already have cs_main locked. Make it consistent by adding the remaining locks and the TSAN annotation.

Despite not being always necessary, because if the block is in the active chain then nChainTx is already updated, this is a safe change.

Closes #15994.

DrahtBot added the label Refactoring on May 9, 2019

DrahtBot added the label Validation on May 9, 2019

in src/validation.cpp:2286 in a2699d5aed outdated

2286+        LOCK(::cs_main);
2287+        LogPrintf("%s: new best=%s height=%d version=0x%08x log2_work=%.8g tx=%lu date='%s' progress=%f cache=%.1fMiB(%utxo)", __func__, /* Continued */
2288+          pindexNew->GetBlockHash().ToString(), pindexNew->nHeight, pindexNew->nVersion,
2289+          log(pindexNew->nChainWork.getdouble())/log(2.0), (unsigned long)pindexNew->nChainTx,
2290+          FormatISO8601DateTime(pindexNew->GetBlockTime()),
2291+          GuessVerificationProgress(chainParams.TxData(), pindexNew), pcoinsTip->DynamicMemoryUsage() * (1.0 / (1<<20)), pcoinsTip->GetCacheSize());

MarcoFalke commented at 12:34 pm on May 10, 2019:

0          WITH_LOCK(cs_main, GuessVerificationProgress(chainParams.TxData(), pindexNew)), pcoinsTip->DynamicMemoryUsage() * (1.0 / (1<<20)), pcoinsTip->GetCacheSize());

promag commented at 12:54 pm on May 10, 2019:

I think it should also have the lock while reading nChainTx?

MarcoFalke commented at 1:27 pm on May 10, 2019:

cs_main is already held in UpdateTip, so no need to take it again

promag commented at 5:33 pm on May 12, 2019:

Travis last job failed with:

0validation.cpp:2284:7: error: calling function 'GuessVerificationProgress' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
1      GuessVerificationProgress(chainParams.TxData(), pindexNew), pcoinsTip->DynamicMemoryUsage() * (1.0 / (1<<20)), pcoinsTip->GetCacheSize());

Added

0 /** Check warning conditions and do some notifications on new chain tip set. */
1-void static UpdateTip(const CBlockIndex *pindexNew, const CChainParams& chainParams) {
2+void static UpdateTip(const CBlockIndex *pindexNew, const CChainParams& chainParams) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
3+{

promag force-pushed on May 10, 2019

promag marked this as ready for review on May 10, 2019

promag force-pushed on May 12, 2019

DrahtBot commented at 7:18 pm on May 12, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

promag force-pushed on May 21, 2019

promag commented at 10:10 am on May 21, 2019: member

Updated to use WITH_LOCK(...).

MarcoFalke commented at 10:54 am on May 21, 2019: member

Should remove the (no longer applicable?) comment that was added in 90ba2df11b5bc943ac48b49b5da8023864dc842d?

promag force-pushed on May 21, 2019

promag commented at 11:03 am on May 21, 2019: member

Done.

in src/validation.cpp:4823 in fb61ad7752 outdated

4886@@ -4886,8 +4887,8 @@ bool DumpMempool(const CTxMemPool& pool)
4887 }
4888 
4889 //! Guess how far we are in the verification process at the given block index

MarcoFalke commented at 11:06 am on May 21, 2019:

Should move this comment to the header or remove it?

promag commented at 11:34 am on May 21, 2019:

Removed.

MarcoFalke commented at 11:08 am on May 21, 2019: member

Also, should update OP to why #12287 (review) does not apply

promag force-pushed on May 21, 2019

promag commented at 9:32 am on June 30, 2019: member

@MarcoFalke all done here no?

DrahtBot added the label Needs rebase on Aug 15, 2019

refactor: GuessVerificationProgress requires cs_main lock fb12032530

promag commented at 0:40 am on August 19, 2019: member

Rebased due to #16443.

promag force-pushed on Aug 19, 2019

DrahtBot removed the label Needs rebase on Aug 19, 2019

laanwj commented at 12:53 pm on September 30, 2019: member

Can you clarify (in the PR title) whether this is a bug fix or refactor?

promag commented at 1:58 pm on September 30, 2019: member

Well this changes behavior so I’d say a fix.

Also note that interfaces::Chain::guessVerificationProgress should probably move to interfaces::Chain::Lock, but considering https://github.com/bitcoin/bitcoin/blob/c3a8e097b1afe27bce2f71817fd23cb857236d9d/src/interfaces/chain.h#L59-L64 I’d like to know first if it’s ok cc @ryanofsky

promag renamed this:
~~refactor: GuessVerificationProgress requires cs_main lock~~
fix: GuessVerificationProgress requires cs_main lock
on Sep 30, 2019

MarcoFalke commented at 2:05 pm on September 30, 2019: member

This is not a bugfix. See:

0src/validation.cpp://! require cs_main if pindex has not been validated yet (because nChainTx might be unset)

ryanofsky commented at 2:35 pm on September 30, 2019: member

Also note that interfaces::Chain::guessVerificationProgress should probably move to interfaces::Chain::Lock

As the code comment you linked to suggests, it would not be good to add another method to Chain::Lock, and @ariard has PR to completely remove it in #16426

New wallet code should be written in a more asynchronous way that locks cs_main intermittently instead of bringing the node to a halt when the wallet wants to do a calculation, so it would be preferable to add LOCK(cs_main) to getVerificationProgress if necessary than to move the method.

I haven’t looked closely at the motivating issue, but Marco could also be right about there not being a bug.

promag commented at 2:41 pm on September 30, 2019: member

Stepping back then in favor of #16426.

interfaces::Node::getVerificationProgress already locks so GVP can be called with the lock held. As for NotifyBlockTip and NotifyHeaderTip, these could also notify the the verification progress.

promag closed this on Sep 30, 2019

promag deleted the branch on Sep 30, 2019

DrahtBot locked this on Dec 16, 2021

fix: GuessVerificationProgress requires cs_main lock #15997

Conflicts