Skip to content

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

Merged
laanwj merged 2 commits intobitcoin:masterfrom
jonatack:UnloadBlockIndex-and-ChainstateManager-dtor-cleanups
Mar 7, 2022
Merged

validation, refactor: UnloadBlockIndex and ChainstateManager::Reset thread safety cleanups#24299
laanwj merged 2 commits intobitcoin:masterfrom
jonatack:UnloadBlockIndex-and-ChainstateManager-dtor-cleanups

Conversation

@jonatack
Copy link
Member

@jonatack jonatack commented Feb 9, 2022 •
edited
Loading

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)

@jonatack jonatack mentioned this pull request Feb 9, 2022
Merged
laanwj
laanwj reviewed Feb 9, 2022
View reviewed changes
@jonatack jonatack force-pushed the UnloadBlockIndex-and-ChainstateManager-dtor-cleanups branch from acb92c4 to b63e704 Compare February 9, 2022 16:10
jamesob
jamesob reviewed Feb 9, 2022
View reviewed changes
src/validation.cpp Outdated
Copy link
Contributor

@jamesob jamesob Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I make use of this in #24232

Copy link
Member Author

@jonatack jonatack Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@maflcko maflcko Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@jonatack jonatack Feb 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@maflcko maflcko Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jamesob jamesob Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@maflcko maflcko Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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 jonatack changed the title validation, refactor: UnloadBlockIndex and ChainstateManager dtor cleanups validation, refactor: UnloadBlockIndex and ChainstateManager::Reset thread safety cleanups Feb 9, 2022
@jonatack jonatack force-pushed the UnloadBlockIndex-and-ChainstateManager-dtor-cleanups branch from b63e704 to 7edab1e Compare February 9, 2022 17:00
@jonatack jonatack force-pushed the UnloadBlockIndex-and-ChainstateManager-dtor-cleanups branch from 7edab1e to ae9ceed Compare February 9, 2022 17:05
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 11, 2022 •
edited
Loading

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.

This was referenced Feb 11, 2022
Closed
Closed
Closed
vasild
vasild approved these changes Feb 18, 2022
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK ae9ceed

This was referenced Feb 24, 2022
Merged
Closed
klementtan
klementtan approved these changes Mar 3, 2022
View reviewed changes
Copy link
Contributor

@klementtan klementtan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crACK ae9ceed

@laanwj
Copy link
Member

laanwj commented Mar 7, 2022

Code review ACK ae9ceed

@laanwj laanwj merged commit cba41db into bitcoin:master Mar 7, 2022
@jonatack jonatack deleted the UnloadBlockIndex-and-ChainstateManager-dtor-cleanups branch March 7, 2022 11:37
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 7, 2022
@laanwj
Merge bitcoin#24299: validation, refactor: UnloadBlockIndex and Chain…
9b47172 
…stateMa…
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 18, 2023
@jonatack @PiRK
validation: replace lock with annotation in UnloadBlockIndex()
7f1cc5f 
Summary:
This is a partial backport of [[bitcoin/bitcoin#24299 | core#24299]]
bitcoin/bitcoin@daad009

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D12988
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 18, 2023
@jonatack @PiRK
validation, refactoring: remove ChainstateManager::Reset()
be4d788 
Summary:
remove ChainstateManager::Reset(), as it is currently unused (can be reintroduced in the test utilities if needed for unit testing)

Co-authored-by: Vasil Dimov <[email protected]>
Co-authored-by: laanwj <[email protected]>

This concludes backport of [[bitcoin/bitcoin#24299 | core#24299]]
bitcoin/bitcoin@ae9ceed

Depends on D12988

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D12989
@bitcoin bitcoin locked and limited conversation to collaborators Mar 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

+4 more reviewers

@jamesob jamesob jamesob left review comments

@maflcko maflcko maflcko left review comments

@vasild vasild vasild approved these changes

@klementtan klementtan klementtan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@jonatack @DrahtBot @laanwj @jamesob @vasild @maflcko @klementtan