net processing: Move peer_map to PeerManager #19910

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.

    bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats);

nit for new code

fixed. Thanks!

322e87b6039877ed3457baea93cc87d78f3f4d96

     * by the m_peer_mutex. Once a shared pointer reference is

good catch. Fixed.

322e87b6039877ed3457baea93cc87d78f3f4d96 nit: This comment duplicates the code :) Consider to drop it.

Normally I'd agree with removing unnecessary commenting, but I think it's useful for every member variable to have a doxygen comment. Happy to go either way if others think it should be dropped.

I'm for keeping it, it's better for consistency to have a comment for every one.

Does it change something if this check is moved above at the m_context->connman level ? You may save one more control flow.

Does it change something if this check is moved above at the m_context->connman level ? You may save one more control flow.

Why increase code block with locked ::cs_main?

Does it change something if this check is moved above at the m_context->connman level ? You may save one more control flow.

Yes, it's a try lock. It's possible that we're not able to take the lock, and therefore that we're not able to get the node state stats, but we'd still get the node stats. If we move the try above the m_context->connman level, then if we couldn't get the cs_main lock, we wouldn't be able to get any stats.

Eventually, none of the statistics should require cs_main, so the try lock can be removed entirely.

It's a bit confusing to have GetNodeStateStats which is actually a member of PeerManager, and still have a CNodeState, even if inside this function you effectively fetch it. Maybe rename GetNodeProcessingStats ?

I think the name right now is fine. It's getting stats related to the node's state.

Using GetPeerRef() with an encapsulated lock for m_peer_map, then manually locking the private mutex 3 lines later looks strange. It also requires two locks... all the drawbacks of mutex encapsulation without exploiting the benefits :)

Since misbehavior changes to the peer will go unobserved after this point anyway, why not just take and remove with a single lock?

Something like (untested):

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;
}

Yes, that's much better. Thank you!

I've implemented this within GetPeerRef() to avoid the code duplication, and also added another commit that tidies up FinalizeNode() a bit more. Let me know what you think.

EDIT: The comment below is wrong. I introduced a different bug when implementing this as Cory points out below.

~oops. There's a bug in the code above which caused this to fail. You can't move the shared_ptr out of the map and then erase it. More precisely, I think something like the following is happening:~

• ~ret = std::move(it->second); means ret is reset to be a shared pointer to Peer. It steals the refcount from it->second, since std::move(it->second) is an rvalue. std::move doesn't do anything to the map entry.~
• ~the entry is then erased from the map, decrementing the refcount to zero and destructing the Peer object.~

~Removing the std::move fixed this.~

This is really uninteresting minutia, but @jnewbery and I happened to get into a discussion/debate about it today so I'm following up with details. Everyone else can safely ignore this as boring.

I don't believe the comment above is correct. If removing the std::move helps, I'm wondering if there might be another issue causing problems.

• ret = std::move(it->second); means ret is reset to be a shared pointer to Peer. It steals the refcount from it->second, since std::move(it->second) is an rvalue. std::move doesn't do anything to the map entry.

Indeed std::move doesn't do anything to the map entry as it's essentially just a cast, but the operator= does likely (definitely?) reset it. See libc++'s implementation of shared_ptr's move assignment operator, which calls the move constructor, where the rhs is null'd.

• the entry is then erased from the map, decrementing the refcount to zero and destructing the Peer object.

We can write a quick test to see that the refcount never hits zero after removal from the map, that copy vs. move refcounts are different as we would expect them to be, and that a shared_ptr moved from a map's value is still safe to use:

#include <cassert>
#include <cstdio>
#include <map>
#include <memory>
#include <string>

static std::shared_ptr<std::string> copytest()
{
    std::map<char, std::shared_ptr<std::string> > foo;
    foo.emplace('c', std::make_shared<std::string>("hello"));
    auto it = foo.find('c');
    std::shared_ptr<std::string> ret;
    if (it != foo.end()) {
        assert(it->second.use_count() == 1);
        ret = it->second;
        assert(it->second != nullptr); // <------------------
        assert(ret.use_count() == 2);  // <------------------
        foo.erase(it);
        assert(ret.use_count() == 1);
    }
    return ret;
}

static std::shared_ptr<std::string> movetest()
{
    std::map<char, std::shared_ptr<std::string> > foo;
    foo.emplace('c', std::make_shared<std::string>("world"));
    auto it = foo.find('c');
    std::shared_ptr<std::string> ret;
    if (it != foo.end()) {
        assert(it->second.use_count() == 1);
        ret = std::move(it->second);
        assert(it->second == nullptr); // <------------------
        assert(ret.use_count() == 1);  // <------------------
        foo.erase(it);
        assert(ret.use_count() == 1);
    }
    return ret;
}

int main()
{
    auto copyptr = copytest();
    assert(copyptr.use_count() == 1);
    auto moveptr = movetest();
    assert(moveptr.use_count() == 1);

    printf("%s %s!\n", copyptr->c_str(), moveptr->c_str());
}

Note that this doesn't actually prove that the map itself is still in working order after the move+removal, but I don't see why that should break anything.

Lastly, we have cases in our code where we move from a map's value. One example: https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1012

tl;dr: If moving a shared_ptr out of a mapped value causes a problem that a copy doesn't, we should make sure to understand why and potentially audit other places where we do it as well.

Aha, I think I found the problem. From your previous version of this commit:

...
    if (it != m_peer_map.end()) {
        ret = std::move(it->second);
        if (erase) m_peer_map.erase(it);
    }
    return ret;

Moving only makes sense when erasing.

Indeed, the failure was caused by that. Sorry for the noise (and for casting aspersions on your code!)

This undoes this assumption: #19607 (review) , which I'm still not convinced is a good idea.

ok, removed that commit

This is (mostly) a style preference, but I'm not a fan of adding a boolean option to substantially change the functionality of a function as opposed to just creating two functions. GetPeerRef() is logically a read-only operation.

It's also easier easy to slip up and make mistakes like this one: #19910 (review) ;)

You got me. I've made it a separate function.

GetPeerRef() is logically a read-only operation.

Good point. I've made it const (and made the mutex mutable)

aedd418fed7abfd00cc731bd57ed5b013259eb5a

To be precise, it may return an empty shared pointer rather nullptr.

fixed

6bfcef3797d4d105180e325033929684f84cfc08

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 :)

This also avoids duplicate lookup on m_peer_map :+1:

This is a little awkward because of the comment in RemovePeer:

/** Get a shared pointer to the Peer object and remove it from m_peer_map.
 *  May return an empty shared_ptr if the Peer object can't be found. */

Might be worth of a comment explaining that at this point the peer is guaranteed to exist, but not afterwards. Then again, that's kinda what the assert means. Shrug.

Yeah, I think it's ok for now. Happy to change this if you decide you feel more strongly about it.

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.

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