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

jnewbery · 2021-08-23T09:50:02Z

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.

jnewbery · 2021-08-23T09:50:50Z

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

src/net_processing.cpp

DrahtBot · 2021-08-23T13:29:00Z

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.

jnewbery · 2021-08-23T17:22:46Z

rebased

ghost · 2021-08-23T17:40:09Z

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~~

mzumsande

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

test/functional/p2p_add_connections.py

src/net.cpp

jnewbery · 2021-08-24T13:18:38Z

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.

amitiuttarwar · 2021-08-24T18:00:32Z

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 · 2021-09-01T16:04:46Z

Concept ACK

ShubhamPalriwala

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

test/functional/test_framework/test_node.py

naumenkogs · 2021-09-17T10:39:57Z

ACK 9a45648

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.

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

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

jnewbery · 2021-09-22T15:14:58Z

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 · 2021-09-24T11:58:48Z

ACK eaf6be0

jnewbery · 2021-12-14T16:26:07Z

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

maflcko

review ACK eaf6be0 🏃

Show signature

Signature:

maflcko · 2021-12-14T16:56:23Z

src/net_processing.cpp

@@ -1096,7 +1096,7 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, int64_t nTime)
    CService addr_you = addr.IsRoutable() && !IsProxy(addr) && addr.IsAddrV1Compatible() ? addr : CService();
    uint64_t your_services{addr.nServices};

-    const bool tx_relay = !m_ignore_incoming_txs && pnode.m_tx_relay != nullptr;
+    const bool tx_relay = !m_ignore_incoming_txs && pnode.m_tx_relay != nullptr && !pnode.IsFeelerConn();


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

… connec…

…ions Summary: > [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. > [net processing] Do not request transaction relay from feeler connections > > Add a test to verify that feeler connections do not request transaction relay. This is a backport of [[bitcoin/bitcoin#22777 | core#22777]] Depends on D10944 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10945

fanquake added the P2P label Aug 23, 2021

maflcko reviewed Aug 23, 2021

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 23, 2021

net processing: Reduce resource usage for inbound block-relay-only connections #22778

Merged

DrahtBot added the Needs rebase label Aug 23, 2021

jnewbery force-pushed the 2021-08-feeler-no-frelay branch from e064b1b to b9f26e7 Compare August 23, 2021 17:22

DrahtBot removed the Needs rebase label Aug 23, 2021

mzumsande reviewed Aug 23, 2021

View reviewed changes

test/functional/p2p_add_connections.py Show resolved Hide resolved

src/net.cpp Show resolved Hide resolved

This was referenced Aug 23, 2021

net/net processing: Move tx inventory into net_processing #21160

Merged

Move special CAddress-without-nTime logic to net_processing #20541

Merged

jnewbery force-pushed the 2021-08-feeler-no-frelay branch from b9f26e7 to 9a45648 Compare August 24, 2021 13:24

DrahtBot mentioned this pull request Aug 31, 2021

net: Make m_mempool optional in PeerManager #22850

Closed

ShubhamPalriwala reviewed Sep 13, 2021

View reviewed changes

naumenkogs reviewed Sep 17, 2021

View reviewed changes

test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved

jnewbery added 2 commits September 22, 2021 16:12

[test] Add testing for outbound feeler connections

0220b83

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

[net processing] Do not request transaction relay from feeler connect…

eaf6be0

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

jnewbery force-pushed the 2021-08-feeler-no-frelay branch from 9a45648 to eaf6be0 Compare September 22, 2021 15:12

DrahtBot mentioned this pull request Dec 8, 2021

p2p: Always serialize local timestamp for version msg #23695

Merged

maflcko approved these changes Dec 14, 2021

View reviewed changes

maflcko merged commit 9635760 into bitcoin:master Dec 14, 2021

jnewbery deleted the 2021-08-feeler-no-frelay branch December 14, 2021 17:00

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 14, 2021

Merge bitcoin#22777: net processing: don't request tx relay on feeler…

a3740b9

… connec…

bitcoin locked and limited conversation to collaborators Dec 14, 2022

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

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

Uh oh!

Conversation

jnewbery commented Aug 23, 2021

Uh oh!

jnewbery commented Aug 23, 2021

Uh oh!

Uh oh!

DrahtBot commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery commented Aug 23, 2021

Uh oh!

ghost commented Aug 23, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Aug 24, 2021

Uh oh!

amitiuttarwar commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

naumenkogs commented Sep 1, 2021

Uh oh!

ShubhamPalriwala left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

naumenkogs commented Sep 17, 2021

Uh oh!

jnewbery commented Sep 22, 2021

Uh oh!

naumenkogs commented Sep 24, 2021

Uh oh!

jnewbery commented Dec 14, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Aug 23, 2021 •

edited

Loading

ghost commented Aug 23, 2021 •

edited by ghost

Loading

amitiuttarwar commented Aug 24, 2021 •

edited

Loading