Merge #5978: backport: merge bitcoin#21594, bitcoin#21843, bitcoin#22306

, bitcoin#22211, bitcoin#22387, bitcoin#21528, bitcoin#22616, bitcoin#22604, bitcoin#22960, bitcoin#23218 (networking backports: part 3)

1fedf47 test: add type annotation for `ADDRS` in `p2p_addrv2_relay` (Kittywhiskers Van Gogh)
022b76f merge bitcoin#23218: Use mocktime for ping timeout (Kittywhiskers Van Gogh)
45d9e58 merge bitcoin#22960: Set peertimeout in write_config (Kittywhiskers Van Gogh)
06e909b merge bitcoin#22604: address rate-limiting follow-ups (Kittywhiskers Van Gogh)
60b3e08 merge bitcoin#22616: address relay fixups (Kittywhiskers Van Gogh)
8b8fbc5 merge bitcoin#22618: Small follow-ups to 21528 (Kittywhiskers Van Gogh)
18fe765 merge bitcoin#21528: Reduce addr blackholes (Kittywhiskers Van Gogh)
c1874c6 net_processing: gate `m_tx_relay` access behind `!IsBlockOnlyConn()` (Kittywhiskers Van Gogh)
602d13d merge bitcoin#22387: Rate limit the processing of rumoured addresses (Kittywhiskers Van Gogh)
fe66202 merge bitcoin#22211: relay I2P addresses even if not reachable (by us) (Kittywhiskers Van Gogh)
7e08db5 merge bitcoin#22306: Improvements to p2p_addr_relay.py (Kittywhiskers Van Gogh)
ff3497c merge bitcoin#21843: enable GetAddr, GetAddresses, and getnodeaddresses by network (Kittywhiskers Van Gogh)
51edeb0 merge bitcoin#21594: add network field to getnodeaddresses (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependency for #5982
  * Population of `ADDRS` in `p2p_addr`(`v2`)`_relay` in Dash is done in the test object ([source](https://github.com/dashpay/dash/blob/0a62b9f985866249964ed703108a8dc617c73ba3/test/functional/p2p_addrv2_relay.py#L42-L49)) as opposed to upstream, where it is done in the global state ([source](https://github.com/maflcko/bitcoin-core/blob/d930c7f5b091687eb4208a5ffe8a2abe311d8054/test/functional/p2p_addrv2_relay.py#L23-L35)). This is because Dash specifically relies on `self.mocktime` instead of Bitcoin, which will work with simply sampling current time (`time.time()`).
    * [bitcoin#22211](bitcoin#22211) adds changes ([source](https://github.com/bitcoin/bitcoin/pull/22211/files#diff-d3d7b1bb23f25a96c9c7444a79159ad1799895565f99efebf1618e41e886bd53R44-R46)) that add usage of `ADDRS` outside the test object. That, alongside with other considerations, resulted in [dash#5967](#5967) and a discussion ([source](https://github.com/dashpay/dash/pull/5967/files#r1548101561))
    * Eventually, following the footsteps of [dash#5967](#5967), `ADDRS` was defined outside but setup within the test object. This worked just fine ([build](https://gitlab.com/dashpay/dash/-/jobs/6594036014)) but displeased the linter ([build](https://gitlab.com/dashpay/dash/-/jobs/6594035886)) because `ADDRS` type could not be implicitly determined solely on usage in the global scope.
    * An attempt to correct this was done by realignment with upstream ([commit](262d006)), which pleased the linter ([build](https://gitlab.com/dashpay/dash/-/jobs/6597322521)) but broken the test ([build](https://gitlab.com/dashpay/dash/-/jobs/6597322548)) for the reasons as mentioned above.
    * Therefore, to keep the linter happy, `ADDRS` has been annotated as a `List[CAddress]` (which involved importing `List` but that's fine) ([commit](cb6d36d))
  * Working on [bitcoin#21528](bitcoin#21528) proved challenging due to differences in Dash's and Bitcoin's approach to relaying and the workarounds used to accommodate for that.
    * Bitcoin conditionally initializes `m_tx_relay` ([source](https://github.com/amitiuttarwar/bitcoin/blob/3f7250b328b8b2f5d63f323702445ac5c989b73d/src/net.cpp#L2989-L2991)) and can always check if transaction relaying is permitted by checking if it's initialized ([source](https://github.com/amitiuttarwar/bitcoin/blob/3f7250b328b8b2f5d63f323702445ac5c989b73d/src/net_processing.cpp#L1820-L1826)).
    * Dash unconditionally initializes it ([source](https://github.com/dashpay/dash/blob/0a62b9f985866249964ed703108a8dc617c73ba3/src/net.h#L605-L607)). Earlier, Dash used to check if it's _appropriate_ to relay transactions by checking if it can relay addresses ([source](https://github.com/dashpay/dash/blob/dc6f52ac99a3b6fb8b3dfe37db4d6db1f7b1e719/src/net_processing.cpp#L2134-L2140)), which at the time, simply meant, it wasn't a block-only connection ([source](https://github.com/dashpay/dash/blob/dc6f52ac99a3b6fb8b3dfe37db4d6db1f7b1e719/src/net.h#L568-L572)).
    * This mutual exclusivity no longer held true in [dash#5964](#5964) and therefore, some transaction relay decisions were bound to **not** being a block-only connection ([commit](26c39f5)) but some were left behind, adopting `RelayAddrsWithPeer()` ([source](https://github.com/dashpay/dash/blob/0a62b9f985866249964ed703108a8dc617c73ba3/src/net_processing.cpp#L2215-L2221)), which, to be noted, is determined by the initialization status of `Peer::m_addr_known` ([source](https://github.com/dashpay/dash/blob/0a62b9f985866249964ed703108a8dc617c73ba3/src/net_processing.cpp#L839-L842)), which, so far, was pegged to **not** block-relay connection status ([source](https://github.com/dashpay/dash/blob/0a62b9f985866249964ed703108a8dc617c73ba3/src/net_processing.cpp#L1319)).
    * [bitcoin#21528](bitcoin#21528) got rid of `RelayAddrsWithPeer()` and replaced it with `Peer::m_addr_relay_enabled` ([source](https://github.com/amitiuttarwar/bitcoin/blob/3f7250b328b8b2f5d63f323702445ac5c989b73d/src/net_processing.cpp#L237-L251)), which is setup using `Peer::SetupAddressRelay()` ([source](https://github.com/amitiuttarwar/bitcoin/blob/3f7250b328b8b2f5d63f323702445ac5c989b73d/src/net_processing.cpp#L637-L643)). This means, rather than defining the address relay status during construction, it is setup during the first address-related message (i.e. `ADDR`, `ADDRV2`, `GETADDR`) ([source](https://github.com/amitiuttarwar/bitcoin/blob/3f7250b328b8b2f5d63f323702445ac5c989b73d/src/net_processing.cpp#L227-L236)).
      * Meaning, until the first addr-related message happens, the state is has not been determined and defaults to `false`. Because some `m_tx_relay` usage still piggybacked on addr-relay permission to determine tx-relay, if a transaction message is processed before an address message is processed, there will be a false-negative condition.

        The transaction relay logic won't run since it's expecting that if transactions can be relayed, so can addresses and checks for address relaying but believes that it cannot do address relaying, borrowing that state for transaction relaying, despite address relaying permissions actually being indeterminate since it hasn't had a chance to validate its eligibility.
      * There were two approaches, run `SetupAddressRelay()` as early in the connection as possible to substitute for the "determine at construction" behaviour and change no other conditional statements... and break address-related tests _or_ move the remaining conditional transaction relay logic to use **not** block-only connection checks instead.
      * We've gone with the latter, resulting in some changes where the condition only changes form but is the same (`RelayAddrsWithPeer()` > `Peer::m_addr_relay_enabled`) ([source](109c5a9#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3L2131-L2134)) but other changes where the condition itself has been changed (`RelayAddrsWithPeer()` > `!CNode::IsBlockOnlyConn()`) ([source](109c5a9#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R2256-R2259))
    * This does mean that in [dash#5982](#5982), `Peer::m_block_relay_only` is introduced to be the counterpart to `Peer::m_addr_relay_enabled` ([source](https://github.com/dashpay/dash/blame/45b48dae0a66fd918834d012cc8e1e17b5824abe/src/net_processing.cpp#L321-L322)) to account for some `CConnman` logic being moved into `PeerManager` ([source](https://github.com/dashpay/dash/blame/45b48dae0a66fd918834d012cc8e1e17b5824abe/src/net_processing.cpp#L2186-L2195)), which, in a way, reverts [dash#5339](#5339) but also, doesn't, since it moves the information into `Peer` instead of reinstating it into `CNode`.
      * This was eventual since the underlying presumption that `CNode::IsAddrRelayPeer() == !CNode::IsBlockOnlyConn()` no longer holds true (also because `CNode::IsAddrRelayPeer()` doesn't exist anymore).

  Special thanks to @UdjinM6 for help with understanding Dash-specifics with respect to functional tests through help on [dash#5964](#5964) and [dash#5967](#5967)

  ## Breaking Changes

  None expected.

  RPC changes have been introduced in `getnodeaddresses`, where a new input `network`, can filter addresses based on desired network and a new output, also `network`, will associate the address with the origin network. This change is expected to be backwards-compatible.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK 1fedf47

Tree-SHA512: 533d33f79a0d9fd730073b3b9a58baf1dd3b0c95823e765c88a43cc974970ed3609bf1863c63ac7fc5586d1437e5250b0a2d3005468da09e407110a412bd0264

doc/release-notes-5978.md

@@ -0,0 +1,15 @@
+RPC changes
+-----------------------
+
+- The `getnodeaddresses` RPC now returns a "network" field indicating the
+  network type (ipv4, ipv6, onion, or i2p) for each address.
+
+- `getnodeaddresses` now also accepts a "network" argument (ipv4, ipv6, onion,
+  or i2p) to return only addresses of the specified network.
+
+P2P and network changes
+-----------------------
+
+- A dashd node will no longer rumour addresses to inbound peers by default.
+  They will become eligible for address gossip after sending an ADDR, ADDRV2,
+  or GETADDR message.

src/net.cpp

@@ -1492,9 +1492,8 @@ void CConnman::CalculateNumConnectionsChangedStats()
     statsClient.gauge("peers.torConnections", torNodes, 1.0f);
 }
 
-bool CConnman::ShouldRunInactivityChecks(const CNode& node, std::optional<int64_t> now_in) const
+bool CConnman::ShouldRunInactivityChecks(const CNode& node, int64_t now) const
 {
-    const int64_t now = now_in ? now_in.value() : GetTimeSeconds();
     return node.nTimeConnected + m_peer_connect_timeout < now;
 }
 

src/net.h

@@ -607,7 +607,8 @@ class CNode
     };
 
     // in bitcoin: m_tx_relay == nullptr if we're not relaying transactions with this peer
-    // in dash: m_tx_relay should never be nullptr, use `!IsBlockOnlyConn() == false` instead
+    // in dash: m_tx_relay should never be nullptr, we don't relay transactions if
+    //          `IsBlockOnlyConn() == true` is instead
     std::unique_ptr<TxRelay> m_tx_relay{std::make_unique<TxRelay>()};
 
     /** UNIX epoch time of the last block received from this peer that we had
@@ -1241,7 +1242,7 @@ friend class CNode;
     void SetAsmap(std::vector<bool> asmap) { addrman.m_asmap = std::move(asmap); }
 
     /** Return true if we should disconnect the peer for failing an inactivity check. */
-    bool ShouldRunInactivityChecks(const CNode& node, std::optional<int64_t> now=std::nullopt) const;
+    bool ShouldRunInactivityChecks(const CNode& node, int64_t secs_now) const;
 
 private:
     struct ListenSocket {

src/net_permissions.h

@@ -30,7 +30,8 @@ enum class NetPermissionFlags : uint32_t {
     NoBan = (1U << 4) | Download,
     // Can query the mempool
     Mempool = (1U << 5),
-    // Can request addrs without hitting a privacy-preserving cache
+    // Can request addrs without hitting a privacy-preserving cache, and send us
+    // unlimited amounts of addrs.
     Addr = (1U << 7),
 
     // True if the user did not specifically set fine grained permissions

src/net_processing.h

@@ -43,6 +43,9 @@ struct CNodeStateStats {
     int m_starting_height = -1;
     std::chrono::microseconds m_ping_wait;
     std::vector<int> vHeightInFlight;
+    uint64_t m_addr_processed = 0;
+    uint64_t m_addr_rate_limited = 0;
+    bool m_addr_relay_enabled{false};
 };
 
 class PeerManager : public CValidationInterface, public NetEventsInterface

src/netaddress.h

@@ -232,7 +232,7 @@ class CNetAddr
      */
     bool IsRelayable() const
     {
-        return IsIPv4() || IsIPv6() || IsTor();
+        return IsIPv4() || IsIPv6() || IsTor() || IsI2P();
     }
 
     /**

src/rpc/net.cpp

@@ -145,6 +145,9 @@ static RPCHelpMan getpeerinfo()
                     {
                         {RPCResult::Type::NUM, "n", "The heights of blocks we're currently asking from this peer"},
                     }},
+                    {RPCResult::Type::BOOL, "addr_relay_enabled", "Whether we participate in address relay with this peer"},
+                    {RPCResult::Type::NUM, "addr_processed", "The total number of addresses processed, excluding those dropped due to rate limiting"},
+                    {RPCResult::Type::NUM, "addr_rate_limited", "The total number of addresses dropped due to rate limiting"},
                     {RPCResult::Type::BOOL, "whitelisted", "Whether the peer is whitelisted"},
                     {RPCResult::Type::ARR, "permissions", "Any special permissions that have been granted to this peer",
                     {
@@ -243,6 +246,9 @@ static RPCHelpMan getpeerinfo()
                 heights.push_back(height);
             }
             obj.pushKV("inflight", heights);
+            obj.pushKV("addr_relay_enabled", statestats.m_addr_relay_enabled);
+            obj.pushKV("addr_processed", statestats.m_addr_processed);
+            obj.pushKV("addr_rate_limited", statestats.m_addr_rate_limited);
         }
         obj.pushKV("whitelisted", stats.m_legacyWhitelisted);
         UniValue permissions(UniValue::VARR);
@@ -906,25 +912,30 @@ static RPCHelpMan setnetworkactive()
 static RPCHelpMan getnodeaddresses()
 {
     return RPCHelpMan{"getnodeaddresses",
-                "\nReturn known addresses which can potentially be used to find new nodes in the network\n",
+                "\nReturn known addresses, which can potentially be used to find new nodes in the network.\n",
                 {
                     {"count", RPCArg::Type::NUM, /* default */ "1", "The maximum number of addresses to return. Specify 0 to return all known addresses."},
+                    {"network", RPCArg::Type::STR, /* default */ "all networks", "Return only addresses of the specified network. Can be one of: " + Join(GetNetworkNames(), ", ") + "."},
                 },
                 RPCResult{
                     RPCResult::Type::ARR, "", "",
                     {
                         {RPCResult::Type::OBJ, "", "",
                         {
-                            {RPCResult::Type::NUM_TIME, "time", "The " + UNIX_EPOCH_TIME + " of when the node was last seen"},
-                            {RPCResult::Type::NUM, "services", "The services offered"},
+                            {RPCResult::Type::NUM_TIME, "time", "The " + UNIX_EPOCH_TIME + " when the node was last seen"},
+                            {RPCResult::Type::NUM, "services", "The services offered by the node"},
                             {RPCResult::Type::STR, "address", "The address of the node"},
-                            {RPCResult::Type::NUM, "port", "The port of the node"},
+                            {RPCResult::Type::NUM, "port", "The port number of the node"},
+                            {RPCResult::Type::STR, "network", "The network (" + Join(GetNetworkNames(), ", ") + ") the node connected through"},
                         }},
                     }
                 },
                 RPCExamples{
                     HelpExampleCli("getnodeaddresses", "8")
-            + HelpExampleRpc("getnodeaddresses", "8")
+                    + HelpExampleCli("getnodeaddresses", "4 \"i2p\"")
+                    + HelpExampleCli("-named getnodeaddresses", "network=onion count=12")
+                    + HelpExampleRpc("getnodeaddresses", "8")
+                    + HelpExampleRpc("getnodeaddresses", "4, \"i2p\"")
                 },
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
@@ -933,15 +944,16 @@ static RPCHelpMan getnodeaddresses()
         throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
     }
 
-    int count = 1;
-    if (!request.params[0].isNull()) {
-        count = request.params[0].get_int();
-        if (count < 0) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, "Address count out of range");
-        }
+    const int count{request.params[0].isNull() ? 1 : request.params[0].get_int()};
+    if (count < 0) throw JSONRPCError(RPC_INVALID_PARAMETER, "Address count out of range");
+
+    const std::optional<Network> network{request.params[1].isNull() ? std::nullopt : std::optional<Network>{ParseNetwork(request.params[1].get_str())}};
+    if (network == NET_UNROUTABLE) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Network not recognized: %s", request.params[1].get_str()));
     }
+
     // returns a shuffled list of CAddress
-    std::vector<CAddress> vAddr = node.connman->GetAddresses(count, /* max_pct */ 0, /* network */ std::nullopt);
+    const std::vector<CAddress> vAddr{node.connman->GetAddresses(count, /* max_pct */ 0, network)};
     UniValue ret(UniValue::VARR);
 
     for (const CAddress& addr : vAddr) {
@@ -950,6 +962,7 @@ static RPCHelpMan getnodeaddresses()
         obj.pushKV("services", (uint64_t)addr.nServices);
         obj.pushKV("address", addr.ToStringIP());
         obj.pushKV("port", addr.GetPort());
+        obj.pushKV("network", GetNetworkName(addr.GetNetClass()));
         ret.push_back(obj);
     }
     return ret;
@@ -1027,7 +1040,7 @@ static const CRPCCommand commands[] =
     { "network",            "clearbanned",            &clearbanned,            {} },
     { "network",            "cleardiscouraged",       &cleardiscouraged,        {} },
     { "network",            "setnetworkactive",       &setnetworkactive,       {"state"} },
-    { "network",            "getnodeaddresses",       &getnodeaddresses,       {"count"} },
+    { "network",            "getnodeaddresses",       &getnodeaddresses,       {"count", "network"} },
 
     { "hidden",             "addconnection",          &addconnection,          {"address", "connection_type"} },
     { "hidden",             "addpeeraddress",         &addpeeraddress,         {"address", "port"} },

src/test/denialofservice_tests.cpp

@@ -64,6 +64,8 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
 {
     const CChainParams& chainparams = Params();
     auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman);
+    // Disable inactivity checks for this test to avoid interference
+    static_cast<ConnmanTestMsg*>(connman.get())->SetPeerConnectTimeout(99999);
     auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, nullptr, *m_node.scheduler,
                                        *m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync,
                                        *m_node.govman, *m_node.sporkman, ::deterministicMNManager,

src/test/util/net.h

@@ -15,6 +15,12 @@
 
 struct ConnmanTestMsg : public CConnman {
     using CConnman::CConnman;
+
+    void SetPeerConnectTimeout(int64_t timeout)
+    {
+        m_peer_connect_timeout = timeout;
+    }
+
     void AddTestNode(CNode& node)
     {
         LOCK(cs_vNodes);

test/functional/feature_bip68_sequence.py

@@ -38,10 +38,7 @@ class BIP68Test(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 2
         self.extra_args = [
-            [
-                "-acceptnonstdtxn=1",
-                "-peertimeout=9999",  # bump because mocktime might cause a disconnect otherwise
-            ],
+            ["-acceptnonstdtxn=1"],
             ["-acceptnonstdtxn=0"],
         ]
 

test/functional/feature_maxuploadtarget.py

@@ -38,7 +38,6 @@ def set_test_params(self):
         self.extra_args = [[
             "-maxuploadtarget=200",
             "-blockmaxsize=999000",
-            "-peertimeout=9999",  # bump because mocktime might cause a disconnect otherwise
             "-maxtipage="+str(2*60*60*24*7),
             "-acceptnonstdtxn=1"
         ]]