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.
1403@@ -1404,7 +1404,10 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
1404 node.chainman = &g_chainman;
1405 ChainstateManager& chainman = *Assert(node.chainman);
1406
1407- node.peerman.reset(new PeerManager(chainparams, *node.connman, node.banman.get(), *node.scheduler, chainman, *node.mempool));
1408+ assert(!node.peerman);
1409+ const bool blocks_only = args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY);
Any reason to change the name from relay_txes to blocks_only? With block-relay-only connections, I’d prefer to stick to the old name to avoid confusion.
The purpose is to ignore incoming inv and tx messages, so another name suggestion could be ignore_incoming_tx_relay, as outgoing tx relay is unaffected.
94@@ -94,6 +95,9 @@ class PeerManager final : public CValidationInterface, public NetEventsInterface
95 */
96 void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message);
97
98+ /** Whether this node is in blocks only mode */
0 /** Whether to ignore incoming tx relay traffic */
Left some style questions. Feel free to ignore, obviously.
Concept ACK on the change.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
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.
Yes, this has been confusing for people, which is why I think it’s important to be precise with terminology:
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.
- 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
relayto 0 in theversionmessage that we send to them. We also won’t initialize them_tx_relaydata structure for that peer, since we’re not doing tx relay with them.-blocksonlymode is a configuration option where we don’t process transactions from any of our peers. We’ll setrelayto 0 in all of ourversionmessages. 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?
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:
-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.-blocksonly mode, we could theoretically turn off other components, such as the fee estimator (which relies on participating in p2p transaction relay). #18766 does that.There may be others.
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
-blocksonlymode, 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
-blocksonlymode, we could theoretically turn off other components, such as the fee estimator (which relies on participating in p2p transaction relay). #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 (https://github.com/bitcoin/bitcoin/issues/17326#issuecomment-550521262), 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 (https://github.com/bitcoin/bitcoin/issues/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.
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:
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index a69efa4fc0..3ea2c1a1f9 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -1657,7 +1657,7 @@ void static ProcessGetData(CNode& pfrom, Peer& peer, const CChainParams& chainpa
5
6 const CInv &inv = *it++;
7
8- if (pfrom.m_tx_relay == nullptr) {
9+ if (peer.IsBlockOnlyConn()) {
10 // Ignore GETDATA requests for transactions from blocks-only peers.
11 continue;
12 }
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!
2350@@ -2351,7 +2351,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2351
2352 // Be shy and don't send version until we hear
2353 if (pfrom.IsInboundConn())
style-nit in commit 3b81ec59d15c17bb1c39eeb044c113117c41071d:
Could add {
93@@ -94,6 +94,9 @@ class PeerManager final : public CValidationInterface, public NetEventsInterface
94 */
95 void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message);
96
97+ /** Whether this node ignores txs received over p2p */
1403@@ -1404,7 +1404,8 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
1404 node.chainman = &g_chainman;
1405 ChainstateManager& chainman = *Assert(node.chainman);
1406
1407- node.peerman.reset(new PeerManager(chainparams, *node.connman, node.banman.get(), *node.scheduler, chainman, *node.mempool));
1408+ assert(!node.peerman);
1409+ node.peerman = MakeUnique<PeerManager>(chainparams, *node.connman, node.banman.get(), *node.scheduler, chainman, *node.mempool);
std::make_unique, not the deprecated wrapper.
review ACK 179616f8211883739df6e06852bd976dcdd0c3c9 nice cleanup! 🚔
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3review ACK 179616f8211883739df6e06852bd976dcdd0c3c9 nice cleanup! 🚔
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUhgiwv8DdUGvchEGZlcyNFDb/+qNnq3WawkfpGWNb6Ncw1UhyOpoKtyLHCU8YtX
88KFBToEOqMSQw9vljAJx9oRYEd/ugMBJgCxpdlLMuFeOpw9q7DIxGCUxJdtte3sU
9HQUPMduUvIb3PD6AXZC0uiTJk7S2DtZ7GhdPiVC1MK9prUZv/t17EA4o2daYFYZQ
102kz5vsvRXk2I5oh56sk0IlXr2Wid0+m6XDLj1Nf1CKZntg1DrR7+FeF9KN+pDuu5
11FV6hUE7Enqnv67MY8L2qDegLAxdHGhCoS6wWwZIciahEtXbUHPLexERdE7Mm38pp
12nHyidWXW1BVpYPIu8EssHboMreNxiX2BN4kyA2YPlKwmsmQjprBil+SnYaIVAWD4
138B5hxjH+f+DIxDliIphn3ABiLvJmXXh7GVG1XKKGdoBv5Po+YXh0UezPli6Fon49
14gqWmaqYeRfD1gxFAdMaM0Bl1bbFUuDKarN1knvU99h87t7u4op6PcxDl52r2mb93
15ePmRESnn
16=hNL8
17-----END PGP SIGNATURE-----
python: error while loading shared libraries: libpython3.8.so.1.0: cannot open shared object file: No such file or directory
1375@@ -1376,7 +1376,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
1376 // see Step 2: parameter interactions for more information about these
re-ACK ceedf230fd only change is rebase 🏑
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3re-ACK ceedf230fd only change is rebase 🏑
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUgCvAv+K3VviMunaCVXNAeZ/hZH34QUHITi3yW5mVjRux8HD/eTowDKtm+kMo/p
8t2+ADIT+xufMOh4xBjJ/tk18daxL7IHceABiNuAP/uDUVOn+QeCYbfueVj0UBepS
9G1mBS750Cy/frouSFD2Gn35H/8fNBtSTyAggKAveJ27cSK9h/nk2pp7xiyo+pG27
108+KoEt//u44zMRlLKFiEkMG6O60/RtxIndpLuU0t+T3qUv8CFyJYaWVjMng8ZA5B
112CvEI/7reaSWzyvfLakbkPdUtX6YqMixGSphq2U0AjcKShOSGF6wRnpo3bspsHXy
12aBo8OOp/E3X74Ui8DaAEnXP7VQj6wYLZsCKAsv/f0Dn3AohEawRPkiNG/4pgArbH
13iN746lD/VpJIABh3LdgY2FsyQ+lvf0mwol7WpNzMzIpy3qzvs8mixN+2HAuDWo02
14/XNDqfOP2sb22+leSeiujZTm0YmkLdBfJbjSpVeLWBmEbHZ8aUY4GTewFe0CvULf
15GcvKddFt
16=+MY6
17-----END PGP SIGNATURE-----
Timestamp of file with hash c72658247447f9b3ea4dd216ccb2048076b4de79b559adad9feea0a1f8001eea -
Would be nice if the previous feedback about MakeUnique was addressed, but looks fine already.
re-ACK 4fdca52007 🎞
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3re-ACK 4fdca52007 🎞
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUjHMAv/ZPMhJbZefHl6UIHBCAxBD64CT2DiBLoPw/GmGmgxq1vBr8LIPR1RYfvB
8tr8VcMtrqa+WaMszsEE3f+WAC86C49rbGn4r1MIEkFMiBZWn4N1nno5thhtCv9h8
9mfnFpJUqlgeFv+5ZXh63Y0mA1PbA4B7KQ1V1hc9WXK+tTzTN06B7wKJGf9/PJWUl
10sCWYpN5KBr4KGjlw6eJB9C141aIyUOgXzzep7waYeQqA4kUaJ6h7xk2g/1pLRA2f
11v5Ds/DrHY5wHfdLEv82Jdpmi+GC2DZ+buiuo+CpHMli3VdclrOtPLEVkAwPJTSMu
12UMOLzgZ9cOm0B9yCm2DtgRh1bVLJfAN3lzKpv2NObVZgW1sUQYHg1AFBJDMtj7dA
130zssUKJWLkbwKrND5LAocCHWlu0hBoYeM2wycWNRNvTUJlO8b9tzwrh2jpFlmVBK
14AjM40R1MbjIl3SirWjbB7a4DD0ubl9KPCYCm6eGEZryT5VuIvOAfDz7IeC6iKjrq
15uQVHH7b8
16=1TZV
17-----END PGP SIGNATURE-----
Timestamp of file with hash 7771d7eb2920b1e3060dc2d3e986607c8393b9886bc4213c99ec34be0994af1c -
re-ACK a21667ace8 🖱
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3re-ACK a21667ace8 🖱
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUhcBAwAqm9wdxwAE32mOhS0kcpLhVxGPmTuZ3Xwxca3DJNo20MX3qzUkmfG+izj
80it+s6TycxiPR0EXKDutg3qWxUcu0Js2pAcYnYrNAbUx91SwcT31kQ/VCSU/5MNj
9xrrMKZLJ5nVeKSMnF76jvpc8yoQO6wJpbWCnt5w38noa9BhwDU2pvHQF9vhVA7V1
10wZsqtr8z3gR54Ov7fQbN1DQ7eX/K8MjAjuUZvWTuEWVZPLUp5ewbm4r1iFQzskBJ
11JhjwoIfg/dxAscOQzrXhwlt1aVHQPygQAICNw06W41jxZpVyGVsIOOd6h7nWc5O4
12uSMF6W1HWuS12JhfCLl6W6HCRFt6/vt1pHu5sdTCpyFZPy66nTlDpbJBT7XnBh3T
133JGJTq+h5EP/p8A4Fh3bBnWmSqTGX4b3vCTcejO6nq4Knn2BGVjy4irYirXAkR//
14ReIsxEwclRLo0iCl9LgmOGV25MUASd1BKlGl4NknohG3LoIra7ZJeuQW6ZnoWqhZ
15GvvZqsbN
16=mv04
17-----END PGP SIGNATURE-----
Timestamp of file with hash 0174d24f81d7b43cdafd8412778651b9496d8a3add89e495cf7fddb037bd18f1 -
re-ACK 22d960b124 🥃
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3re-ACK 22d960b124 🥃
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUjh9wv9GVZefOvKGq4C4k3P+5QHVrXbC5F9CYF6K/n+QKgIole9iTT5rRNT0C+o
8WHb2Q3I3cdyo8WirtL/pBytgn3wBKu1Ap2v4Wygzb1p9+KbM/e2lcQLhGH7hfrKo
9yt30H+V+mnAqiln7iPc9BtskrIcL/hMvgkyU01p7UguvPzKzfo+2wVNkJFialPqe
10VrdpDO2cHbIexxD7Fjr12Q3Gdo2SRR66qo0poHNZkLZ1FRhzlRFsaixAwJTdDz9j
11YG3qZl7QuZtEIwqlDDZSoGiplqA7zvNSxxlcEGCPHvHuPkHnv2Fc51iu+q0VduQA
12H0hBpIVHeezSZpCAKbPz2f1dBPvRgw2H3Y5dReAuP8AFnJQmEqSmqIKMct7cqYR0
13eUJCauczOLfaNCB6iiEP6aUXgzIv9GWcumx0LSh46JHuMUk690lj3uwMrqOFNjbv
14WXEMx9a318Mu/nZNIkxItEJ579qhRyt7L/blV+hhUfupezceehR57UKP6oZVXJWq
15yDQK6uHW
16=altt
17-----END PGP SIGNATURE-----
Timestamp of file with hash 63b2a105ac69ff10887f137cd6a439977e3e14920cb1b46d6faaf934b73581b9 -
Also remove vestigial commend in init.cpp
re-ACK 34e33ab85 💐
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3re-ACK 34e33ab85 💐
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUjrNwwAhgjzJwknztCmt65CG19czsI/ftTRBzjJQ2dkkxI/DpPNtVRdzMyiDX7C
8kG7nZI+4PS2/DmzD46EwllFvndY3vLqVuGvr4I7/JIwAA49caGSABfY+5dW95BNp
9sc3N0/w8TpM/HEsW71xoz2ebRMG7tV0WoY4rYuQm3ynQltQ51uk+DYB997QKqI4/
10WRPyK0tXb0rG8nLj3zUBHBB4V1+4Qsjn22KS1O0MhSJh8/zrmooHcL4d1Z+GC3At
11BkcyzLRWbAq5uVnqHgJyQcZsakWR/fUAx4LMjw4D4n7WV/ja2BOL3NlCtMKarqvO
1288Ts9JcuI03rDfagB1K7JzpoSVDwpzP2E/7eE7fyB3Bo8mzNZ9OrwyoHDu957ydp
13rUvZj7jzyg4LBFxwxvVEf0AvGCID4rYMDxdqjYNs5dLKee2UaqNaB6VY76lrj7fJ
14szECMByBUJmqj0uZJNGVlzd3KcrfySW9KRAT+EoRPDSByehI6hfJtzJS/phOQ1Je
15ngdR9Lq0
16=kxdj
17-----END PGP SIGNATURE-----
Timestamp of file with hash 50413236396d5df336002d2266fca66543a43dc9ee2cb2ba69a71db2cca5df6a -