refactor: Introduce PeerManagerImpl::RejectIncomingTxs

MarcoFalke commented at 1:19 pm on May 17, 2022: member

Currently there are some confusions in net_processing:

There is confusion between -blocksonly mode and block-relay-only, so adjust all comments to use the same nomenclature.
Whether to disconnect peers for providing invs/txs is implemented differently. For example, it seems a bit confusing to disconnect block-relay-only peers with relay permission when they send a tx message, but not when they send an inv message. Also, keeping track of their inv announcements seems both wasteful and confusing, as it does nothing. This isn’t possible in practice, as outbound connections do not have permissions assigned, but sees fragile to rely on. Especially in light of proposed changes to make that possible: https://github.com/bitcoin/bitcoin/pull/17167

fanquake added the label P2P on May 17, 2022

MarcoFalke renamed this:
~~Disconnect block-relay-only peers with relay permission when they fill tx announcements~~
refactor: Introduce PeerManagerImpl::RejectIncomingTxs
on May 17, 2022

MarcoFalke added the label Refactoring on May 17, 2022

MarcoFalke force-pushed on May 17, 2022

MarcoFalke marked this as a draft on May 17, 2022

DrahtBot commented at 5:22 am on May 18, 2022: 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:

#25203 (logging: update to severity-based logging by jonatack)
#24170 (p2p, rpc: Manual block-relay-only connections with addnode by mzumsande)

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 May 19, 2022

in src/net_processing.cpp:983 in fa20bae7a2 outdated

979@@ -971,7 +980,7 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid)
980 {
981     AssertLockHeld(cs_main);
982 
983-    // Never request high-bandwidth mode from peers if we're blocks-only. Our
984+    // Never request high-bandwidth mode from peers in -blocksonly mode. Our

ajtowns commented at 5:37 pm on May 20, 2022:

This reads to me as the peer that’s in -blocksonly mode. Maybe “When in -blocksonly mode, never request high-bandwidth mode from peers.” ?

MarcoFalke commented at 7:56 am on May 21, 2022:

Thx, done

in src/net_processing.cpp:1198 in fa20bae7a2 outdated

1194@@ -1186,7 +1195,7 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer)
1195     CService addr_you = addr.IsRoutable() && !IsProxy(addr) && addr.IsAddrV1Compatible() ? addr : CService();
1196     uint64_t your_services{addr.nServices};
1197 
1198-    const bool tx_relay = !m_ignore_incoming_txs && peer.m_tx_relay != nullptr && !pnode.IsFeelerConn();
1199+    const bool tx_relay{!m_ignore_incoming_txs && !pnode.IsBlockOnlyConn() && !pnode.IsFeelerConn()};

ajtowns commented at 5:47 pm on May 20, 2022:

This change is already merged…

Would it make sense to rename this method to IsBlockRelayOnlyConn for consistency?

MarcoFalke commented at 7:56 am on May 21, 2022:

Thx, removed

ajtowns commented at 5:49 pm on May 20, 2022: member

Concept ACK

MarcoFalke force-pushed on May 21, 2022

MarcoFalke removed the label Needs rebase on May 21, 2022

MarcoFalke marked this as ready for review on May 21, 2022

MarcoFalke requested review from jnewbery on May 21, 2022

in src/net_processing.cpp:608 in faefdbdaab outdated

601@@ -602,9 +602,18 @@ class PeerManagerImpl final : public PeerManager
602     /** Next time to check for stale tip */
603     std::chrono::seconds m_stale_tip_check_time{0s};
604 
605-    /** Whether this node is running in blocks only mode */
606+    /** Whether this node is running in -blocksonly mode */
607     const bool m_ignore_incoming_txs;
608 
609+    bool RejectIncomingTxs(const CNode& peer) const

jnewbery commented at 9:40 am on May 23, 2022:

Could you declare this alongside all the other member functions, and then define the function body separately (as is done for all other members of PeerManagerImpl. It’s much more pleasant to read the code if the member functions and member variables are collected together.

MarcoFalke commented at 1:32 pm on May 27, 2022:

dropping the “refactor” tag.

This does not change any end-user behaviour of the compiled binary, so I think “refactor” is fine.

Could you declare this alongside all the other member functions

Not sure where “all the other […] functions” are. They are mixed around and I thought having related members and functions close by each other is useful. So I kept this as-is for now.

define the function body separately

Thx, done.

Can you copy the rationale to the commit log

Thx, copy-pasted

jnewbery commented at 9:43 am on May 23, 2022: member

Concept ACK

Can you copy the rationale to the commit log? It currently doesn’t explain why you’re introducing the new RejectIncomingTxs() function. I’d also suggest dropping the “refactor” tag.

MarcoFalke added the label Needs rebase on May 27, 2022

MarcoFalke force-pushed on May 27, 2022

MarcoFalke commented at 1:34 pm on May 27, 2022: member

Fixed feedback

DrahtBot removed the label Needs rebase on May 27, 2022

jnewbery commented at 4:10 pm on May 27, 2022: member

ACK fa2b5fe0c19035df3030e39ba919077a0cda8ac1

in test/functional/p2p_blocksonly.py:92 in fa2b5fe0c1 outdated

88@@ -89,6 +89,11 @@ def blocks_relay_conn_tests(self):
89         assert_equal(self.nodes[0].getpeerinfo()[0]['relaytxes'], False)
90         _, txid, _, tx_hex = self.check_p2p_tx_violation()
91 
92+        self.log.info("Tests with node in normal mode with block-relay-only connections with relay permission")

mzumsande commented at 10:07 pm on June 13, 2022:

“with relay permission”: I think that even if block-relay-only connections could have relay permission, they still wouldn’t have that here because the last restart disabling -blocksonly mode cleared the -whitelist=relay@127.0.0.1 arg from above.

MarcoFalke commented at 6:36 am on June 14, 2022:

0        self.log.info("Tests with node in normal mode with block-relay-only connection, sending an inv")

Oh good catch. I changed the test to an inv once I realized that permission flags can’t be set, but forgot to update the comment.

mzumsande commented at 10:10 pm on June 13, 2022: member

Code Review ACK fa2b5fe0c19035df3030e39ba919077a0cda8ac1

refactor: Introduce PeerManagerImpl::RejectIncomingTxs

Currently there are some confusions in net_processing:

* There is confusion between `-blocksonly mode` and `block-relay-only`,
  so adjust all comments to use the same nomenclature.
* Whether to disconnect peers for providing invs/txs is implemented
  differently. For example, it seems a bit confusing to disconnect
  `block-relay-only` peers with `relay` permission when they send a tx
  message, but not when they send an inv message. Also, keeping track of
  their inv announcements seems both wasteful and confusing, as it does
  nothing. This isn't possible in practice, as outbound connections do
  not have permissions assigned, but sees fragile to rely on. Especially
  in light of proposed changes to make that possible:
  https://github.com/bitcoin/bitcoin/pull/17167

fafddafc2c

MarcoFalke force-pushed on Jun 14, 2022

MarcoFalke commented at 6:42 am on June 14, 2022: member

Fixed typo in test log. (Thanks Martin!)

Should be trivial to re-ACK with git range-diff bitcoin-core/master fa2b5fe0c1 fafddafc2c.

jnewbery commented at 10:24 am on June 14, 2022: member

Code review ACK fafddafc2c

in test/functional/p2p_blocksonly.py:108 in fafddafc2c

104@@ -100,6 +105,13 @@ def blocks_relay_conn_tests(self):
105         conn.sync_send_with_ping()
106         assert(int(txid, 16) not in conn.get_invs())
107 
108+    def check_p2p_inv_violation(self, peer):

mzumsande commented at 2:05 am on June 15, 2022:

This doesn’t need to be done here, but if there is a new function for this anyway, it could also be used in the first part blocksonly_mode_tests to remove some (almost) duplicate code.

mzumsande commented at 2:08 am on June 15, 2022: member

ACK fafddafc2c60c7a176c5367967181cc1cabf3196

MarcoFalke merged this on Jun 15, 2022

MarcoFalke closed this on Jun 15, 2022

MarcoFalke deleted the branch on Jun 15, 2022

sidhujag referenced this in commit a46aa0a2b9 on Jun 15, 2022

DrahtBot locked this on Jun 15, 2023

refactor: Introduce PeerManagerImpl::RejectIncomingTxs #25156

Conflicts