net processing: Move cleanSubVer, fPreferredDownload and nLocalHostNonce to Peer #24970

jnewbery commented at 11:48 AM on April 25, 2022: member

Next step in #19398. Moves additional members from CNode and CNodeState into Peer.

Also renames CNodeStateStats to PeerStats, since most of the stats are now taken from the Peer object.

MarcoFalke commented at 11:49 AM on April 25, 2022: member

Concept ACK

fanquake added the label P2P on Apr 25, 2022

hebasto commented at 12:17 PM on April 25, 2022: member

Concept ACK.

DrahtBot commented at 6:01 PM on April 25, 2022: 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:

#25355 (I2P: add support for transient addresses for outbound connections by vasild)
#25268 (refactor: Introduce EvictionManager by dergoegge)
#25174 (net/net_processing: Add thread safety related annotations for CNode and Peer by ajtowns)
#24571 (p2p: Prevent block index fingerprinting by sending additional getheaders messages by dergoegge)
#24170 (p2p, rpc: Manual block-relay-only connections with addnode by mzumsande)
#23443 (p2p: Erlay support signaling by naumenkogs)

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.

theStack commented at 12:56 AM on April 26, 2022: member

Concept ACK

jnewbery force-pushed on Apr 26, 2022

jnewbery commented at 8:47 AM on April 26, 2022: member

Fixed a bug (subver may not be present in the getpeerinfo results while the peer connection is being torn down, so needs to be marked as optional) and rebased on master.

I'm going to mark this as a draft since it conflicts with and overlaps with #24543, which has been open for longer.

jnewbery marked this as a draft on Apr 26, 2022

jnewbery force-pushed on Apr 30, 2022

jnewbery commented at 10:40 AM on April 30, 2022: member

Rebased on master now that #24543 is merged.

Leaving this as draft for now. I think there may be a better way of handling the version nonces.

DrahtBot added the label Needs rebase on May 16, 2022

jnewbery force-pushed on May 16, 2022

jnewbery commented at 6:21 PM on May 16, 2022: member

Rebased on master and tidied up the nonce handling code. This is now ready for review.

jnewbery marked this as ready for review on May 16, 2022

DrahtBot removed the label Needs rebase on May 16, 2022

in src/net_processing.cpp:210 in 53d581f187 outdated

 205 | @@ -204,6 +206,15 @@ struct Peer {
 206 |      /** Same id as the CNode object for this peer */
 207 |      const NodeId m_id{0};
 208 |  
 209 | +    /** Protects subversion data member */
 210 | +    RecursiveMutex m_subver_mutex;

theStack commented at 8:14 PM on May 16, 2022:

This has changed from RecursiveMutex to Mutex (since #24079 has been merged a few months ago).

    Mutex m_subver_mutex;

jnewbery commented at 9:46 PM on May 16, 2022:

Thanks. Fixed!

jnewbery force-pushed on May 16, 2022

jnewbery force-pushed on May 17, 2022

DrahtBot added the label Needs rebase on Jun 2, 2022

scripted-diff: rename CNodeStateStats

Most of the stats in CNodeStateStats are now taken from the Peer object.
Rename the struct to PeerStats.

-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }

ren CNodeStateStats          PeerStats
ren GetNodeStateStats        GetPeerStats
ren nodeStateStats           m_peer_stats
ren statestats               peer_stats
ren fNodeStateStatsAvailable m_peer_stats_available
ren fStateStats              peer_stats_available

-END VERIFY SCRIPT-

6934b2028e

[net processing] Move cleanSubVer to net_processing

Also rename to m_clean_subversion.

Since the subversion is now contained in the Peer object, it may not be
available in `getpeerinfo` while the peer connection is being torn down.
Update the rpc help text to mark the field optional and update the tests
to tolerate the field being absent.

f952da6e08

[net processing] Move fPreferredDownload to Peer

Also rename to m_preferred_download

4680a64075

jnewbery force-pushed on Jun 3, 2022

jnewbery commented at 10:51 AM on June 3, 2022: member

Rebased

DrahtBot removed the label Needs rebase on Jun 3, 2022

in src/net_processing.cpp:1344 in f6bf0d1787 outdated

1337 | @@ -1311,6 +1338,10 @@ void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler)
1338 |  void PeerManagerImpl::FinalizeNode(const CNode& node)
1339 |  {
1340 |      NodeId nodeid = node.GetId();
1341 | +    {
1342 | +        LOCK(m_version_nonces_mutex);
1343 | +        m_version_nonces.erase(nodeid);
1344 | +    }

theStack commented at 7:56 PM on June 5, 2022:

    WITH_LOCK(m_version_nonces_mutex, m_version_nonces.erase(nodeid));

jnewbery commented at 11:21 AM on June 6, 2022:

Done. Thanks!

in src/net_processing.cpp:2941 in f6bf0d1787 outdated

2934 | @@ -2910,6 +2935,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2935 |              // they may wish to request compact blocks from us
2936 |              m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION));
2937 |          }
2938 | +        {
2939 | +            LOCK(m_version_nonces_mutex);
2940 | +            m_version_nonces.erase(pfrom.GetId());
2941 | +        }

theStack commented at 7:57 PM on June 5, 2022:

        WITH_LOCK(m_version_nonces_mutex, m_version_nonces.erase(pfrom.GetId());

jnewbery commented at 11:21 AM on June 6, 2022:

Done

theStack approved

theStack commented at 7:59 PM on June 5, 2022: member

Code-review ACK f6bf0d1787673e83e0aefd4ef6678655aef0e299

Nice idea to use a std::map for the sent nonce values and only store them as long as necessary (that is, until the connection is fully established). Left two refactoring nits where one-liners could be used instead, feel free to ignore.

[net processing] Remove CNode::nLocalHostNonce

Handle version nonces entirely within net_processing.

7df3d82395

jnewbery commented at 11:22 AM on June 6, 2022: member

Thanks for the review @theStack. I've taken your suggestions.

jnewbery force-pushed on Jun 6, 2022

theStack approved

theStack commented at 4:05 PM on June 6, 2022: member

re-ACK 7df3d82395ab87a5f9f85af1988d568c07cfb945

in src/net_processing.cpp:214 in 4680a64075 outdated

 209 | @@ -210,6 +210,9 @@ struct Peer {
 210 |       *  This cleaned string can safely be logged or displayed.  */
 211 |      std::string m_clean_subversion GUARDED_BY(m_subver_mutex){};
 212 |  
 213 | +    /** Whether we consider this a preferred download peer. */
 214 | +    bool m_preferred_download{false};

MarcoFalke commented at 12:51 PM on June 9, 2022:

fPreferredDownload used to be protected by cs_main. Can you explain why the lock is not needed anymore, considering that the socket thread and the p2p thread will both read this value?

in src/net_processing.cpp:660 in 4680a64075 outdated

 656 | @@ -656,7 +657,7 @@ class PeerManagerImpl final : public PeerManager
 657 |      int m_outbound_peers_with_protect_from_disconnect GUARDED_BY(cs_main) = 0;
 658 |  
 659 |      /** Number of preferable block download peers. */
 660 | -    int m_num_preferred_download_peers GUARDED_BY(cs_main){0};
 661 | +    std::atomic<int> m_num_preferred_download_peers{0};

MarcoFalke commented at 12:59 PM on June 9, 2022:

can you explain why atomic is sufficient, given that the value is assumed to be changed atomically with fPreferredDownload, and is used in calculations with that assumption:

m_num_preferred_download_peers - state.fPreferredDownload >= 1

MarcoFalke commented at 1:00 PM on June 9, 2022: member

concept ACK 4680a64075758f3a08a69dc2bf9243631628900d 🚡

<details><summary>Show signature</summary>

Signature:

</details>

jnewbery commented at 11:13 AM on June 20, 2022: member

Closing this since it needs a bit of work and I don't have time to maintain it. In terms of priority, I think there's more immediate benefit in moving the services fields from CNode to Peer (the last 6 commits in https://github.com/bitcoin/bitcoin/compare/master...jnewbery:2020-06-cs-main-split), since that allows PeerManagerImpl to be unit tested through the public p2p interfaces.

jnewbery closed this on Jun 20, 2022

DrahtBot locked this on Jun 20, 2023