Optimise lock behaviour for GuessVerificationProgress() by jonasschnelli · Pull Request #12287 · bitcoin/bitcoin

jonasschnelli · 2018-01-28T19:19:56Z

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 Avoid permanent cs_main/cs_wallet lock during RescanFromTime #11281)
GUI, ClientModel::getVerificationProgress() <--- this may have GUI performance impacts, but could be relaxed later with a cache or something more efficient.

jonasschnelli · 2018-01-28T19:20:08Z

Not meant for 0.16.

practicalswift · 2018-01-29T08:52:39Z

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

promag

utACK.

promag · 2018-01-29T14:24:22Z

src/qt/clientmodel.cpp

        LOCK(cs_main);
        tip = chainActive.Tip();
    }
+    LOCK(cs_main);


Just move the above lock up?

ryanofsky · 2018-01-29T18:21:38Z

src/validation.cpp

 //! Guess how far we are in the verification process at the given block index
+//! needs cs_main due to access of pindex->nChainTx (mutable)
 double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex *pindex) {
+    AssertLockHeld(cs_main);


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.

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

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.

promag · 2018-01-29T21:03:58Z

src/validation.cpp

-        DateTimeStrFormat("%Y-%m-%d %H:%M:%S", chainActive.Tip()->GetBlockTime()),
-        GuessVerificationProgress(chainparams.TxData(), chainActive.Tip()));
+    {
+        LOCK(cs_main);


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 · 2018-01-29T21:06:09Z

src/validation.cpp

 //! Guess how far we are in the verification process at the given block index
+//! needs cs_main due to access of pindex->nChainTx (mutable)
 double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex *pindex) {
+    AssertLockHeld(cs_main);


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

promag · 2018-01-30T23:53:32Z

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

jonasschnelli · 2018-01-31T03:38:42Z

~~@promag 6e36821 seems good work. I'll propose to do this in a separate PR.~~

jonasschnelli · 2018-01-31T03:50:59Z

Changed my mind. Included 6e36821.

Fixed @ryanofsky point about AssertLockHeld()
Followed @promag's advice and moved cs_main lock up (Optimise lock behaviour for GuessVerificationProgress() #12287 (comment))

promag · 2018-01-31T14:24:57Z

Thread #12287 (comment)

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

Nice code simplifications and comments.

Conditional utACK 90ba2df 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 · 2018-02-09T06:55:35Z

Changed the PR title.

promag · 2018-02-22T01:13:03Z

Tested ACK 90ba2df.

Let's get this merged? 🙄

90ba2df 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

…gress() 90ba2df 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 bitcoin#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

laanwj added the Validation label Jan 29, 2018

promag reviewed Jan 29, 2018

View reviewed changes

src/qt/clientmodel.cpp Outdated

LOCK(cs_main);

tip = chainActive.Tip();

}

LOCK(cs_main);

Copy link

Contributor

promag Jan 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just move the above lock up?

ryanofsky reviewed Jan 29, 2018

View reviewed changes

promag reviewed Jan 29, 2018

View reviewed changes

jonasschnelli force-pushed the 2018/01/rescan_lock branch from 5b790b8 to 2ceb308 Compare January 31, 2018 03:47

Fix missing cs_main lock for GuessVerificationProgress()

90ba2df

jonasschnelli force-pushed the 2018/01/rescan_lock branch from 2ceb308 to 90ba2df Compare January 31, 2018 03:49

jonasschnelli changed the title ~~Fix missing cs_main lock for GuessVerificationProgress()~~ Fix missing cs_main lock for GuessVerificationProgress() (and optimise rescan) Jan 31, 2018

ryanofsky reviewed Jan 31, 2018

View reviewed changes

jonasschnelli changed the title ~~Fix missing cs_main lock for GuessVerificationProgress() (and optimise rescan)~~ Optimise lock behaviour for GuessVerificationProgress() Feb 9, 2018

jonasschnelli merged commit 90ba2df into bitcoin:master Feb 25, 2018

promag mentioned this pull request Feb 25, 2018

Interrupt rescan on shutdown request #12507

Merged

maflcko mentioned this pull request May 21, 2019

fix: GuessVerificationProgress requires cs_main lock #15997

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

jonasschnelli commented Jan 28, 2018

Uh oh!

jonasschnelli commented Jan 28, 2018

Uh oh!

practicalswift commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

promag Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

promag Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

promag commented Jan 30, 2018

Uh oh!

jonasschnelli commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Jan 31, 2018

Uh oh!

promag commented Jan 31, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Feb 9, 2018

Uh oh!

promag commented Feb 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

practicalswift commented Jan 29, 2018 •

edited

Loading

promag Jan 29, 2018 •

edited

Loading

jonasschnelli commented Jan 31, 2018 •

edited

Loading