refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate #28218

pull ryanofsky wants to merge 1 commits into bitcoin:master from ryanofsky:pr/assumeibd changing 14 files +79 −66
  1. ryanofsky commented at 9:14 pm on August 4, 2023: contributor

    This change makes IsInitialBlockDownload and NotifyHeaderTip functions no longer tied to individual Chainstate objects. It makes them work with the ChainstateManager object instead so code is simpler and it is no longer possible to call them incorrectly with an inactive Chainstate.

    This change also makes m_cached_finished_ibd caching easier to reason about, because now there is only one cached value instead of two (for background and snapshot chainstates) so the cached IBD state now no longer gets reset when a snapshot is loaded.

    There should be no change in behavior because these functions were always called on the active ChainState objects.

    These changes were discussed previously #27746 (review) and #27746 (review) as possible followups for that PR.

  2. DrahtBot commented at 9:14 pm on August 4, 2023: contributor

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK MarcoFalke, naumenkogs, dergoegge
    Stale ACK TheCharlatan

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #28031 (Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module by glozow)
    • #27826 (validation: log which peer sent us a header by Sjors)
    • #27596 (assumeutxo (2) by jamesob)
    • #20827 (During IBD, prune as much as possible until we get close to where we will eventually keep blocks by luke-jr)

    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.

  3. ryanofsky force-pushed on Aug 4, 2023
  4. dergoegge commented at 10:06 am on August 7, 2023: member
    Concept ACK
  5. in src/validation.cpp:3057 in 3d70aa3c60 outdated 
    3053@@ -3053,24 +3054,24 @@ static SynchronizationState GetSynchronizationState(bool init)
3054     return SynchronizationState::INIT_DOWNLOAD;
3055 }
3056 
3057-static bool NotifyHeaderTip(Chainstate& chainstate) LOCKS_EXCLUDED(cs_main) {
3058+static bool NotifyHeaderTip(ChainstateManager& chainman) LOCKS_EXCLUDED(cs_main) {
    maflcko commented at 3:41 pm on August 7, 2023:
    nit: Missing clang-format on touched line?
    ryanofsky commented at 8:05 pm on August 8, 2023:

    re: #28218 (review)

    nit: Missing clang-format on touched line?

    Thanks, ran clang-format-diff

  6. in src/validation.cpp:279 in 3d70aa3c60 outdated 
    275@@ -276,7 +276,7 @@ static void LimitMempoolSize(CTxMemPool& pool, CCoinsViewCache& coins_cache)
276 static bool IsCurrentForFeeEstimation(Chainstate& active_chainstate) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
277 {
278     AssertLockHeld(cs_main);
279-    if (active_chainstate.IsInitialBlockDownload())
280+    if (active_chainstate.m_chainman.IsInitialBlockDownload())
    maflcko commented at 3:42 pm on August 7, 2023:
    nit: Add missing {} on touched line for multiline if?
    ryanofsky commented at 8:05 pm on August 8, 2023:

    re: #28218 (review)

    nit: Add missing {} on touched line for multiline if?

    Sure, done

  7. in src/validation.cpp:1636 in 3d70aa3c60 outdated 
    1633+    if (m_blockman.LoadingBlocks()) {
1634         return true;
1635     }
1636-    if (m_chain.Tip() == nullptr)
1637+    CChain& chain{ActiveChain()};
1638+    if (chain.Tip() == nullptr)
    maflcko commented at 3:42 pm on August 7, 2023:
    Same?
    ryanofsky commented at 8:05 pm on August 8, 2023:

    re: #28218 (review)

    Same?

    Done

  8. maflcko approved
  9. maflcko commented at 3:43 pm on August 7, 2023: member
    lgtm. Left style nits
  10. DrahtBot renamed this:
    assumeutxo cleanup: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager
    refactor: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager
    on Aug 7, 2023
  11. DrahtBot added the label Refactoring on Aug 7, 2023
  12. ryanofsky force-pushed on Aug 8, 2023
  13. ryanofsky commented at 8:16 pm on August 8, 2023: contributor
    Updated 3d70aa3c60f07ed6c9eed237b2c82a59674239cf -> bda557e5ebe7f754984a34ddfd33d2540c0303b9 (pr/assumeibd.2 -> pr/assumeibd.3, compare) with suggested changes
  14. TheCharlatan commented at 1:09 pm on August 10, 2023: contributor
    Why not make NotifyHeaderTip a private ChainstateManager member? The PR title seems to already hint as much.
  15. ryanofsky commented at 1:32 pm on August 10, 2023: contributor

    re: #28218#pullrequestreview-1571859693

    Why not make NotifyHeaderTip a private ChainstateManager member?

    Because it doesn’t access private state. I think the point of classes in c++ is to encapsulate private state. If there is a group of functions operating on public state, they should be standalone functions, so they can be split up into files and organized by functionality. This way we can avoid being permanently stuck with a sprawling mess of code like validation.cpp. The urge to make functions class members instead of standalone functions when they only use public interfaces and do not manage private state leads to bad outcomes like CWallet in wallet.cpp.

  16. ryanofsky renamed this:
    refactor: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager
    refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate
    on Aug 10, 2023
  17. TheCharlatan approved
  18. TheCharlatan commented at 1:57 pm on August 10, 2023: contributor
    ACK bda557e5ebe7f754984a34ddfd33d2540c0303b9
  19. fanquake requested review from jamesob on Aug 10, 2023
  20. in src/validation.cpp:2649 in bda557e5eb outdated 
    2643@@ -2641,7 +2644,7 @@ void Chainstate::UpdateTip(const CBlockIndex* pindexNew)
2644     }
2645 
2646     bilingual_str warning_messages;
2647-    if (!this->IsInitialBlockDownload()) {
2648+    if (!m_chainman.IsInitialBlockDownload()) {
    dergoegge commented at 1:38 pm on August 11, 2023:

    (somewhat unrelated to this pull)

    Are there any plans to remove the chainman reference from Chainstate? I really don’t like this pattern in general, i.e. managed object having a reference to the manager. Imo, it is an indication that the responsibilities of Chainstate and ChainstateManager are not well defined.

    (It’s probably also more or less just an artifact of the de-globalization work)

    ryanofsky commented at 2:20 pm on August 14, 2023:

    re: #28218 (review)

    Are there any plans to remove the chainman reference from Chainstate? I really don’t like this pattern in general, i.e. managed object having a reference to the manager. Imo, it is an indication that the responsibilities of Chainstate and ChainstateManager are not well defined.

    I agree, and if I were cleaning this up would get rid of this back reference and make a lot of ChainState members functions into standalone functions that take explicit ChainState and ChainStateManager arguments. But I think this would mainly be an aesthetic improvement and not have a lot of consequences, because in practice after #27746 and this PR the responsibilities of Chainstate and ChainstateManager are pretty clear if you just look at the actual state the classes contain and ignore the member functions. Detaching member functions and passing references explicitly instead of explicitly are good things, but less consequential than deciding how to represent and update state, which is what this PR and #27746 are focused on.

  21. dergoegge approved
  22. dergoegge commented at 1:39 pm on August 11, 2023: member

    Code review ACK bda557e5ebe7f754984a34ddfd33d2540c0303b9

    Nice interface cleanup!

  23. naumenkogs commented at 11:45 am on August 18, 2023: member
    ACK bda557e5ebe7f754984a34ddfd33d2540c0303b9
  24. fanquake commented at 12:57 pm on August 18, 2023: member

    This will need to be rebased after #27460:

    0  CXX      rpc/libbitcoin_node_a-signmessage.o
1  CXX      rpc/libbitcoin_node_a-txoutproof.o
2rpc/mempool.cpp:757:28: error: no member named 'IsInitialBlockDownload' in 'Chainstate'
3            if (chainstate.IsInitialBlockDownload()) {
4                ~~~~~~~~~~ ^
51 error generated.
6gmake[2]: *** [Makefile:10829: rpc/libbitcoin_node_a-mempool.o] Error 1
  25. in src/test/fuzz/process_message.cpp:66 in bda557e5eb outdated 
    62@@ -63,9 +63,9 @@ FUZZ_TARGET(process_message, .init = initialize_process_message)
63     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
64 
65     ConnmanTestMsg& connman = *static_cast<ConnmanTestMsg*>(g_setup->m_node.connman.get());
66-    TestChainState& chainstate = *static_cast<TestChainState*>(&g_setup->m_node.chainman->ActiveChainstate());
67+    TestChainstateManager& chainman = static_cast<TestChainstateManager&>(*g_setup->m_node.chainman);
    maflcko commented at 1:01 pm on August 18, 2023:

    Can use auto to avoid using the same type twice on the same line?

    0    auto& chainman = static_cast<TestChainstateManager&>(*g_setup->m_node.chainman);
    ryanofsky commented at 3:22 pm on August 18, 2023:

    re: #28218 (review)

    Can use auto to avoid using the same type twice on the same line?

    Thanks, switched

  26. in src/test/fuzz/process_messages.cpp:41 in bda557e5eb outdated 
    37@@ -38,9 +38,9 @@ FUZZ_TARGET(process_messages, .init = initialize_process_messages)
38     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
39 
40     ConnmanTestMsg& connman = *static_cast<ConnmanTestMsg*>(g_setup->m_node.connman.get());
41-    TestChainState& chainstate = *static_cast<TestChainState*>(&g_setup->m_node.chainman->ActiveChainstate());
42+    TestChainstateManager& chainman = static_cast<TestChainstateManager&>(*g_setup->m_node.chainman);
    maflcko commented at 1:01 pm on August 18, 2023:
    Same?
    ryanofsky commented at 3:23 pm on August 18, 2023:

    re: #28218 (review)

    Same?

    Done

  27. in src/test/net_tests.cpp:846 in bda557e5eb outdated 
    845+    // Force ChainstateManager::IsInitialBlockDownload() to return false.
846     // Otherwise PushAddress() isn't called by PeerManager::ProcessMessage().
847-    TestChainState& chainstate =
848-        *static_cast<TestChainState*>(&m_node.chainman->ActiveChainstate());
849-    chainstate.JumpOutOfIbd();
850+    TestChainstateManager& chainman =
    maflcko commented at 1:01 pm on August 18, 2023:
    Same?
    ryanofsky commented at 3:23 pm on August 18, 2023:

    re: #28218 (review)

    Same?

    Done

  28. maflcko approved
  29. maflcko commented at 1:02 pm on August 18, 2023: member

    ACK ebda557e5ebe7f754984a34ddfd33d2540c0303b9 🦈

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK ebda557e5ebe7f754984a34ddfd33d2540c0303b9 🦈
3yZn/RDpC1vSfiuda6JpUwUzxyXXsJQLislGF0AVNnNF1NjceFmcwfzF5NfWYhjExtf0o0YmRSA9OhqhPHvt0Ag==
  30. DrahtBot added the label CI failed on Aug 18, 2023
  31. ryanofsky force-pushed on Aug 18, 2023
  32. assumeutxo cleanup: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager 
    This change makes IsInitialBlockDownload and NotifyHeaderTip functions no
longer tied to individual Chainstate objects. It makes them work with the
ChainstateManager object instead so code is simpler and it is no longer
possible to call them incorrectly with an inactive Chainstate.

This change also makes m_cached_finished_ibd caching easier to reason about,
because now there is only one cached value instead of two (for background and
snapshot chainstates) so the cached IBD state now no longer gets reset when a
snapshot is loaded.

There should be no change in behavior because these functions were always
called on the active ChainState objects.

These changes were discussed previously
https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246868905 and
https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1237552792 as
possible followups for that PR.
    94a98fbd1d
  33. ryanofsky force-pushed on Aug 18, 2023
  34. ryanofsky commented at 4:54 pm on August 18, 2023: contributor
    Rebased bda557e5ebe7f754984a34ddfd33d2540c0303b9 -> 94a98fbd1d14a4ea10629b917771d50f3191e944 (pr/assumeibd.3 -> pr/assumeibd.4, compare) applying suggestions and fixing silent conflict with #27460
  35. maflcko commented at 9:19 am on August 21, 2023: member

    re-ACK 94a98fbd1d14a4ea10629b917771d50f3191e944 🐺

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 94a98fbd1d14a4ea10629b917771d50f3191e944 🐺
3CT8xcHF/XxSnBppkGSnBSZJKvavuTVcWp510ritBnxEmqwOXZKjLlzuMpi7eowa7i7ESB0Wd917CaS5A+fDpCg==
  36. DrahtBot requested review from dergoegge on Aug 21, 2023
  37. DrahtBot requested review from naumenkogs on Aug 21, 2023
  38. DrahtBot requested review from TheCharlatan on Aug 21, 2023
  39. DrahtBot removed the label CI failed on Aug 21, 2023
  40. naumenkogs commented at 9:31 am on August 21, 2023: member
    ACK 94a98fbd1d14a4ea10629b917771d50f3191e944
  41. DrahtBot removed review request from naumenkogs on Aug 21, 2023
  42. dergoegge approved
  43. dergoegge commented at 9:34 am on August 21, 2023: member
    reACK 94a98fbd1d14a4ea10629b917771d50f3191e944
  44. fanquake merged this on Aug 21, 2023
  45. fanquake closed this on Aug 21, 2023
  46. Frank-GER referenced this in commit 5c76e06330 on Sep 8, 2023
  47. bitcoin locked this on Aug 20, 2024


ryanofsky DrahtBot dergoegge maflcko TheCharlatan naumenkogs fanquake


jamesob TheCharlatan

Labels
Refactoring

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: 2024-12-19 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me