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 |
|---|---|
| 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
`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.