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

dergoegge commented at 6:01 PM on June 30, 2022: member

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

fanquake added the label P2P on Jun 30, 2022

dergoegge force-pushed on Jun 30, 2022

DrahtBot commented at 7:52 PM on June 30, 2022: contributor

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.

dergoegge force-pushed on Jun 30, 2022

in src/test/fuzz/util.cpp:327 in 10a4ebab27 outdated

 323 | @@ -324,7 +324,6 @@ void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman,
 324 |      if (node.fDisconnect) return;
 325 |      assert(node.nVersion == version);
 326 |      assert(node.GetCommonVersion() == std::min(version, PROTOCOL_VERSION));
 327 | -    assert(node.nServices == remote_services);

MarcoFalke commented at 2:20 PM on July 1, 2022:

why is this removed when it can be moved down?

dergoegge commented at 4:18 PM on July 4, 2022:

Moved the assert down.

in src/test/fuzz/util.h:299 in 10a4ebab27 outdated

 295 | @@ -296,7 +296,6 @@ template <bool ReturnUniquePtr = false>
 296 |  auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<NodeId>& node_id_in = std::nullopt) noexcept
 297 |  {
 298 |      const NodeId node_id = node_id_in.value_or(fuzzed_data_provider.ConsumeIntegralInRange<NodeId>(0, std::numeric_limits<NodeId>::max()));
 299 | -    const ServiceFlags local_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);

MarcoFalke commented at 2:20 PM on July 1, 2022:

why is this removed and replaced with a hardcoded value?

jnewbery commented at 8:45 AM on July 4, 2022:

Which hardcoded value? The CNode object no longer contains nServices, so there's nothing to set here.

MarcoFalke commented at 9:34 AM on July 4, 2022:

The symbol is moved from CNode to Peer. I think a refactoring pull request should not change the value of the symbol.

If the local service flags were previously determined by the fuzz engine, they should still be after this change, regardless of the namespace of the symbol.

jnewbery commented at 11:15 AM on July 4, 2022:

To be clear, are you suggesting that the InitializeNode() calls in fuzz/processmessage.cpp and fuzz/processmessages.cpp should be updated to take ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS)?

MarcoFalke commented at 11:21 AM on July 4, 2022:

Yes.

I am also thinking if InitializeNode can be moved into FillNode, so that only one place needs to be updated.

dergoegge commented at 4:19 PM on July 4, 2022:

Moved all InitializeNode calls (there were only 2) into FillNode with service flags consumed from the fuzzer.

in src/net_processing.cpp:656 in 10a4ebab27 outdated

 652 | @@ -636,7 +653,7 @@ class PeerManagerImpl final : public PeerManager
 653 |      /** Get a pointer to a mutable CNodeState. */
 654 |      CNodeState* State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 655 |  
 656 | -    uint32_t GetFetchFlags(const CNode& pfrom) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 657 | +    uint32_t GetFetchFlags(const Peer& peer) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);

jnewbery commented at 8:05 AM on July 4, 2022:

This function no longer requires cs_main.

in src/net_processing.cpp:777 in 10a4ebab27 outdated

 773 | @@ -757,7 +774,7 @@ class PeerManagerImpl final : public PeerManager
 774 |      /** Update pindexLastCommonBlock and add not-in-flight missing successors to vBlocks, until it has
 775 |       *  at most count entries.
 776 |       */
 777 | -    void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vector<const CBlockIndex*>& vBlocks, NodeId& nodeStaller) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 778 | +    void FindNextBlocksToDownload(NodeId nodeid, const Peer& peer, unsigned int count, std::vector<const CBlockIndex*>& vBlocks, NodeId& nodeStaller) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

jnewbery commented at 8:05 AM on July 4, 2022:

Perhaps it makes sense to remove NodeId nodeid as an argument to this function, now that it can easily be accessed from the peer argument.

dergoegge commented at 4:20 PM on July 4, 2022:

Yup, removed the NodeId arg.

in src/rpc/net.cpp:128 in 10a4ebab27 outdated

 124 | @@ -130,6 +125,11 @@ static RPCHelpMan getpeerinfo()
 125 |                      {RPCResult::Type::BOOL, "inbound", "Inbound (true) or Outbound (false)"},
 126 |                      {RPCResult::Type::BOOL, "bip152_hb_to", "Whether we selected peer as (compact blocks) high-bandwidth peer"},
 127 |                      {RPCResult::Type::BOOL, "bip152_hb_from", "Whether peer selected us as (compact blocks) high-bandwidth peer"},
 128 | +                    {RPCResult::Type::STR_HEX, "services", /*optional=*/true, "The services offered"},

jnewbery commented at 8:25 AM on July 4, 2022:

I'm not sure if changing these fields to optional requires a release note, since missing fields that used to be always present could break clients that assume they'll be there. See #25176 for example.

dergoegge commented at 4:45 PM on July 4, 2022:

I changed this back to be non-optional just to stick with the previous behavior (i.e. when the peer has not sent a version message yet we default to NODE_NONE).

jnewbery commented at 12:14 PM on July 6, 2022:

:+1: Seems pragmatic

in src/net.cpp:240 in 10a4ebab27 outdated

 234 | @@ -235,10 +235,10 @@ bool IsPeerAddrLocalGood(CNode *pnode)
 235 |  
 236 |  std::optional<CAddress> GetLocalAddrForPeer(CNode *pnode)
 237 |  {
 238 | -    CAddress addrLocal = GetLocalAddress(&pnode->addr, pnode->GetLocalServices());
 239 | +    CAddress addrLocal = GetLocalAddress(&pnode->addr);
 240 |      if (gArgs.GetBoolArg("-addrmantest", false)) {
 241 |          // use IPv4 loopback during addrmantest
 242 | -        addrLocal = CAddress(CService(LookupNumeric("127.0.0.1", GetListenPort())), pnode->GetLocalServices());
 243 | +        addrLocal = CAddress(CService(LookupNumeric("127.0.0.1", GetListenPort())), NODE_NONE);

jnewbery commented at 8:40 AM on July 4, 2022:

I think the line lower down:

addrLocal = CAddress{pnode->GetAddrLocal(), addrLocal.nServices, addrLocal.nTime};

No longer makes sense, since addrLocal.nServices will always be NODE_NONE. Perhaps change addrLocal.nServices to NODE_NONE, since it is the caller's job to set the services.

Perhaps even better would be to change GetLocalAddrForPeer and GetLocalAddress to return a CService instead of a CAddress, so the caller has to set the nTime and nServices fields (which might help avoid bugs like #25314).

dergoegge commented at 4:22 PM on July 4, 2022:

Perhaps even better would be to change GetLocalAddrForPeer and GetLocalAddress to return a CService

Added an extra commit to do this. Makes it obvious that the caller is supposed to set the nTime and nServices fields.

jnewbery commented at 8:45 AM on July 4, 2022: member

Concept ACK.

MarcoFalke removed the label P2P on Jul 4, 2022

DrahtBot added the label P2P on Jul 4, 2022

dergoegge force-pushed on Jul 4, 2022

DrahtBot added the label Needs rebase on Jul 4, 2022

dergoegge force-pushed on Jul 5, 2022

DrahtBot removed the label Needs rebase on Jul 5, 2022

jnewbery commented at 12:14 PM on July 6, 2022: member

utACK d5b6ba717686ccb06ae06a553f2a563e97f53327

dergoegge force-pushed on Jul 6, 2022

in src/test/denialofservice_tests.cpp:86 in 40e83ad09d outdated

  81 | +        LOCK(node.cs_sendProcessing);
  82 | +        peerman.SendMessages(&node);
  83 | +    }
  84 | +    BOOST_CHECK_EQUAL(node.fSuccessfullyConnected, true);
  85 | +
  86 | +    TestOnlyResetTimeData();

dergoegge commented at 3:56 PM on July 6, 2022:

The Ci was failing because the peer manager calls AddTimeData when receiving a version message and since the time offset data for adjusted time is a global this can mess with other tests. Calling TestOnlyResetTimeData() here should fix it.

jnewbery commented at 4:22 PM on July 6, 2022: member

utACK c38a00d0e3

in src/test/net_tests.cpp:860 in bd93b709ea outdated

 856 | @@ -857,7 +857,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
 857 |          *static_cast<TestChainState*>(&m_node.chainman->ActiveChainstate());
 858 |      chainstate.JumpOutOfIbd();
 859 |  
 860 | -    m_node.peerman->InitializeNode(&peer);
 861 | +    m_node.peerman->InitializeNode(&peer, ServiceFlags(NODE_NETWORK|NODE_WITNESS));

MarcoFalke commented at 2:43 PM on July 8, 2022:

bd93b709ea4e8e295a3438f30d9d3f6999e7623c: Why are you changing this from NODE_NETWORK to something else?

MarcoFalke commented at 3:15 PM on July 8, 2022:

If this is supposed to be an alias for peer.nLocalServices, why not simply type that to make review easier?

dergoegge commented at 7:31 PM on July 8, 2022:

Changed this to pass GetLocalServices() to all the InitializeNode calls and only later on when CNode::nLocalServices is actually removed, do we set the service flags directly (corresponding to their previous values).

in src/test/fuzz/process_messages.cpp:49 in bd93b709ea outdated

  45 | @@ -46,7 +46,7 @@ FUZZ_TARGET_INIT(process_messages, initialize_process_messages)
  46 |          peers.push_back(ConsumeNodeAsUniquePtr(fuzzed_data_provider, i).release());
  47 |          CNode& p2p_node = *peers.back();
  48 |  
  49 | -        g_setup->m_node.peerman->InitializeNode(&p2p_node);
  50 | +        g_setup->m_node.peerman->InitializeNode(&p2p_node, ServiceFlags(NODE_NETWORK|NODE_WITNESS));

MarcoFalke commented at 2:44 PM on July 8, 2022:

Same commit, this should be p2p_node.nLocalServices, not something hardcoded

in src/test/fuzz/process_message.cpp:83 in bd93b709ea outdated

  79 | @@ -80,7 +80,7 @@ void fuzz_target(FuzzBufferType buffer, const std::string& LIMIT_TO_MESSAGE_TYPE
  80 |      CNode& p2p_node = *ConsumeNodeAsUniquePtr(fuzzed_data_provider).release();
  81 |  
  82 |      connman.AddTestNode(p2p_node);
  83 | -    g_setup->m_node.peerman->InitializeNode(&p2p_node);
  84 | +    g_setup->m_node.peerman->InitializeNode(&p2p_node, ServiceFlags(NODE_NETWORK|NODE_WITNESS));

MarcoFalke commented at 2:44 PM on July 8, 2022:

Same

in src/net_processing.cpp:225 in bd93b709ea outdated

 224 | +     *  TODO: remove redundant CNode::nLocalServices*/
 225 | +    const ServiceFlags m_our_services;
 226 | +    /** Services this peer offered to us.
 227 | +     *
 228 | +     * TODO: remove redundant CNode::nServices */
 229 | +    std::atomic<ServiceFlags> m_their_services{NODE_NONE};

MarcoFalke commented at 2:47 PM on July 8, 2022:

Same commit: (nit) struct doesn't have the m_ prefix, see also #25555 (review)

dergoegge commented at 7:32 PM on July 8, 2022:

Removed the m_ prefix.

jnewbery commented at 10:49 AM on July 13, 2022:

I'm going to push back against this. It's entirely possible that Peer picks up some member functions in future, and therefore becomes a class, in which case these data members would be named incorrectly and inconsistently.

dergoegge commented at 1:31 PM on July 14, 2022:

That seems reasonable, re-added the m_ prefix.

in src/test/denialofservice_tests.cpp:119 in 40e83ad09d outdated

 113 | @@ -63,10 +114,9 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
 114 |                       /*addrNameIn=*/"",
 115 |                       ConnectionType::OUTBOUND_FULL_RELAY,
 116 |                       /*inbound_onion=*/false};
 117 | -    dummyNode1.SetCommonVersion(PROTOCOL_VERSION);
 118 |  
 119 | -    peerLogic->InitializeNode(&dummyNode1, ServiceFlags(NODE_NETWORK|NODE_WITNESS));
 120 | -    dummyNode1.fSuccessfullyConnected = true;
 121 | +    peerman.InitializeNode(&dummyNode1, ServiceFlags(NODE_NETWORK|NODE_WITNESS));
 122 | +    ConnectNode(connman, peerman, dummyNode1, ServiceFlags(NODE_NETWORK|NODE_WITNESS));

MarcoFalke commented at 2:53 PM on July 8, 2022:

40e83ad09dae3bd958dafeb54c6da3193c6e0c6a: Previously it was NODE_NODE, now it is something else, why?

dergoegge commented at 7:00 PM on July 8, 2022:

This is done because this PR removes CNode::fClient and CNode::m_limited_node members. These two variables are default initialized as false and then set to the correct value when a version message is received. In production this is fine, as the two bools are not used if the version handshake hasn't happened. In the test however, we don't do the version handshake and manually set fSuccessfullyConnected = true. The default false then results in the logic behaving as expected (e.g. the node is not a client, so we ask them for headers).

Now with the PR the two bools are removed and replaced with static functions that operate on Peer::their_services to determine client and pruned node status. In the test here, if we keep Peer::their_services as NODE_NONE then we would consider dummyNode1 a client and not ask for headers.

I think this makes the test more realistic (also mentioned in the commit message) and shouldn't change behavior.

in src/net_processing.h:37 in ded0727ba0 outdated

  33 | @@ -34,6 +34,7 @@ struct CNodeStateStats {
  34 |      uint64_t m_addr_processed = 0;
  35 |      uint64_t m_addr_rate_limited = 0;
  36 |      bool m_addr_relay_enabled{false};
  37 | +    ServiceFlags m_their_services;

MarcoFalke commented at 2:59 PM on July 8, 2022:

ded0727ba0747cf1161e29ec33d16998f63ab3ef: Same nit about m_, but feel free to ignore, since I am pretty sure I "violated" that style rule myself by accident.

in src/test/denialofservice_tests.cpp:356 in d4759a02b5 outdated

 352 | @@ -356,7 +353,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
 353 |                           ConnectionType::INBOUND,
 354 |                           /*inbound_onion=*/false};
 355 |      nodes[0]->SetCommonVersion(PROTOCOL_VERSION);
 356 | -    peerLogic->InitializeNode(nodes[0], NODE_NETWORK);
 357 | +    peerLogic->InitializeNode(nodes[0], ServiceFlags(NODE_NETWORK | NODE_WITNESS));

MarcoFalke commented at 3:12 PM on July 8, 2022:

d4759a02b51368870526f04558797bb7f5749c73: Looks like you are changing the value, why?

dergoegge commented at 7:33 PM on July 8, 2022:

All the service flags should now have the same values as before.

MarcoFalke commented at 3:16 PM on July 8, 2022: member

Concept ACK, but I'd prefer if behaviour changes (even if they are in tests) are either explained in the commit description or done in a separate commit from the refactoring commit.

dergoegge force-pushed on Jul 8, 2022

in src/net_processing.cpp:1324 in 8960caf8b3 outdated

1320 | @@ -1299,15 +1321,15 @@ void PeerManagerImpl::UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_s
1321 |      if (state) state->m_last_block_announcement = time_in_seconds;
1322 |  }
1323 |  
1324 | -void PeerManagerImpl::InitializeNode(CNode *pnode)
1325 | +void PeerManagerImpl::InitializeNode(CNode *pnode, ServiceFlags our_services)

MarcoFalke commented at 3:20 PM on July 11, 2022:

nit in 8960caf8b333ba5b9d226d479329039253161ba6: As you are already changing all call sites, might as well change * into &. Feel free to ignore, as it is otherwise unrelated to the changes here.

dergoegge commented at 9:58 AM on July 13, 2022:

Done.

in src/test/denialofservice_tests.cpp:70 in c132abf166 outdated

  65 | +        peerman.SendMessages(&node);
  66 | +    }
  67 | +
  68 | +    TestOnlyResetTimeData();
  69 | +
  70 | +    if (node.fDisconnect) return;

MarcoFalke commented at 6:51 AM on July 12, 2022:

c132abf1667ea9367ce797d808d6b6c885196601: Why would this disconnect? Seems like copy-pasted dead code?

Also, it looks like you forgot to copy-paste the peer-state asserts.

Also, I don't see why this is copied in the first place. In the commit message you claim that it is better to make the version handshake consistent with "live code". So wouldn't it be better to make this helper easily accessible for all tests instead of forcing reviewers to re-do the copy-paste every time it is and will be used?

See #25591, which should also make this pull's diff smaller.

dergoegge commented at 10:00 AM on July 13, 2022:

Rebased on top of the changes in #25591. This test now uses ConnmanTestMsg::Handshake().

in src/net.h:238 in 9d9a6677c4 outdated

 234 | @@ -234,7 +235,7 @@ void RemoveLocal(const CService& addr);
 235 |  bool SeenLocal(const CService& addr);
 236 |  bool IsLocal(const CService& addr);
 237 |  bool GetLocal(CService &addr, const CNetAddr *paddrPeer = nullptr);
 238 | -CAddress GetLocalAddress(const CNetAddr *paddrPeer, ServiceFlags nLocalServices);
 239 | +CAddress GetLocalAddress(const CNetAddr *paddrPeer);

MarcoFalke commented at 6:58 AM on July 12, 2022:

9d9a6677c483772fe636b61480dfdde7d4586073 (nit): Could run clang-format on this line while touching it?

dergoegge commented at 10:00 AM on July 13, 2022:

Done.

MarcoFalke commented at 6:59 AM on July 12, 2022: member

Sorry for the incremental review, but I left more questions.

DrahtBot added the label Needs rebase on Jul 12, 2022

dergoegge force-pushed on Jul 13, 2022

DrahtBot removed the label Needs rebase on Jul 13, 2022

in src/net.h:607 in 8f6ed20aca outdated

 603 | @@ -625,7 +604,7 @@ class NetEventsInterface
 604 |  {
 605 |  public:
 606 |      /** Initialize a peer (setup state, queue any initial messages) */
 607 | -    virtual void InitializeNode(CNode* pnode) = 0;
 608 | +    virtual void InitializeNode(CNode& pnode, ServiceFlags our_services) = 0;

jnewbery commented at 10:52 AM on July 13, 2022:

This is beginning to feel very mission-creepy, but if you're changing the argument from being a pointer to a ref, then the name of the argument should also change from pnode to node (p is the prefix for pointer).

Same comment internally in net_processing for the InitializeNode() methods.

dergoegge commented at 3:59 PM on July 13, 2022:

Done.

in src/test/denialofservice_tests.cpp:11 in e97192fc87 outdated

   7 | @@ -8,13 +8,15 @@
   8 |  #include <chainparams.h>
   9 |  #include <net.h>
  10 |  #include <net_processing.h>
  11 | +#include <netmessagemaker.h>

MarcoFalke commented at 11:16 AM on July 13, 2022:

nit in the second commit: Can be removed?

dergoegge commented at 3:30 PM on July 13, 2022:

Done.

in src/test/denialofservice_tests.cpp:49 in e97192fc87 outdated

  45 | @@ -44,11 +46,10 @@ BOOST_FIXTURE_TEST_SUITE(denialofservice_tests, TestingSetup)
  46 |  // work.
  47 |  BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
  48 |  {
  49 | -    auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman);
  50 | +    ConnmanTestMsg& connman = *static_cast<ConnmanTestMsg*>(m_node.connman.get());

MarcoFalke commented at 11:20 AM on July 13, 2022:

nit in the second commit: Can be written shorter as static_cast<ConnmanTestMsg&>(*m_node.connman)?

dergoegge commented at 3:30 PM on July 13, 2022:

Yes, done.

in src/test/denialofservice_tests.cpp:52 in e97192fc87 outdated

  51 |      // Disable inactivity checks for this test to avoid interference
  52 | -    static_cast<ConnmanTestMsg*>(connman.get())->SetPeerConnectTimeout(99999s);
  53 | -    auto peerLogic = PeerManager::make(*connman, *m_node.addrman, nullptr,
  54 | -                                       *m_node.chainman, *m_node.mempool, false);
  55 | +    connman.SetPeerConnectTimeout(99999s);
  56 | +    PeerManager& peerman = *m_node.peerman.get();

MarcoFalke commented at 11:21 AM on July 13, 2022:

Same: no need for get?

in src/test/denialofservice_tests.cpp:69 in e97192fc87 outdated

  69 | -    peerLogic->InitializeNode(dummyNode1, dummyNode1.GetLocalServices());
  70 | -    dummyNode1.fSuccessfullyConnected = true;
  71 | +    connman.Handshake(
  72 | +        /*node=*/dummyNode1,
  73 | +        /*successfully_connected=*/true,
  74 | +        /*remote_services=*/ServiceFlags(NODE_NETWORK | NODE_WITNESS),

MarcoFalke commented at 11:23 AM on July 13, 2022:

second commit: Maybe mention in the commit message that this is a behaviour change that is needed for the next commit? Previously this was NODE_NODE, now it is something else?

dergoegge commented at 3:32 PM on July 13, 2022:

It is actually also needed in this commit because CNode::fClient and CNode::m_limited_node are set during the version handshake. The commit message now mentions this.

in src/net.h:377 in b41c44dcdc outdated

 373 | @@ -376,6 +374,8 @@ class CNode
 374 |  
 375 |      uint64_t nRecvBytes GUARDED_BY(cs_vRecv){0};
 376 |  
 377 | +    /** Whether this peer provides all services that we want. Used for eviction decisions */
 378 | +    std::atomic_bool m_has_all_wanted_services{false};
 379 |      std::atomic<std::chrono::seconds> m_last_send{0s};

MarcoFalke commented at 11:43 AM on July 13, 2022:

nit in b41c44dcdc49941c4c2e70bcdfdcd8b1d956c69f: Maybe move m_has_all_wanted_services down to the other eviction stuff (m_last_tx_time, ...). Otherwise, the comment "Used for eviction decisions" will (incorrectly) imply that m_last_send is also used for eviction, when it is currently not.

dergoegge commented at 3:32 PM on July 13, 2022:

Done.

in src/net.cpp:242 in 2c38d6b594 outdated

 235 | @@ -236,10 +236,10 @@ bool IsPeerAddrLocalGood(CNode *pnode)
 236 |  
 237 |  std::optional<CAddress> GetLocalAddrForPeer(CNode *pnode)
 238 |  {
 239 | -    CAddress addrLocal = GetLocalAddress(&pnode->addr, pnode->GetLocalServices());
 240 | +    CAddress addrLocal = GetLocalAddress(&pnode->addr);
 241 |      if (gArgs.GetBoolArg("-addrmantest", false)) {
 242 |          // use IPv4 loopback during addrmantest
 243 | -        addrLocal = CAddress(CService(LookupNumeric("127.0.0.1", GetListenPort())), pnode->GetLocalServices());
 244 | +        addrLocal = CAddress(CService(LookupNumeric("127.0.0.1", GetListenPort())), NODE_NONE);

MarcoFalke commented at 11:53 AM on July 13, 2022:

2c38d6b594db08427afaa8609ce3c8495305279f: Is there an explanation why this can be hardcoded to NODE_NONE everywhere? Maybe put commit 8f6ed20acae2a4f93f5b52c3937ab704980db44b before this one to clarify that this is not (supposed?) to change behavior?

dergoegge commented at 3:32 PM on July 13, 2022:

Switched the order of the commits. NODE_NONE is not used anymore now.

MarcoFalke approved

MarcoFalke commented at 11:54 AM on July 13, 2022: member

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

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

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

Signature:

</details>

in src/qt/rpcconsole.cpp:1199 in b41c44dcdc outdated

1195 | @@ -1197,6 +1196,7 @@ void RPCConsole::updateDetailWidget()
1196 |      // This check fails for example if the lock was busy and
1197 |      // nodeStateStats couldn't be fetched.
1198 |      if (stats->fNodeStateStatsAvailable) {
1199 | +        ui->peerServices->setText(GUIUtil::formatServicesStr(stats->nodeStateStats.their_services));

MarcoFalke commented at 12:09 PM on July 13, 2022:

Tested the result when the stats aren't available, which looks good:

Screenshot from 2022-07-13 14-07-22

MarcoFalke commented at 12:14 PM on July 13, 2022: member

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

dergoegge force-pushed on Jul 13, 2022

dergoegge commented at 4:01 PM on July 13, 2022: member

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

All commits should now build without issues.

in src/net.cpp:209 in 4c59e37c18 outdated

 205 | @@ -206,15 +206,13 @@ static std::vector<CAddress> ConvertSeeds(const std::vector<uint8_t> &vSeedsIn)
 206 |  // Otherwise, return the unroutable 0.0.0.0 but filled in with
 207 |  // the normal parameters, since the IP may be changed to a useful
 208 |  // one by discovery.
 209 | -CAddress GetLocalAddress(const CNetAddr *paddrPeer, ServiceFlags nLocalServices)
 210 | +CService GetLocalAddress(const CNetAddr *paddrPeer)

MarcoFalke commented at 4:18 PM on July 13, 2022:

nit in 4c59e37c18b177fc50786de37ffffb52de6daaa7: Only if you retouch, clang-format this line

in src/net.cpp:234 in 4c59e37c18 outdated

 230 | @@ -233,12 +231,12 @@ bool IsPeerAddrLocalGood(CNode *pnode)
 231 |             IsReachable(addrLocal.GetNetwork());
 232 |  }
 233 |  
 234 | -std::optional<CAddress> GetLocalAddrForPeer(CNode *pnode)
 235 | +std::optional<CService> GetLocalAddrForPeer(CNode *pnode)

MarcoFalke commented at 4:18 PM on July 13, 2022:

same

in src/net.cpp:263 in 4c59e37c18 outdated

 259 | @@ -262,7 +260,7 @@ std::optional<CAddress> GetLocalAddrForPeer(CNode *pnode)
 260 |      if (addrLocal.IsRoutable() || gArgs.GetBoolArg("-addrmantest", false))
 261 |      {
 262 |          LogPrint(BCLog::NET, "Advertising address %s to peer=%d\n", addrLocal.ToString(), pnode->GetId());
 263 | -        return addrLocal;
 264 | +        return {addrLocal};

MarcoFalke commented at 4:28 PM on July 13, 2022:

4c59e37c18b177fc50786de37ffffb52de6daaa7: Is this change needed? For me it compiles without.

dergoegge commented at 1:30 PM on July 14, 2022:

No its not, reverted.

in src/net.h:148 in 4c59e37c18 outdated

 143 | @@ -144,8 +144,8 @@ enum
 144 |  };
 145 |  
 146 |  bool IsPeerAddrLocalGood(CNode *pnode);
 147 | -/** Returns a local address that we should advertise to this peer */
 148 | -std::optional<CAddress> GetLocalAddrForPeer(CNode *pnode);
 149 | +/** Returns a local address that we should advertise to this peer. */
 150 | +std::optional<CService> GetLocalAddrForPeer(CNode* pnode);

MarcoFalke commented at 4:32 PM on July 13, 2022:

4c59e37c18b177fc50786de37ffffb52de6daaa7: While touching this method, could switch * to &? (And remove "p")

dergoegge commented at 1:30 PM on July 14, 2022:

Done.

in src/net.h:166 in 4c59e37c18 outdated

 162 | @@ -163,7 +163,7 @@ void RemoveLocal(const CService& addr);
 163 |  bool SeenLocal(const CService& addr);
 164 |  bool IsLocal(const CService& addr);
 165 |  bool GetLocal(CService &addr, const CNetAddr *paddrPeer = nullptr);
 166 | -CAddress GetLocalAddress(const CNetAddr *paddrPeer, ServiceFlags nLocalServices);
 167 | +CService GetLocalAddress(const CNetAddr* paddrPeer);

MarcoFalke commented at 4:32 PM on July 13, 2022:

Same

MarcoFalke approved

MarcoFalke commented at 4:43 PM on July 13, 2022: member

lgtm

review ACK e7aea6ae6d0cd668dc7228cc24d0a58419c6ebcd 🏞

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

Signature:

</details>

jnewbery commented at 11:59 AM on July 14, 2022: member

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

[net processing] Add m_our_services and m_their_services to Peer

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

1f52c47d5c

[tests] Connect peer in outbound_slow_chain_eviction by sending p2p messages

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

fc5eb528f7

[net processing] Remove fClient and m_limited_node

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

f65e83d51b

[net processing] Replace fHaveWitness with CanServeWitnesses() 7d1c036934

[net processing] Remove CNode::nServices

Use Peer::m_their_services instead

d9079fe18d

[net] Return CService from GetLocalAddrForPeer and GetLocalAddress 5961f8eea1

[net processing] Remove CNode::nLocalServices 8d8eeb422e

dergoegge force-pushed on Jul 14, 2022

MarcoFalke commented at 1:47 PM on July 14, 2022: member

ACK 8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67 🔑

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

Signature:

</details>

in src/net.h:1025 in 8d8eeb422e

1026 |       * This data is not marked const, but after being set it should not
1027 | -     * change. See the note in CNode::nLocalServices documentation.
1028 | +     * change.
1029 |       *
1030 | -     * \sa CNode::nLocalServices
1031 | +     * \sa Peer::our_services

jnewbery commented at 5:27 PM on July 14, 2022:

     * \sa Peer::m_our_services

jnewbery approved

jnewbery commented at 5:30 PM on July 14, 2022: member

utACK 8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67

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

fanquake requested review from mzumsande on Jul 15, 2022

in src/net_processing.cpp:2652 in 8d8eeb422e

2648 | @@ -2650,7 +2649,7 @@ bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& peer,
2649 |      return true;
2650 |  }
2651 |  
2652 | -void PeerManagerImpl::ProcessGetCFilters(CNode& peer, CDataStream& vRecv)
2653 | +void PeerManagerImpl::ProcessGetCFilters(CNode& node,Peer& peer, CDataStream& vRecv)

mzumsande commented at 10:12 PM on July 18, 2022:

nit:space after node

in src/net.h:1020 in 8d8eeb422e

1019 | +     * Services this node offers.
1020 |       *
1021 | -     * This data is replicated in each CNode instance we create during peer
1022 | -     * connection (in ConnectNode()) under a member also called
1023 | -     * nLocalServices.
1024 | +     * This data is replicated in each Peer instance we create.

mzumsande commented at 10:48 PM on July 18, 2022:

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.

mzumsande commented at 4:05 AM on July 19, 2022: contributor

Code Review ACK 8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67

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

MarcoFalke merged this on Jul 19, 2022

MarcoFalke closed this on Jul 19, 2022

sidhujag referenced this in commit bea2180c6a on Jul 19, 2022

dhruv commented at 6:34 PM on July 22, 2022: contributor

@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 commented at 11:08 AM on July 25, 2022: member

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

bitcoin locked this on Jul 25, 2023