refactoring: [Net Processing] Follow-ups to #21160

jnewbery commented at 8:18 am on March 28, 2022: member

#21160 ([net/net processing]: Move tx inventory into net_processing) had some unaddressed review comments when it was merged. This branch addresses those comments.

net_processing: move CNode data access out of lock

CNode::m_relays_tx and CNode::m_bloom_filter_loaded access don't require
the Peer::TxRelay::m_bloom_filter_mutex lock, so move them out of the
lock scope.

See https://github.com/bitcoin/bitcoin/pull/21160#discussion_r736785417
and https://github.com/bitcoin/bitcoin/pull/21160#discussion_r736785662.

21154ff927

jnewbery commented at 8:22 am on March 28, 2022: member

@theuni I believe this addresses all your remaining review comments from #21160

fanquake added the label P2P on Mar 28, 2022

fanquake commented at 8:32 am on March 28, 2022: member

cc @dergoegge

in src/net_processing.cpp:450 in 2432ec4af8 outdated

446@@ -447,7 +447,7 @@ class PeerManagerImpl final : public PeerManager
447     void RelayAddress(NodeId originator, const CAddress& addr, bool fReachable);
448 
449     /** Send `feefilter` message. */
450-    void MaybeSendFeefilter(CNode& node, Peer& peer, std::chrono::microseconds current_time);
451+    void MaybeSendFeefilter(CNode& node, Peer::TxRelay* tx_relay, std::chrono::microseconds current_time);

ajtowns commented at 9:08 am on March 28, 2022:

Perhaps pass in Peer:TxRelay& and leave it to the caller to check tx_relay != nullptr?

jnewbery commented at 2:56 pm on March 29, 2022:

Reverted change so this passes in a Peer&.

in src/net_processing.cpp:1239 in 2432ec4af8 outdated

1235@@ -1236,7 +1236,7 @@ void PeerManagerImpl::InitializeNode(CNode *pnode)
1236         m_peer_map.emplace_hint(m_peer_map.end(), nodeid, peer);
1237     }
1238     if (!pnode->IsInboundConn()) {
1239-        PushNodeVersion(*pnode, *peer);
1240+        PushNodeVersion(*pnode, peer->m_tx_relay != nullptr);

dergoegge commented at 11:33 am on March 28, 2022:

0        PushNodeVersion(*pnode, peer->m_tx_relay == nullptr);

jnewbery commented at 8:27 pm on March 28, 2022:

Fixed. Thanks!

in src/net_processing.cpp:2678 in 2432ec4af8 outdated

2674@@ -2675,7 +2675,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2675         // Inbound peers send us their version message when they connect.
2676         // We send our version message in response.
2677         if (pfrom.IsInboundConn()) {
2678-            PushNodeVersion(pfrom, *peer);
2679+            PushNodeVersion(pfrom, peer->m_tx_relay != nullptr);

dergoegge commented at 11:33 am on March 28, 2022:

0            PushNodeVersion(pfrom, peer->m_tx_relay == nullptr);

jnewbery commented at 8:27 pm on March 28, 2022:

Fixed. Thanks!

dergoegge changes_requested

dergoegge commented at 11:40 am on March 28, 2022: member

The tests are failing because the wrong value is being passed to PushNodeVersion for the block_relay_only param.

jnewbery force-pushed on Mar 28, 2022

jnewbery commented at 8:27 pm on March 28, 2022: member

The tests are failing because the wrong value is being passed to PushNodeVersion for the block_relay_only param.

Oops. Now fixed. Thank you!

in src/net_processing.cpp:2678 in 08071168b4 outdated

2674@@ -2675,7 +2675,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2675         // Inbound peers send us their version message when they connect.
2676         // We send our version message in response.
2677         if (pfrom.IsInboundConn()) {
2678-            PushNodeVersion(pfrom, *peer);
2679+            PushNodeVersion(pfrom, peer->m_tx_relay == nullptr);

ajtowns commented at 10:50 pm on March 28, 2022:

Duplicating peer->m_tx_relay == nullptr at both call sites rather than just leaving it in the function seems kind of backwards? Also seems demonstrably a bit bug-prone, presumably warranting a /*block_relay_only=*/ at least.

MarcoFalke commented at 9:08 am on March 29, 2022:

Alternatively, pass the tx_relay pointer?

jnewbery commented at 2:56 pm on March 29, 2022:

Reverted change so this passes in a const Peer&

MarcoFalke commented at 9:08 am on March 29, 2022: member

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24474 (Additional thread safety annotations for CNode/Peer by ajtowns)
#22778 (net processing: Reduce resource usage for inbound block-relay-only connections by jnewbery)
#21527 (net_processing: lock clean up by ajtowns)

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.

[net processing] PushNodeVersion() takes a const Peer&

The peer object is not mutated by PushNodeVersion, so pass a const reference

0bca5f2b46

[fuzz] Assert that Peer.m_tx_relay.m_relay_txs has been set correctly a40978dcbd

jnewbery commented at 2:55 pm on March 29, 2022: member

See also suggestion in #21160 (review)

Added.

I’ve removed the net_processing: MaybeSendFeeFilter() takes a Peer::TxFilter* and net_processing: PushNodeVersion() takes a bool instead of a Peer& commits. There doesn’t seem to be agreement on what should be passed down to these functions, and in the absence of agreement, continuing to pass just a reference to the Peer object seems fine. I have however changed the function signature of PushNodeVersion() to take a const reference to Peer. That seems like an uncontroversial improvement.

MarcoFalke commented at 3:17 pm on March 29, 2022: member

I think you forgot to force push your local branch?

jnewbery force-pushed on Mar 29, 2022

MarcoFalke renamed this:
~~[Net Processing] Follow-ups to #21160~~
refactoring: [Net Processing] Follow-ups to #21160
on Mar 29, 2022

MarcoFalke added the label Refactoring on Mar 29, 2022

MarcoFalke commented at 3:28 pm on March 29, 2022: member

review ACK a40978dcbd6608695bc7f5191c4d0a3e48cbca0b

dergoegge commented at 4:08 pm on March 29, 2022: member

ACK a40978dcbd6608695bc7f5191c4d0a3e48cbca0b

ajtowns commented at 3:08 am on March 30, 2022: member

ACK a40978dcbd6608695bc7f5191c4d0a3e48cbca0b

fanquake merged this on Mar 30, 2022

fanquake closed this on Mar 30, 2022

jnewbery deleted the branch on Mar 30, 2022

sidhujag referenced this in commit 7a50d84190 on Apr 3, 2022

DrahtBot locked this on Mar 30, 2023

refactoring: [Net Processing] Follow-ups to #21160 #24692

Conflicts