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

pull jonatack wants to merge 2 commits into bitcoin:master from jonatack:UnloadBlockIndex-and-ChainstateManager-dtor-cleanups changing 3 files +4 −18

jonatack commented at 3:00 PM on February 9, 2022: contributor
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)
validation: replace lock with annotation in UnloadBlockIndex() daad0093e3
jonatack cross-referenced this on Feb 9, 2022 from issue validation, refactor: add missing thread safety lock assertions by jonatack
laanwj added the label Refactoring on Feb 9, 2022

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!)

jonatack force-pushed on Feb 9, 2022
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)
jonatack renamed this:
~~validation, refactor: UnloadBlockIndex and ChainstateManager dtor cleanups~~
validation, refactor: UnloadBlockIndex and ChainstateManager::Reset thread safety cleanups
on Feb 9, 2022
jonatack force-pushed on Feb 9, 2022

validation, refactoring: remove ChainstateManager::Reset()

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

ae9ceed3e2

jonatack force-pushed on Feb 9, 2022
DrahtBot commented at 12:31 PM on February 11, 2022: contributor


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.
DrahtBot cross-referenced this on Feb 11, 2022 from issue assumeutxo: add init and completion logic by jamesob
DrahtBot cross-referenced this on Feb 12, 2022 from issue validation: Remove useless call to mempool->clear() by MarcoFalke
DrahtBot cross-referenced this on Feb 13, 2022 from issue assumeutxo by jamesob
vasild approved
vasild commented at 2:44 PM on February 18, 2022: contributor

ACK ae9ceed3e23288b163b7d7b1840b06b8d332f4ce
DrahtBot cross-referenced this on Feb 24, 2022 from issue refactor: Move mutable globals cleared in `::UnloadBlockIndex` to `BlockManager` by dongcarl
DrahtBot cross-referenced this on Mar 2, 2022 from issue blockman: Properly guard blockfile members by dongcarl
klementtan approved
klementtan commented at 4:29 PM on March 3, 2022: contributor

crACK ae9ceed3e23288b163b7d7b1840b06b8d332f4ce
laanwj commented at 11:13 AM on March 7, 2022: member

Code review ACK ae9ceed3e23288b163b7d7b1840b06b8d332f4ce
laanwj merged this on Mar 7, 2022
laanwj closed this on Mar 7, 2022
jonatack deleted the branch on Mar 7, 2022
sidhujag referenced this in commit 9b47172ced on Mar 7, 2022
Fabcien referenced this in commit 7f1cc5ffd8 on Jan 18, 2023
Fabcien referenced this in commit be4d788d3b on Jan 18, 2023
bitcoin locked this on Mar 7, 2023

Contributors

Labels

Linked (view graph)

#24145 mempool: Clear vTxHashes when mapTx is cleared #24177 validation, refactor: add missing thread safety lock assertions #24232 assumeutxo: add init and completion logic

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

Conflicts