Merge #16847: doc: add comments clarifying how local services are advertised

82e53f37e1bfa6e34eac16b33329d70c3c0127da doc: add comments clarifying how local services are advertised (James O'Beirne)

Pull request description:

  Recent questions have come up regarding dynamic service registration
  (see https://github.com/bitcoin/bitcoin/pull/16442#discussion_r308702676
  and the assumeutxo project ~~which needs to dynamically flip NODE_NETWORK~~).

  While investigating how dynamic service registration might work, I was
  confused about how we convey local services to peers. This adds some
  documentation that hopefully clarifies this process.

ACKs for top commit:
  laanwj:
    ACK 82e53f37e1bfa6e34eac16b33329d70c3c0127da
  darosior:
    ACK 82e53f37e1bfa6e34eac16b33329d70c3c0127da

Tree-SHA512: a30c1020387d7a75b5b3cdde45f8b7f2ae46293da97e6227b2ee17e290b93deb5b16c0bbc2b1676972300e5c3c2ad74eb8b3910d6b93e028dac1ae2700468ef9

2 files changed, 38 insertions, 2 deletions

diff --git a/src/net.h b/src/net.h
index e9ea0162c9..f33147155a 100644
--- a/src/net.h
+++ b/src/net.h
@@ -282,6 +282,12 @@ public:
     bool DisconnectNode(const CNetAddr& addr);
     bool DisconnectNode(NodeId id);
 
+    //! Used to convey which local services we are offering peers during node
+    //! connection.
+    //!
+    //! The data returned by this is used in CNode construction,
+    //! which is used to advertise which services we are offering
+    //! that peer during `net_processing.cpp:PushNodeVersion()`.
     ServiceFlags GetLocalServices() const;
 
     //!set the max outbound target in bytes
@@ -413,7 +419,18 @@ private:
     std::atomic<NodeId> nLastNodeId{0};
     unsigned int nPrevNodeCount{0};
 
-    /** Services this instance offers */
+    /**
+     * Services this instance offers.
+     *
+     * This data is replicated in each CNode instance we create during peer
+     * connection (in ConnectNode()) under a member also called
+     * nLocalServices.
+     *
+     * This data is not marked const, but after being set it should not
+     * change. See the note in CNode::nLocalServices documentation.
+     *
+     * \sa CNode::nLocalServices
+     */
     ServiceFlags nLocalServices;
 
     std::unique_ptr<CSemaphore> semOutbound;
@@ -786,8 +803,24 @@ public:
 private:
     const NodeId id;
     const uint64_t nLocalHostNonce;
-    // Services offered to this peer
+
+    //! Services offered to this peer.
+    //!
+    //! This is supplied by the parent CConnman during peer connection
+    //! (CConnman::ConnectNode()) from its attribute of the same name.
+    //!
+    //! This is const because there is no protocol defined for renegotiating
+    //! services initially offered to a peer. The set of local services we
+    //! offer should not change after initialization.
+    //!
+    //! An interesting example of this is NODE_NETWORK and initial block
+    //! download: a node which starts up from scratch doesn't have any blocks
+    //! to serve, but still advertises NODE_NETWORK because it will eventually
+    //! fulfill this role after IBD completes. P2P code is written in such a
+    //! way that it can gracefully handle peers who don't make good on their
+    //! service advertisements.
     const ServiceFlags nLocalServices;
+
     const int nMyStartingHeight;
     int nSendVersion{0};
     NetPermissionFlags m_permissionFlags{ PF_NONE };
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 7f2fea5584..93a98974eb 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -415,6 +415,9 @@ static void UpdatePreferredDownload(CNode* node, CNodeState* state) EXCLUSIVE_LO
 
 static void PushNodeVersion(CNode *pnode, CConnman* connman, int64_t nTime)
 {
+    // Note that pnode->GetLocalServices() is a reflection of the local
+    // services we were offering when the CNode object was created for this
+    // peer.
     ServiceFlags nLocalNodeServices = pnode->GetLocalServices();
     uint64_t nonce = pnode->GetLocalNonce();
     int nNodeStartingHeight = pnode->GetMyStartingHeight();