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

8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67 [net processing] Remove CNode::nLocalServices (John Newbery)
5961f8eea1ad5be1a4bf8da63651e197a20359b2 [net] Return CService from GetLocalAddrForPeer and GetLocalAddress (dergoegge)
d9079fe18dc5d81ce290876353555b51125127d1 [net processing] Remove CNode::nServices (John Newbery)
7d1c0369340cb752f0d78e24f4251595534bf5e9 [net processing] Replace fHaveWitness with CanServeWitnesses() (John Newbery)
f65e83d51bfb6a34f1d5efccfb3d8786a51a4534 [net processing] Remove fClient and m_limited_node (John Newbery)
fc5eb528f7d7b33e2f2e443c5610a1551c7f099b [tests] Connect peer in outbound_slow_chain_eviction by sending p2p messages (John Newbery)
1f52c47d5c09b59fd3153700751c74e63edc7d7e [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 8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67 🔑
  jnewbery:
    utACK 8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67
  mzumsande:
    Code Review ACK 8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67

Tree-SHA512: e772eb2a0a85db346dd7b453a41011a12756fc7cbfda6a9ef6daa9633b9a47b9770ab3dc02377690f9d02127301c3905ff22905977f758bf90b17a9a35b37523

7 files changed, 33 insertions, 45 deletions

diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
index c87ed82c88..66c4605afa 100644
--- a/src/test/denialofservice_tests.cpp
+++ b/src/test/denialofservice_tests.cpp
@@ -15,6 +15,7 @@
 #include <serialize.h>
 #include <test/util/net.h>
 #include <test/util/setup_common.h>
+#include <timedata.h>
 #include <util/string.h>
 #include <util/system.h>
 #include <util/time.h>
@@ -44,17 +45,15 @@ BOOST_FIXTURE_TEST_SUITE(denialofservice_tests, TestingSetup)
 // work.
 BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
 {
-    auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman);
+    ConnmanTestMsg& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);
     // Disable inactivity checks for this test to avoid interference
-    static_cast<ConnmanTestMsg*>(connman.get())->SetPeerConnectTimeout(99999s);
-    auto peerLogic = PeerManager::make(*connman, *m_node.addrman, nullptr,
-                                       *m_node.chainman, *m_node.mempool, false);
+    connman.SetPeerConnectTimeout(99999s);
+    PeerManager& peerman = *m_node.peerman;
 
     // Mock an outbound peer
     CAddress addr1(ip(0xa0b0c001), NODE_NONE);
     NodeId id{0};
     CNode dummyNode1{id++,
-                     ServiceFlags(NODE_NETWORK | NODE_WITNESS),
                      /*sock=*/nullptr,
                      addr1,
                      /*nKeyedNetGroupIn=*/0,
@@ -63,10 +62,16 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
                      /*addrNameIn=*/"",
                      ConnectionType::OUTBOUND_FULL_RELAY,
                      /*inbound_onion=*/false};
-    dummyNode1.SetCommonVersion(PROTOCOL_VERSION);
 
-    peerLogic->InitializeNode(&dummyNode1);
-    dummyNode1.fSuccessfullyConnected = true;
+    connman.Handshake(
+        /*node=*/dummyNode1,
+        /*successfully_connected=*/true,
+        /*remote_services=*/ServiceFlags(NODE_NETWORK | NODE_WITNESS),
+        /*local_services=*/ServiceFlags(NODE_NETWORK | NODE_WITNESS),
+        /*permission_flags=*/NetPermissionFlags::None,
+        /*version=*/PROTOCOL_VERSION,
+        /*relay_txs=*/true);
+    TestOnlyResetTimeData();
 
     // This test requires that we have a chain with non-zero work.
     {
@@ -78,7 +83,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
     // Test starts here
     {
         LOCK(dummyNode1.cs_sendProcessing);
-        BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); // should result in getheaders
+        BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in getheaders
     }
     {
         LOCK(dummyNode1.cs_vSend);
@@ -91,7 +96,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
     SetMockTime(nStartTime+21*60);
     {
         LOCK(dummyNode1.cs_sendProcessing);
-        BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); // should result in getheaders
+        BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in getheaders
     }
     {
         LOCK(dummyNode1.cs_vSend);
@@ -101,18 +106,17 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
     SetMockTime(nStartTime+24*60);
     {
         LOCK(dummyNode1.cs_sendProcessing);
-        BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); // should result in disconnect
+        BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in disconnect
     }
     BOOST_CHECK(dummyNode1.fDisconnect == true);
 
-    peerLogic->FinalizeNode(dummyNode1);
+    peerman.FinalizeNode(dummyNode1);
 }
 
 static void AddRandomOutboundPeer(NodeId& id, std::vector<CNode*>& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman, ConnectionType connType)
 {
     CAddress addr(ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE);
     vNodes.emplace_back(new CNode{id++,
-                                  ServiceFlags(NODE_NETWORK | NODE_WITNESS),
                                   /*sock=*/nullptr,
                                   addr,
                                   /*nKeyedNetGroupIn=*/0,
@@ -124,7 +128,7 @@ static void AddRandomOutboundPeer(NodeId& id, std::vector<CNode*>& vNodes, PeerM
     CNode &node = *vNodes.back();
     node.SetCommonVersion(PROTOCOL_VERSION);
 
-    peerLogic.InitializeNode(&node);
+    peerLogic.InitializeNode(node, ServiceFlags(NODE_NETWORK | NODE_WITNESS));
     node.fSuccessfullyConnected = true;
 
     connman.AddTestNode(node);
@@ -292,7 +296,6 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
     banman->ClearBanned();
     NodeId id{0};
     nodes[0] = new CNode{id++,
-                         NODE_NETWORK,
                          /*sock=*/nullptr,
                          addr[0],
                          /*nKeyedNetGroupIn=*/0,
@@ -302,7 +305,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
                          ConnectionType::INBOUND,
                          /*inbound_onion=*/false};
     nodes[0]->SetCommonVersion(PROTOCOL_VERSION);
-    peerLogic->InitializeNode(nodes[0]);
+    peerLogic->InitializeNode(*nodes[0], NODE_NETWORK);
     nodes[0]->fSuccessfullyConnected = true;
     connman->AddTestNode(*nodes[0]);
     peerLogic->UnitTestMisbehaving(nodes[0]->GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged
@@ -315,7 +318,6 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
     BOOST_CHECK(!banman->IsDiscouraged(other_addr)); // Different address, not discouraged
 
     nodes[1] = new CNode{id++,
-                         NODE_NETWORK,
                          /*sock=*/nullptr,
                          addr[1],
                          /*nKeyedNetGroupIn=*/1,
@@ -325,7 +327,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
                          ConnectionType::INBOUND,
                          /*inbound_onion=*/false};
     nodes[1]->SetCommonVersion(PROTOCOL_VERSION);
-    peerLogic->InitializeNode(nodes[1]);
+    peerLogic->InitializeNode(*nodes[1], NODE_NETWORK);
     nodes[1]->fSuccessfullyConnected = true;
     connman->AddTestNode(*nodes[1]);
     peerLogic->UnitTestMisbehaving(nodes[1]->GetId(), DISCOURAGEMENT_THRESHOLD - 1);
@@ -353,7 +355,6 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
     // Make sure non-IP peers are discouraged and disconnected properly.
 
     nodes[2] = new CNode{id++,
-                         NODE_NETWORK,
                          /*sock=*/nullptr,
                          addr[2],
                          /*nKeyedNetGroupIn=*/1,
@@ -363,7 +364,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
                          ConnectionType::OUTBOUND_FULL_RELAY,
                          /*inbound_onion=*/false};
     nodes[2]->SetCommonVersion(PROTOCOL_VERSION);
-    peerLogic->InitializeNode(nodes[2]);
+    peerLogic->InitializeNode(*nodes[2], NODE_NETWORK);
     nodes[2]->fSuccessfullyConnected = true;
     connman->AddTestNode(*nodes[2]);
     peerLogic->UnitTestMisbehaving(nodes[2]->GetId(), DISCOURAGEMENT_THRESHOLD);
@@ -398,7 +399,6 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
     CAddress addr(ip(0xa0b0c001), NODE_NONE);
     NodeId id{0};
     CNode dummyNode{id++,
-                    NODE_NETWORK,
                     /*sock=*/nullptr,
                     addr,
                     /*nKeyedNetGroupIn=*/4,
@@ -408,7 +408,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
                     ConnectionType::INBOUND,
                     /*inbound_onion=*/false};
     dummyNode.SetCommonVersion(PROTOCOL_VERSION);
-    peerLogic->InitializeNode(&dummyNode);
+    peerLogic->InitializeNode(dummyNode, NODE_NETWORK);
     dummyNode.fSuccessfullyConnected = true;
 
     peerLogic->UnitTestMisbehaving(dummyNode.GetId(), DISCOURAGEMENT_THRESHOLD);
diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp
index 4981287152..741810f6a2 100644
--- a/src/test/fuzz/net.cpp
+++ b/src/test/fuzz/net.cpp
@@ -68,7 +68,6 @@ FUZZ_TARGET_INIT(net, initialize_net)
     (void)node.GetAddrLocal();
     (void)node.GetId();
     (void)node.GetLocalNonce();
-    (void)node.GetLocalServices();
     const int ref_count = node.GetRefCount();
     assert(ref_count >= 0);
     (void)node.GetCommonVersion();
diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp
index f0cff74f94..fabcea22c3 100644
--- a/src/test/fuzz/util.cpp
+++ b/src/test/fuzz/util.cpp
@@ -294,6 +294,7 @@ void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman,
     connman.Handshake(node,
                       /*successfully_connected=*/fuzzed_data_provider.ConsumeBool(),
                       /*remote_services=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS),
+                      /*local_services=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS),
                       /*permission_flags=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS),
                       /*version=*/fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(MIN_PEER_PROTO_VERSION, std::numeric_limits<int32_t>::max()),
                       /*relay_txs=*/fuzzed_data_provider.ConsumeBool());
diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h
index d189a50a51..406e11c573 100644
--- a/src/test/fuzz/util.h
+++ b/src/test/fuzz/util.h
@@ -296,7 +296,6 @@ template <bool ReturnUniquePtr = false>
 auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<NodeId>& node_id_in = std::nullopt) noexcept
 {
     const NodeId node_id = node_id_in.value_or(fuzzed_data_provider.ConsumeIntegralInRange<NodeId>(0, std::numeric_limits<NodeId>::max()));
-    const ServiceFlags local_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);
     const auto sock = std::make_shared<FuzzedSock>(fuzzed_data_provider);
     const CAddress address = ConsumeAddress(fuzzed_data_provider);
     const uint64_t keyed_net_group = fuzzed_data_provider.ConsumeIntegral<uint64_t>();
@@ -307,7 +306,6 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
     const bool inbound_onion{conn_type == ConnectionType::INBOUND ? fuzzed_data_provider.ConsumeBool() : false};
     if constexpr (ReturnUniquePtr) {
         return std::make_unique<CNode>(node_id,
-                                       local_services,
                                        sock,
                                        address,
                                        keyed_net_group,
@@ -318,7 +316,6 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
                                        inbound_onion);
     } else {
         return CNode{node_id,
-                     local_services,
                      sock,
                      address,
                      keyed_net_group,
diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index 115c4b9b24..f2eaa0179f 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -58,7 +58,6 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test)
     std::string pszDest;
 
     std::unique_ptr<CNode> pnode1 = std::make_unique<CNode>(id++,
-                                                            NODE_NETWORK,
                                                             /*sock=*/nullptr,
                                                             addr,
                                                             /*nKeyedNetGroupIn=*/0,
@@ -77,7 +76,6 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test)
     BOOST_CHECK_EQUAL(pnode1->ConnectedThroughNetwork(), Network::NET_IPV4);
 
     std::unique_ptr<CNode> pnode2 = std::make_unique<CNode>(id++,
-                                                            NODE_NETWORK,
                                                             /*sock=*/nullptr,
                                                             addr,
                                                             /*nKeyedNetGroupIn=*/1,
@@ -96,7 +94,6 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test)
     BOOST_CHECK_EQUAL(pnode2->ConnectedThroughNetwork(), Network::NET_IPV4);
 
     std::unique_ptr<CNode> pnode3 = std::make_unique<CNode>(id++,
-                                                            NODE_NETWORK,
                                                             /*sock=*/nullptr,
                                                             addr,
                                                             /*nKeyedNetGroupIn=*/0,
@@ -115,7 +112,6 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test)
     BOOST_CHECK_EQUAL(pnode3->ConnectedThroughNetwork(), Network::NET_IPV4);
 
     std::unique_ptr<CNode> pnode4 = std::make_unique<CNode>(id++,
-                                                            NODE_NETWORK,
                                                             /*sock=*/nullptr,
                                                             addr,
                                                             /*nKeyedNetGroupIn=*/1,
@@ -629,7 +625,6 @@ BOOST_AUTO_TEST_CASE(ipv4_peer_with_ipv6_addrMe_test)
     ipv4AddrPeer.s_addr = 0xa0b0c001;
     CAddress addr = CAddress(CService(ipv4AddrPeer, 7777), NODE_NETWORK);
     std::unique_ptr<CNode> pnode = std::make_unique<CNode>(/*id=*/0,
-                                                           NODE_NETWORK,
                                                            /*sock=*/nullptr,
                                                            addr,
                                                            /*nKeyedNetGroupIn=*/0,
@@ -648,7 +643,7 @@ BOOST_AUTO_TEST_CASE(ipv4_peer_with_ipv6_addrMe_test)
     pnode->SetAddrLocal(addrLocal);
 
     // before patch, this causes undefined behavior detectable with clang's -fsanitize=memory
-    GetLocalAddrForPeer(&*pnode);
+    GetLocalAddrForPeer(*pnode);
 
     // suppress no-checks-run warning; if this test fails, it's by triggering a sanitizer
     BOOST_CHECK(1);
@@ -675,19 +670,15 @@ BOOST_AUTO_TEST_CASE(get_local_addr_for_peer_port)
     const uint16_t bind_port = 20001;
     m_node.args->ForceSetArg("-bind", strprintf("3.4.5.6:%u", bind_port));
 
-    const uint32_t current_time = static_cast<uint32_t>(GetAdjustedTime());
-    SetMockTime(current_time);
-
     // Our address:port as seen from the peer, completely different from the above.
     in_addr peer_us_addr;
     peer_us_addr.s_addr = htonl(0x02030405);
-    const CAddress peer_us{CService{peer_us_addr, 20002}, NODE_NETWORK, current_time};
+    const CService peer_us{peer_us_addr, 20002};
 
     // Create a peer with a routable IPv4 address (outbound).
     in_addr peer_out_in_addr;
     peer_out_in_addr.s_addr = htonl(0x01020304);
     CNode peer_out{/*id=*/0,
-                   /*nLocalServicesIn=*/NODE_NETWORK,
                    /*sock=*/nullptr,
                    /*addrIn=*/CAddress{CService{peer_out_in_addr, 8333}, NODE_NETWORK},
                    /*nKeyedNetGroupIn=*/0,
@@ -700,16 +691,15 @@ BOOST_AUTO_TEST_CASE(get_local_addr_for_peer_port)
     peer_out.SetAddrLocal(peer_us);
 
     // Without the fix peer_us:8333 is chosen instead of the proper peer_us:bind_port.
-    auto chosen_local_addr = GetLocalAddrForPeer(&peer_out);
+    auto chosen_local_addr = GetLocalAddrForPeer(peer_out);
     BOOST_REQUIRE(chosen_local_addr);
-    const CAddress expected{CService{peer_us_addr, bind_port}, NODE_NETWORK, current_time};
+    const CService expected{peer_us_addr, bind_port};
     BOOST_CHECK(*chosen_local_addr == expected);
 
     // Create a peer with a routable IPv4 address (inbound).
     in_addr peer_in_in_addr;
     peer_in_in_addr.s_addr = htonl(0x05060708);
     CNode peer_in{/*id=*/0,
-                  /*nLocalServicesIn=*/NODE_NETWORK,
                   /*sock=*/nullptr,
                   /*addrIn=*/CAddress{CService{peer_in_in_addr, 8333}, NODE_NETWORK},
                   /*nKeyedNetGroupIn=*/0,
@@ -722,7 +712,7 @@ BOOST_AUTO_TEST_CASE(get_local_addr_for_peer_port)
     peer_in.SetAddrLocal(peer_us);
 
     // Without the fix peer_us:8333 is chosen instead of the proper peer_us:peer_us.GetPort().
-    chosen_local_addr = GetLocalAddrForPeer(&peer_in);
+    chosen_local_addr = GetLocalAddrForPeer(peer_in);
     BOOST_REQUIRE(chosen_local_addr);
     BOOST_CHECK(*chosen_local_addr == peer_us);
 
@@ -837,7 +827,6 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
     in_addr peer_in_addr;
     peer_in_addr.s_addr = htonl(0x01020304);
     CNode peer{/*id=*/0,
-               /*nLocalServicesIn=*/NODE_NETWORK,
                /*sock=*/nullptr,
                /*addrIn=*/CAddress{CService{peer_in_addr, 8333}, NODE_NETWORK},
                /*nKeyedNetGroupIn=*/0,
@@ -857,7 +846,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
         *static_cast<TestChainState*>(&m_node.chainman->ActiveChainstate());
     chainstate.JumpOutOfIbd();
 
-    m_node.peerman->InitializeNode(&peer);
+    m_node.peerman->InitializeNode(peer, NODE_NETWORK);
 
     std::atomic<bool> interrupt_dummy{false};
     std::chrono::microseconds time_received_dummy{0};
diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp
index e70ad036bd..223db16c6c 100644
--- a/src/test/util/net.cpp
+++ b/src/test/util/net.cpp
@@ -16,6 +16,7 @@
 void ConnmanTestMsg::Handshake(CNode& node,
                                bool successfully_connected,
                                ServiceFlags remote_services,
+                               ServiceFlags local_services,
                                NetPermissionFlags permission_flags,
                                int32_t version,
                                bool relay_txs)
@@ -24,7 +25,7 @@ void ConnmanTestMsg::Handshake(CNode& node,
     auto& connman{*this};
     const CNetMsgMaker mm{0};
 
-    peerman.InitializeNode(&node);
+    peerman.InitializeNode(node, local_services);
 
     CSerializedNetMsg msg_version{
         mm.Make(NetMsgType::VERSION,
@@ -51,10 +52,10 @@ void ConnmanTestMsg::Handshake(CNode& node,
     if (node.fDisconnect) return;
     assert(node.nVersion == version);
     assert(node.GetCommonVersion() == std::min(version, PROTOCOL_VERSION));
-    assert(node.nServices == remote_services);
     CNodeStateStats statestats;
     assert(peerman.GetNodeStateStats(node.GetId(), statestats));
     assert(statestats.m_relay_txs == (relay_txs && !node.IsBlockOnlyConn()));
+    assert(statestats.their_services == remote_services);
     node.m_permissionFlags = permission_flags;
     if (successfully_connected) {
         CSerializedNetMsg msg_verack{mm.Make(NetMsgType::VERACK)};
diff --git a/src/test/util/net.h b/src/test/util/net.h
index 0cf55f8a22..7f61a03d27 100644
--- a/src/test/util/net.h
+++ b/src/test/util/net.h
@@ -42,6 +42,7 @@ struct ConnmanTestMsg : public CConnman {
     void Handshake(CNode& node,
                    bool successfully_connected,
                    ServiceFlags remote_services,
+                   ServiceFlags local_services,
                    NetPermissionFlags permission_flags,
                    int32_t version,
                    bool relay_txs);