Optimise lock behaviour for GuessVerificationProgress() #12287

pull jonasschnelli wants to merge 1 commits into bitcoin:master from jonasschnelli:2018/01/rescan_lock changing 3 files +6 −9

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
Related:
- #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

Contributors

Labels

Linked (view graph)

#11281 Avoid permanent cs_main/cs_wallet lock during RescanFromTime