Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move remaining application layer data to net processing #19398

Closed
16 tasks done
jnewbery opened this issue Jun 27, 2020 · 24 comments
Closed
16 tasks done

Move remaining application layer data to net processing #19398

jnewbery opened this issue Jun 27, 2020 · 24 comments

Comments

@jnewbery
Copy link
Contributor

jnewbery commented Jun 27, 2020

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:

CNode with comments. See TODO comments.
/** 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);
};
@jnewbery
Copy link
Contributor Author

jnewbery commented Jun 27, 2020

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.

@jnewbery
Copy link
Contributor Author

jnewbery commented Jul 8, 2020

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.

@practicalswift
Copy link
Contributor

Concept ACK

@jnewbery
Copy link
Contributor Author

#19472 is merged. Next up is #19583.

@jnewbery
Copy link
Contributor Author

#19583 is merged. Next is #19607.

@sdaftuar
Copy link
Member

sdaftuar commented Aug 5, 2020

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

@jnewbery
Copy link
Contributor Author

jnewbery commented Aug 6, 2020

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:

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.

@sdaftuar
Copy link
Member

sdaftuar commented Aug 6, 2020

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

laanwj added a commit that referenced this issue Aug 28, 2020
8e35bf5 scripted-diff: rename misbehavior members (John Newbery)
1f96d2e [net processing] Move misbehavior tracking state to Peer (John Newbery)
7cd4159 [net processing] Add Peer (John Newbery)
aba0335 [net processing] Remove CNodeState.name (John Newbery)

Pull request description:

  We currently have two structures for per-peer data:

  - `CNode` in net, which should just contain connection layer data (eg socket, send/recv buffers, etc), but currently also contains some application layer data (eg tx/block inventory).
  - `CNodeState` in net processing, which contains p2p application layer data, but requires cs_main to be locked for access.

  This PR adds a third struct `Peer`, which is for p2p application layer data, and doesn't require cs_main. Eventually all application layer data from `CNode` should be moved to `Peer`, and any data that doesn't strictly require cs_main should be moved from `CNodeState` to `Peer` (probably all of `CNodeState` eventually).

  `Peer` objects are stored as shared pointers in a net processing global map `g_peer_map`, which is protected by `g_peer_mutex`. To use a `Peer` object, `g_peer_mutex` is locked, a copy of the shared pointer is taken, and the lock is released. Individual members of `Peer` are protected by different mutexes that guard related data. The lifetime of the `Peer` object is managed by the shared_ptr refcount.

  This PR adds the `Peer` object and moves the misbehaving data from `CNodeState` to `Peer`. This allows us to immediately remove 15 `LOCK(cs_main)` instances.

  For more motivation see #19398

ACKs for top commit:
  laanwj:
    Code review ACK 8e35bf5
  troygiorshev:
    reACK 8e35bf5 via `git range-diff master 9510938 8e35bf5`
  theuni:
    ACK 8e35bf5.
  jonatack:
    ACK 8e35bf5 keeping in mind Cory's comment (#19607 (comment)) for the follow-up

Tree-SHA512: ad84a92b78fb34c9f43813ca3dfbc7282c887d55300ea2ce0994d134da3e0c7dbc44d54380e00b13bb75a57c28857ac3236bea9135467075d78026767a19e4b1
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Aug 28, 2020
…rocessing

8e35bf5 scripted-diff: rename misbehavior members (John Newbery)
1f96d2e [net processing] Move misbehavior tracking state to Peer (John Newbery)
7cd4159 [net processing] Add Peer (John Newbery)
aba0335 [net processing] Remove CNodeState.name (John Newbery)

Pull request description:

  We currently have two structures for per-peer data:

  - `CNode` in net, which should just contain connection layer data (eg socket, send/recv buffers, etc), but currently also contains some application layer data (eg tx/block inventory).
  - `CNodeState` in net processing, which contains p2p application layer data, but requires cs_main to be locked for access.

  This PR adds a third struct `Peer`, which is for p2p application layer data, and doesn't require cs_main. Eventually all application layer data from `CNode` should be moved to `Peer`, and any data that doesn't strictly require cs_main should be moved from `CNodeState` to `Peer` (probably all of `CNodeState` eventually).

  `Peer` objects are stored as shared pointers in a net processing global map `g_peer_map`, which is protected by `g_peer_mutex`. To use a `Peer` object, `g_peer_mutex` is locked, a copy of the shared pointer is taken, and the lock is released. Individual members of `Peer` are protected by different mutexes that guard related data. The lifetime of the `Peer` object is managed by the shared_ptr refcount.

  This PR adds the `Peer` object and moves the misbehaving data from `CNodeState` to `Peer`. This allows us to immediately remove 15 `LOCK(cs_main)` instances.

  For more motivation see bitcoin#19398

ACKs for top commit:
  laanwj:
    Code review ACK 8e35bf5
  troygiorshev:
    reACK 8e35bf5 via `git range-diff master 9510938 8e35bf5`
  theuni:
    ACK 8e35bf5.
  jonatack:
    ACK 8e35bf5 keeping in mind Cory's comment (bitcoin#19607 (comment)) for the follow-up

Tree-SHA512: ad84a92b78fb34c9f43813ca3dfbc7282c887d55300ea2ce0994d134da3e0c7dbc44d54380e00b13bb75a57c28857ac3236bea9135467075d78026767a19e4b1
@jnewbery
Copy link
Contributor Author

#20624 is the next PR in this sequence

@jnewbery
Copy link
Contributor Author

#19829 is up next

maflcko pushed a commit that referenced this issue Dec 22, 2020
…ssing

3002b4a [net processing] Guard m_continuation_block with m_block_inv_mutex (John Newbery)
184557e [net processing] Move hashContinue to net processing (John Newbery)
c853ef0 scripted-diff: rename vBlockHashesToAnnounce and vInventoryBlockToSend (John Newbery)
53b7ac1 [net processing] Move block inventory data to Peer (John Newbery)
78040f9 [net processing] Rename nStartingHeight to m_starting_height (John Newbery)
77a2c2f [net processing] Move nStartingHeight to Peer (John Newbery)
717a374 [net processing] Improve documentation for Peer destruction/locking (John Newbery)

Pull request description:

  This continues the work of moving application layer data into net_processing, by moving all block inventory state into the new Peer object added in #19607.

  For motivation, see #19398.

ACKs for top commit:
  jonatack:
    re-ACK 3002b4a per `git diff 9aad3e4 3002b4a`
  Sjors:
    Code review re-ACK 3002b4a
  MarcoFalke:
    re-ACK 3002b4a 🌓

Tree-SHA512: eb2b474b73b025791ee3e6e41809926b332b48468763219f31638ca390f427632f05902dfc6a2c6bdc1ce47b215782f67874ddbf05b97d77d5897b7e2abfe4d9
@jnewbery
Copy link
Contributor Author

Next is #20721

@ajtowns
Copy link
Contributor

ajtowns commented Dec 23, 2020

cf #20758

@jnewbery
Copy link
Contributor Author

jnewbery commented Feb 12, 2021

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

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

nolim1t pushed a commit to nolim1t/bitcoin-upstream that referenced this issue Feb 16, 2021
a5e15ae scripted-diff: rename ping members (John Newbery)
45dcf22 [net processing] Move ping data fields to net processing (John Newbery)
dd2646d [net processing] Move ping timeout logic to net processing (John Newbery)
0b43b81 [net processing] Move send ping message logic into function (John Newbery)
1a07600 [net] Add RunInactivityChecks() (John Newbery)
f8b3058 [net processing] Add Peer& arg to MaybeDiscourageAndDisconnect() (John Newbery)

Pull request description:

  This continues the work of moving application layer data into net_processing, by moving all ping data into the new Peer object added in bitcoin#19607.

  For motivation, see bitcoin#19398.

ACKs for top commit:
  glozow:
    reACK bitcoin@a5e15ae
  MarcoFalke:
    review ACK a5e15ae 🥉
  amitiuttarwar:
    ACK a5e15ae

Tree-SHA512: fb84241613d6a6e1f2832fa5378030b5877a02e8308188f57ab545a6eaf2ab731a93abb7dcd3a7f7285bb66700f938096378a8e90cd6a3e6f3309f81d85a344e
@jnewbery
Copy link
Contributor Author

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

@jnewbery
Copy link
Contributor Author

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

@jnewbery
Copy link
Contributor Author

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

maflcko pushed a commit that referenced this issue Apr 1, 2021
…MaybeSendAddr()

935d488 [net processing] Refactor MaybeSendAddr() (John Newbery)
01a79ff [net processing] Fix overindentation in MaybeSendAddr() (John Newbery)
38c0be5 [net processing] Refactor MaybeSendAddr() - early exits (John Newbery)
c87423c [net processing] Change MaybeSendAddr() to take a reference (John Newbery)
ad71929 [net processing] Extract `addr` send functionality into MaybeSendAddr() (John Newbery)
4ad4abc [net] Change addr send times fields to be guarded by new mutex (John Newbery)
c02fa47 [net processing] Only call GetTime() once in SendMessages() (John Newbery)

Pull request description:

  This continues the work of moving application layer data into net_processing. It refactors `addr` send functionality into its own function `MaybeSendAddr()` and flattens/simplifies the code. Isolating and simplifying the addr handling code makes subsequent changes (which will move addr data and logic into net processing) easier to review.

  This is a pure refactor. There are no functional changes.

  For motivation of the project, see #19398.

ACKs for top commit:
  sipa:
    utACK 935d488
  hebasto:
    ACK 935d488, I have reviewed the code and it looks OK, I agree it can be merged.
  MarcoFalke:
    review ACK 935d488 🐑

Tree-SHA512: 4e9dc84603147e74f479a211b42bcf315bdf5d14c21c08cf0b17d6c252775b90b012f0e0d834f1a607ed63c7ed5c63d5cf49b134344e7b64a1695bfcff111c92
@jnewbery
Copy link
Contributor Author

jnewbery commented Apr 1, 2021

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

@rebroad
Copy link
Contributor

rebroad commented May 11, 2021

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.

@jnewbery
Copy link
Contributor Author

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.

fanquake added a commit that referenced this issue May 24, 2021
0829516 [refactor] Remove unused ForEachNodeThen() template (John Newbery)
09cc66c scripted-diff: rename address relay fields (John Newbery)
76568a3 [net processing] Move addr relay data and logic into net processing (John Newbery)
caba7ae [net processing] Make RelayAddress() a member function of PeerManagerImpl (John Newbery)
86acc96 [net processing] Take NodeId instead of CNode* as originator for RelayAddress() (John Newbery)

Pull request description:

  This continues the work of moving application layer data into net_processing, by moving all addr data into the new Peer object added in #19607.

  For motivation, see #19398.

ACKs for top commit:
  laanwj:
    Code review ACK 0829516
  mzumsande:
    ACK 0829516, reviewed the code and ran tests.
  sipa:
    utACK 0829516
  hebasto:
    re-ACK 0829516

Tree-SHA512: efe0410fac288637f203eb37d1999910791e345872d37e1bd5cde50e25bb3cb1c369ab86b3a166ffd5e06ee72e4508aa2c46d658be6a54e20b4f220d2f57d0a6
@jnewbery
Copy link
Contributor Author

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

fanquake added a commit that referenced this issue Mar 25, 2022
1066d10 scripted-diff: rename TxRelay members (John Newbery)
575bbd0 [net processing] Move tx relay data to Peer (John Newbery)
785f55f [net processing] Move m_wtxid_relay to Peer (John Newbery)
3634670 [net] Add CNode.m_relays_txs and CNode.m_bloom_filter_loaded (John Newbery)

Pull request description:

  This continues the work of moving application layer data into net_processing, by moving all tx data into the new Peer object added in #19607.

  For motivation, see #19398.

ACKs for top commit:
  dergoegge:
    ACK 1066d10 - This is a good layer separation improvement with no behavior changes.
  glozow:
    utACK 1066d10

Tree-SHA512: 0c9d6b8a0a05e2d816b6d6588b7df133842ec960ae67667813422aa7bd8eb5308599c714f3822a98ddbdf364ffab9050b055079277ba4aff24092557ff99ebcc
@jnewbery
Copy link
Contributor Author

#21160 is merged. Next PR is #24970.

@jnewbery
Copy link
Contributor Author

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.

maflcko pushed a commit that referenced this issue Jul 19, 2022
…Services to Peer

8d8eeb4 [net processing] Remove CNode::nLocalServices (John Newbery)
5961f8e [net] Return CService from GetLocalAddrForPeer and GetLocalAddress (dergoegge)
d9079fe [net processing] Remove CNode::nServices (John Newbery)
7d1c036 [net processing] Replace fHaveWitness with CanServeWitnesses() (John Newbery)
f65e83d [net processing] Remove fClient and m_limited_node (John Newbery)
fc5eb52 [tests] Connect peer in outbound_slow_chain_eviction by sending p2p messages (John Newbery)
1f52c47 [net processing] Add m_our_services and m_their_services to Peer (John Newbery)

Pull request description:

  Another step in #19398. Which services we offer to a peer and which services they offer to us is application layer data and should not be stored on `CNode`.

  This is also a prerequisite for adding `PeerManager` unit tests (See #25515).

ACKs for top commit:
  MarcoFalke:
    ACK 8d8eeb4 🔑
  jnewbery:
    utACK 8d8eeb4
  mzumsande:
    Code Review ACK 8d8eeb4

Tree-SHA512: e772eb2a0a85db346dd7b453a41011a12756fc7cbfda6a9ef6daa9633b9a47b9770ab3dc02377690f9d02127301c3905ff22905977f758bf90b17a9a35b37523
@bitcoin bitcoin locked and limited conversation to collaborators Jun 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants
@ajtowns @jnewbery @rebroad @sdaftuar @practicalswift and others