Optimise lock behaviour for GuessVerificationProgress()

jonasschnelli commented at 7:19 pm on January 28, 2018: contributor

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.

jonasschnelli commented at 7:20 pm on January 28, 2018: contributor

Not meant for 0.16.

practicalswift commented at 8:52 am on January 29, 2018: contributor

#11652 (“Add missing locks to init.cpp (in AppInitMain + ThreadImport) and validation.cpp”)
#11694 (“rpc: Add missing cs_main lock in getblocktemplate(…)”)
#11754 (“wallet: Add missing cs_wallet locks when accessing m_last_block_processed”)

laanwj added the label Validation on Jan 29, 2018

in src/qt/clientmodel.cpp:146 in 61b20a100d outdated

142@@ -143,6 +143,7 @@ double ClientModel::getVerificationProgress(const CBlockIndex *tipIn) const
143         LOCK(cs_main);
144         tip = chainActive.Tip();
145     }
146+    LOCK(cs_main);

promag commented at 2:24 pm on January 29, 2018:

Just move the above lock up?

promag commented at 2:24 pm on January 29, 2018: member

utACK.

in src/validation.cpp:4667 in 61b20a100d outdated

4661@@ -4659,7 +4662,9 @@ bool DumpMempool(void)
4662 }
4663 
4664 //! Guess how far we are in the verification process at the given block index
4665+//! needs cs_main due to access of pindex->nChainTx (mutable)
4666 double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex *pindex) {
4667+    AssertLockHeld(cs_main);

ryanofsky commented at 6:21 pm on January 29, 2018:

I think maybe the new lock and assert here could be dropped and comment above could be changed to: “require cs_main if pindex has not been validated yet (because nChainTx might be unset).”

If we’re calling this on blocks from chainActive, it seems like locking should be unnecessary.

promag commented at 9:06 pm on January 29, 2018:

After 3d40869, ClientModel::getVerificationProgress is the only caller without the lock, so AssertLockHeld seems fine to me.

ryanofsky commented at 9:32 pm on January 29, 2018:

AssertLockHeld seems fine to me

Getting rid of this could avoid a new lock in Rescan, and remove an existing lock. It also seems not ideal if code and documentation implies locking is required when it isn’t.

in src/validation.cpp:3807 in 61b20a100d outdated

3802@@ -3803,10 +3803,13 @@ bool LoadChainTip(const CChainParams& chainparams)
3803 
3804     g_chainstate.PruneBlockIndexCandidates();
3805 
3806-    LogPrintf("Loaded best chain: hashBestChain=%s height=%d date=%s progress=%f\n",
3807-        chainActive.Tip()->GetBlockHash().ToString(), chainActive.Height(),
3808-        DateTimeStrFormat("%Y-%m-%d %H:%M:%S", chainActive.Tip()->GetBlockTime()),
3809-        GuessVerificationProgress(chainparams.TxData(), chainActive.Tip()));
3810+    {
3811+        LOCK(cs_main);

promag commented at 9:03 pm on January 29, 2018:

The whole function should lock cs_main. However this is only called in AppInitMain, and #11041 adds the lock there.

If this goes first, I suggest cherry pick 3d40869 instead.

promag commented at 11:53 pm on January 30, 2018: member

@jonasschnelli please see 6e36821 which came after discussing with @ryanofsky on IRC.

jonasschnelli commented at 3:38 am on January 31, 2018: contributor

~~@promag https://github.com/bitcoin/bitcoin/commit/6e36821823b498c307787543ad59b9e9ccffcc63 seems good work. I’ll propose to do this in a separate PR.~~

jonasschnelli force-pushed on Jan 31, 2018

Fix missing cs_main lock for GuessVerificationProgress() 90ba2df11b

jonasschnelli force-pushed on Jan 31, 2018

jonasschnelli commented at 3:50 am on January 31, 2018: contributor

Changed my mind. Included https://github.com/bitcoin/bitcoin/commit/6e36821823b498c307787543ad59b9e9ccffcc63.

Fixed @ryanofsky point about AssertLockHeld()
Followed @promag’s advice and moved cs_main lock up (https://github.com/bitcoin/bitcoin/pull/12287#discussion_r164444275)

jonasschnelli renamed this:
~~Fix missing cs_main lock for GuessVerificationProgress()~~
Fix missing cs_main lock for GuessVerificationProgress() (and optimise rescan)
on Jan 31, 2018

promag commented at 2:24 pm on January 31, 2018: member

Thread #12287 (review)

In this case, where nChainTx may require the lock and GuessVerificationProgress() is always called with the lock, I’d prefer to assert the lock and be correct like @gmaxwell said in IRC.

Other than that, utACK 90ba2df.

ryanofsky commented at 8:24 pm on January 31, 2018: member

Nice code simplifications and comments.

Conditional utACK 90ba2df11b5bc943ac48b49b5da8023864dc842d if PR title (“Fix missing cs_main lock for GuessVerificationProgress”) is updated, because the added locking is just defensive, not because locks are missing. Also think adding the assert would be misleading for the same reason.

jonasschnelli renamed this:
~~Fix missing cs_main lock for GuessVerificationProgress() (and optimise rescan)~~
Optimise lock behaviour for GuessVerificationProgress()
on Feb 9, 2018

jonasschnelli commented at 6:55 am on February 9, 2018: contributor

Changed the PR title.

promag commented at 1:13 am on February 22, 2018: member

Tested ACK 90ba2df.

Let’s get this merged? 🙄

jonasschnelli merged this on Feb 25, 2018

jonasschnelli closed this on Feb 25, 2018

jonasschnelli referenced this in commit bf3353de90 on Feb 25, 2018

PastaPastaPasta referenced this in commit 6b43c67c05 on Jun 9, 2020

PastaPastaPasta referenced this in commit 0ad89acaee on Jun 9, 2020

PastaPastaPasta referenced this in commit 3773cad14d on Jun 10, 2020

PastaPastaPasta referenced this in commit 31a01211c1 on Jun 10, 2020

PastaPastaPasta referenced this in commit fe6b44d228 on Jun 11, 2020

PastaPastaPasta referenced this in commit f1ccbca7e3 on Jun 11, 2020

DrahtBot locked this on Sep 8, 2021

Optimise lock behaviour for GuessVerificationProgress() #12287