It is a design smell to have a variable without GUARDED_BY() and rely on EXCLUSIVE_LOCKS_REQUIRED() on some methods to protect it. This way, if the code goes wrong (like, new access is added to the variable from a function not marked with EXCLUSIVE_LOCKS_REQUIRED()) then the compiler has no way to detect that. This defeats the purpose of thread safety annotations which is to enforce proper access and detect mistakes if the code goes wrong.
These are mostly incorrect claims made without specific reasoning or justification. If you have an inner variable that does not require lucking, and an outer data structure containing the variable, or an outer pointer pointing to the variable, or an outer function accessing the variable, and the outer container/pointer/function does require locking, the correct way of annotating the code is to not annotate the variable, and to annotate the outer container/pointer/function.
If you look at the actual code I posted in #24199 (review), you can see that the compiler will detect all unsafe accesses. If you want to talk about a code smell, NO_THREAD_SAFETY_ANALYSIS is the code smell, and the design in that post shows how to get rid of it.
That said, there is a difference between the bigger change I posted yesterday #24199 (review), and the smaller suggestion #24199 (review) I posted two days ago. In both cases the compiler can guarantee thread safety outside of chain.h/chain.cpp if these files are annotated correctly. But in the bigger change where I split up CBlockIndex class into separate BlockInfo / BlockState / BlockReference components, it’s easy to annotate everything correctly, while in the smaller change where CBlockIndex is kept as a monolithic class, what you are saying about it being trickier to see notice missing EXCLUSIVE_LOCKS_REQUIRED annotations on CBlockIndex methods is true.
If you want to argue that using incorrect annotations is better than using correct ones for practical reasons (e.g. it is better to use NO_THREAD_SAFETY_ANALYSIS in one method, so the rest of the file is easier to maintain), that’s fine. But let’s not create confusion about what correct annotations are and what incorrect annotations are. Correct annotations do not require using NO_THREAD_SAFETY_ANALYSIS, and that’s just the most obvious clue.
