m_mutex_message_handling is taken from #21527, but should be independent and fine to merge in either this pull or the other.

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

Code review ACK faca41443120c254f45d7a4dd72d5d80efacaf59

cc @vasild @ajtowns for Approach ACK/NACK

Approach ACK

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

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.

Thanks for the review. Addressed all feedback.

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.

Still trivial to re-ACK as there are no code changes: git range-diff bitcoin-core/master faca414431 fab56fb47d531.

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:

#include <vector>

class A
{
public:
    A(std::vector<int>& arg) : m1{arg}, m2{nullptr} {};

    std::vector<int>& m1;
    std::vector<int>* const m2;
};

int main(int, char**)
{
    std::vector<int> v1;
    std::vector<int> v2;
    A a(v1);

    // ok
    a.m1 = v2;

    // error: cannot assign to non-static data member 'm2' with const-qualified type 'std::vector<int> *const'
    a.m2 = nullptr;

    return 0;
}

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

TIL

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.

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

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

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):

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

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

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.

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.

Closing for now. Reviewers, please ACK/NACK #24427 instead.