net: Remove g_relay_txes #20217

jnewbery · 2020-10-22T11:19:38Z

g_relay_txes is only required inside net_processing and is set only once at startup. Instead of having a global, move it to be a const member of PeerManager.

This requires moving PushNodeVersion() into PeerManager, which also allows us to remove the connman argument.

maflcko

Left some style questions. Feel free to ignore, obviously.

Concept ACK on the change.

src/init.cpp

src/net_processing.h

jnewbery · 2020-10-22T12:36:06Z

Thanks for the review @MarcoFalke. I've taken your suggested name change.

practicalswift · 2020-10-22T18:53:09Z

Concept ACK: globals going down is good!

DrahtBot · 2020-10-22T22:42:26Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Move special CAddress-without-nTime logic to net_processing #20541 (Move special CAddress-without-nTime logic to net_processing by sipa)
addrman: Make addrman a top-level component #20228 (addrman: Make addrman a top-level component by jnewbery)
Periodically make block-relay connections and sync headers #19858 (Periodically make block-relay connections and sync headers by sdaftuar)

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.

fanquake · 2020-10-23T02:57:41Z

Concept ACK

ervanipank88

.

jnewbery · 2020-11-04T14:51:53Z

Rebased

sipa · 2020-11-05T02:11:56Z

utACK 179616f

narula · 2020-11-14T23:07:31Z

code review ACK.

I was confused about the relationship between g_relay_txes (now m_ignore_incoming_txs) and m_tx_relay. I think it's if m_tx_relay is null, this is a blocks-only peer, and if m_ignore_incoming_txs is true, I am a blocks-only peer. Is that right? I think unifying everything under names with "blocks-only" would be clearer but is out of scope for this change.

jnewbery · 2020-11-16T10:08:02Z

Yes, this has been confusing for people, which is why I think it's important to be precise with terminology:

a block-relay-only connection is a type of connection to a peer where we only send and process blocks. We do not send or process transactions or addresses to/from that peer. We'll request that the peer not send us transactions by setting relay to 0 in the version message that we send to them. We also won't initialize the m_tx_relay data structure for that peer, since we're not doing tx relay with them.
-blocksonly mode is a configuration option where we don't process transactions from any of our peers. We'll set relay to 0 in all of our version messages. It's mostly used to reduce bandwidth usage for people who don't need a mempool.

think unifying everything under names with "blocks-only" would be clearer

I think that's probably less clear since it would confuse those two concepts.

narula · 2020-11-16T17:09:24Z

a block-relay-only connection is a type of connection to a peer where we only send and process blocks. We do not send or process transactions or addresses to/from that peer. We'll request that the peer not send us transactions by setting relay to 0 in the version message that we send to them. We also won't initialize the m_tx_relay data structure for that peer, since we're not doing tx relay with them.

-blocksonly mode is a configuration option where we don't process transactions from any of our peers. We'll set relay to 0 in all of our version messages. It's mostly used to reduce bandwidth usage for people who don't need a mempool.

Hmm, these still seem like the same concept to me but I'm probably missing something; let me ask the question in a different way: What is the difference between me being a -blocksonly node and me setting all of my connections to block-relay-only (let's hypothetically say you could)? Is it how the node addresses incoming transactions? If there is no difference, I would say they are the same concept?

jnewbery · 2020-11-16T17:52:55Z

What is the difference between me being a -blocksonly node and me setting all of my connections to block-relay-only (let's hypothetically say you could)?

Some differences:

block-relay-only isn't a configuration option. The user has no control over how many block-relay-only connections the node makes, or to which addresses.
in -blocksonly mode, we still participate in address relay (we send getaddr messages to our peers, process the addr messages in response, and gossip new incoming addresses). block-relay-only connections do not participate in address relay, to preserve the privacy of those connections.
in -blocksonly mode, we could theoretically turn off other components, such as the fee estimator (which relies on participating in p2p transaction relay). Disable fee estimation in blocksonly mode (by removing the fee estimates global) #18766 does that.
block-relay-only connections are persisted across restart as 'anchor connections'.

There may be others.

narula · 2020-11-16T19:29:07Z

What is the difference between me being a -blocksonly node and me setting all of my connections to block-relay-only (let's hypothetically say you could)?

Some differences:

block-relay-only isn't a configuration option. The user has no control over how many block-relay-only connections the node makes, or to which addresses.

in -blocksonly mode, we still participate in address relay (we send getaddr messages to our peers, process the addr messages in response, and gossip new incoming addresses). block-relay-only connections do not participate in address relay, to preserve the privacy of those connections.

in -blocksonly mode, we could theoretically turn off other components, such as the fee estimator (which relies on participating in p2p transaction relay). Disable fee estimation in blocksonly mode (by removing the fee estimates global) #18766 does that.

block-relay-only connections are persisted across restart as 'anchor connections'.

There may be others.

Thank you, that's very helpful! Just to wrap up, it sounds like in my thought experiment (a hypothetical node with all block-relay-only connections vs. a node with -blocksonly) the applicable differences are bullet points 2 and 4, in that the -blocksonly node would still send around and process addrs and its connections would not be persisted across restart.

This makes sense because in -blocksonly mode a node still needs to learn about other nodes, so processing addrs is important. block-relay-only connections were never designed to be all of the connections, so it's fine for them not to process addrs because the node can rely on the other connections to do that.

We wouldn't want to persist all connections across a restart (#17326 (comment)), but because block-relay-only connections are more privacy-preserving and they are extra connections (we can rely on the other connections cycling) it's fine for them to be persisted. And persisting them sort of helps with eclipse attacks (#17326).

I think I'm confused because the check for block-relay-only connections in the code is sometimes to see if m_tx_relay exists, which is non-intuitive because block-relay-only also implies that addrs are not gossiped with this peer either, not just transactions, and sometimes to call IsBlockOnlyConn().

I think the new name for !g_relay_txes, m_ignore_incoming_txs, is much clearer.

jnewbery · 2020-11-19T11:20:10Z

I think I'm confused because the check for block-relay-only connections in the code is sometimes to see if m_tx_relay exists, which is non-intuitive because block-relay-only also implies that addrs are not gossiped with this peer either, not just transactions, and sometimes to call IsBlockOnlyConn().

IsBlockOnlyConn() was added later, and I think is much clearer since it's instantly obvious what the test is for, rather than if (m_tx_relay), which might just be defensive code to ensure that a null ptr isn't dererenced. I'd support a PR that made changes like:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index a69efa4fc0..3ea2c1a1f9 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1657,7 +1657,7 @@ void static ProcessGetData(CNode& pfrom, Peer& peer, const CChainParams& chainpa
 
         const CInv &inv = *it++;
 
-        if (pfrom.m_tx_relay == nullptr) {
+        if (peer.IsBlockOnlyConn()) {
             // Ignore GETDATA requests for transactions from blocks-only peers.
             continue;
         }

That's out of scope for this PR.

I think the new name for !g_relay_txes, m_ignore_incoming_txs, is much clearer.

Great. @MarcoFalke gets credit for that!

maflcko

review ACK 179616f nice cleanup! 🚔

Show signature and timestamp

Signature:

python: error while loading shared libraries: libpython3.8.so.1.0: cannot open shared object file: No such file or directory

src/net_processing.cpp

src/net_processing.h

src/init.cpp

maflcko

re-ACK ceedf23 only change is rebase 🏑

Show signature and timestamp

Signature:

Timestamp of file with hash c72658247447f9b3ea4dd216ccb2048076b4de79b559adad9feea0a1f8001eea -

src/init.cpp

maflcko

Would be nice if the previous feedback about MakeUnique was addressed, but looks fine already.

re-ACK 4fdca52 🎞

Show signature and timestamp

Signature:

Timestamp of file with hash 7771d7eb2920b1e3060dc2d3e986607c8393b9886bc4213c99ec34be0994af1c -

jnewbery · 2020-12-07T18:17:46Z

@MarcoFalke addressed your review comments. Sorry for dropping them earlier.

maflcko · 2020-12-07T18:40:19Z

re-ACK a21667a 🖱

Show signature and timestamp

Signature:

Timestamp of file with hash 0174d24f81d7b43cdafd8412778651b9496d8a3add89e495cf7fddb037bd18f1 -

jnewbery · 2020-12-09T11:37:01Z

rebased

jnewbery · 2020-12-09T11:38:14Z

At different times, this PR has had ACKs from @sipa @narula and @MarcoFalke. There have only been minor changes and rebases since those ACKs. Some reACKs at the same commit hash would be very appreciated!

maflcko · 2020-12-09T11:50:29Z

re-ACK 22d960b 🥃

Show signature and timestamp

Signature:

Timestamp of file with hash 63b2a105ac69ff10887f137cd6a439977e3e14920cb1b46d6faaf934b73581b9 -

maflcko · 2020-12-09T14:39:56Z

needs rebase 🗡️

…ternally

Also remove vestigial commend in init.cpp

jnewbery · 2020-12-09T18:23:59Z

rebased

narula · 2020-12-09T19:00:18Z

utACK 34e33ab

maflcko · 2020-12-10T06:28:25Z

re-ACK 34e33ab 💐

Show signature and timestamp

Signature:

Timestamp of file with hash 50413236396d5df336002d2266fca66543a43dc9ee2cb2ba69a71db2cca5df6a -

Summary: ``` g_relay_txes is only required inside net_processing and is set only once at startup. Instead of having a global, move it to be a const member of PeerManager. This requires moving PushNodeVersion() into PeerManager, which also allows us to remove the connman argument. ``` Backport of [[bitcoin/bitcoin#20217 | core#20217]]. Ref T1696. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10884

fanquake added the P2P label Oct 22, 2020

maflcko added the Refactoring label Oct 22, 2020

maflcko reviewed Oct 22, 2020

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

src/net_processing.h Outdated Show resolved Hide resolved

jnewbery force-pushed the 2020-10-remove-grelaytxes branch from 45e81e1 to a5c8535 Compare October 22, 2020 12:34

This was referenced Oct 22, 2020

net: fix output of peer address in version message #20212

Merged

net processing: Move peer_map to PeerManager #19910

Merged

Periodically make block-relay connections and sync headers #19858

Merged

ervanipank88 reviewed Oct 29, 2020

View reviewed changes

DrahtBot added the Needs rebase label Nov 4, 2020

jnewbery force-pushed the 2020-10-remove-grelaytxes branch from a5c8535 to 179616f Compare November 4, 2020 14:51

DrahtBot removed the Needs rebase label Nov 4, 2020

DrahtBot mentioned this pull request Nov 16, 2020

Erlay: bandwidth-efficient transaction relay protocol #18261

Closed

maflcko approved these changes Nov 19, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.h Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

maflcko reviewed Dec 7, 2020

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

jnewbery force-pushed the 2020-10-remove-grelaytxes branch from ceedf23 to 4fdca52 Compare December 7, 2020 15:00

DrahtBot removed the Needs rebase label Dec 7, 2020

maflcko reviewed Dec 7, 2020

View reviewed changes

jnewbery force-pushed the 2020-10-remove-grelaytxes branch from 4fdca52 to a21667a Compare December 7, 2020 18:17

DrahtBot added the Needs rebase label Dec 9, 2020

jnewbery force-pushed the 2020-10-remove-grelaytxes branch from a21667a to 22d960b Compare December 9, 2020 11:36

DrahtBot removed the Needs rebase label Dec 9, 2020

maflcko added the Needs rebase label Dec 9, 2020

jnewbery added 5 commits December 9, 2020 18:09

[net processing] Move PushNodeVersion into PeerManager

5805b82

[net processing] Add IgnoresIncomingTxs() function to PeerManager

f3f61d0

[init] Use MakeUnique<> to construct peerman

4d510aa

[net processing] Add m_ignores_incoming_txs to PeerManager and use in…

68334b3

…ternally

Remove g_relay_txes

34e33ab

Also remove vestigial commend in init.cpp

jnewbery force-pushed the 2020-10-remove-grelaytxes branch from 22d960b to 34e33ab Compare December 9, 2020 18:22

DrahtBot removed the Needs rebase label Dec 9, 2020

maflcko merged commit f5b2ea3 into bitcoin:master Dec 10, 2020

jnewbery deleted the 2020-10-remove-grelaytxes branch December 10, 2020 09:13

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 10, 2020

Merge bitcoin#20217: net: Remove g_relay_txes

91c8703

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: Remove g_relay_txes #20217

net: Remove g_relay_txes #20217

jnewbery commented Oct 22, 2020 •

edited

Loading

maflcko left a comment

jnewbery commented Oct 22, 2020

practicalswift commented Oct 22, 2020

DrahtBot commented Oct 22, 2020 •

edited

Loading

fanquake commented Oct 23, 2020

ervanipank88 left a comment •

edited by jnewbery

Loading

jnewbery commented Nov 4, 2020

sipa commented Nov 5, 2020

narula commented Nov 14, 2020

jnewbery commented Nov 16, 2020

narula commented Nov 16, 2020

jnewbery commented Nov 16, 2020

narula commented Nov 16, 2020

jnewbery commented Nov 19, 2020

maflcko left a comment

maflcko left a comment

maflcko left a comment

jnewbery commented Dec 7, 2020

maflcko commented Dec 7, 2020

jnewbery commented Dec 9, 2020

jnewbery commented Dec 9, 2020

maflcko commented Dec 9, 2020

maflcko commented Dec 9, 2020

jnewbery commented Dec 9, 2020

narula commented Dec 9, 2020

maflcko commented Dec 10, 2020

net: Remove g_relay_txes #20217

net: Remove g_relay_txes #20217

Conversation

jnewbery commented Oct 22, 2020 • edited Loading

maflcko left a comment

Choose a reason for hiding this comment

jnewbery commented Oct 22, 2020

practicalswift commented Oct 22, 2020

DrahtBot commented Oct 22, 2020 • edited Loading

Conflicts

fanquake commented Oct 23, 2020

ervanipank88 left a comment • edited by jnewbery Loading

Choose a reason for hiding this comment

jnewbery commented Nov 4, 2020

sipa commented Nov 5, 2020

narula commented Nov 14, 2020

jnewbery commented Nov 16, 2020

narula commented Nov 16, 2020

jnewbery commented Nov 16, 2020

narula commented Nov 16, 2020

jnewbery commented Nov 19, 2020

maflcko left a comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

jnewbery commented Dec 7, 2020

maflcko commented Dec 7, 2020

jnewbery commented Dec 9, 2020

jnewbery commented Dec 9, 2020

maflcko commented Dec 9, 2020

maflcko commented Dec 9, 2020

jnewbery commented Dec 9, 2020

narula commented Dec 9, 2020

maflcko commented Dec 10, 2020

jnewbery commented Oct 22, 2020 •

edited

Loading

DrahtBot commented Oct 22, 2020 •

edited

Loading

ervanipank88 left a comment •

edited by jnewbery

Loading