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

dongcarl commented at 4:49 PM on April 18, 2022: member

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

validation: Load pindexBestHeader in ChainMan

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.

5d670173a3

fanquake added the label Refactoring on Apr 18, 2022

dongcarl force-pushed on Apr 18, 2022

ajtowns commented at 11:19 PM on April 18, 2022: member

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

DrahtBot commented at 2:11 AM on April 19, 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:

#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.

in src/node/blockstorage.cpp:109 in c51e611262 outdated

 105 | @@ -106,8 +106,8 @@ CBlockIndex* BlockManager::AddToBlockIndex(const CBlockHeader& block)
 106 |      pindexNew->nTimeMax = (pindexNew->pprev ? std::max(pindexNew->pprev->nTimeMax, pindexNew->nTime) : pindexNew->nTime);
 107 |      pindexNew->nChainWork = (pindexNew->pprev ? pindexNew->pprev->nChainWork : 0) + GetBlockProof(*pindexNew);
 108 |      pindexNew->RaiseValidity(BLOCK_VALID_TREE);
 109 | -    if (pindexBestHeader == nullptr || pindexBestHeader->nChainWork < pindexNew->nChainWork)
 110 | -        pindexBestHeader = pindexNew;
 111 | +    if (best_header == nullptr || best_header->nChainWork < pindexNew->nChainWork)
 112 | +        best_header = pindexNew;

ajtowns commented at 7:11 AM on April 19, 2022:

If retouching, add { }

MarcoFalke commented at 8:53 AM on April 19, 2022:

c51e6112624e3f4c6282be5f4c78ee3e95aef797 Same

ajtowns approved

ajtowns commented at 8:06 AM on April 19, 2022: member

ACK 7b434e7d6bc5ea92b14694375ac4d429e644e61d ; code review only

5d670173a32ccdcb25d3a6bf97317f0ac774e0ed 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

in src/net_processing.cpp:4673 in c51e611262 outdated

4669 | @@ -4670,28 +4670,28 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
4670 |          CNodeState &state = *State(pto->GetId());
4671 |  
4672 |          // Start block sync
4673 | -        if (pindexBestHeader == nullptr)
4674 | -            pindexBestHeader = m_chainman.ActiveChain().Tip();
4675 | +        if (m_chainman.pindexBestHeader == nullptr)

MarcoFalke commented at 8:50 AM on April 19, 2022:

nit in c51e6112624e3f4c6282be5f4c78ee3e95aef797:

add {}

in src/validation.cpp:282 in c51e611262 outdated

 278 | @@ -280,7 +279,7 @@ static bool IsCurrentForFeeEstimation(CChainState& active_chainstate) EXCLUSIVE_
 279 |          return false;
 280 |      if (active_chainstate.m_chain.Tip()->GetBlockTime() < count_seconds(GetTime<std::chrono::seconds>() - MAX_FEE_ESTIMATION_TIP_AGE))
 281 |          return false;
 282 | -    if (active_chainstate.m_chain.Height() < pindexBestHeader->nHeight - 1)
 283 | +    if (active_chainstate.m_chain.Height() < active_chainstate.m_chainman.pindexBestHeader->nHeight - 1)

MarcoFalke commented at 8:53 AM on April 19, 2022:

c51e6112624e3f4c6282be5f4c78ee3e95aef797 Same

MarcoFalke approved

MarcoFalke commented at 9:10 AM on April 19, 2022: member

review ACK 7b434e7d6bc5ea92b14694375ac4d429e644e61d 🌜

<details><summary>Show signature</summary>

Signature:

</details>

MarcoFalke commented at 9:38 AM on April 19, 2022: member

In reply to #24909#pullrequestreview-945162779 private and friend:

Seems unrelated to the commits here, see https://github.com/bitcoin/bitcoin/pull/24917

move-mostly: Make pindexBestHeader a ChainMan member

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

0d567daf23

style-only: Miscellaneous whitespace changes

...of touched lines and surrounding

73eedaaacc

Clear pindexBestHeader in ChainstateManager::Unload()

-----

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.

c96524113c

move-mostly: Make fHavePruned a BlockMan member

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

3308ecd3fc

Clear fHavePruned in 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.

a401402125

scripted-diff: Rename pindexBestHeader, fHavePruned

...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-

f0a2fb3c5d

dongcarl force-pushed on Apr 19, 2022

dongcarl commented at 6:41 PM on April 19, 2022: member

Pushed 7b434e7d6bc5ea92b14694375ac4d429e644e61d -> f0a2fb3c5dbf3c4bec7faf934baff3e723734b3f

Addressed all if {} comments

dongcarl commented at 6:42 PM on April 19, 2022: member

@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 commented at 9:37 AM on April 20, 2022: member

ACK f0a2fb3c5dbf3c4bec7faf934baff3e723734b3f -- code review only

MarcoFalke commented at 10:13 AM on April 20, 2022: member

kirby ACK f0a2fb3c5dbf3c4bec7faf934baff3e723734b3f 😋

<details><summary>Show signature</summary>

Signature:

</details>

MarcoFalke merged this on Apr 20, 2022

MarcoFalke closed this on Apr 20, 2022

sidhujag referenced this in commit f218e8cd1b on Apr 22, 2022

Fabcien referenced this in commit 4447e3fc3e on Jan 24, 2023

Fabcien referenced this in commit e52a93e568 on Jan 24, 2023

Fabcien referenced this in commit 75289e8ea8 on Jan 24, 2023

Fabcien referenced this in commit 09cfec6062 on Jan 24, 2023

Fabcien referenced this in commit 353b5d80b4 on Jan 24, 2023

Fabcien referenced this in commit 5c2046a489 on Jan 24, 2023

DrahtBot locked this on Apr 20, 2023