refactor: Avoid locking cs_main in ProcessNewBlockHeaders #16793
Conversation
|
Tend to NACK, as this seems like a layer violation |
|
@MarcoFalke I don't understand why, this change makes |
|
NACK as-is, but IsInitialBlockDownload() doesn't need cs_main anyway (it takes it itself if it needs it, and has an atomic cache first), so it looks like you should be able to just drop the cs_mains here wholesale with no other changes. |
|
the lock is needed for |
|
Huh? g_chainstate is set during init (pre-threading) and never edited after. No reason it would need cs_main, either? |
|
I figured that would change with assumeutxo? Maybe @jamesob knows |
|
Later on |
|
...though don't we need cs_main to safely use |
|
Not for the members that are read ( |
|
@MarcoFalke why are you linking that comment? |
|
cs_main might be needed for |
|
Done. |
promag
force-pushed
the
2019-09-csmain-headers
branch
from
e21478b to
3109a1f
Compare
3109a1f refactor: Avoid locking cs_main in ProcessNewBlockHeaders (João Barbosa) Pull request description: Builds on #16774, this change avoids locking `cs_main` in `ProcessNewBlockHeaders` when the tip has changed - in this case the removed lock was necessary to just log a message. Top commit has no ACKs. Tree-SHA512: 31be6d319fa122804f72fa813cec5ed041dd7e4aef3c1921124a1f03016925c43cd4d9a272d80093e77fa7600e3506ef47b7bb821afcbffe01e6be9bceb6dc00
…ders Summary: The locking was added in a [[bitcoin/bitcoin#15615 (comment) | previous pull request]] during the review phase, but the reason for this lock was later [[bitcoin/bitcoin#15615 (review) | removed during the same review]]. Backport of Core [[bitcoin/bitcoin#16793 | PR16793]] Depends on D7634 Test Plan: ``` cmake .. -GNinja -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_BUILD_TYPE=Debug ninja all ckeck-all ``` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7637
…ckHeaders 3109a1f refactor: Avoid locking cs_main in ProcessNewBlockHeaders (João Barbosa) Pull request description: Builds on bitcoin#16774, this change avoids locking `cs_main` in `ProcessNewBlockHeaders` when the tip has changed - in this case the removed lock was necessary to just log a message. Top commit has no ACKs. Tree-SHA512: 31be6d319fa122804f72fa813cec5ed041dd7e4aef3c1921124a1f03016925c43cd4d9a272d80093e77fa7600e3506ef47b7bb821afcbffe01e6be9bceb6dc00
…ckHeaders 3109a1f refactor: Avoid locking cs_main in ProcessNewBlockHeaders (João Barbosa) Pull request description: Builds on bitcoin#16774, this change avoids locking `cs_main` in `ProcessNewBlockHeaders` when the tip has changed - in this case the removed lock was necessary to just log a message. Top commit has no ACKs. Tree-SHA512: 31be6d319fa122804f72fa813cec5ed041dd7e4aef3c1921124a1f03016925c43cd4d9a272d80093e77fa7600e3506ef47b7bb821afcbffe01e6be9bceb6dc00
5 participants
Builds on #16774, this change avoids locking
cs_maininProcessNewBlockHeaderswhen the tip has changed - in this case the removed lock was necessary to just log a message.