Move remaining application layer data to net processing #19398

The application layer is any data that is transmitted within P2P message payloads, and the processing of that data. Examples are tx inventory, addr gossiping, ping/pong processing.

CNode currently contains many data and function members that are concerned with the application layer. These should be moved into net processing, so that CNode is only concerned with the network layer (sending/receiving bytes, keeping statistics, eviction logic, etc).

One blocker to moving these is that the existing peer data structure in net processing is CNodeState, which is guarded by cs_main. Moving all application layer data into CNodeState would expand where we need to take and hold cs_main locks. Instead, we should create a new data structure in net processing called Peer which doesn't require a cs_main lock, and move the application layer data there.

https://github.com/jnewbery/bitcoin/tree/2020-06-cnode-comments is a move/comment only branch that re-orders the CNode data members into logical groups and adds comments for each member, including TODOs for members that should be moved to net processing. The branch isn't intended for merging, but is a guide for what I think needs to change in CNode.

https://github.com/jnewbery/bitcoin/tree/2020-06-cs-main-split is a branch that implements Peer and starts moving application layer data into that structure. I intend to peel off commits from that branch into separate PRs. That branch also starts moving data that doesn't require the cs_main lock from CNodeState into Peer. Longer term, I believe almost all CNodeState data can be moved into Peer, greatly reducing the scope that cs_main locks are held in net processing.

Any help reviewing or implementing these changes would be very much appreciated!

PRs:

• #19219 Replace automatic bans with discouragement filter
• #19472 Reduce cs_main scope in MaybeDiscourageAndDisconnect()
• #19583 clean up Misbehaving()
• #19607 Add Peer struct for per-peer data in net processing
• #19910 Move peer_map to PeerManager
• #20624 Remove nStartingHeight check from block relay
• #19829 Move block inventory state to net_processing
• #20651 Make p2p recv buffer timeout 20 minutes for all peers
• #20811 Move net_processing implementation details out of header
• #20927 Clean up InactivityCheck()
• #20721 Move ping data to net_processing
• #21187 Only call PushAddress() from net_processing
• #21236 Extract addr send functionality into MaybeSendAddr()
• #21186 Move addr data into net_processing
• #21162 Move RelayTransaction() into PeerManager
• #21160 Move tx data into net_processing

<details><summary><code>CNode</code> with comments. See TODO comments.</summary>

/** Information about a peer */
class CNode
{
    friend class CConnman;
    friend struct ConnmanTestMsg;

public:
    /** A semaphore limits the number of outbound and manual peers. This
     *  CNode holds the grant until the connection is closed, at which point
     *  it's released to allow another connection. */
    CSemaphoreGrant grantOutbound;
    /** Reference count to prevent the CNode from being deleted while there
     *  are still references to it being held.
     *  TODO: replace with std::shared_ptr */
    std::atomic<int> nRefCount{0};

    /** Socket mutex */
    RecursiveMutex cs_hSocket;
    /** Socket */
    SOCKET hSocket GUARDED_BY(cs_hSocket);

    /** Send buffer mutex */
    RecursiveMutex cs_vSend;
    /** Send buffer */
    std::deque<std::vector<unsigned char>> vSendMsg GUARDED_BY(cs_vSend);
    /** Total size of all vSendMsg entries */
    size_t nSendSize{0};
    /** Offset inside the first vSendMsg already sent */
    size_t nSendOffset{0};
    /** Total bytes sent to this peer */
    uint64_t nSendBytes GUARDED_BY(cs_vSend){0};
    /** Whether the send buffer is full and we should pause sending
     *  data to this peer. */
    std::atomic_bool fPauseSend{false};

    /** Send processing mutex. Ensures that we don't enter SendMessages()
     *  for this peer on multiple threads */
    RecursiveMutex cs_sendProcessing;

    /** Receive buffer mutex */
    RecursiveMutex cs_vProcessMsg;
    /** Buffer of deserialized net messages */
    std::list<CNetMessage> vProcessMsg GUARDED_BY(cs_vProcessMsg);
    /** Total size of receive buffer mutex */
    size_t nProcessQueueSize{0} GUARDED_BY(cs_vProcessMsg);
    /** Whether the receive buffer is full and we should pause receiving
     *  data from this peer. */
    std::atomic_bool fPauseRecv{false};

    /** Receive buffer statistics mutex */
    RecursiveMutex cs_vRecv;
    /** Total bytes received from this peer */
    uint64_t nRecvBytes GUARDED_BY(cs_vRecv){0};

    /** Address of this peer */
    const CAddress addr;
    /** Bind address of our side of the connection */
    const CAddress addrBind;
    /** Mutex guarding the cleanSubVer field.
     *  TODO: replace with atomic */
    RecursiveMutex cs_SubVer;
    /** Sanitized string of the user agent byte array we read from the wire.
     *  This cleaned string can safely be logged or displayed.  */
    std::string cleanSubVer GUARDED_BY(cs_SubVer){};
    /** Unusued in actual processing, only present for backward compatibility at RPC/QT level */
    bool m_legacyWhitelisted{false};

    /** If this peer is being used as a short lived feeler. */
    bool fFeeler{false}; 
    /** If this peer is being used to fetch addresses and then disconnect */
    bool fOneShot{false};
    /** If this peer is a manual connection added by command-line argument or RPC */
    bool m_manual_connection{false};
    /** If the connection with this peer was initiated by the peer */
    const bool fInbound;

    /** If the version-verack handshake has successfully completed. */
    std::atomic_bool fSuccessfullyConnected{false};
    /** Setting fDisconnect to true will cause the node to be disconnected the
    / * next time DisconnectNodes() runs */
    std::atomic_bool fDisconnect{false};

    /** If this peer is a light client (doesn't serve blocks).
     *  TODO: move this application layer data to net processing. */
    bool fClient{false};
    /** If this peer is 'limited' (can only serve recent blocks).
     *  TODO: move this application layer data to net processing. */
    bool m_limited_node{false};

    /** Whether this peer is preferred for eviction */
    bool m_prefer_evict{false};
    /** The time of the last message sent to this peer. Used in inactivity checks */
    std::atomic<int64_t> nLastSend{0};
    /** The time of the last message received from this peer. Used in inactivity checks */
    std::atomic<int64_t> nLastRecv{0};
    /** Which netgroup this peer is in. Used in eviction logic */
    const uint64_t nKeyedNetGroup;
    /** Last time we accepted a block from this peer. Used in eviction logic */
    std::atomic<int64_t> nLastBlockTime{0};
    /** Last time we accepted a transaction from this peer. Used in eviction logic */
    std::atomic<int64_t> nLastTXTime{0};
    /** Best measured round-trip time for this peer. Used in eviction logic */
    std::atomic<int64_t> nMinPingUsecTime{std::numeric_limits<int64_t>::max()};

    /** The time that the connection with this node was established. Used in eviction logic */
    const int64_t nTimeConnected;
    /** The difference between the peer's clock and our own. Only used in logging */
    std::atomic<int64_t> nTimeOffset{0};

    /** The P2P version announced by the peer in its version message.
     *  TODO: this is only used in the application layer. Move to net processing */
    std::atomic<int> nRecvVersion{INIT_PROTO_VERSION};
    /** The P2P version announced by the peer in its version message.
     *  TODO: This seems to largely a duplicate of nRecvVersion. Remove. */
    std::atomic<int> nVersion{0};
    /** The supported services announced by the peer in its version message.
     *  TODO: Move this application layer data to net processing. */
    std::atomic<ServiceFlags> nServices{NODE_NONE};

    /** Addresses to send to this peer.
     *  TODO: move this application layer data to net processing. */
    std::vector<CAddress> vAddrToSend;
    /** Probabilistic filter of addresses that this peer already knows.
     *  TODO: move this application layer data to net processing. */
    const std::unique_ptr<CRollingBloomFilter> m_addr_known;
    /** Whether a GETADDR request is pending from this node.
     *  TODO: move this application layer data to net processing. */
    bool fGetAddr{false};
    /** Timestamp after which we should send the next addr message to this peer.
     *  TODO: move this application layer data to net processing. */
    std::chrono::microseconds m_next_addr_send GUARDED_BY(cs_sendProcessing){0};
    /** Timestamp after which we should advertise our local address to this peer.
     *  TODO: move this application layer data to net processing. */
    std::chrono::microseconds m_next_local_addr_send GUARDED_BY(cs_sendProcessing){0};
    /** If we've sent an initial ADDR message to this peer.
     *  TODO: move this application layer data to net processing. */
    bool fSentAddr{false};

    /** Address relay mutex.
     *  TODO: move this application layer data to net processing. */
    RecursiveMutex cs_inventory;
    /** List of block ids we still have announce.
    / * There is no final sorting before sending, as they are always sent immediately
    / * and in the order requested.
     *  TODO: move this application layer data to net processing. */
    std::vector<uint256> vInventoryBlockToSend GUARDED_BY(cs_inventory);
    /** List of block hashes to relay in headers messages.
     *  TODO: move this application layer data to net processing. */
    std::vector<uint256> vBlockHashesToAnnounce GUARDED_BY(cs_inventory);
    /** When the peer requests this block, we send an inv that
      * triggers the peer to send a getblocks to fetch the next batch of
      * inventory. Only used for peers that don't do headers-first syncing.
      *  TODO: move this application layer data to net processing. */
    uint256 hashContinue;
    /** This peer's height, as announced in its version message.
      *  TODO: move this application layer data to net processing. */
    std::atomic<int> nStartingHeight{-1};

    struct TxRelay {
        /** bloom filter mutex */
        mutable RecursiveMutex cs_filter;
        /** We use fRelayTxes for two purposes -
         *  a) it allows us to not relay tx invs before receiving the peer's version message
         *  b) the peer may tell us in its version message that we should not relay tx invs
         *     unless it loads a bloom filter. */
        bool fRelayTxes GUARDED_BY(cs_filter){false};
        /** BIP 31 bloom filter */
        std::unique_ptr<CBloomFilter> pfilter PT_GUARDED_BY(cs_filter) GUARDED_BY(cs_filter){nullptr};

        /** Transaction relay mutex */
        mutable RecursiveMutex cs_tx_inventory;
        /** Probabilistic filter of txids that the peer already knows */
        CRollingBloomFilter filterInventoryKnown GUARDED_BY(cs_tx_inventory){50000, 0.000001};
        /** Set of transaction ids we still have to announce.
         * They are sorted by the mempool before relay, so the order is not important. */
        std::set<uint256> setInventoryTxToSend;
        /** Timestamp after which we should send the next transaction INV message to this peer */
        std::chrono::microseconds nNextInvSend{0};

        /** If the peer has a pending BIP 35 MEMPOOL request to us */
        bool fSendMempool GUARDED_BY(cs_tx_inventory){false};
        /** Last time a MEMPOOL request was serviced. */
        std::atomic<std::chrono::seconds> m_last_mempool_req{std::chrono::seconds{0}};

        /** Feefilter mutex */
        RecursiveMutex cs_feeFilter;
        /** Minimum fee rate with which to filter inv's to this node */
        CAmount minFeeFilter GUARDED_BY(cs_feeFilter){0};
        /** Last feefilter value we sent to the peer */
        CAmount lastSentFeeFilter{0};
        /** Timestamp after which we should send the next FEEFILTER message to this peer */
        int64_t nextSendTimeFeeFilter{0};
    };

    /** Transaction relay data for this peer. If m_tx_relay == nullptr then we don't
     *  relay transactions with this peer.
     *  TODO: move this application layer data to net processing. */
    std::unique_ptr<TxRelay> m_tx_relay;

    /** List of inv items requested by this peer in a getdata message.
     *  TODO: move this application layer data to net processing. */
    std::deque<CInv> vRecvGetData;

    /** The pong reply we're expecting, or 0 if no pong expected.
     *  TODO: move this application layer data to net processing. */
    std::atomic<uint64_t> nPingNonceSent{0};
    /** Time (in usec) the last ping was sent, or 0 if no ping was ever sent.
     *  TODO: move this application layer data to net processing. */
    std::atomic<int64_t> nPingUsecStart{0};
    /** Last measured ping round-trip time.
     *  TODO: move this application layer data to net processing. */
    std::atomic<int64_t> nPingUsecTime{0};
    /** Whether a ping request is pending to this peer.
     *  TODO: move this application layer data to net processing. */
    std::atomic<bool> fPingQueued{false};

    /** Orphan transactions to reconsider after the parent was accepted.
     *  TODO: move this application layer data to a global in net processing. */
    std::set<uint256> orphan_work_set;

private:
    /** Unique numeric identifier for this node */
    const NodeId id;
    /** Node name mutex
     *  TODO: replace with atomic */
    mutable RecursiveMutex cs_addrName;
    /** Node name */
    std::string addrName GUARDED_BY(cs_addrName);
    /** This node's permission flags. */
    NetPermissionFlags m_permissionFlags{ PF_NONE };
    /** addrLocal mutex
     *  TODO: replace with atomic */
    mutable RecursiveMutex cs_addrLocal;
    /** Our address, as reported by the peer */
    CService addrLocal GUARDED_BY(cs_addrLocal);

    /** Random nonce sent in our VERSION message to detect connecting to ourselves.
     *  TODO: move this application layer data to net processing */
    const uint64_t nLocalHostNonce;
    /** Services offered to this peer.
     *
     * This is supplied by the parent CConnman during peer connection
     * (CConnman::ConnectNode()) from its attribute of the same name.
     *
     * This is const because there is no protocol defined for renegotiating
     * services initially offered to a peer. The set of local services we
     * offer should not change after initialization.
     *
     * An interesting example of this is NODE_NETWORK and initial block
     * download: a node which starts up from scratch doesn't have any blocks
     * to serve, but still advertises NODE_NETWORK because it will eventually
     * fulfill this role after IBD completes. P2P code is written in such a
     * way that it can gracefully handle peers who don't make good on their
     * service advertisements.
     *
     * TODO: move this application layer data to net processing. */
    const ServiceFlags nLocalServices;
    /** Our starting height that we advertised to this node in our VERSION message.
     * TODO: this value is not used after sending the version message. We can remove this field. */
    const int nMyStartingHeight;
    /** The version that we advertised to the peer in our VERSION message.
     *  TODO: move this application layer data to net processing */
    int nSendVersion{0};

    /** Deserializer for messages received over the network. This is a derived
     * class of TransportDeserializer based on the P2P version used with this
     * peer. */
    std::unique_ptr<TransportDeserializer> m_deserializer;
    /** Serializer for messages sent over the network. This is a derived
     * class of TransportDeserializer based on the P2P version used with this
     * peer. */
    std::unique_ptr<TransportSerializer> m_serializer;

    /** Temporary buffer used by the SocketHandler thread for received messages,
     *  before they're pushed onto the vProcessMsg buffer. */
    std::list<CNetMessage> vRecvMsg;

    /** Statistics of bytes sent to this peer, broken out by message type */
    mapMsgCmdSize mapSendBytesPerMsgCmd GUARDED_BY(cs_vSend);
    /** Statistics of bytes received from this peer, broken out by message type */
    mapMsgCmdSize mapRecvBytesPerMsgCmd GUARDED_BY(cs_vRecv);

public:
    CNode(NodeId id, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn,
          const CAddress &addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn,
          const CAddress &addrBindIn, const std::string &addrNameIn = "",
          bool fInboundIn = false, bool block_relay_only = false);
    ~CNode();
    CNode(const CNode&) = delete;
    CNode& operator=(const CNode&) = delete;

    NodeId GetId() const {
        return id;
    }

    /** TODO: move this application layer function to net processing */
    uint64_t GetLocalNonce() const {return nLocalHostNonce;}

    /** TODO: move this application layer function to net processing */
    int GetMyStartingHeight() const {return nMyStartingHeight;}

    /** TODO: move this application layer function to net processing */
    ServiceFlags GetLocalServices() const { return nLocalServices; }

    /** TODO: move these application layer functions to net processing */
    void SetRecvVersion(int nVersionIn) { nRecvVersion = nVersionIn; }
    int GetRecvVersion() const { return nRecvVersion; }
    void SetSendVersion(int nVersionIn);
    int GetSendVersion() const;

    /** TODO: move this application layer function to net processing */
    bool IsAddrRelayPeer() const { return m_addr_known != nullptr; }

    /** TODO: Replace with std::shared_ptr refcounts */
    int GetRefCount() const
    {
        assert(nRefCount >= 0);
        return nRefCount;
    }

    CNode* AddRef()
    {
        nRefCount++;
        return this;
    }

    void Release()
    {
        nRefCount--;
    }

    bool ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete);

    CService GetAddrLocal() const;
    //! May not be called more than once
    void SetAddrLocal(const CService& addrLocalIn);

    std::string GetAddrName() const;
    //! Sets the addrName only if it was not previously set
    void MaybeSetAddrName(const std::string& addrNameIn);

    bool HasPermission(NetPermissionFlags permission) const {
        return NetPermissions::HasFlag(m_permissionFlags, permission);
    }

    /** TODO: move this application layer function to net processing */
    void AddAddressKnown(const CAddress& _addr)
    {
        assert(m_addr_known);
        m_addr_known->insert(_addr.GetKey());
    }

    /** TODO: move this application layer function to net processing */
    void PushAddress(const CAddress& _addr, FastRandomContext &insecure_rand)
    {
        // Known checking here is only to save space from duplicates.
        // SendMessages will filter it again for knowns that were added
        // after addresses were pushed.
        assert(m_addr_known);
        if (_addr.IsValid() && !m_addr_known->contains(_addr.GetKey())) {
            if (vAddrToSend.size() >= MAX_ADDR_TO_SEND) {
                vAddrToSend[insecure_rand.randrange(vAddrToSend.size())] = _addr;
            } else {
                vAddrToSend.push_back(_addr);
            }
        }
    }

    /** TODO: move this application layer function to net processing */
    void AddInventoryKnown(const CInv& inv)
    {
        if (m_tx_relay != nullptr) {
            LOCK(m_tx_relay->cs_tx_inventory);
            m_tx_relay->filterInventoryKnown.insert(inv.hash);
        }
    }

    /** TODO: move this application layer function to net processing */
    void PushTxInventory(const uint256& hash)
    {
        if (m_tx_relay == nullptr) return;
        LOCK(m_tx_relay->cs_tx_inventory);
        if (!m_tx_relay->filterInventoryKnown.contains(hash)) {
            m_tx_relay->setInventoryTxToSend.insert(hash);
        }
    }

    /** TODO: move this application layer function to net processing */
    void PushBlockInventory(const uint256& hash)
    {
        LOCK(cs_inventory);
        vInventoryBlockToSend.push_back(hash);
    }

    /** TODO: move this application layer function to net processing */
    void PushBlockHash(const uint256 &hash)
    {
        LOCK(cs_inventory);
        vBlockHashesToAnnounce.push_back(hash);
    }

    void CloseSocketDisconnect();

    void copyStats(CNodeStats &stats, const std::vector<bool> &m_asmap);
};

</details>

The cs_main split branch builds on top of #19219 (since the misbehavior/discouragement data members are moved to Peer). If you want to help with this effort, you can do so by reviewing that PR.

The next PR to review is #19472. It reduces the scope of cs_main locking in misbehaviour logic with the goal of eventually moving misbehavior data to the Peer struct.

Concept ACK

#19472 is merged. Next up is #19583.

#19583 is merged. Next is #19607.

One blocker to moving these is that the existing peer data structure in net processing is CNodeState, which is guarded by cs_main. Moving all application layer data into CNodeState would expand where we need to take and hold cs_main locks. Instead, we should create a new data structure in net processing called Peer which doesn't require a cs_main lock, and move the application layer data there.

Could you explain why this is a blocker? In practice, most of our net code is single-threaded anyway (one message processing thread), so my intuition is that there are limited places where it's actually problematic that we might hold cs_main longer. Could you point me to those places where it'd be a concern?

It seems to me that there are two things going on here: one is moving application layer code to net_processing data structures and out of net data structures, which seems like an obvious win to me (adding some new p2p features can be tedious because of the current split). The other is redoing the locking, which is less clear to me; I can imagine some potential use cases, but given our current architecture and behavior it's not clear to me that it's necessary to make changes here at the same time, as I don't expect there to be any real improvements until someone proposes more fundamental changes to our message processing logic.

Also, relating to the moving of data structures between net and net_processing, I think it'll be important to understand what you imagine the interface between those two modules to look like. For instance I think we've run into implementation difficulties/complexity when writing code that is right at the intersection of those two things -- such as stale tip checking, block-relay-only peer negotiation, eviction logic for inbound peers, etc -- so it would be helpful in a new design to talk about how you envision that working.

I suppose understanding how you expect the locking/synchronization to work would make sense as well, as that might also show how tightly coupled these changes are?

(A gist that explains all this at a high level would be very helpful.)

Could you explain why this is a blocker? In practice, most of our net code is single-threaded anyway (one message processing thread), so my intuition is that there are limited places where it's actually problematic that we might hold cs_main longer. Could you point me to those places where it'd be a concern?

"Most of our net code is single-threaded, so it's not a problem to hold cs_main longer" is thinking about this the wrong way round. Instead, it makes more sense to say "we hold cs_main almost everywhere in net_processing, so our current opportunities for parallelism are limited". Breaking up cs_main has a couple of very important benefits: firstly, it reduces the coupling between validation and net_processing and enforces a cleaner separation between those components. That's good because it allows better testing of individual components and reduces the cognitive overhead when thinking about changes in either. Secondly it allows the possibility of parallelism between net_processing and validation. That's something that's been discussed and proposed many times before:

• "The biggest gain I'm aware of from concurrency is that right now when connecting multiple blocks at a time (due to out of order fetching in IBD) we stop downloading new blocks ... fixing that would be a big gain; so it might be worth prioritizing improvements that would let that happen." http://www.erisian.com.au/meetbot/bitcoin-core-dev/2018/bitcoin-core-dev.2018-05-03-19.00.log.html#l-256
• "relaying compact block gettxn during block connection [...] is the one big cheapish win left for block-relay-latency" http://www.erisian.com.au/meetbot/bitcoin-core-dev/2018/bitcoin-core-dev.2018-05-03-19.00.log.html#l-258
• "Explicitly simplifying the concurrency model hopefully reduces a bit the cognitive burden of future code changes..." #12934 (comment)
• #16323

Implementing parallelism between validation and net_processing would be useful, but without reducing cs_main scope in net_processing, the gains are limited:

It's not going to be really at all useful until we do a ton of locking cleanups in net_processing (it'll let us run ahead to the next message, which almost always requires cs_main, and we'll end up blocking on validation anyway)."

#12934 (comment)

It seems to me that there are two things going on here: one is moving application layer code to net_processing data structures and out of net data structures, which seems like an obvious win to me (adding some new p2p features can be tedious because of the current split). The other is redoing the locking, which is less clear to me;

Currently, the model is that the per-peer data in CNode is owned by net, with synchronized access managed by mutexes in net. The proposal here is to change that so the data is owned by net_processing, with synchronized access managed by mutexes in net_processing. That's not currently possible because access to CNodeState is managed by a mutex in validation (cs_main). Hence the need to add a structure to net_processing whose access is managed by mutexes in net_processing.

The alternative (moving the data to have synchronized access managed by validation) is adding technical debt. Everything moved under cs_main will need to be unwound if we want to get the full benefits of making validation asynchronous.

More generally speaking, it's just good practice to have finer grained locks and to hold those locks for the minimum time possible:

Hopefully it's obvious that if you have one mutex protecting an entire data structure, not only is there likely to be more contention for the lock, but also the potential for reducing the time that the lock is held is diminished. More of the operation steps will require a lock on the same mutex, so the lock must be held longer. This double whammy of a cost is also a double incentive to move toward finer-grained locking wherever possible.

As this example shows, locking at an appropriate granularity isn't only about the amount of data locked; it's also about how long the lock is held and what operations are performed while the lock is held. In general, a lock should be held for only the minimum possible time needed to perform the required operations. [emphasis his] This also means that time-consuming operations such as acquiring another lock (even if you know it won't deadlock) or waiting for I/O to complete shouldn't be done while holding a lock unless absolutely necessary.

Anthony Williams, C++ Concurrency In Action

Also, relating to the moving of data structures between net and net_processing, I think it'll be important to understand what you imagine the interface between those two modules to look like.

I'm not proposing any changes to the interface here (except moving data that is only used in net_processing into net_processing). Once that movement is complete, further improvements to the interface may suggest themselves (and be easier to implement), but for now the goal is just to untangle the data ownership model between the two.

I suppose understanding how you expect the locking/synchronization to work would make sense as well, as that might also show how tightly coupled these changes are?

(A gist that explains all this at a high level would be very helpful.)

This is implemented in #19607. It's a pretty small PR, so should be fairly easy to review. I don't think I can add any clarity in a gist, but if there's anything that's unclear in the code, I'm very happy to expand the code comments so it's clear what's going on. https://github.com/jnewbery/bitcoin/tree/2020-06-cs-main-split builds on that and moves the CNode members into the new Peer structure.

"Most of our net code is single-threaded, so it's not a problem to hold cs_main longer" is thinking about this the wrong way round. Instead, it makes more sense to say "we hold cs_main almost everywhere in net_processing, so our current opportunities for parallelism are limited".

My point was that you're calling this locking issue a blocker; work gets done more quickly when there are fewer blockers, so I was trying to probe whether this is actually so. This is not the same as suggesting that we not pursue parallelism! But a question about whether we can pursue parallelism, ahem, in parallel with these other changes, rather than serially.

For example, another way to achieve your goals would be to dump everything into CNodeState, and then have a separate PR that unlocks parallelism by moving that which should not be synchronized by cs_main into something protected by its own lock. That's a worse strategy than doing it right the first time, to be sure, but to code reviewers who don't yet know how it's going to work eventually, it's hard to verify that what you're doing is right to begin with.

From reading everything you wrote, it seems like the main motivation behind the locking changes you're proposing is in support of parallelizing validation with net_processing. Is that correct? If so, I think it would be helpful to understand what the semantics of such parallelism would look like, in order to understand what locking semantics are useful in support of that concurrency. Ideas on how to parallelize net processing with validation have indeed been thrown around for years by many different people, but I'm not aware of any design that has been circulated to help us all understand what can be parallelized and, importantly, what cannot or should not be.

For example, one question I have is whether your design will allow for parallelizing net_processing with itself -- can we support more than 1 message processing thread? Are the design considerations for that different than if we just have 1 message processing thread?

Alternatively, if there is a small specific proposal (like asynchronous ProcessNewBlock) that is achievable on top of the specific refactors you're proposing, then that could be a great way to demonstrate why the design you're picking makes sense. But in the absence of a concrete goal it's very hard for me to say that one design is better than another -- I took a brief look at #19607 and it seems fine to me, but if you moved everything into CNodeState that would seem fine to me as well. If one of those designs works and the other doesn't for a feature we want, then obviously we should pick the design that works. But until that point, it seems to me that in the absence of a design document that we all agree on, other contributors may offer up code changes that don't conform to your particular design philosophy, and it will be hard to argue whether one or another is correct/incorrect.

Anyway I certainly don't mean to throw sand in the gears at all, because moving data structures out of net and into net_processing seems like a clear win, however you do it; I just want to explain why it's hard for me to understand the bigger picture about the path forward and why it should to be done in a particular way that you're proposing.

#20624 is the next PR in this sequence

#19829 is up next

Next is #20721

cf #20758

#20927 next

#20721 is next

I've opened the next four PRs in this series:

• #21187 Only call PushAddress() from net_processing
• #21186 Move addr data into net_processing
• #21162 Move RelayTransaction() into PeerManager
• #21160 Move tx data into net_processing

as drafts to share what the next steps in the project are. They're all based on #20721, so please review that PR first.

#20721 is merged. Next up are #21187 and #21162.

#21187 is merged. #21236 is ready for review.

#21162 is merged. Moving #21160 out of draft.

#21236 is merged. #21186 is now ready for review.

Ideas on how to parallelize net processing with validation have indeed been thrown around for years by many different people, but I'm not aware of any design that has been circulated to help us all understand what can be parallelized and, importantly, what cannot or should not be.

I thought #9599 worked quite well, and I was using it for years (until I rebased and haven't bothered re-including it) - I might try again as it might work better now that the locks have been made more granular. I never did quite understand the reasons for not merging #9599.

I've added #21527 (net_processing: lock clean up) to the list above since #21160 (Move tx data into net_processing) requires it. Anyone wanting to help with this project should review #21186 (Move addr data into net_processing) or #21527 (net_processing: lock clean up) next.

#21186 is merged. Next PRs in this series are blocked on #21527.

#21160 is merged. Next PR is #24970.

Closing since I don't have time to work on this. I think there's still enormous benefit in simplifying and rationalizing this interface, since it would allow much better testing of both the net and net_processing layers.