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

pull dongcarl wants to merge 7 commits into bitcoin:master from dongcarl:2022-04-kirby-p3-pre changing 12 files +100 −99
  1. dongcarl commented at 4:49 PM on April 18, 2022: contributor

    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:

    1. pindexBestHeader -> ChainstateManager::m_best_header
    2. fHavePruned -> BlockManager::m_have_pruned
  2. 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
  3. fanquake added the label Refactoring on Apr 18, 2022
  4. dongcarl force-pushed on Apr 18, 2022
  5. dongcarl cross-referenced this on Apr 18, 2022 from issue refactor: Move mutable globals cleared in `::UnloadBlockIndex` to `BlockManager` by dongcarl
  6. ajtowns commented at 11:19 PM on April 18, 2022: contributor

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

  7. DrahtBot commented at 2:11 AM on April 19, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  8. DrahtBot cross-referenced this on Apr 19, 2022 from issue p2p: Prevent block index fingerprinting by sending additional getheaders messages by dergoegge
  9. 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

  10. DrahtBot cross-referenced this on Apr 19, 2022 from issue net processing: Move remaining globals into PeerManagerImpl by dergoegge
  11. ajtowns approved
  12. ajtowns commented at 8:06 AM on April 19, 2022: contributor

    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
  13. 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 {}

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

  15. MarcoFalke approved
  16. MarcoFalke commented at 9:10 AM on April 19, 2022: member

    review ACK 7b434e7d6bc5ea92b14694375ac4d429e644e61d 🌜

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

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK 7b434e7d6bc5ea92b14694375ac4d429e644e61d 🌜
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhK5AwAnnxOI4tcYCP4IleWGvPN536u+3aq40UauEOEzMSOcfqOzRjfZqpBAqwC
GBvAzOjwaTAkG6S6H77mLAPxDGZZB2GdUWFT1RqMd1GRJmhyeer9wtqQ/NATvETg
qKUoJiCn5o1xZEOkTMgkAJyUJ8vkVF2UxjL/AbeAu3l79/Qzh527vUQ+s7xsR0Pe
fSfJQlrxqaB1p3yrwdsZwEYN2g+nzDIz3xEKG7P2sGoY9Vboh8FqggJW1+LOJfYE
/STQmMjcWGLFzQzUa5lUD64WT5dHruXrLgoJQ+jlOFfNd89he07ybmN8mP91Iu2I
ZNucJcKDoDQVJHIz/vSNWltIeTNfc/Xxj54gCW6N6IUSyRr4uzs9aVkToacWO5kP
e+1hT8EK5gn0an0NWxQmS2PBmsgrAZ/AeenrDuaMkYl5DjBe70v7rX6IG7/kbHu3
d3/pqVWzugeA+LH6XUyt1a0TdH1JEX6IgQo6N06a1i2WzVOpG/Ol4TY4jonziIHy
5/TITCQv
=Uk9X
-----END PGP SIGNATURE-----

    </details>

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

  18. DrahtBot cross-referenced this on Apr 19, 2022 from issue p2p: Sync chain more readily from inbound peers during IBD by sdaftuar
  19. move-mostly: Make pindexBestHeader a ChainMan member
    [META] In the next commit, we move the clearing of pindexBestHeader to
       ChainstateManager::Unload()
    0d567daf23
  20. style-only: Miscellaneous whitespace changes
    ...of touched lines and surrounding
    73eedaaacc
  21. 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
  22. move-mostly: Make fHavePruned a BlockMan member
    [META] In the next commit, we move the clearing of fHavePruned to
       BlockManager::Unload()
    3308ecd3fc
  23. 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
  24. 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
  25. dongcarl force-pushed on Apr 19, 2022
  26. dongcarl commented at 6:41 PM on April 19, 2022: contributor

    Pushed 7b434e7d6bc5ea92b14694375ac4d429e644e61d -> f0a2fb3c5dbf3c4bec7faf934baff3e723734b3f

    • Addressed all if {} comments
  27. dongcarl commented at 6:42 PM on April 19, 2022: contributor

    @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?

  28. DrahtBot cross-referenced this on Apr 20, 2022 from issue [WIP] DRAFT NOMERGE Tidy up RPCTxSerializationFlags by MarcoFalke
  29. DrahtBot cross-referenced this on Apr 20, 2022 from issue Improve Indices on pruned nodes via prune blockers by fjahr
  30. DrahtBot cross-referenced this on Apr 20, 2022 from issue validation: Remove useless call to mempool->clear() by MarcoFalke
  31. ajtowns commented at 9:37 AM on April 20, 2022: contributor

    ACK f0a2fb3c5dbf3c4bec7faf934baff3e723734b3f -- code review only

  32. DrahtBot cross-referenced this on Apr 20, 2022 from issue rpc, test: Improve getblockstats for unspendables by fjahr
  33. MarcoFalke commented at 10:13 AM on April 20, 2022: member

    kirby ACK f0a2fb3c5dbf3c4bec7faf934baff3e723734b3f 😋

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

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

kirby ACK f0a2fb3c5dbf3c4bec7faf934baff3e723734b3f 😋
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjhaAwAmXnXA0Who3Rfgotbw/O9rKr6Yf2XxjGSxmW8JYWxbC0HsT72v/PjMOnQ
ofXjbI3dkZjTL4fE4jsyenMpSLTMbghvirhnSgVYpTY/pUKHeF4s/5ZXp7rpItaf
3ZcMmluRI7e2xzcuSIRRrGkvcpNQEjfoqfPrIyTnpABYbOyeV7pzWq7+QbM5OBgg
xyiBXPsDf+T1jHjRAsbAbH5/eQdA1B+VdqCjcEn/A27kfmHlSJKx7CNzP4vwTODK
YYi2hu3cGLdjLGZlfYu2dX4YQbpXxImGJ94+tQhZIZZN9f+8nMFd+L0Qeeka5voQ
ZImElHgcs38dVQ+pJSgjZtN3it2KIl1OA/kU1xMxyHQWG+qhzKvawmrd8QCRxGYj
3a2GQAOK+2MJa3q6Jq31+Dd46zXm23tgvQdzY2zeySPJz+4IS+Yibu8yWWJjSo/Z
RB66PlDNW8Exyx57mPsVEbO2gJvZi7kI52lgncDnNi0aQ+s/Q3MIUar9OCTcNmv6
LdqFVdTh
=MEJR
-----END PGP SIGNATURE-----

    </details>

  34. MarcoFalke merged this on Apr 20, 2022
  35. MarcoFalke closed this on Apr 20, 2022
  36. sidhujag referenced this in commit f218e8cd1b on Apr 22, 2022
  37. Fabcien referenced this in commit 4447e3fc3e on Jan 24, 2023
  38. Fabcien referenced this in commit e52a93e568 on Jan 24, 2023
  39. Fabcien referenced this in commit 75289e8ea8 on Jan 24, 2023
  40. Fabcien referenced this in commit 09cfec6062 on Jan 24, 2023
  41. Fabcien referenced this in commit 353b5d80b4 on Jan 24, 2023
  42. Fabcien referenced this in commit 5c2046a489 on Jan 24, 2023
  43. bitcoin locked this on Apr 20, 2023
Contributors
dongcarlajtownsDrahtBotMarcoFalke
Labels
Refactoring
Linked (view graph)
#22564 refactor: Move mutable globals cleared in `::UnloadBlockIndex` to `BlockManager`#24917 Make BlockManager::LoadBlockIndex private
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:53 UTC