refactor: Move and rename `pindexBestHeader`, `fHavePruned` by dongcarl · Pull Request #24909 · bitcoin/bitcoin

dongcarl · 2022-04-18T16:49:02Z

Split off from #22564 per Marco's suggestion: #22564 (comment)

This is basically the move-mostly parts of #22564. The overall intent is to move mutable globals manually reset by ::UnloadBlockIndex into appropriate structs such that they are cleared at the appropriate times. Please read #22564's description for more rationale.

In summary , this PR moves:

pindexBestHeader -> ChainstateManager::m_best_header
fHavePruned -> BlockManager::m_have_pruned

Now BlockManager::LoadBlockIndex() will ACTUALLY only load BlockMan members. [META] In a later commit, pindexBestHeader will be moved to ChainMan as a member ----- Code Reviewer Notes Call graph of relevant functions: ChainstateManager::LoadBlockIndex() <-- Moved to calls BlockManager::LoadBlockIndexDB() which calls BlockManager::LoadBlockIndex() <-- Moved from There is only one call to each of inner functions, meaning that no behavior is changing.

ajtowns · 2022-04-18T23:19:28Z

"most-mostly" -> "move-mostly" ?

DrahtBot · 2022-04-19T02:11:13Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24571 (p2p: Prevent block index fingerprinting by sending additional getheaders messages by dergoegge)
#24543 (net processing: Move remaining globals into PeerManagerImpl by dergoegge)
#24171 (p2p: Sync chain more readily from inbound peers during IBD by sdaftuar)
#23599 (Tidy up RPCTxSerializationFlags by MarcoFalke)
#21726 (Improve Indices on pruned nodes via prune blockers by fjahr)
#20030 (validation: Remove useless call to mempool->clear() by MarcoFalke)
#19888 (rpc, test: Improve getblockstats for unspendables by fjahr)

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.

ajtowns

ACK 7b434e7d6bc5ea92b14694375ac4d429e644e61d ; code review only

5d67017 validation: Load pindexBestHeader in ChainMan
- Could make BlockManager::LoadBlockIndex private
- validation.h: LoadBlockIndex does not need to be a friend of ChainstateManager ?
c51e6112624e3f4c6282be5f4c78ee3e95aef797 move-mostly: Make pindexBestHeader a ChainMan member
d66a2c0d0c5fb47f3a5a92841f576b0a2324a7e7 style-only: Miscellaneous whitespace changes
8cc5f5c880c35db7719b93d7c87f6e593986d368 Clear pindexBestHeader in ChainstateManager::Unload()
c32b9d88529c0ea0859eda4523716c25a8dee07c move-mostly: Make fHavePruned a BlockMan member
- Seems like fHavePruned should someday be private to blockman and the pruning logic should move from CChainState::FlushStateToDisk to blockman
4032fc9cf943e512319217d21349f2bb3be55129 Clear fHavePruned in BlockManager::Unload()
7b434e7d6bc5ea92b14694375ac4d429e644e61d scripted-diff: Rename pindexBestHeader, fHavePruned

src/node/blockstorage.cpp

maflcko

review ACK 7b434e7d6bc5ea92b14694375ac4d429e644e61d 🌜

Show signature

Signature:

src/net_processing.cpp

src/validation.cpp

src/node/blockstorage.cpp

maflcko · 2022-04-19T09:38:35Z

In reply to #24909 (review) private and friend:

Seems unrelated to the commits here, see #24917

[META] In the next commit, we move the clearing of pindexBestHeader to ChainstateManager::Unload()

...of touched lines and surrounding

----- Code Reviewer Notes Call graph of relevant functions: UnloadBlockIndex() <-- Moved from calls ChainstateManager::Unload() <-- Moved to Safe because ChainstateManager::Unload() is called only by UnloadBlockIndex() and no other callers.

[META] In the next commit, we move the clearing of fHavePruned to BlockManager::Unload()

----- Code Reviewer Notes Call graph of relevant functions: UnloadBlockIndex() <-- Moved from calls ChainstateManager::Unload() which calls BlockManager::Unload() <-- Moved to So calling UnloadBlockIndex() would still run this moved code. The code will also now run when ~BlockManager gets called, which makes sense.

...to m_best_header and m_have_pruned -BEGIN VERIFY SCRIPT- find_regex="\bpindexBestHeader\b" \ && git grep -l -E "$find_regex" -- src \ | xargs sed -i -E "s@$find_regex@m_best_header@g" find_regex="\bfHavePruned\b" \ && git grep -l -E "$find_regex" -- src \ | xargs sed -i -E "s@$find_regex@m_have_pruned@g" -END VERIFY SCRIPT-

dongcarl · 2022-04-19T18:41:21Z

Pushed 7b434e7d6bc5ea92b14694375ac4d429e644e61d -> f0a2fb3

Addressed all if {} comments

dongcarl · 2022-04-19T18:42:42Z

@ajtowns

Seems like fHavePruned should someday be private to blockman and the pruning logic should move from CChainState::FlushStateToDisk to blockman

Looks reasonable at first glance, perhaps you wanna open an issue for that?

ajtowns · 2022-04-20T09:37:41Z

ACK f0a2fb3 -- code review only

maflcko · 2022-04-20T10:13:02Z

kirby ACK f0a2fb3 😋

Show signature

Signature:

…vePru… …ned`

Summary: Now BlockManager::LoadBlockIndex() will ACTUALLY only load BlockMan members. [META] In a later commit, pindexBestHeader will be moved to ChainMan as a member ----- Code Reviewer Notes Call graph of relevant functions: `ChainstateManager::LoadBlockIndex()`<-- Moved to calls `BlockManager::LoadBlockIndexDB()` which calls `BlockManager::LoadBlockIndex()` <-- Moved from There is only one call to each of inner functions, meaning that no behavior is changing. This is a partial backport of [[bitcoin/bitcoin#24909 | core#24909]] bitcoin/bitcoin@5d67017 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D13043

Summary: [META] In the next commit, we move the clearing of pindexBestHeader to ChainstateManager::Unload() This is a partial backport of [[bitcoin/bitcoin#24909 | core#24909]] bitcoin/bitcoin@0d567da Depends on D13043 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D13044

Summary: ----- Code Reviewer Notes Call graph of relevant functions: `UnloadBlockIndex()` <-- Moved from calls `ChainstateManager::Unload()` <-- Moved to Safe because `ChainstateManager::Unload()` is called only by `UnloadBlockIndex()` and no other callers. This is a partial backport of [[bitcoin/bitcoin#24909 | core#24909]] bitcoin/bitcoin@c965241 Depends on D13044 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D13045

Summary: [META] In the next commit, we move the clearing of fHavePruned to BlockManager::Unload() This is a partial backport of [[bitcoin/bitcoin#24909 | core#24909]] bitcoin/bitcoin@3308ecd Depends on D13045 Test Plan: `ninja all check-all bench-bitcoin` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D13046

Summary: ----- Code Reviewer Notes Call graph of relevant functions: `UnloadBlockIndex()` <-- Moved from calls `ChainstateManager::Unload()` which calls `BlockManager::Unload()` <-- Moved to So calling UnloadBlockIndex() would still run this moved code. The code will also now run when ~BlockManager gets called, which makes sense. This is a partial backport of [[bitcoin/bitcoin#24909 | core#24909]] bitcoin/bitcoin@a401402 Depends on D13046 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D13047

…and m_have_pruned Summary: ``` -BEGIN VERIFY SCRIPT- find_regex="\bpindexBestHeader\b" \ && git grep -l -E "$find_regex" -- src \ | xargs sed -i -E "s@$find_regex@m_best_header@g" find_regex="\bfHavePruned\b" \ && git grep -l -E "$find_regex" -- src \ | xargs sed -i -E "s@$find_regex@m_have_pruned@g" -END VERIFY SCRIPT- ``` This concludes backport of [[bitcoin/bitcoin#24909 | core#24909]] bitcoin/bitcoin@f0a2fb3 Depends on D13047 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D13048

fanquake added the Refactoring label Apr 18, 2022

dongcarl force-pushed the 2022-04-kirby-p3-pre branch from c1228dc to 7b434e7 Compare April 18, 2022 17:22

dongcarl mentioned this pull request Apr 18, 2022

refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager #22564

Merged

This was referenced Apr 19, 2022

p2p: Prevent block index fingerprinting by sending additional getheaders messages #24571

Closed

net processing: Move remaining globals into PeerManagerImpl #24543

Merged

ajtowns approved these changes Apr 19, 2022

View reviewed changes

src/node/blockstorage.cpp Outdated Show resolved Hide resolved

maflcko approved these changes Apr 19, 2022

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/node/blockstorage.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Apr 19, 2022

p2p: Sync chain more readily from inbound peers during IBD #24171

Merged

dongcarl added 6 commits April 19, 2022 14:34

move-mostly: Make pindexBestHeader a ChainMan member

0d567da

[META] In the next commit, we move the clearing of pindexBestHeader to ChainstateManager::Unload()

style-only: Miscellaneous whitespace changes

73eedaa

...of touched lines and surrounding

move-mostly: Make fHavePruned a BlockMan member

3308ecd

[META] In the next commit, we move the clearing of fHavePruned to BlockManager::Unload()

dongcarl force-pushed the 2022-04-kirby-p3-pre branch from 7b434e7 to f0a2fb3 Compare April 19, 2022 18:39

This was referenced Apr 20, 2022

[WIP] DRAFT NOMERGE Tidy up RPCTxSerializationFlags #23599

Closed

Improve Indices on pruned nodes via prune blockers #21726

Merged

validation: Remove useless call to mempool->clear() #20030

Closed

DrahtBot mentioned this pull request Apr 20, 2022

rpc, test: Improve getblockstats for unspendables #19888

Merged

maflcko merged commit dbdc83a into bitcoin:master Apr 20, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 22, 2022

Merge bitcoin#24909: refactor: Move and rename pindexBestHeader, `fHa…

f218e8c

…vePru… …ned`

bitcoin locked and limited conversation to collaborators Apr 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Move and rename `pindexBestHeader`, `fHavePruned`#24909

refactor: Move and rename `pindexBestHeader`, `fHavePruned`#24909
maflcko merged 7 commits intobitcoin:masterfrom
dongcarl:2022-04-kirby-p3-pre

dongcarl commented Apr 18, 2022 •

edited

Loading

Uh oh!

ajtowns commented Apr 18, 2022

Uh oh!

DrahtBot commented Apr 19, 2022

Uh oh!

ajtowns left a comment

Uh oh!

Uh oh!

maflcko left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Apr 19, 2022

Uh oh!

dongcarl commented Apr 19, 2022

Uh oh!

dongcarl commented Apr 19, 2022

Uh oh!

ajtowns commented Apr 20, 2022

Uh oh!

maflcko commented Apr 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

dongcarl commented Apr 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajtowns commented Apr 18, 2022

Uh oh!

DrahtBot commented Apr 19, 2022

Conflicts

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Apr 19, 2022

Uh oh!

dongcarl commented Apr 19, 2022

Uh oh!

dongcarl commented Apr 19, 2022

Uh oh!

ajtowns commented Apr 20, 2022

Uh oh!

maflcko commented Apr 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dongcarl commented Apr 18, 2022 •

edited

Loading