net processing: Move CNode::nServices and CNode::nLocalServices to Peer #25514

dergoegge · 2022-06-30T18:01:24Z

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

DrahtBot · 2022-06-30T19:52:17Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25619 (net: avoid overriding non-virtual ToString() in CService and use better naming by vasild)
#25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
#25555 (refactor: Move num_unconnecting_headers_msgs out of cs_main guard by MarcoFalke)
#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)
#24697 (refactor address relay time by MarcoFalke)
#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)
#24008 (assumeutxo: net_processing changes by jamesob)
#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.

src/test/fuzz/util.cpp

src/test/fuzz/util.h

jnewbery

Concept ACK.

src/net_processing.cpp

src/rpc/net.cpp

src/net.cpp

src/test/fuzz/util.h

jnewbery · 2022-07-06T12:14:12Z

utACK d5b6ba7

src/test/denialofservice_tests.cpp

jnewbery · 2022-07-06T16:22:50Z

utACK c38a00d

src/net.h

maflcko

Looking a lot better with the Handshake test helper. Thanks.

review ACK 8f6ed20 though I have not yet reviewed the last two commits 🏼

Show signature

Signature:

src/test/denialofservice_tests.cpp

src/net.h

src/net.cpp

maflcko

I tried to do some testing, but the second commit doesn't compile:

test/denialofservice_tests.cpp:75:23: error: too many arguments to function call, expected 6, have 7

src/qt/rpcconsole.cpp

dergoegge · 2022-07-13T16:01:07Z

Thanks @MarcoFalke and @jnewbery for the reviews! I addressed all comments and rebased.

All commits should now build without issues.

maflcko

lgtm

review ACK e7aea6a 🏞

Show signature

Signature:

src/net.cpp

src/net.h

jnewbery · 2022-07-14T11:59:06Z

@dergoegge just checking that you saw my comment about member naming: #25514 (comment).

Track services offered by us and the peer in the Peer object.

…essages Prior to this commit, the peer was connected, and then the services and connectivity fields in the CNode object were manually set. Instead, send p2p `version` and `verack` messages, and have net_processing's internal logic set the state of the node. This ensures that the node's internal state is consistent with how it would be set in the live code. Prior to this commit, `dummyNode1.nServices` was set to `NODE_NONE` which was not a problem since `CNode::fClient` and `CNode::m_limited_node` are default initialised to false. Now that we are doing the actual version handshake, the values of `fClient` and `m_limited_node` are set during the handshake and cause the test to fail if we do not set `dummyNode1.nServices` to a reasonable value (NODE_NETWORK | NODE_WITNESS).

fClient is replaced by CanServeBlocks(), and m_limited_node is replaced by IsLimitedPeer().

Use Peer::m_their_services instead

maflcko · 2022-07-14T13:47:27Z

ACK 8d8eeb4 🔑

Show signature

Signature:

jnewbery

utACK 8d8eeb4

One missed our_services -> m_our_services in a comment, but that shouldn't hold up merge.

jnewbery · 2022-07-14T17:27:17Z

src/net.h

     *
-     * \sa CNode::nLocalServices
+     * \sa Peer::our_services


Suggested change

* \sa Peer::our_services

* \sa Peer::m_our_services

mzumsande

Code Review ACK 8d8eeb4

This looks good to me, only found nits/doc comments during review.

mzumsande · 2022-07-18T22:12:21Z

src/net_processing.cpp

@@ -2650,7 +2649,7 @@ bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& peer,
    return true;
 }

-void PeerManagerImpl::ProcessGetCFilters(CNode& peer, CDataStream& vRecv)
+void PeerManagerImpl::ProcessGetCFilters(CNode& node,Peer& peer, CDataStream& vRecv)


nit:space after node

mzumsande · 2022-07-18T22:48:44Z

src/net.h

-     * This data is replicated in each CNode instance we create during peer
-     * connection (in ConnectNode()) under a member also called
-     * nLocalServices.
+     * This data is replicated in each Peer instance we create.


This confused me during review, "replicated" makes it sound like the data is just copied to each peer (which was technically true before, when the comment specifically addressed ConnectNode() and ignored the CreateNodeFromAcceptedSocket() call site ).

It might be good to mention here and/or in the Peer::m_our_services doc that different peers can be offered different local services based on permissions - otherwise m_our_services wouldn't need to be a member of Peer but could exist on the level of PeerManagerImpl.

…:nLocal…

dhruv · 2022-07-22T18:34:45Z

@dergoegge @jnewbery I am still digging deeper here, but I have a question (and might be terribly wrong). Moving nServices and nLocalServices into Peer seems to imply that the service bits only gain importance during the VERSION/VERACK handshake and are useful in the P2P layer, not in the transport layer.

However, I think service bits (also propagated with addr relay) are useful at the transport layer. For instance in BIP324, the proposed signaling method uses nServices from addr relay to branch logic in the establishment of the transport. Moving the service flags to net_processing will mean that I'll end up having to pass a boolean around. Is that an expected and acceptable consequence?

dergoegge · 2022-07-25T11:08:14Z

@dhruv Currently all service bits signal readiness to serve application layer data (blocks, bloom/compact filters, witness data). The BIP324 service bit differs from that as it signals support for a new transport protocol, which would make it the only bit that is needed within our net layer. I would therefore argue that passing around a bool to/within the net layer for the v2 bit seems more appropriate than passing around all service bits.

fanquake added the P2P label Jun 30, 2022

dergoegge mentioned this pull request Jun 30, 2022

net, test: Virtualise CConnman and add initial PeerManager unit tests #25515

Closed

dergoegge force-pushed the 2022-06-move-services branch from 37b03bb to dd46511 Compare June 30, 2022 18:18

This was referenced Jun 30, 2022

build: Increase MS Visual Studio minimum version #25472

Merged

p2p: Avoid multiple getheaders messages in flight to the same peer #25454

Merged

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

Merged

dergoegge force-pushed the 2022-06-move-services branch from dd46511 to 10a4eba Compare June 30, 2022 22:09

maflcko reviewed Jul 1, 2022

View reviewed changes

src/test/fuzz/util.cpp Outdated Show resolved Hide resolved

src/test/fuzz/util.h Show resolved Hide resolved

jnewbery reviewed Jul 4, 2022

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/rpc/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/test/fuzz/util.h Show resolved Hide resolved

maflcko removed the P2P label Jul 4, 2022

DrahtBot added the P2P label Jul 4, 2022

dergoegge force-pushed the 2022-06-move-services branch 2 times, most recently from f7965ef to fae6a00 Compare July 4, 2022 16:43

DrahtBot added the Needs rebase label Jul 4, 2022

DrahtBot mentioned this pull request Jul 4, 2022

refactor address relay time #24697

Merged

dergoegge force-pushed the 2022-06-move-services branch from fae6a00 to d5b6ba7 Compare July 5, 2022 15:48

DrahtBot removed the Needs rebase label Jul 5, 2022

dergoegge force-pushed the 2022-06-move-services branch from d5b6ba7 to c38a00d Compare July 6, 2022 15:50

dergoegge commented Jul 6, 2022

View reviewed changes

src/test/denialofservice_tests.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jul 12, 2022

move-only: Version handshake to libtest_util #25591

Merged

DrahtBot added the Needs rebase label Jul 12, 2022

dergoegge force-pushed the 2022-06-move-services branch from 991ea8e to 8f6ed20 Compare July 13, 2022 09:57

DrahtBot removed the Needs rebase label Jul 13, 2022

jnewbery reviewed Jul 13, 2022

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

maflcko approved these changes Jul 13, 2022

View reviewed changes

maflcko reviewed Jul 13, 2022

View reviewed changes

src/qt/rpcconsole.cpp Show resolved Hide resolved

dergoegge force-pushed the 2022-06-move-services branch 2 times, most recently from 54c7f95 to e7aea6a Compare July 13, 2022 15:59

maflcko approved these changes Jul 13, 2022

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

jnewbery and others added 7 commits July 14, 2022 14:44

[net processing] Add m_our_services and m_their_services to Peer

1f52c47

Track services offered by us and the peer in the Peer object.

[net processing] Remove fClient and m_limited_node

f65e83d

fClient is replaced by CanServeBlocks(), and m_limited_node is replaced by IsLimitedPeer().

[net processing] Replace fHaveWitness with CanServeWitnesses()

7d1c036

[net processing] Remove CNode::nServices

d9079fe

Use Peer::m_their_services instead

[net] Return CService from GetLocalAddrForPeer and GetLocalAddress

5961f8e

[net processing] Remove CNode::nLocalServices

8d8eeb4

dergoegge force-pushed the 2022-06-move-services branch from e7aea6a to 8d8eeb4 Compare July 14, 2022 13:29

jnewbery approved these changes Jul 14, 2022

View reviewed changes

fanquake requested a review from mzumsande July 15, 2022 11:19

DrahtBot mentioned this pull request Jul 16, 2022

net: avoid overriding non-virtual ToString() in CService and use better naming #25619

Merged

mzumsande reviewed Jul 19, 2022

View reviewed changes

maflcko merged commit 2bdce7f into bitcoin:master Jul 19, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 19, 2022

Merge bitcoin#25514: net processing: Move CNode::nServices and CNode:…

bea2180

…:nLocal…

bitcoin locked and limited conversation to collaborators Jul 25, 2023

net processing: Move CNode::nServices and CNode::nLocalServices to Peer #25514

net processing: Move CNode::nServices and CNode::nLocalServices to Peer #25514

Uh oh!

Conversation

dergoegge commented Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Jul 6, 2022

Uh oh!

Uh oh!

jnewbery commented Jul 6, 2022

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dergoegge commented Jul 13, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Jul 14, 2022

Uh oh!

maflcko commented Jul 14, 2022

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

mzumsande Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

dhruv commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dergoegge commented Jul 25, 2022

Uh oh!

Uh oh!

dergoegge commented Jun 30, 2022 •

edited

Loading

DrahtBot commented Jun 30, 2022 •

edited

Loading

dhruv commented Jul 22, 2022 •

edited

Loading