Remove cs_main lock annotation from ChainstateManager.m

ryanofsky commented at 9:28 pm on January 10, 2022: member

BlockManager is a large data structure, and cs_main is not required to take its address or access every part of it. Individual BlockManager fields and methods which do require cs_main like m_block_index and LookupBlockIndex are already annotated separately, and these other annotations describe locking requirements more accurately and do a better job enforcing thread safety.

Since cs_main is not needed to access the address of the m_block object, this commit drops cs_main LOCK calls which were added pointlessly to satisfy this annotation in the past.

Code changes were made by dongcarl, I just wrote the commit description

DrahtBot added the label Validation on Jan 10, 2022

DrahtBot commented at 2:13 am on January 11, 2022: member

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

Conflicts

No conflicts as of last run.

MarcoFalke commented at 9:25 am on January 11, 2022: member

cr ACK 5a1c413a33a64bc23da7f0ccb3bf6a82adaf8525

cs_main is a validation lock. It has nothing to do with writing or reading blocks from disk (other than being historically used for that). Ideally nothing in blockman depends on cs_main.

Remove cs_main lock annotation from ChainstateManager.m_blockman

BlockManager is a large data structure, and cs_main is not required to
take its address or access every part of it. Individual BlockManager
fields and methods which do require cs_main like m_block_index and
LookupBlockIndex are already annotated separately, and these other
annotations describe locking requirements more accurately and do a
better job enforcing thread safety.

Since cs_main is not needed to access the address of the m_block object,
this commit drops cs_main LOCK calls which were added pointlessly to
satisfy this annotation in the past.

Co-authored-by: Carl Dong <contact@carldong.me>

ce95fb36af

DrahtBot added the label Needs rebase on Jan 11, 2022

in src/validation.h:829 in 5a1c413a33 outdated

825@@ -826,7 +826,7 @@ class ChainstateManager
826     std::thread m_load_block;
827     //! A single BlockManager instance is shared across each constructed
828     //! chainstate to avoid duplicating block metadata.
829-    BlockManager m_blockman GUARDED_BY(::cs_main);
830+    BlockManager m_blockman{};

jonatack commented at 1:02 pm on January 11, 2022:

Why add the braces? m_blockman is initialized in the ctor (unless I’m missing something). Question for my benefit :) constructor implementation.

ryanofsky commented at 1:55 pm on January 11, 2022:

re: #24024 (review)

Why add the braces? m_blockman is initialized in the ctor (unless I’m missing something). Question for my benefit :)

Not sure since Carl sent me this commit, but it seems good to clean this up. I dropped the braces just to avoid unnecessary punctuation. The braces would never do anything since BlockManager isn’t a primitive type.

MarcoFalke commented at 2:23 pm on January 11, 2022:

This is the ctor

MarcoFalke commented at 2:31 pm on January 11, 2022:

I liked them, because they clarified that there is no ctor and the initialization happens inline

ryanofsky commented at 2:55 pm on January 11, 2022:

I liked them, because they clarified that there is no ctor and the initialization happens inline

I wouldn’t object to adding them back if people have strong opinions, but they don’t guarantee this. Someone could add a ChainStateMananger constructor in the future which does pass arguments to the m_blockman constructor, and this code would still compile as long as m_blockman also has a default constructor. The nice thing about plain BlockManager m_blockman; is it says “ChainStateMananger has a m_blockman member. If you care about how it will be constructed, go look at the constructor implementation, like you need to do anyway.”

jonatack commented at 9:21 am on January 12, 2022:

Oh indeed, thanks for the responses!

jonatack commented at 1:05 pm on January 11, 2022: member

Light ACK. Attempted removing a few additional related cs_main locks and saw the expected warnings.

ryanofsky force-pushed on Jan 11, 2022

ryanofsky commented at 2:27 pm on January 11, 2022: member

Rebased 5a1c413a33a64bc23da7f0ccb3bf6a82adaf8525 -> ce95fb36af7db6582216adc64f2a66aaa06b55b3 (pr/block.1 -> pr/block.2, compare) due to conflict with #23497

ryanofsky commented at 2:33 pm on January 11, 2022: member

re: #24024 (comment)

cs_main is a validation lock. It has nothing to do with writing or reading blocks from disk (other than being historically used for that). Ideally nothing in blockman depends on cs_main.

That’s a good point. It is important to have some lock guarding things like m_block_index but it it doesn’t need to be cs_main and maybe ideally it would be a more narrow lock. It looks like there is more discussion about this at #22932 (review)

DrahtBot removed the label Needs rebase on Jan 11, 2022

MarcoFalke commented at 3:04 pm on January 11, 2022: member

cr ACK ce95fb36af7db6582216adc64f2a66aaa06b55b3

dongcarl commented at 5:10 pm on January 11, 2022: member

crACK ce95fb36af7db6582216adc64f2a66aaa06b55b3

Thanks for PRing this!

jonatack commented at 9:18 am on January 12, 2022: member

ACK ce95fb36af7db6582216adc64f2a66aaa06b55b3 per git range-diff db1f04f 5a1c413 ce95fb3 change since last push is rebase and dropping braces on the member initialization

MarcoFalke merged this on Jan 12, 2022

MarcoFalke closed this on Jan 12, 2022

sidhujag referenced this in commit a30a4ddf6d on Jan 12, 2022

Fabcien referenced this in commit 593aa43cde on Dec 19, 2022

DrahtBot locked this on Jan 19, 2023

Remove cs_main lock annotation from ChainstateManager.m_blockman #24024

Conflicts