net processing: don't request tx relay on feeler connections

jnewbery commented at 9:50 am on August 23, 2021: member

Feelers are short-lived connections used to test the viability of peers. The bitcoind node will periodically open feeler connections to addresses in its addrman, wait for a version message from the peer, and then close the connection.

Currently, we set fRelay to 1 in the version message for feeler connections, indicating that we want the peer to relay transactions to us. However, we close the connection immediately on receipt of the version message, and so never process any incoming transaction announcements. This PR changes that behaviour to instead set fRelay to 0 indicating that we do not wish to receive transaction announcements from the peer.

This PR also extends the addconnection RPC to allow creating outbound feeler connections from the node to the test framework, and a test to verify that the node sets fRelay to 0 in the version message to feeler connections.

fanquake added the label P2P on Aug 23, 2021

jnewbery commented at 9:50 am on August 23, 2021: member

Previously attempted in #9403, which was abandoned by the author.

in src/net_processing.cpp:1099 in e064b1bed2 outdated

1097@@ -1098,7 +1098,7 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, int64_t nTime)
1098                            CAddress(CService(), addr.nServices);
1099     CAddress addrMe = CAddress(CService(), nLocalNodeServices);
1100 
1101-    const bool tx_relay = !m_ignore_incoming_txs && pnode.m_tx_relay != nullptr;
1102+    const bool tx_relay = !m_ignore_incoming_txs && pnode.m_tx_relay != nullptr && !pnode.IsFeelerConn();

MarcoFalke commented at 9:55 am on August 23, 2021:

nit: Doesn’t this mean we won’t request tx relay, but are (theoretically) still happy to process them?

Wouldn’t it be better if this check here wasn’t needed and m_tx_relay was never initialized? See also #21160 (review)

jnewbery commented at 12:29 pm on August 23, 2021:

nit: Doesn’t this mean we won’t request tx relay, but are (theoretically) still happy to process them?

No. For feeler connections, we disconnect as soon as we receive the version message, and we don’t process any other message types before receiving version, so it’s not possible for us to process any incoming transactions or announcements.

Wouldn’t it be better if this check here wasn’t needed and m_tx_relay was never initialized?

I agree that it’d be better if m_tx_relay wasn’t initialized until we receive the version message, and have implemented that in #22778. However, I don’t think that’s relevant here. For outbound peers, PushNodeVersion is sent before receiving the version message, so this check can’t be dependent on whether m_tx_relay has been initialized.

DrahtBot commented at 1:29 pm on August 23, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23695 (p2p: Always serialize local timestamp for version msg by MarcoFalke)

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.

DrahtBot added the label Needs rebase on Aug 23, 2021

jnewbery commented at 5:22 pm on August 23, 2021: member

rebased

jnewbery force-pushed on Aug 23, 2021

ghost commented at 5:40 pm on August 23, 2021: none

Currently, we set fRelay to 1 in the version message for feeler connections, indicating that we want the peer to relay transactions to us. However, we close the connection immediately on receipt of the version message, and so never process any incoming transaction announcements. This PR changes that behaviour to instead set fRelay to 0 indicating that we do not wish to receive transaction announcements from the peer.

Concept ACK. Approach ACK.

~~Do we have any hidden RPC to test this without using python test framework? Or maybe I can wait for some feeler connections and see messages in Wireshark~~

DrahtBot removed the label Needs rebase on Aug 23, 2021

in test/functional/p2p_add_connections.py:23 in 4d972c4438 outdated

18+    def on_version(self, message):
19+        # The bitcoind node closes feeler connections as soon as a version
20+        # message is received from the test framework. Don't send any responses
21+        # to the node's version message since the connection will already be
22+        # closed.
23+        pass

mzumsande commented at 10:37 pm on August 23, 2021:

I don’t understand the reason for P2PFeelerReceiver. A real peer also wouldn’t know that they are about to get disconnected by us as a feeler, so why simulate it here instead of just using P2PInterface?

jnewbery commented at 1:20 pm on August 24, 2021:

Since the node will disconnect as soon as it receives our version message, there’s a race between it disconnecting and the P2PInterface sending the responses to its version message here: https://github.com/bitcoin/bitcoin/blob/eb09c26724e3f714b613788fc506f2ff3a208d2c/test/functional/test_framework/p2p.py#L435-L441. If it disconnects first, those calls to send_message() may fail, causing the test to fail.

in src/net.cpp:1231 in 4d972c4438 outdated

1226@@ -1228,6 +1227,9 @@ bool CConnman::AddConnection(const std::string& address, ConnectionType conn_typ
1227     // no limit for ADDR_FETCH because -seednode has no limit either
1228     case ConnectionType::ADDR_FETCH:
1229         break;
1230+    // no limit for FEELER connections since they're short-lived
1231+    case ConnectionType::FEELER:

mzumsande commented at 11:07 pm on August 23, 2021:

nit: there are several comments that describe the possible connection types and could be updated for feelers, I hope I caught them all here

jnewbery commented at 1:23 pm on August 24, 2021:

Thanks. Updated!

mzumsande commented at 11:14 pm on August 23, 2021: member

If I understand it correctly, the problem solved here is that our feeler peer might assemble/send out unnecessary tx announcements in the short time before they notice that they got disconnected.

Aren’t tx invs are just one of several unnecessary messages? e.g. we also send them an GETADDR before disconnecting, so that they might prepare an answer which we’ll never receive. So I wonder if a more general solution would make sense. (would it maybe work to skip sending VERACK to feelers?)

jnewbery commented at 1:18 pm on August 24, 2021: member

If I understand it correctly, the problem solved here is that our feeler peer might assemble/send out unnecessary tx announcements in the short time before they notice that they got disconnected.

Not quite. The fRelay field in the version message indicates whether the peer should announce new unconfirmed transactions to us (https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki#extensions-to-existing-messages). When we open a feeler connection to a peer, we obviously don’t want it to announce txs to us, since we’re going to disconnect as soon as we receive their version message.

This PR is maybe better understood when considered alongside #22778, where we’ll save having to allocate the TxRelay data structure when receiving inbound connections with fRelay set to 0.

Aren’t tx invs are just one of several unnecessary messages? e.g. we also send them an GETADDR before disconnecting, so that they might prepare an answer which we’ll never receive. So I wonder if a more general solution would make sense. (would it maybe work to skip sending VERACK to feelers?)

I agree that we probably shouldn’t send getaddr, sendaddrv2 and wtxidrelay to the peer. I’m not sure about skipping verack. Those changes could happen in a separate PR.

jnewbery force-pushed on Aug 24, 2021

amitiuttarwar commented at 6:00 pm on August 24, 2021: contributor

trying to wrap my head around not acquiring the CSemaphoreGrant / having a limit to the amount of feelers we open from the tests…

when a bitcoind node opens a feeler connection, it acquires the grant in ThreadOpenConnections. but I think its fine to not replicate this from the tests because there’s no logic that is ensuring the current connection count matches the maximums for feelers (like with outbound-full-relay or block-relay-only connections). the addconnection RPC only works for regtest, so we don’t have to worry about potential resource usage on mainnet. and other than not enforcing a maximum number of connections, I don’t think there are other logic conditions that change. @jnewbery does that match your reasoning?

naumenkogs commented at 4:04 pm on September 1, 2021: member

Concept ACK

ShubhamPalriwala commented at 7:50 pm on September 13, 2021: contributor

Was working on something similar. I’m quite unsure as to why we are removing the limit of Feeler connections in the tests? Having the limit as it is helps us as:

We mimic the actual scenario where there’s a limit to 1 outgoing Feeler connection at a time too
Such checks prevent any sort of throttling that might happen in the future (even though its just on regtest for now)
There’s no such obstacle that the connection limit is obstructing us with

However, The test written for Feeler connections looks good to me (can be expanded further in the future to test full functionality of feelers).

in test/functional/test_framework/test_node.py:577 in a9ed3e6900 outdated

574-        self.p2ps.append(p2p_conn)
575+        if connection_type == "feeler":
576+            # feeler connections are closed as soon as the node receives a `version` message
577+            p2p_conn.wait_until(lambda: p2p_conn.message_count["version"] == 1, check_connected=False)
578+            p2p_conn.wait_until(lambda: not p2p_conn.is_connected, check_connected=False)
579+            return p2p_conn

naumenkogs commented at 10:14 am on September 17, 2021:

this line could be dropped?

jnewbery commented at 3:12 pm on September 22, 2021:

Indeed. Thanks!

naumenkogs commented at 10:39 am on September 17, 2021: member

ACK 9a45648438ae62c4087a6d01d14050035dd0e78c

One thing I was thinking about is whether someone in the future would want to rely on fRelay=1 to distinguish whether a peer is tx-relaying or not (e.g. SPV-readonly), e.g. to populate addrman. I don’t think that’s a real concern, but I thought I’d bring it up this behaviour would make that impossible.

[test] Add testing for outbound feeler connections

Extend the addconnection RPC method to allow creating outbound
feeler connections. Extend the test framework to accept those
feeler connections.

0220b834b1

[net processing] Do not request transaction relay from feeler connections

Add a test to verify that feeler connections do not request transaction relay.

eaf6be0114

jnewbery force-pushed on Sep 22, 2021

jnewbery commented at 3:14 pm on September 22, 2021: member

Thanks for the reviews @amitiuttarwar @ShubhamPalriwala @naumenkogs

trying to wrap my head around not acquiring the CSemaphoreGrant / having a limit to the amount of feelers we open from the tests… @amitiuttarwar - I don’t understand this. In CConnman::AddConnection() we always try to acquire a semaphore grant before calling OpenNetworkConnection(). If that TryAcquire fails, then we return false and the rpc throws.

I’m quite unsure as to why we are removing the limit of Feeler connections in the tests? @ShubhamPalriwala - I also don’t understand this. There have never been any limits on the number of feelers, so this PR doesn’t remove a limit. In normal operation, the number of feelers is implicitly limited because we’ll only open them every 2 minutes on average, and the connections are short-lived. However, there’s never been a hard limit on the number of feelers, and this PR doesn’t propose to add one.

One thing I was thinking about is whether someone in the future would want to rely on fRelay=1 to distinguish whether a peer is tx-relaying or not (e.g. SPV-readonly), e.g. to populate addrman. @naumenkogs - I think this is already impossible. Bitcoin Core will set fRelay=0 for block-relay-only connections. You can’t interpret from fRelay=0 on a certain connection that the peer doesn’t relay transactions at all, just that it doesn’t want to receive transactions on this connection.

naumenkogs commented at 11:58 am on September 24, 2021: member

ACK eaf6be0114a6d7763767da9496907fe8a670ff9e

jnewbery commented at 4:26 pm on December 14, 2021: member

@mzumsande @MarcoFalke mind taking another look at this one? Should be fairly straightforward.

in src/net_processing.cpp:1099 in eaf6be0114

1095@@ -1096,7 +1096,7 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, int64_t nTime)
1096     CService addr_you = addr.IsRoutable() && !IsProxy(addr) && addr.IsAddrV1Compatible() ? addr : CService();
1097     uint64_t your_services{addr.nServices};
1098 
1099-    const bool tx_relay = !m_ignore_incoming_txs && pnode.m_tx_relay != nullptr;
1100+    const bool tx_relay = !m_ignore_incoming_txs && pnode.m_tx_relay != nullptr && !pnode.IsFeelerConn();

MarcoFalke commented at 4:56 pm on December 14, 2021:

style-nit for the future: const bool tx_relay{...};.

MarcoFalke approved

MarcoFalke commented at 4:56 pm on December 14, 2021: member

review ACK eaf6be0114a6d7763767da9496907fe8a670ff9e 🏃

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK eaf6be0114a6d7763767da9496907fe8a670ff9e 🏃
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgyawv+I5/mXrAjLcYkne7KDbXAjbm/EtM/cX13nX96td7HK9PGpiEUaX/aMt6X
 81Qsowv3dsv6UZVqJgvANWc1UZSvcOPyMHHGXy4tD3sTDjVA/iKD0CGZKUHDWBqAV
 9Ar+NJ20VK8bkyY4fF6zRqHE0bILiT0vWbkG88yF91gv8Hpz+Xc03MyGbqmg08CwU
10LY+ul2MuioxuTz//NRoIBIO8HCTU2FSeP3ChZeRlcBuyPGCapYnYcesIFSHeM/+E
11czZVpwB9UyGXjONXqFjhr8FMkRNDqV/2kmqVWewwQNzFTE3BpAE93JbSF/bZfLgT
12Mh4dZ1NNPbaOVb569aE7UUcILhpbc+zGhUFFHHpJ6V+28UIc2zaJmAhKlxJ/WSSX
13/yjNIS+4lxskLYgiQ+DQsIdXdz04QAbme4wA1It0cD5g+ope1/iuePgWm4uJ/8y8
14fOb2uHr4bajys6x4hhi0kJhVnfS8f3euWLWVriZEj8/huq1B7CrSiErmN/I/IV6o
155+uSyAO7
16=dERP
17-----END PGP SIGNATURE-----

MarcoFalke merged this on Dec 14, 2021

MarcoFalke closed this on Dec 14, 2021

jnewbery deleted the branch on Dec 14, 2021

sidhujag referenced this in commit a3740b9e77 on Dec 14, 2021

Fabcien referenced this in commit 5193e1777e on Jan 31, 2022

DrahtBot locked this on Dec 14, 2022

net processing: don’t request tx relay on feeler connections #22777

Conflicts