validation, refactor: UnloadBlockIndex and ChainstateManager::Reset thread safety cleanups #24299

1. 
   jonatack commented at 3:00 pm on February 9, 2022: member

   Thread safety refactoring seen in #24177:

   • replace re-acquiring lock cs_main with a thread safety annotation in UnloadBlockIndex()
   • remove ChainstateManager::Reset(), as it is currently unused (can be reintroduced in the test utilities if needed for unit testing)
2. validation: replace lock with annotation in UnloadBlockIndex() daad0093e3
3. laanwj added the label Refactoring on Feb 9, 2022
4. in src/validation.h:995 in acb92c46e0 outdated

    995     ~ChainstateManager() {
    996         LOCK(::cs_main);
    997-        UnloadBlockIndex(/* mempool */ nullptr, *this);
    998-        Reset();
    999+        UnloadBlockIndex(/*mempool=*/nullptr, *this);
   1000+        // Clear (deconstruct) chainstate data.
   

   ---

   ---

   

   laanwj commented at 3:40 pm on February 9, 2022:

   Do any of these matter if the object is going to be destroyed anyway?

   ---

   

   jonatack commented at 4:07 pm on February 9, 2022:

   Right! Removed (thanks!)
5. jonatack force-pushed on Feb 9, 2022
6. in src/validation.cpp:5012 in b63e704c14 outdated

   5008@@ -5009,15 +5009,6 @@ void ChainstateManager::Unload()
   5009     m_best_invalid = nullptr;
   5010 }
   5011 
   5012-void ChainstateManager::Reset()
   

   ---

   ---

   

   jamesob commented at 4:42 pm on February 9, 2022:

   I make use of this in #24232

   ---

   

   jonatack commented at 4:54 pm on February 9, 2022:

   Ok, reverted to the original version in 686d35de6 that keeps Reset() and replaces the lock with annotation+assertion.

   ---

   

   MarcoFalke commented at 5:02 pm on February 9, 2022:

   Generally I am not a fan of putting test-only code in the “real” source code. Especially if it is validation. Longer term this may be used in non-test code accidentally, after which changes to it (and changes unrelated to it) become almost impossible to review. (See for example: #24145 (comment))

   I think this should be removed from validation and added back in the test code where needed. Either src/test/util or elsewhere.

   ---

   

   jonatack commented at 5:04 pm on February 9, 2022:

   Un-reverted back to removing Reset() and will review #24232.

   ---

   

   MarcoFalke commented at 5:05 pm on February 9, 2022:

   (not saying what this pull should do, just saying what the long term goal should be)

   ---

   

   jamesob commented at 3:30 pm on March 7, 2022:

   This was not just “test-only code” - the Reset() was used to detect snapshot dirs that needed to be cleaned up. The merge here isn’t now just a matter of copy/pasting some code.

   ---

   

   MarcoFalke commented at 3:59 pm on March 7, 2022:

   (Already replied out-of-band) The code that isn’t test-only (validatedSnapshotShutdownCleanup) can be called in the destructor (where Reset used to be called). The test-only code (if there is any) can be inlined, put in a test utility class, or a method. (The new method may even be called Reset)
7. jonatack renamed this:
   validation, refactor: UnloadBlockIndex and ChainstateManager dtor cleanups
   validation, refactor: UnloadBlockIndex and ChainstateManager::Reset thread safety cleanups
   on Feb 9, 2022
8. jonatack force-pushed on Feb 9, 2022
9. validation, refactoring: remove ChainstateManager::Reset()

   Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
   Co-authored-by: laanwj <126646+laanwj@users.noreply.github.com>

   ae9ceed3e2
10. jonatack force-pushed on Feb 9, 2022
11. 
    DrahtBot commented at 12:31 pm on February 11, 2022: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #24456 (blockman: Properly guard blockfile members by dongcarl)
    • #22564 (refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager by dongcarl)
    • #20030 (validation: Remove useless call to mempool->clear() by MarcoFalke)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

12. vasild approved
13. 
    vasild commented at 2:44 pm on February 18, 2022: member

    ACK ae9ceed3e23288b163b7d7b1840b06b8d332f4ce
14. klementtan approved
15. 
    klementtan commented at 4:29 pm on March 3, 2022: contributor

    crACK ae9ceed3e23288b163b7d7b1840b06b8d332f4ce
16. 
    laanwj commented at 11:13 am on March 7, 2022: member

    Code review ACK ae9ceed3e23288b163b7d7b1840b06b8d332f4ce
17. laanwj merged this on Mar 7, 2022
18. laanwj closed this on Mar 7, 2022

    ---

19. jonatack deleted the branch on Mar 7, 2022
20. sidhujag referenced this in commit 9b47172ced on Mar 7, 2022
21. Fabcien referenced this in commit 7f1cc5ffd8 on Jan 18, 2023
22. Fabcien referenced this in commit be4d788d3b on Jan 18, 2023
23. DrahtBot locked this on Mar 7, 2023

Contributors
jonatack laanwj jamesob MarcoFalke DrahtBot vasild klementtan

Labels
Refactoring