m_is_inbound cannot be changed throughout the life of a Peer. However, we are currently storing it in CNodeState, which requires locking cs_main in order to access it. This can be moved to the outside scope and only require m_peer_mutex.
This is a refactor in preparation for Erlay reworks.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | marcofleon, naumenkogs, maflcko, achow101 |
| Concept ACK | tdb3 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
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.
Concept ACK. Mind sharing how you see this fitting in with Erlay work?
Will review in more depth soon.
1943 // Exempt HB compact block peers. Manual connections are always protected from discouragement.
1944- if (!via_compact_block && !node_state->m_is_inbound) {
1945- if (peer) Misbehaving(*peer, 100, message);
1946+ if (peer && !via_compact_block && !peer->m_is_inbound) {
1947+ Misbehaving(*peer, 100, message);
1948 return true;
true. Now, it would return false.
You’re right. This was an oversimplification. However, CNodeState and Peer should be tightly coupled, shouldn’t they? Both are wiped in PeerManagerImpl::FinalizeNode (synchronously I assume), so I think there shouldn’t have been a way to trigger that true when peer was a nullptr.
Zooming out a little bit, MaybePunishNodeForBlock return value is never used in the codebase (AFAICT at least), plus the docs also seem to be confusing when true needs to be returned:
if (peer && ... in the other cases as well for consistency, if this is correct.
Will rebase on top of that
Concept ACK. Mind sharing how you see this fitting in with Erlay work?
Will review in more depth soon.
Sure. For Erlay purposes, some transactions need to be sent via fanout even to Erlay-enabled peers. In order to know to how many peers to send this via fanout, we need to count the amount of tx-relaying peers that we have, both inbound and outbound.
This is done, in the current patch, by going over all peers when constructing their corresponding INV message and deciding whether to add the transaction or not (this requires accesing the CNodeState, which involves locking cs_main). In the new revision, this would be done in PeerManagerImpl::RelayTransaction, and two sets will be kept, per peer, one for fanout and one for reconciliation.
TL;DR; checking if a peer is inbound from the PeerManager perspective should not require locking cs_main
222@@ -223,6 +223,9 @@ struct Peer {
223 /** Services this peer offered to us. */
224 std::atomic<ServiceFlags> m_their_services{NODE_NONE};
225
226+ //! Whether this peer is an inbound connection
227+ const bool m_is_inbound;
CNode::m_conn_type. Is it necessary to have a partial copy of that in Peer?
If not, an alternative approach might be to use that, as is done for various other spots in net_processing that call IsInboundConn(). We might also need helper functions similar to GetExtraBlockRelayCount but for inbounds. I haven’t tried it or looked into it in depth, but just wanted to bring up where you’ve considered that option.
We do, at least to my understanding, given some PeerMan methods that are not called with any CNode reference need to check whether the peer that sent us the data they are processing is inbound or outbound.
A good example is PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs, which is called from PeerManagerImpl::BlockChecked and is passed a NodeId grabbed from mapBlockSource
CNode by NodeId from m_nodes, as the current code does it for Peer from m_peer_map.
I think the lookup would be O(N) for iterating through the vector m_nodes instead of O(log N) for the std::map, so maybe that is the reason for the redundancy?
Peer
1374- if (state != nullptr && !state->m_is_inbound) ++num_outbound_hb_peers;
1375+ PeerRef peer{GetPeerRef(*it)};
1376+ if (peer && !peer->m_is_inbound) ++num_outbound_hb_peers;
1377 }
1378- if (nodestate->m_is_inbound) {
1379+ PeerRef peer{GetPeerRef(nodeid)};
PeerRef construction always at the same place as the previous CNodeState construction. This makes this review easier and also follow-up changes that move more state into Peer. Otherwise, this will have to be moved up again in the future anyway.
1675@@ -1676,7 +1676,7 @@ void PeerManagerImpl::InitializeNode(const CNode& node, ServiceFlags our_service
1676 NodeId nodeid = node.GetId();
1677 {
1678 LOCK(cs_main); // For m_node_states
1679- m_node_states.emplace_hint(m_node_states.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(node.IsInboundConn()));
1680+ m_node_states.emplace_hint(m_node_states.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple());
nit: Now that C++17 is available, you can just use the equivalent try_emplace (5) from https://en.cppreference.com/w/cpp/container/map/try_emplace
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index 23f5265ccd..2b9147b590 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -1720,7 +1720,7 @@ void PeerManagerImpl::InitializeNode(const CNode& node, ServiceFlags our_service
5 NodeId nodeid = node.GetId();
6 {
7 LOCK(cs_main); // For m_node_states
8- m_node_states.emplace_hint(m_node_states.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple());
9+ m_node_states.try_emplace(m_node_states.end(), nodeid);
10 }
11 {
12 LOCK(m_tx_download_mutex);
review ACK 129f73cc6e2ed4147f57c3d1dfe2841b8a46e9fe 💼
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 129f73cc6e2ed4147f57c3d1dfe2841b8a46e9fe 💼
33fNrvhGgwV/rlyCL2reeVteCqpRAZlpmoMI8JHAIWCqKWygCm+5M3v3l0MFsTZYLbjBCz0smaZA1gVcmFLWNCA==
`m_is_inbound` cannot be changed throughout the life of a `Peer`. However, we
are currently storing it in `CNodeState`, which requires locking `cs_main` in
order to access it. This can be moved to the outside scope and only require
`m_peer_mutex`.
This is a refactor in preparation for Erlay reworks.
ACK 07f4cebe5780f1039541d989e64b70eccc5b4eb5
I took a look at c308e3200d6968be553ed031d09b4bccb46b504b and 2e0b6742b82e60ea685afd25f2d19b8b558678ce in #30116 (assuming those commits are mostly final) to understand a bit better how m_is_inbound will be used for Erlay. Based on that, I’d say the changes in this PR make sense.
1719@@ -1720,7 +1720,7 @@ void PeerManagerImpl::InitializeNode(const CNode& node, ServiceFlags our_service
1720 NodeId nodeid = node.GetId();
1721 {
1722 LOCK(cs_main); // For m_node_states
1723- m_node_states.emplace_hint(m_node_states.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(node.IsInboundConn()));
1724+ m_node_states.try_emplace(m_node_states.end(), nodeid);
m_peer_map either, so I’d lean towards no for consistency, but I don’t have a strong opinion about it
FWIW I tried adding an assert here while reviewing this pull and found it unexpectedly difficult. Happy to review if someone does it.
As a sanity check verified both unit and functional tests appear to fail if this line is changed or omitted.
ACK 07f4cebe5780f1039541d989e64b70eccc5b4eb5 🗞
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 07f4cebe5780f1039541d989e64b70eccc5b4eb5 🗞
3vc3UpVVyQ+RrINm3XUgOY7h0eDX56YxXxoCxlgTWq0GWjKWtW1knsO2+FfWuSpoj41wZ7jBq6X9Tfpn3fXFCCw==
