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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
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;
Can you explain why this change in behavior is correct? Previously, if the peer was missing, it would return 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 the return value is wrong or unused, it could make sense to remove it? Also, it could make sense to update the pattern if (peer && ... in the other cases as well for consistency, if this is correct.
Will rebase on top of that
Rebased
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;
The primary source of that information is 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
Yes, but we could attempt to retrieve the 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?
Yeah, that'd make sense. I don't think increasing the lookup complexity for this, even if we are talking about a vector of hundreds of nodes at most, is worth not keeping a boolean in Peer
Rebased on top of master to cover #30233 (review), which was addressed by #29575
Rebased to deal with merge conflicts
ACK 129f73cc6e2ed4147f57c3d1dfe2841b8a46e9fe
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)};
nit: It would be nice to keep the 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
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 23f5265ccd..2b9147b590 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1720,7 +1720,7 @@ void PeerManagerImpl::InitializeNode(const CNode& node, ServiceFlags our_service
NodeId nodeid = node.GetId();
{
LOCK(cs_main); // For m_node_states
- m_node_states.emplace_hint(m_node_states.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple());
+ m_node_states.try_emplace(m_node_states.end(), nodeid);
}
{
LOCK(m_tx_download_mutex);
review ACK 129f73cc6e2ed4147f57c3d1dfe2841b8a46e9fe 💼
<details><summary>Show signature</summary>
Signature:
untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 129f73cc6e2ed4147f57c3d1dfe2841b8a46e9fe 💼
3fNrvhGgwV/rlyCL2reeVteCqpRAZlpmoMI8JHAIWCqKWygCm+5M3v3l0MFsTZYLbjBCz0smaZA1gVcmFLWNCA==
</details>
`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);
nit should we assert the result of this code instead of dropping it on the floor?
We were not checking it before, nor are we doing so later when emplacing it in 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
ACK 07f4cebe5780f1039541d989e64b70eccc5b4eb5 🗞
<details><summary>Show signature</summary>
Signature:
untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 07f4cebe5780f1039541d989e64b70eccc5b4eb5 🗞
vc3UpVVyQ+RrINm3XUgOY7h0eDX56YxXxoCxlgTWq0GWjKWtW1knsO2+FfWuSpoj41wZ7jBq6X9Tfpn3fXFCCw==
</details>
ACK 07f4cebe5780f1039541d989e64b70eccc5b4eb5
Post-merge ACK 07f4cebe5780f1039541d989e64b70eccc5b4eb5