refactor: Release cs_main before MaybeSendFeefilter #22053

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2105-lessCsMain changing 1 files +38 −7
  1. MarcoFalke commented at 7:17 am on May 25, 2021: member

    There is no need for any lock to be held, because net processing is single threaded. So holding the validation lock cs_main for sending a feefilter is confusing and might even degrade blockchain-related RPC performance minimally.

    MaybeSendFeefilter will break when called in multiple threads, because g_filter_rounder.round isn’t thread safe (among others). Thus, annotating the function with m_mutex_message_handling seems a nice belt-and-suspenders compiler check for now. If there are more threads in the future, a special feefilter mutex can be introduced.

  2. fanquake added the label Refactoring on May 25, 2021
  3. MarcoFalke commented at 7:19 am on May 25, 2021: member
    m_mutex_message_handling is taken from #21527, but should be independent and fine to merge in either this pull or the other.
  4. DrahtBot commented at 7:40 am on May 25, 2021: 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:

    • #24273 (p2p: Split network logging into two categories #24247 by anshu-khare-design)
    • #22778 (net processing: Reduce resource usage for inbound block-relay-only connections by jnewbery)
    • #21160 (net/net processing: Move tx inventory into net_processing by jnewbery)

    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.

  5. fanquake added the label P2P on May 25, 2021
  6. jnewbery commented at 10:52 am on May 25, 2021: member
    Code review ACK faca41443120c254f45d7a4dd72d5d80efacaf59
  7. lsilva01 approved
  8. lsilva01 commented at 2:37 am on May 30, 2021: contributor

    Tested on mainnet ACK https://github.com/bitcoin/bitcoin/pull/22053/commits/faca41443120c254f45d7a4dd72d5d80efacaf59

    FEEFILTER messages sent normally.

  9. MarcoFalke commented at 2:46 pm on May 30, 2021: member
    cc @vasild @ajtowns for Approach ACK/NACK
  10. in src/net_processing.cpp:337 in faca414431 outdated 
    283 
284 private:
285+    /** Message handling mutex.
286+     *  Message processing is single-threaded, so anything only accessed
287+     *  by ProcessMessage(s) or SendMessages can be guarded by this mutex,
288+     *  which guarantees it's only accessed by a single thread.
    vasild commented at 1:44 pm on May 31, 2021:
    How guarding with a mutex guarantees “it’s only accessed by a single thread”? Is the intention to say “it’s only accessed by a single thread at a given point in time”? If yes, then this applies to any mutex and the comment better be removed to avoid confusion. If it means that the mutex guarantees “it’s only accessed by the single thread ThreadMessageHandler()”, then it is not true because any other thread can grab the mutex and access the relevant variables.
  11. ajtowns commented at 1:44 pm on May 31, 2021: member

    Approach ACK

    Suggest adding a // GUARDED_BY(...) comment on g_filter_rounder (along the lines of https://github.com/bitcoin/bitcoin/pull/20758/commits/9ac47922a3262a290a3139ba65b95c60b110db8f).

  12. vasild commented at 1:49 pm on May 31, 2021: member

    My concerns expressed in #21527 (review) are valid for this PR too.

    In addition, this PR introduces the mutex without any GUARDED_BY(the new mutex). So it is unclear what is being protected by it. There is this vague comment: “anything only accessed by ProcessMessage(s) or SendMessages” which better be replaced by GUARDED_BY() directives on the variables protected by this mutex.

    We have the following call path:

    0CConnman::ThreadMessageHandler() (single thread) ->
1  PeerManagerImpl::SendMessages(CNode pto) (protected by CNode::cs_sendProcessing) ->
2    PeerManagerImpl::MaybeSendFeefilter(CNode pto)

    And PeerManagerImpl::MaybeSendFeefilter(CNode pto) accesses the following variables directly or indirectly:

     0bool PeerManagerImpl::m_ignore_incoming_txs
 1std::unique_ptr<TxRelay> CNode::m_tx_relay
 2CNode::GetCommonVersion()
 3  std::atomic<int> CNode::m_greatest_common_version
 4CNode::HasPermission()
 5  NetPermissionFlags CNode::m_permissionFlags
 6CTxMemPool& PeerManagerImpl::m_mempool
 7ChainstateManager& PeerManagerImpl::m_chainman
 8ChainstateManager::ActiveChainstate()
 9  CChainState* ChainstateManager::m_active_chainstate GUARDED_BY(cs_main)
10CChainState::IsInitialBlockDownload()
11  std::atomic<bool> CChainState::m_cached_finished_ibd // ok
12  CChain CChainState::m_chain // implicit GUARDED_BY(cs_main)
13FeeFilterRounder g_filter_rounder
14FeeFilterRounder::round()
15  FeeFilterRounder::feeset
16CAmount CNode::TxRelay::lastSentFeeFilter
17std::chrono::microseconds CNode::TxRelay::m_next_send_feefilter
18(global) CFeeRate minRelayTxFee
19CFeeRate::GetFeePerK()
20  CFeeRate::GetFee()
21    CAmount CFeeRate::nSatoshisPerK
22PeerManagerImpl::m_connman
23CConnman::PushMessage()
24  const CAddress CNode::addr
25  std::unique_ptr<TransportSerializer> CNode::m_serializer
26  TransportSerializer::prepareForTransport()

    I think that the above variables should be analyzed with respect to access from other threads before changing MaybeSendFeefilter() to be executed without holding cs_main:

    • If a variable is not accessed by other threads than ThreadMessageHandler(), then it is irrelevant (I understand that in this case you may want to add GUARDED_BY(the newly added mutex))
    • If a variable can be accessed by other threads, then it should be clear which mutex is protecting it, have GUARDED_BY() and the mutex held when calling MaybeSendFeefilter().
  13. MarcoFalke added the label Waiting for author on Jun 5, 2021
  14. DrahtBot added the label Needs rebase on Jan 24, 2022
  15. MarcoFalke removed the label Waiting for author on Jan 24, 2022
  16. MarcoFalke force-pushed on Jan 24, 2022
  17. MarcoFalke commented at 4:02 pm on January 24, 2022: member

    Suggest adding a // GUARDED_BY(…) comment on g_filter_rounder (along the lines of 9ac4792).

    Thanks, added a // needed for ... comment.

    My concerns expressed in #21527 (comment) are valid for this PR too.

    Thanks, I’ve replied there. Though, I think this pull makes sense on its own because it (minimally) increases the performance of blockchain-related RPCs that actually need the cs_main lock. As of now, the cs_main lock is both confusing and (minimally) harmful.

    this PR introduces the mutex without any GUARDED_BY(the new mutex). So it is unclear what is being protected by it.

    Thanks, added a comment // needed for ...

    bool PeerManagerImpl::m_ignore_incoming_txs

    This is const, so there can’t possibly be any race.

    std::unique_ptr CNode::m_tx_relay

    This is initialized in the constructor, so there can’t possibly be any race.

    std::atomic CNode::m_greatest_common_version

    This is atomic, so there can’t possibly be any race. Also, there can’t be a logical race introduced in this pr, because it is not guarded by cs_main in other threads.

    NetPermissionFlags CNode::m_permissionFlags

    This is initialized before the Peer is initialized in net_processing. MaybeSendFeefilter isn’t called for objects without a Peer, so there is no race introduced by this patch.

    CTxMemPool& PeerManagerImpl::m_mempool

    A reference as member variable is like a const pointer, so there can’t possibly be any race reading the pointer.

    ChainstateManager& PeerManagerImpl::m_chainman

    Same

    ChainstateManager::ActiveChainstate()

    The lock is taken by this function. Currently we don’t allow loading the active chainstate after init, so this reference can be treated like a “const pointer”. If there is a patch in the future that allows re-seating the active chainstate, a lot more places need to be fixed up. My point is that re-seating shouldn’t be done here, but this is a discussion for the future.

    CChainState::IsInitialBlockDownload()

    The appropriate locks are taken by this function where needed. This function doesn’t care about the return value anyway, as long as it is a bool that goes from true->false.

    FeeFilterRounder g_filter_rounder

    Added comment in this pull

    FeeFilterRounder::round()

    Same

    CAmount CNode::TxRelay::lastSentFeeFilter

    This is only read and written by this function. Happy to add the annotation here as well, if reviewers think it is worth it?

    std::chrono::microseconds CNode::TxRelay::m_next_send_feefilter

    Same

    (global) CFeeRate minRelayTxFee

    This is set in init, and treated as const afterward. If there is a patch in the future making it mutable, that patch should add the appropriate locks.

    PeerManagerImpl::m_connman

    A reference as member variable is like a const pointer, so there can’t possibly be any race reading the pointer.

    CConnman::PushMessage()

    This function takes locks where needed and is thread-safe.

  18. MarcoFalke commented at 4:03 pm on January 24, 2022: member
    Thanks for the review. Addressed all feedback.
  19. DrahtBot removed the label Needs rebase on Jan 24, 2022
  20. fanquake requested review from ajtowns on Jan 25, 2022
  21. fanquake requested review from jnewbery on Jan 25, 2022
  22. net_processing: add m_mutex_message_handling 
    Since message handling is single threaded, this can simplify
guarding any variables only accessed during message handling.
    7148d33801
  23. MarcoFalke force-pushed on Feb 22, 2022
  24. Release cs_main before MaybeSendFeefilter fab56fb47d
  25. MarcoFalke force-pushed on Feb 22, 2022
  26. MarcoFalke commented at 8:47 am on February 22, 2022: member
    Rebased and clarified the goals (as initially pointed out in OP) in the source code as well. The new comment also explains the history and future, so that developers don’t have to use git to lookup this discussion thread.
  27. MarcoFalke commented at 8:48 am on February 22, 2022: member
    Still trivial to re-ACK as there are no code changes: git range-diff bitcoin-core/master faca414431 fab56fb47d531.
  28. vasild commented at 10:33 am on February 22, 2022: member

    After re-reading carefully the comments and the diff, I still think this is flawed because it introduces a mutex that protects “anything only accessed by” func1(), func2() or func3(). Mutexes protect data (variables). Those variables must be annotated with GUARDED_BY(). No such clause is used with the newly introduced mutex. It is supposed to protect something vague which is hard to check and very easy to change or break in the future - all the variables accessed directly or indirectly by a list of functions.

    What about this:

    1. Make the newly introduced mutex global
    2. Make g_filter_rounder global and annotate it with GUARDED_BY(the new mutex)
    3. Annotate CNode::TxRelay::lastSentFeeFilter and CNode::TxRelay::m_next_send_feefilter with GUARDED_BY(the new mutex)

    A reference as member variable is like a const pointer

    I don’t think so, a const pointer cannot change, but a reference member can. The following does not compile:

     0#include <vector>
 1
 2class A
 3{
 4public:
 5    A(std::vector<int>& arg) : m1{arg}, m2{nullptr} {};
 6
 7    std::vector<int>& m1;
 8    std::vector<int>* const m2;
 9};
10
11int main(int, char**)
12{
13    std::vector<int> v1;
14    std::vector<int> v2;
15    A a(v1);
16
17    // ok
18    a.m1 = v2;
19
20    // error: cannot assign to non-static data member 'm2' with const-qualified type 'std::vector<int> *const'
21    a.m2 = nullptr;
22
23    return 0;
24}
  29. MarcoFalke commented at 10:38 am on February 22, 2022: member

    I don’t think so, a const pointer cannot change, but a reference member can. The following does not compile:

    TIL

  30. MarcoFalke commented at 11:04 am on February 22, 2022: member

    Mutexes protect data (variables).

    Yes, but this is not the only use of Mutexes. In this context the mutex is not strictly needed to guard from UB. It is possible to remove the existing cs_main annotation in the function and compile without compiler warnings and run the program without UB.

    The goal of the mutex here is to allow only one caller to execute the function.

    very easy to change or break in the future

    The assumption that I touch here is documented in the source code and has a compiler annotation, both before the changes here and after. Breaking it would require removing the compiler annotation and runtime check, as well as the comment.

  31. vasild commented at 3:52 pm on February 22, 2022: member
    I mean that “anything only accessed by …list of functions…” is “something vague which is hard to check and very easy to change or break in the future”.
  32. MarcoFalke commented at 4:03 pm on February 22, 2022: member

    It is trivial to check with AssertLockHeld. Forgetting to add the annotation to a function is no different than forgetting to add the annotation to a data member.

    In any case, this pull isn’t changing anything about that. Previously there was a AssertLockHeld for cs_main, now there is one for a different mutex. If you have concerns about the code in current master, it might be best to create a separate discussion thread.

    I think the discussion in this pull request should focus on the goal and how the code changes help to achieve the goal. The goal is to reduce the use of cs_main to mean “global lock for everything” and work toward cs_main to mean “validation lock for validation stuff only”.

  33. vasild commented at 5:51 pm on February 22, 2022: member

    Then just moving the call to MaybeSendFeefilter() after cs_main is released would achieve the goal. It is 1-line change.

    Another attempt to explain: right now there are e.g. 25 variables accessed directly or indirectly by ProcessMessage(), ProcessMessages() and SendMessages(). If we presume that they are protected by the newly added m_mutex_message_handling, then any other change anywhere in the code that touches existent variables has to check (manually by code inspection!) if the touched variable is any of those 25 and if it is, it has to acquire m_mutex_message_handling. If it does not acquire it, then there will be a race without a compilation or runtime error. This comment, added in this PR, is confusing (or even bizarre):

    0/** Message handling mutex.
1 *  Message processing is single-threaded, so anything only accessed
2 *  by ProcessMessage(s) or SendMessages can be guarded by this mutex,
3 *  which guarantees it's only accessed by a single thread.

    at least somebody else finds the comment confusing too: #21527 (review)

  34. MarcoFalke commented at 8:05 am on February 23, 2022: member

    Then just moving the call to MaybeSendFeefilter() after cs_main is released would achieve the goal. It is 1-line change.

    It would require also removing the AssertLockHeld(cs_main);. If reviewers think this 2-line patch is preferable, I am more than happy to go with it, as long as I can make progress removing cs_main.

  35. vasild commented at 9:16 am on February 23, 2022: member

    Then just moving the call to MaybeSendFeefilter() after cs_main is released would achieve the goal. It is 1-line change.

    It would require also removing the AssertLockHeld(cs_main);. If reviewers think this 2-line patch is preferable, I am more than happy to go with it, as long as I can make progress removing cs_main.

    +1, should be ok, I will review carefully if you go that way.

  36. MarcoFalke commented at 9:29 am on February 23, 2022: member
    Closing for now. Reviewers, please ACK/NACK #24427 instead.
  37. MarcoFalke closed this on Feb 23, 2022
  38. MarcoFalke deleted the branch on Feb 23, 2022
  39. DrahtBot locked this on Feb 23, 2023


MarcoFalke DrahtBot jnewbery lsilva01 vasild ajtowns


ajtowns jnewbery

Labels
Refactoring P2P

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