net processing: Move peer_map to PeerManager #19910

jnewbery · 2020-09-07T17:17:27Z

This moves g_peer_map from a global in net_processing.cpp's unnamed namespace to being a member m_peer_map of PeerManager.

jnewbery · 2020-09-07T17:18:04Z

Requested by @MarcoFalke @sdaftuar and @theuni . Review very much appreciated 🙏

maflcko

Concept ACK

maflcko · 2020-09-07T17:25:02Z

src/rpc/net.cpp

@@ -156,7 +156,10 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
    for (const CNodeStats& stats : vstats) {
        UniValue obj(UniValue::VOBJ);
        CNodeStateStats statestats;
-        bool fStateStats = GetNodeStateStats(stats.nodeid, statestats);
+        bool fStateStats{false};
+        if (node.peerman) {


It is currently not possible to not have node.peerman. I think an Ensure..() like EnsureMempool would be good. It doesn't really make sense to call getpeerinfo without a peerman anyway?

I've done something slightly different and checked existence at the top of this function. Let me know what you think.

src/net_processing.h

hebasto · 2020-09-07T18:34:00Z

Concept ACK.

practicalswift · 2020-09-07T19:23:42Z

Concept ACK: decoupling is good!

hebasto

ACK 322e87b, I have reviewed the code and it looks OK.

src/net_processing.h

hebasto

re-ACK d350feb

ariard

Concept ACK

src/interfaces/node.cpp

src/net_processing.cpp

promag · 2020-09-09T08:14:08Z

Concept ACK.

DrahtBot · 2020-09-19T13:46:28Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpc: getblockfrompeer #20295 (rpc: getblockfrompeer by Sjors)
net: Remove g_relay_txes #20217 (net: Remove g_relay_txes by jnewbery)
Periodically make block-relay connections and sync headers #19858 (Periodically make block-relay connections and sync headers by sdaftuar)
net processing: Move block inventory state to net_processing #19829 (net processing: Move block inventory state to net_processing by jnewbery)

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.

src/net_processing.cpp

jnewbery · 2020-12-01T11:54:30Z

rebased

hebasto

re-ACK aedd418, added two last commits since my previous review.

Still required rebasing due to the conflict with #20530.

src/net_processing.h

hebasto · 2020-12-03T10:41:08Z

src/net_processing.cpp

+    PeerRef ret;
    LOCK(m_peer_mutex);
    auto it = m_peer_map.find(id);
-    return it != m_peer_map.end() ? it->second : nullptr;
+    if (it != m_peer_map.end()) ret = it->second;
+    return ret;


6bfcef3

Why are these changes required? What are benefits to return an empty shared pointer instead nullptr-constructed one? Btw, in all caller places the return value is checked for nullptr, not being empty.

This makes is clear that NRVO can be used, since there's only one return statement. I'm not sure if the previous version would have used copy elision.

+1 for considering this!

I'd be curious to know if compilers are allowed to rearrange the code before deciding how many return statements there are. I could see this going either way.

Considering the before-change code, to be precise, in C++17 in a return statement, when the operand is a prvalue (the result of the ternary conditional expression), elision of copy/move operations is mandatory.

OTOH, NRVO is non-mandatory.

Thanks @hebasto! Yes, I think you're right. If I'm reading it correctly, the result of the conditional operator is a prvalue (item 5 in https://en.cppreference.com/w/cpp/language/operator_other#Conditional_operator since 4 doesn't apply), and copy elision is mandatory for returning a prvalue.

I've reverted this.

... and copy elision is mandatory for returning a prvalue.

Good to have C++17 :)

dongcarl · 2020-12-03T16:39:38Z

Code Review ACK aedd418

Private members >> Anon-namespaced statics/functions

Especially when the functionality is strongly intertwined with the class of which it's being made a member.

jnewbery · 2020-12-04T11:55:01Z

Rebased and addressed outstanding review comments.

promag

Core review ACK 4fe77ae.

Could squash last commit or fix the comment in 5a1f08c.

src/net_processing.cpp

theuni

ACK 4fe77ae or squashed as @promag suggested.

src/net_processing.cpp

hebasto

re-ACK 4fe77ae, only rebased since my previous review and added a commit to fix comments.

This allows us to avoid repeated locking in FinalizeNode()

jnewbery · 2020-12-07T12:12:04Z

Thanks for the review @hebasto @promag @theuni and @dongcarl. I've addressed all comments, so this should now be ready for re-review.

hebasto · 2020-12-07T12:15:18Z

src/net_processing.cpp

+PeerRef PeerManager::RemovePeer(NodeId id)
+{
+    PeerRef ret;
+    LOCK(m_peer_mutex);
+    auto it = m_peer_map.find(id);
+    if (it != m_peer_map.end()) {
+        ret = std::move(it->second);
+        m_peer_map.erase(it);
+    }
+    return ret;
+}


3025ca9
I'm curios is it possible to force the mandatory copy/move elision for the return value here as well?

hebasto

re-ACK 3025ca9, since my previous review only reverted the change that introduced NRVO in PeerManager::GetPeerRef, and comments are fixed in the proper commits.

theuni · 2020-12-08T19:23:55Z

Re-ACK 3025ca9.

dongcarl · 2020-12-09T13:11:18Z

Re-ACK 3025ca9

Disambiguated between nullptr and empty shared_ptr
Reverted refactoring in PeerManager::GetPeerRef
Made Peer constructor explicit

rebroad · 2021-09-15T08:00:08Z

src/net_processing.cpp

@@ -842,11 +790,9 @@ void PeerManager::FinalizeNode(const CNode& node, bool& fUpdateConnectionTime) {
    LOCK(cs_main);
    int misbehavior{0};
    {
-        PeerRef peer = GetPeerRef(nodeid);
+        PeerRef peer = RemovePeer(nodeid);
        assert(peer != nullptr);


I'm getting bitcoind crashing due to this assert. It appears that sometimes FinalizeNode gets called twice (from DeleteNode()). Perhaps nRefCount is being increased between the two occurances. This only seems to happen though when there's a delay between them due to UpdateTip(). It's probably a bug I've introduced somewhere, but just leaving this comment as a "heads up".

Thanks. I expect if the bug was present in master we'd have seen it in CI or from a user report.

jnewbery mentioned this pull request Sep 7, 2020

[p2p] Add Peer struct for per-peer data in net processing #19607

Merged

maflcko reviewed Sep 7, 2020

View reviewed changes

DrahtBot added P2P RPC/REST/ZMQ labels Sep 7, 2020

jnewbery force-pushed the 2020-08-peer-plv2 branch 2 times, most recently from e4488c3 to 322e87b Compare September 7, 2020 18:09

maflcko added Refactoring and removed P2P RPC/REST/ZMQ labels Sep 7, 2020

hebasto requested changes Sep 8, 2020

View reviewed changes

src/net_processing.h Outdated Show resolved Hide resolved

src/net_processing.h Show resolved Hide resolved

jnewbery force-pushed the 2020-08-peer-plv2 branch from 322e87b to d350feb Compare September 8, 2020 10:33

hebasto approved these changes Sep 8, 2020

View reviewed changes

ariard reviewed Sep 8, 2020

View reviewed changes

src/interfaces/node.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Sep 19, 2020

net: guard vRecvGetData with cs_vRecv and orphan_work_set with g_cs_orphans #19911

Merged

This was referenced Sep 19, 2020

Periodically make block-relay connections and sync headers #19858

Merged

net processing: Move block inventory state to net_processing #19829

Merged

theuni reviewed Sep 22, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

jnewbery force-pushed the 2020-08-peer-plv2 branch from b28f866 to 658ca70 Compare September 24, 2020 10:36

theuni reviewed Sep 24, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

jnewbery force-pushed the 2020-08-peer-plv2 branch from 658ca70 to 1b25b5c Compare September 25, 2020 08:01

This was referenced Sep 29, 2020

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

Multiprocess bitcoin #10102

Draft

jnewbery force-pushed the 2020-08-peer-plv2 branch from 1b25b5c to 2ce2ec1 Compare September 29, 2020 09:31

jnewbery added the P2P label Oct 1, 2020

DrahtBot mentioned this pull request Nov 30, 2020

lint, refactor: Update cppcheck linter to c++17 and improve explicit usage #20530

Merged

DrahtBot added the Needs rebase label Dec 1, 2020

jnewbery force-pushed the 2020-08-peer-plv2 branch from 3bf5917 to aedd418 Compare December 1, 2020 11:54

DrahtBot added Needs rebase and removed Needs rebase labels Dec 1, 2020

hebasto approved these changes Dec 3, 2020

View reviewed changes

[net processing] Move GetNodeStateStats into PeerManager

a529fd3

jnewbery force-pushed the 2020-08-peer-plv2 branch from aedd418 to 5a1f08c Compare December 4, 2020 11:47

promag reviewed Dec 4, 2020

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

DrahtBot removed the Needs rebase label Dec 4, 2020

theuni approved these changes Dec 4, 2020

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

hebasto approved these changes Dec 5, 2020

View reviewed changes

jnewbery added 3 commits December 7, 2020 11:55

[net_processing] Move peer_map to PeerManager

ed7e469

[net processing] Make GetPeerRef const

a20ab22

[net processing] Add RemovePeer()

3025ca9

This allows us to avoid repeated locking in FinalizeNode()

jnewbery force-pushed the 2020-08-peer-plv2 branch from 4fe77ae to 3025ca9 Compare December 7, 2020 12:00

hebasto reviewed Dec 7, 2020

View reviewed changes

hebasto approved these changes Dec 7, 2020

View reviewed changes

fanquake merged commit 795afe6 into bitcoin:master Dec 9, 2020

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 9, 2020

Merge bitcoin#19910: net processing: Move peer_map to PeerManager

c27878a

jnewbery mentioned this pull request Dec 14, 2020

Move remaining application layer data to net processing #19398

Closed

16 tasks

rebroad reviewed Sep 15, 2021

View reviewed changes

jnewbery deleted the 2020-08-peer-plv2 branch September 15, 2021 10:02

bitcoin locked and limited conversation to collaborators Sep 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net processing: Move peer_map to PeerManager #19910

net processing: Move peer_map to PeerManager #19910

jnewbery commented Sep 7, 2020

jnewbery commented Sep 7, 2020

maflcko left a comment

maflcko Sep 7, 2020

jnewbery Sep 7, 2020

hebasto commented Sep 7, 2020

practicalswift commented Sep 7, 2020

hebasto left a comment

hebasto left a comment

ariard left a comment

promag commented Sep 9, 2020

DrahtBot commented Sep 19, 2020 •

edited

Loading

jnewbery commented Dec 1, 2020

hebasto left a comment

hebasto Dec 3, 2020

jnewbery Dec 4, 2020

theuni Dec 4, 2020

hebasto Dec 5, 2020 •

edited

Loading

jnewbery Dec 7, 2020

hebasto Dec 7, 2020

dongcarl commented Dec 3, 2020

jnewbery commented Dec 4, 2020

promag left a comment

theuni left a comment

hebasto left a comment •

edited

Loading

jnewbery commented Dec 7, 2020

hebasto Dec 7, 2020

hebasto left a comment

theuni commented Dec 8, 2020

dongcarl commented Dec 9, 2020

rebroad Sep 15, 2021

jnewbery Sep 15, 2021

net processing: Move peer_map to PeerManager #19910

net processing: Move peer_map to PeerManager #19910

Conversation

jnewbery commented Sep 7, 2020

jnewbery commented Sep 7, 2020

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto commented Sep 7, 2020

practicalswift commented Sep 7, 2020

hebasto left a comment

Choose a reason for hiding this comment

hebasto left a comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

promag commented Sep 9, 2020

DrahtBot commented Sep 19, 2020 • edited Loading

Conflicts

jnewbery commented Dec 1, 2020

hebasto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto Dec 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongcarl commented Dec 3, 2020

jnewbery commented Dec 4, 2020

promag left a comment

Choose a reason for hiding this comment

theuni left a comment

Choose a reason for hiding this comment

hebasto left a comment • edited Loading

Choose a reason for hiding this comment

jnewbery commented Dec 7, 2020

Choose a reason for hiding this comment

hebasto left a comment

Choose a reason for hiding this comment

theuni commented Dec 8, 2020

dongcarl commented Dec 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrahtBot commented Sep 19, 2020 •

edited

Loading

hebasto Dec 5, 2020 •

edited

Loading

hebasto left a comment •

edited

Loading