Net: Move ping data to net_processing #20721

If you're changing every reference to pto anyway because it's now a reference not a pointer, maybe also call it node_to or similar, since it's not a "p" anymore?

Sure. I'll split the commit into a move and a rename/change to reference to make it easier to review.

Done

I don't think exposing more implementation details in net_processing.h is a good idea. Fortunately it's fixable: #20758

Totally agree! I think #20758 can happen before or after this PR.

Yes, I don't think either need gate the other, the above was literally my thought process in getting to that PR -- sipa may not have nerd sniped me with twitter maths, but that doesn't mean I didn't get nerd sniped anyway...

This seems like a more productive form of aj-sniping.

I like this kind of aj-sniping.

Move the test change to its own commit so it's easy to verify behaviour doesn't change with the move logic to net_processing commit? I don't follow why the test change is necessary/desirable though.

Move the test change to its own commit so it's easy to verify behaviour doesn't change

good idea. Will do.

I don't follow why the test change is necessary/desirable though.

The change is required because previously ping timeouts happened in in CConnman, so it was fine to bump the time in this test because there's no CConnman to time out the connection. When the ping data and timout logic is moved to PeerManager, then the connection times out in this test if you bump the time forwards by 21 minutes without resetting the ping nonce.

Makes sense; might be a useful commit message for the test change?

Seems kind-of annoying to have to remember to constrain your mocktime bumps by random ping timeout limits. Shouldn't this also (already) cause problems in functional tests?

The functional tests have an asyncio event loop thread that will automatically respond to ping messages:

https://github.com/bitcoin/bitcoin/blob/f656165e9c0d09e654efabd56e6581638e35c26c/test/functional/test_framework/p2p.py#L392-L393

The unit tests are single threaded, so any logic to handle messages needs to be done in the main test logic.

ok, split this into its own commit.

Noob / practices question: why not just create this with a bool return type, since you change it 3 commits later? I don't have an opinion, but why is it better to do it this way?

Just so that the commits are hygienic. For people reviewing commit by commit, it wouldn't make sense to have a function that returns a bool which is just ignored. Changing the return type in a subsequent commit is as easy as changing void to bool in the declaration and definition.

I was considering not returning a bool here, since it's a bit of a strange interface (the function sets pto.fDisconnect and returns false. Perhaps it should stay as returning void, and have SendMessages() check for fDisconnect after calling this function?

Genuine question, why should best ping time be in CNode but all the others in Peer? We use best ping time for eviction logic which seems like an application-layer thing...

Answered here: #20721 (comment)

Since you removed this, CNodeStats doesn't know min ping time anymore. The p2p_eviction and p2p_ping functional tests fail since getpeerinfo() always returns 0 for minping :(

Adding it back fixed the tests for me:

stats.m_min_ping_usec  = m_best_ping_time;

Oh hey, if s/m_min_ping_usec/m_best_ping_time then

X(m_best_ping_time);

(wow so clean)

Good catch! I lost this in a rebase. I've now restored it.

I'm going to leave the m_min_ping_usec name to avoid increasing the diff further.

    std::atomic<int64_t> m_min_ping_time{std::numeric_limits<int64_t>::max()};

I think renaming from "min" to "best" needlessly sows confusion or more needed code churn for no gain:

• "min" is more precise than "best" (best how?)
• getpeerinfo and its help, -netinfo and its new help (#20764), and the GUI refer to "min ping", "Min Ping", "mping", "minimum observed ping time". Updating all of them would create unneeded churn; not updating them would create unnecessary confusion.

I agree. I've changed it to m_min_ping_time (it's still code churn since it was previously named nMinPingUsecTime, but I agree with you about the consistent naming).

Can you use named casts in lines 234 and 237 while touching this code?

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#Res-casts-named

I'd rather not change this while moving it. There's another C-style cast from int64_t to double on line 206. Let's fix them all at the same time.

nit: docs specify minimum is 1, but it seems here min is 0?

This error condition is hit if pingtimeout is set to 0.

Are cases where the response is malformed (e.g. user inputs a string instead of a number) handled elsewhere?

Argument handling is in ArgsManager::GetArg(). Since the second argument here is an int, the GetArg(string, int) version is called:

https://github.com/bitcoin/bitcoin/blob/1e69800d5e60c7805931f2c5b978720c6aee180b/src/util/system.cpp#L473-L477

which will try to convert the string into an int.

This might result in a slightly inaccurate error message, but I think it's fine. It's the same behaviour as for -peertimeout:

→ ./src/bitcoind --pingtimeout=abc
Error: pingtimeout cannot be configured with a negative value.

Are the inner sets of parenthesis required or is this a code-style thing?

No, these aren't required. In C++ && has a lower precedence than != so the parens aren't changing the order of operations (https://en.cppreference.com/w/cpp/language/operator_precedence). They're not doing any harm either.

This code is moved unaltered in this PR. I think it's fine to keep it as it is.

    // Don't let pings time out due to mocktime

Sounds good. Added!

(Rookie question) Would the previous commit(9ad48f3965a32a6eba67ba49da19499fbe9b89e3) change anything with respect to mocktime unless -pingtimeout is used? If not, why did some tests start to fail due to ping timeouts?

Previously, when ping timeouts were handled in CConnman::InactivityCheck(), the whole logic was gated on the following guard:

    if (now <= node.nTimeConnected + m_peer_connect_timeout) {
        // Only run inactivity checks if the peer has been connected longer
        // than m_peer_connect_timeout.
        return false;
    }

Because tests generally run for less than m_peer_connect_timeout, the ping timeout logic was not hit at all. Once the logic moves to net_processing, that guard no longer exists, so we start testing for ping timeouts as soon as the peer is connected.

    /** Send a ping message every PING_INTERVAL or if requested via rpc. Disconnect
     * the peer if a ping has timed out. Return true upon disconnection. */
    [[nodiscard]] bool ManagePing(CNode& node_to);

I've changed the signature to return a void, so the nodiscard tag is no longer required (good suggestion though!)

I've taken the suggested comment changes.

Nit: Is it a bit pedantic to call it a nonpositive value here to include zero?

Although I see it's consistent with -peertimeout error so that's probably why :)

I've changed to option to be a binary true/false, so this check is no longer required.

So would -pingtimeout means timeout=true? And -nopingtimeout is the same thing as -pingtimeout=0? I couldn't find where the implicit "no" logic was :(

I think it's done in InterpretOption() invoked from ArgsManager::ParseParameters() which is invoked in AppInit().

Thanks @dhruv!

ahhhh thank you @dhruv!

I appreciate the added documentation 🙏

nit

    /** We have completed a successful ping-pong keepalive. Update latest and best ping times. */

:facepalm: I've fixed the doxygen style and also tweaked the comment wording a bit.

just noting that this changed from >= to >. makes sense to call it a "timing mishap" if the pong was received in the same microsecond as we recorded for the ping going out, so seems fine. just highlighting the change since it's easy to miss.

Oops, this was unintentional. You're right that it shouldn't have any impact, but I'm going to revert it to how it was. Good catch!

tangential because you didn't change this logic, but what's the point of sending a ping message to peers who are too old to support / respond with a pong?

pong messages were introduced in 2012 with p2p version 60000. I imagine that prior to that, the reason for a ping message was simply a keepalive (make sure that TCP connections stay up, NAT pinholes stay open, etc). This logic maintains backwards compatibility with clients older than 60000.

In practice, I expect there are almost no clients older than 60000, but if there are any it'd be a shame to arbitrarily disconnect them for failing ping timeouts.

isn't this more of a PongReceived?

Yes. Much better. Fixed

this comment previously followed the fSuccesfullyConnected check, which I think(?) was the intent -> fSuccesfullyConnected is set when processing the VERACK message, and m_greatest_common_version gets set when processing the VERSION message.

but now this comment is below MaybeSendPing, which also might retrieve m_greatest_common_version to construct the ping message. I find the intent of this comment to be less clear after these changes.

This is a comment on the line below, which is contructing a CNetMsgMaker object with the correct serialization version:

const CNetMsgMaker msgMaker(pto->GetCommonVersion());

I've moved this back to immediately after the fSuccessfullyConnected check. Hopefully that's clearer.

I know you only moved this code, but I think we could handle fDisconnect better.

This comment seems misleading since I think it only refers to fSuccesfullyConnected. Then, we might set fDisconnect to true in MaybeSendPing, so we check this bool again a couple lines down to also return early.

A simpler logic flow might be removing this check here, starting MaybeSendPing with return if node_to.fDisconnect, and then removing this first early return from SendMessages. And the comment on 4377 could be updated to something along the lines of Don't send messages to peers that are marked for disconnection

That seems like a reasonable change, but I'm not sure it needs to happen in this PR. Very happy to review a PR that moves this around if you think it'd make it clearer.

    /** Send a ping message every PING_INTERVAL or if requested via RPC. May  
     *  mark the peer to be disconnected if a ping has timed out. */

Done!

another comment on code that you simply moved-

I found this definition odd. Why is it defined as an int but set as a boolean? Also, why is it an int64_t, that seems like more space needed than for a simple bool value?

also note that this could be constexpr, but I don't know if that actually impacts anything. I tried it out in godbolt explorer and looks like for such simple logic, it compiled down to the same assembly regardless 🤷🏽‍♀️

No, I added this code. It's an int because originally the config option was to set how long the timeout. I later changed the config option to be binary on/off (https://github.com/bitcoin/bitcoin/pull/20721#issuecomment-773227475), but failed to change the type to bool. Will fix.

Recommend if (x < y) rather than if (y > x) generally -- smaller numbers generally go to the left of larger numbers, so keeping that convention is one less thing to have to think about. (This patch changes the tests from timeout < GetTime() to now > timeout)

I find that "now < timeout" or "now > timeout" is much more natural to parse and understand - the thing that we're measuring (the time now) is on the left, and the thing that it's being measured against (the timeout) is on the right. That's how most people speak ("Is it after 10 o'clock?"), and the other way is a bit yoda-esque ("was 10 o'clock before now?"). Ultimately, I think this is a matter of taste.

If you write "if (10 o'clock < now)" you've literally put now after 10 o'clock and are asking if that's a true statement. I do get what you're saying; I just find the always less-than strategy lets you read the ordering much more directly in general -- which one comes first is in exactly the order you're reading, and not dictated by a single bit of punctuation. It's also helpful for bounds checking (low_value < x && x < high_value) and (x < low_value || high_value < x) read much clearer than (x > low_value && x < high_value) and (x < low_value || x > high_value).

Would probably be better to set m_pingtimeout = gArgs.GetArg() in the constructor, rather than re-query it every loop.

I think losing the check that GetSystemTimeInSeconds() <= node_to.nTimeConnected + peer_connect_timeout (which is done in InactivityCheck but not MaybeSendPing) is a mistake -- the docs for -peertimeout say "Specify a p2p connection timeout delay in seconds. After connecting to a peer, wait this amount of time before considering disconnection based on inactivity" which seems like it should apply here: not sending a ping reply certainly seems like a form of inactivity to me! (Setting m_peer_connect_timeout from gArgs in the PeerManagerImpl constructor seems like it would work here too, without violating layering)

Having -peertimeout continue to apply to (a lack of) PONG messages seems to make -pingtimeout unnecessary; the tests don't need to be modified anymore, and I'm not seeing any reasons other than initial mocktime confusion as to why you'd want to prevent disconnections?

I've removed pingtimeout as requested.

I think it might make more sense in general to set -peertimeout=9999 for all the test cases (ie, in the framework generated bitcoin.conf) -- or at least all the test cases that use mocktime -- and override it anywhere that's explicitly testing disconnection due to timeouts (p2p_ping.py).

I think it might make more sense in general to set -peertimeout=9999 for all the test cases (ie, in the framework generated bitcoin.conf)

Changing the behaviour of all tests is outside the scope of this PR, but I'm happy to review any proposal you have for making the tests more robust.

If I'm following the logic here right, then if we backdate mocktime after sending a PING, we'll never send a PING to that node again. I'm not seeing an obvious problem with that (and don't think it's a change in this PR), but it's somewhat surprising.

That seems right to me. As you say, that's not changed by this PR.

If you're doing disconnects here as well, doesn't that make it MaybeSendPingOrDisconnectDueToLackOfPong() ? Not sure the Maybe is useful -- SendMessages doesn't have a Maybe despite it not necessarily doing something every time it's called.

This moves ping disconnections from ThreadSocketHandler (which calls SocketHandler which calls InactivityCheck, I think on a 50ms cycle) to ThreadMessageHandler (which calls SendMessages which calls MaybeSendPing, I think on a 100ms cycle, though will reinvoke SendMessages more frequently if any node's ProcessMessages indicates there's more work). I think that's fine.

make it MaybeSendPingOrDisconnectDueToLackOfPong()

I don't think the function name needs to document all the functionality. It's documented in the function comment:

    /** Send a ping message every PING_INTERVAL or if requested via RPC. May
     *  mark the peer to be disconnected if a ping has timed out. */

Not sure the Maybe is useful -- SendMessages doesn't have a Maybe despite it not necessarily doing something every time it's called.

Right, and MaybeDiscourageAndDisconnect() does have maybe in it.

I don't really care what this function is called. If you feel strongly, I can change it.

"MaybeSendPing" just kinda bugs me as a name. I'd suggest "HandleBIP31PingPong()" or something, but that would bug me a bit too. Maybe some day it'll be possible to just register a PingPongMessages class that deals with both the send and receive side :pray:

Space after "if" if you're changing punctuation?

Done

I think ping_time.count == 0 is expected if you do setmocktime X, then a ping goes out, and the pong is received before you call setmocktime Y.

Leaving this so as not to change behaviour

(This was meant to be a reply to #20721 (review) except github wasn't let me comment there)

think this could just be..

        peer.m_ping_start = now;

Done!

I'd prefer if this was a chrono duration, but I understand the context of keeping it consistent with net.

I've now removed this again. Sorry :grimacing:

Adding:

    /** Return true if inactivity checks should be ignored */
    inline bool IgnoreInactivityChecks(const CNode& node) const
    {
        return GetSystemTimeInSeconds() <= node.nTimeConnected + m_peer_connect_timeout;
    }

to CConnman would let you avoid adding m_peer_connect_timeout in PeerManagerImpl and keep the non-mockable times in net.cpp. (You have access to connman anyway via PeerManagerImpl::m_connman and access to node in order to set the disconnect flag)

Done

Should there be a if (peer == nullptr) return true; here? We check that everywhere else.

Rather than move this line, might be better to continue doing it first thing and pass the result into MaybeDiscourageAndDisconnect rather than having that function look it up too. (At least, that's where I'm expecting to end up with #20758 eventually)

Done

Question about 6a03a1f7e4: Is the idea that, in the future, MaybeDiscourageAndDisconnect() will only need to accept a Peer? And right now, even though you could grab the PeerRef from the m_peer_map in here again, passing in a Peer is a bit less overhead?

MaybeDiscourageAndDisconnect should continue needing a CNode in order to signal the disconnection, I think.

MaybeDiscourageAndDisconnect should continue needing a CNode in order to signal the disconnection, I think.

This is true. It's also currently needed for the connection type, which I don't think belongs in net_processing.

Even though you could grab the PeerRef from the m_peer_map in here again, passing in a Peer is a bit less overhead?

We want to grab a PeerRef at the entry functions (ProcessMessages and SendMessages for the message handler thread), and then pass that reference down through the call stack. Grabbing PeerRef incurs overhead because m_peer_mutex needs to be locked and the shared ptr's refcount needs to be incremented/decremented, so we shouldn't do it unnecessarily.

4561423100c1b6a8e778525a027d021b918a6312 Definitely like peerman asking connman if it should RunInactivityChecks() instead of taking a peertimeout param 👌 Strictly speaking, I think it's possible for the time to be different from RunInactivityChecks to InactivityChecks since they each grab the time separately (as opposed to before, where they used the same now)? I don't think it'd be a huge issue since it's in seconds and the system time would only move forward though

~Currently system time isn't guaranteed to move forwards; and I believe we're now doing these checks in different threads so I don't think it's reasonable to try to use the same exact point in time for the two comparisons. We should be doing both checks 10-20 times per second for each peer anyway, I believe, so any lack of precision shouldn't matter.~

Could make this

bool CConnman::RunInactivityChecks(const CNode& node, const uint64_t const* pnow=nullptr)
{
    const uint64_t now = (pnow ? *pnow : GetSystemTimeInSeconds());
    return now > node.nTimeconnected + m_peer_connect_timeout;
}

and call it from within InactivityCheck() as if (!RunInactivityChecks(node, &now) return false; to avoid unnecessarily getting the system time twice in quick succession. Might also make RunInactivityChecks easier to test since you could pass in different times and check the answer, without having to worry that GetSystemTime isn't mockable.

I'll take aj's suggestion here: #20721 (review) in a follow-up

#21198

const NodeId peer_id{peer.m_id}; would've changed fewer lines :)

Indeed it would, but minimizing lines changed isn't the right metric to judge a commit. Instead we should aim to maximize the clarity and expressiveness of the code after the change. If I was writing this from scratch, I wouldn't create a local copy of the peer id just to use in log lines.

Hmm, actually I think I disagree with that (well, at least after exaggerating it into a bigger principle) -- review is a much more important bottleneck than just about anything else, so optimising the code to be easier to review is much more helpful than optimising it to make perfect sense if we were going to erase our git history and anonymously post the current tree to a mailing list. I think just about any time there's a choice between correct code that's easier to review and correct code that's better according to some other aesthetic, easier to review should be the winner. At least until review is no longer a serious bottleneck for the project.

Obviously, peer_id to peer.m_id is trivial, but if we're talking what we should aim to maximise, I think ease of review is the single right answer. I don't think you'll often find that being far off from optimising for clarity, though. (I don't think "expressiveness" applies either way tbh)

This discussion is a bit abstract, but I'm going to stand by my assertion. We should always be aiming to improve the quality of the code by standardizing to a clear and consistent style and by taking advantage of higher-level abstractions. One example: if I find a hand written loop that I need to modify, I may well replace it with an stl algorithm. That's undoubtedly a bit more effort to review, but results in higher-quality code. If we always aim to minimize deltas in code changes, then we'll only ever converge to local maxima of quality.

The point of review being a bottleneck is that we won't ever progress to a global maximum of quality -- if we could do that, by definition we wouldn't be bottlenecked. Every merge is a reflection of that review bottleneck: did we spend our limited review cycles on working out if a while loop does the same thing as the for loop it replaces, or did we spend it on working out if muhash or minisketch does what it claims on the box? If every PR spends 5% of review time checking that shuffling code around is correct, then that's 1-in-20 fewer PRs we can make that affect people that don't see the code. Or it's an incentive to get in the habit of merging code that's not thoroughly reviewed in order to avoid delaying things because we've run out of review resources, which is worse.

That doesn't mean we shouldn't always be aiming to improve the quality of the code; just that we should be spending the resources we have on the changes that are most valuable, even if that means leaving some good things for (possibly much) later.

(Personally, I find most of the STL algorithms worse than the loops they replace -- range based for loops in c++ are pretty pleasant and behave pretty sensibly; for me, dealing explicitly with iterators and lambdas generally isn't pleasant -- they're harder to understand and easier to make mistakes with, for at best a minimal performance benefit, and for me at least any safety benefits are offset by the lack of clarity. There are more important battles to fight, and I'm hoping that perhaps familiarity might eventually breed affection, but for now they certainly don't seem like moving toward even a local maximum to me)

Thanks AJ, these are good points.

You're wrong about stl algorithms though :grin:

if the peer should be connected for inactivity ? -- we could introduce a net permission to prevent that rather than just have a standard timeout for all peers, eg

connected for inactivity?? I don't follow.

are y'all talking about the comment being unclear or ?

#21198

I'm surprised this is pulled out of InactivityCheck(). Seems cleaner to keep the early exit to me

#21198

Only mentioning since you've got RunInactivityChecks as a separate commit, but I think you could reasonably have pulled SendPings() out as a separate commit prior to moving things from net to net_processing too.

Yes, good point.

I'd prefer if this was called something like InactivityChecksEnabled or ShouldRunInactivityChecks because right now the name suggests this function is where they are run (aka what InactivityCheck does).

I'll change this in a follow-up.

#21198

unrelated note: Can use the count seconds double helper from #21015

Thanks @MarcoFalke. I'll address this in the follow-up

#21015 isn't merged yet, but hopefully soon will.

I plan to review it soon

I'll do this in #21198 once #21015 is merged.

Same

same same