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

jnewbery · 2022-04-25T11:48:12Z

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.

maflcko · 2022-04-25T11:49:01Z

Concept ACK

hebasto · 2022-04-25T12:17:04Z

Concept ACK.

DrahtBot · 2022-04-25T18:01:11Z

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 · 2022-04-26T00:56:55Z

Concept ACK

jnewbery · 2022-04-26T08:47:00Z

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 · 2022-04-30T10:40:41Z

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.

jnewbery · 2022-05-16T18:21:34Z

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

src/net_processing.cpp

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-

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.

Also rename to m_preferred_download

jnewbery · 2022-06-03T10:51:36Z

Rebased

theStack

Code-review ACK f6bf0d1

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.

src/net_processing.cpp

Handle version nonces entirely within net_processing.

jnewbery · 2022-06-06T11:22:07Z

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

theStack

re-ACK 7df3d82

maflcko

concept ACK 4680a64 🚡

Show signature

Signature:

maflcko · 2022-06-09T12:51:18Z

src/net_processing.cpp

@@ -210,6 +210,9 @@ struct Peer {
     *  This cleaned string can safely be logged or displayed.  */
    std::string m_clean_subversion GUARDED_BY(m_subver_mutex){};

+    /** Whether we consider this a preferred download peer. */
+    bool m_preferred_download{false};


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?

maflcko · 2022-06-09T12:59:30Z

src/net_processing.cpp

@@ -656,7 +657,7 @@ class PeerManagerImpl final : public PeerManager
    int m_outbound_peers_with_protect_from_disconnect GUARDED_BY(cs_main) = 0;

    /** Number of preferable block download peers. */
-    int m_num_preferred_download_peers GUARDED_BY(cs_main){0};
+    std::atomic<int> m_num_preferred_download_peers{0};


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

jnewbery · 2022-06-20T11:13:26Z

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 master...jnewbery:2020-06-cs-main-split), since that allows PeerManagerImpl to be unit tested through the public p2p interfaces.

jnewbery mentioned this pull request Apr 25, 2022

Move remaining application layer data to net processing #19398

Closed

16 tasks

fanquake added the P2P label Apr 25, 2022

This was referenced Apr 25, 2022

Strengthen thread safety assertions #24931

Merged

net processing: Move remaining globals into PeerManagerImpl #24543

Merged

Additional thread safety annotations for CNode/Peer members accessed via the message processing thread #24474

Closed

This was referenced Apr 26, 2022

p2p, rpc: Manual block-relay-only connections with addnode #24170

Closed

p2p: Erlay support signaling #23443

Merged

net processing: Reduce resource usage for inbound block-relay-only connections #22778

Merged

net_processing: lock clean up #21527

Closed

jnewbery force-pushed the 202204_net_net_processing branch from 529415a to f9763a1 Compare April 26, 2022 08:45

jnewbery marked this pull request as draft April 26, 2022 08:47

This was referenced Apr 26, 2022

net processing: Only support version 2 compact blocks #20799

Merged

p2p: Add DISABLETX message for negotiating block-relay-only connections #20726

Closed

p2p: Prevent block index fingerprinting by sending additional getheaders messages #24571

Closed

jnewbery force-pushed the 202204_net_net_processing branch from f9763a1 to a099b6b Compare April 30, 2022 10:40

This was referenced May 12, 2022

Strengthen AssertLockNotHeld assertions #25109

Merged

Erlay: bandwidth-efficient transaction relay protocol #21515

Closed

DrahtBot added the Needs rebase label May 16, 2022

jnewbery force-pushed the 202204_net_net_processing branch from a099b6b to 53d581f Compare May 16, 2022 18:21

jnewbery marked this pull request as ready for review May 16, 2022 18:21

DrahtBot removed the Needs rebase label May 16, 2022

theStack reviewed May 16, 2022

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

jnewbery force-pushed the 202204_net_net_processing branch from 53d581f to db4eeb2 Compare May 16, 2022 21:46

jnewbery force-pushed the 202204_net_net_processing branch from db4eeb2 to 7ef17fe Compare May 17, 2022 10:59

This was referenced May 18, 2022

p2p: Sync chain more readily from inbound peers during IBD #24171

Merged

net/net_processing: Add thread safety related annotations for CNode and Peer #25174

Merged

DrahtBot mentioned this pull request Jun 2, 2022

refactor: Introduce EvictionManager #25268

Closed

DrahtBot added the Needs rebase label Jun 2, 2022

jnewbery added 3 commits June 3, 2022 11:39

[net processing] Move fPreferredDownload to Peer

4680a64

Also rename to m_preferred_download

jnewbery force-pushed the 202204_net_net_processing branch from 7ef17fe to f6bf0d1 Compare June 3, 2022 10:50

DrahtBot removed the Needs rebase label Jun 3, 2022

theStack approved these changes Jun 5, 2022

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

[net processing] Remove CNode::nLocalHostNonce

7df3d82

Handle version nonces entirely within net_processing.

jnewbery force-pushed the 202204_net_net_processing branch from f6bf0d1 to 7df3d82 Compare June 6, 2022 11:22

theStack approved these changes Jun 6, 2022

View reviewed changes

maflcko reviewed Jun 9, 2022

View reviewed changes

DrahtBot mentioned this pull request Jun 16, 2022

I2P: add support for transient addresses for outbound connections #25355

Merged

jnewbery closed this Jun 20, 2022

dergoegge mentioned this pull request Dec 1, 2022

refactor: Continue moving application data from CNode to Peer #26621

Closed

bitcoin locked and limited conversation to collaborators Jun 20, 2023

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

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

Uh oh!

Conversation

jnewbery commented Apr 25, 2022

Uh oh!

maflcko commented Apr 25, 2022

Uh oh!

hebasto commented Apr 25, 2022

Uh oh!

DrahtBot commented Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

theStack commented Apr 26, 2022

Uh oh!

jnewbery commented Apr 26, 2022

Uh oh!

jnewbery commented Apr 30, 2022

Uh oh!

jnewbery commented May 16, 2022

Uh oh!

Uh oh!

jnewbery commented Jun 3, 2022

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Jun 6, 2022

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 9, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 9, 2022

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jun 20, 2022

Uh oh!

Uh oh!

DrahtBot commented Apr 25, 2022 •

edited

Loading