Move ping data to net_processing (p2p)

https://github.com/bitcoin/bitcoin/pulls/20721

Host: jnewbery  -  PR author: jnewbery

The PR branch HEAD was 2cfa278 at the time of this review club meeting.

Background

  • We can think of the Bitcoin Core node as a stack with three levels:

    • Net - keeps track of potential nodes to peer with, handles active connections to nodes, and reads and writes messages to the wire. The main class in the net layer is CConnman, which is defined in net.cpp. Net hands messages up the stack to …

    • Net Processing - reads and processes application-level messages, and handles application-level view of peers. The main class in net processing is PeerManager, which is declared in net_processing.h. Net processing uses function calls like ProcessNewBlock() and AcceptToMemoryPool() to call into …

    • Validation - is responsible for our view of the block chain and the UTXO set. It may (or may not) include a mempool for unconfirmed transactions. There are various classes and functions exposed to the rest of the node. PR 20158 and linked PRs are an attempt to clarify and rationalize the interface to validation.

  • In early versions of Bitcoin, this distinction wasn’t as clear. The first commit in the Bitcoin Core repository has a separate net.cpp file, but net processing and validation are mixed in the main.cpp file. It was only in PR 9260 in 2016 that net processing and validation were split into separate files.

  • Even today, the division between net, net processing and validation is not as clear as we’d like. Clarifying and rationalizing these interfaces is an ongoing project. See, for example, PR 15141, which moves the denial of service logic from validation into net processing, PR 20158, which rationalizes the interface to validation, and issue 19398, which moves application-layer data and logic from net into net processing.

  • The high level goal of issue 19398 is to split responsibilities cleanly between CConnman and PeerManager:

    • CConnman should be responsible for connections - accepting incoming and opening outbound connections, opening and closing sockets, reading and writing data on those sockets, and first-line-of-defence validation of corrupt/malformed messages.

    • PeerManager should be responsible for peers - parsing and acting on p2p messages from peers, storing state relating to those peers, and deciding when to send messages to those peers.

  • Today’s PR, PR 20721, is part of the larger project tracked by issue 19398. It moves the ping data from net into net processing.

Notes

  • The ping/pong message exchange acts as a keepalive between peers on the peer-to-peer network, and also measures round-trip latency. Lowest ping time and most recent ping time are presented to users in the GUI and RPC interface and lowest ping time is also used by the net layer in its peer eviction logic.

  • Some of the data members, nPingNonceSent, m_ping_start and fPingQueued are only ever used by net processing, so they are obvious candidates for moving into the Peer structure.

  • nMinPingUsecTime is updated by net processing, but is used by net in its inbound node eviction logic.

Questions

  • What’s the difference between system time and mockable time? When is it appropriate to use one or the other?

  • In CConnman::InactivityCheck(), why do we have a ping timeout? Why not just rely on the send buffer timeout and receive buffer timeout?

  • Why do certain tests need to be updated in the second commit Use -pingtimeout in tests that might have pings time out? Why could pings start timing out when the ping timeout logic is moved into net processing?

  • The first commit, Add -pingtimeout to configure ping timeout, adds a new configuration option. Is this desirable? When should we add new configuration options, and when should we avoid adding new options?

  • If the goal of the PR is to move ping data into net processing, why is there still a PingReceived function in CNode (in net) after this PR?

Meeting Log

  118:00 <jnewbery> #startmeeting
  218:00 <jnewbery> Howdy! Welcome to PR Review Club!
  318:00 <shafiunmiraz0> hi
  418:00 <jnewbery> Feel free to say hi to let everyone know you're here.
  518:00 <murtyjones> hiya
  618:00 <schmidty> hi
  718:00 <norisgOG> Hi
  818:00 <willcl_ark> hi
  918:00 <troygiorshev> hi!
 1018:00 <sishir> hi
 1118:00 <dhruvm> hi
 1218:00 <AnthonyRonning> hi
 1318:00 <sunon> hi!
 1418:01 <lightlike> hi
 1518:01 <jnewbery> Anyone here for the first time?
 1618:01 <shafiunmiraz0> i am first time
 1718:01 <glozow> howdy!
 1818:01 <michaelfolkson> hi
 1918:01 <willcl_ark> welcome shafiunmiraz0 :)
 2018:01 <shafiunmiraz0> thank you so much
 2118:01 <AnthonyRonning> shafiunmiraz0: welcome!
 2218:02 <jnewbery> welcome shafiunmiraz0!
 2318:02 <maqusat> hi, it's my first time too
 2418:02 <jnewbery> Before we begin, a few reminders:
 2518:02 <prayank> hi
 2618:02 <jnewbery> welcome maqusat!
 2718:02 <jnewbery> All questions are welcome. We're all here to learn.
 2818:02 <dhruvm> welcome maqusat!
 2918:02 <jnewbery> No need to ask if a question is on-topic. Just ask! (I'll let you know if we've wondered too far off-topic)
 3018:02 <nehan> hi
 3118:02 <jnewbery> And finally, I'm always looking for hosts for future review club meetings. Please let me know after the meeting if you'd like to host. I can help you prepare notes and questions.
 3218:02 <felixweis> hi
 3318:02 <jnewbery> ok, onto the meeting! Notes and questions are in the usual place: https://bitcoincore.reviews/20721
 3418:03 <jnewbery> who had a chance to review the PR this week? (y/n)
 3518:03 <glozow> y
 3618:03 <murtyjones> y
 3718:03 <svav> y
 3818:03 <troygiorshev> n
 3918:03 <nehan> n
 4018:03 <willcl_ark> n
 4118:03 <sunon> n
 4218:03 <sishir> n
 4318:03 <dhruvm> y
 4418:03 <norisgOG> n
 4518:03 <felixweis> y
 4618:03 <emzy> hi
 4718:03 <emzy> n
 4818:03 <amiti> hi
 4918:03 <maqusat> y
 5018:03 <effexzi> Hey
 5118:04 <shafiunmiraz0> n
 5218:04 <jnewbery> good stuff. For those of you who didn't have time this week, hopefully you'll be able to follow along the notes & questions at least.
 5318:04 <AnthonyRonning90> y
 5418:04 <jnewbery> And I'd recommend going back and looking at the PR after. It'll really help cement your learning
 5518:04 <jnewbery> ok, any initial thoughts about the PR? What's it trying to do? Concept ACK/NACK?
 5618:05 <schmidty> ACK for separations of concern
 5718:05 <willcl_ark> at a high level, definitely in favour of furthering modularisation
 5818:05 <svav> ACK for the principle
 5918:05 <murtyjones> makes sense to me and the stack analogy was helpful. although the fact that ping data is used in net and net processing both took me a little bit to reason about
 6018:05 <AnthonyRonning90> ACK, not immediately intuitive that ping should be in net processing but after seeing what additional stuff it did, it made sense.
 6118:06 <maqusat> Concept ACK
 6218:06 <troygiorshev> Concept ACK, and looking closer as to why ping should be in net_processing
 6318:06 <dhruvm> The PR is trying to further separate connection and application layer logic. Bitcoin Core uses ping/pong p2p messages to maintain data about connection health. p2p logic belongs in the application layer, so this PR moves the associated variables and logic there.
 6418:06 <michaelfolkson> Yeah Concept ACK. I'm always interested in what makes it into net, net processing, validation etc because it makes sense logically or because it is convenient from codebase perspective
 6518:07 <jnewbery> great! It sounds like you've all got the high-level concept. I agree that it's a bit strange to think about whether ping belongs in net or net processing. We'll dive into the details in a bit.
 6618:07 <b10c> hi
 6718:07 <jnewbery> onto the technical questions:
 6818:07 <jnewbery> 1. What’s the difference between system time and mockable time? When is it appropriate to use one or the other?
 6918:07 <murtyjones> System time takes the actual time from the machine, whereas mockable time can be set manually. The latter is useful in a context where you want to change the time for testing purposes.
 7018:07 <AnthonyRonning90> System time is the real time, mockable time is a fake time used for tests.
 7118:08 <dhruvm> System time asks the OS for the time. Mockable time uses a mock time value if available, if not, uses system time. Mockable time can make tests more deterministic. However, mockable time does not tick forward so if the functionality being tested needs that, we have to `setmocktime` repeatedly.
 7218:08 <jnewbery> murtyjones AnthonyRonning90 dhruvm: yes, very good
 7318:08 <glozow> mockable time isn't just used for tests, it's just a time that can be manipulated in tests
 7418:08 <fodediop> hi
 7518:09 <michaelfolkson> Where else is mockable time used glozow other than tests?
 7618:09 <dhruvm> glozow: what are the use cases other than tests?
 7718:09 <schmidty> glozow: where else is an example of its non test use?
 7818:09 <michaelfolkson> Ha
 7918:09 <michaelfolkson> Educate us glozow :)
 8018:10 <jnewbery> glozow: I think that mocktime can only be set for regtest
 8118:10 <pinheadmz> glozow isnt rpc setmocktime not even available outside of regtest?
 8218:11 <jnewbery> setmocktime can only be called for regtest: https://github.com/bitcoin/bitcoin/blob/ea96e17e/src/rpc/misc.cpp#L377-L379 . There's also a -mocktime command line arg. I'm not sure whether that can be used outside regtest mode
 8318:11 <glozow> I was thinking mockable versus non-mockable 🤔 like, for nodes running in non-regtest-mode, the pingtimes are still measured in mock-able time but they're not fake?
 8418:11 <sishir> Does tx download use case fall under tests as well? https://github.com/bitcoin/bitcoin/pull/16197
 8518:11 <glozow> like can-be-mocked-but-isn't-right-now
 8618:12 <jnewbery> glozow: ah ok. Yes - we use 'mockable time' for ping timeouts, but mockable time can only be mocked for regtest
 8718:12 <jnewbery> ok, so what timers should use mocktime and what timers shouldn't?
 8818:13 <pinheadmz> anything around MTP should NOT, so csv or block tiemstamps, etc i think
 8918:13 <pinheadmz> er i guess some of the block timestamp check
 9018:13 <murtyjones> What's MTP?
 9118:13 <felixweis> median time past
 9218:13 <pinheadmz> median time past
 9318:13 <pinheadmz> yeah a mean of last 11 blocks timestamps
 9418:13 <pinheadmz> this is the value used to test check locktime and check sequence (relative lock time)
 9518:13 <jnewbery> s/mean/median :)
 9618:13 <felixweis> median timestamp not mean timestamp
 9718:14 <murtyjones> ah okay, thanks
 9818:14 <pinheadmz> er thanks, i priomise i will never get those straight
 9918:14 <felixweis> :)
10018:14 <jnewbery> pinheadmz: so those things are timestamps in the block data. I'm thinking more of timers in the code
10118:14 <lightlike> at some places, a third time ("AdjustedTime") is used, a combination of our system time and what our peers think is the right time
10218:15 <jnewbery> lightlike: right, that's a bit of a strange one, but it does have one of the best satoshi comments in the code
10318:15 <jnewbery> * "Never go to sea with two chronometers; take one or three."
10418:16 <jnewbery> anyway, I digress
10518:16 <pinheadmz> he
10618:16 <michaelfolkson> Lol that's great
10718:16 <pinheadmz> i was actually surpirsed to see that ping timers could be mocked
10818:16 <dhruvm> mocktime does not tick forward, are there use cases dependent on that?
10918:16 <pinheadmz> but i suppose anything you want to test, it makes sense (test ping timeout)
11018:16 <murtyjones> I would think just about anything that needs to be unit/functional tested would want use mockable time?
11118:16 <jnewbery> so, I think we generally want to avoid using mockable time in the low level network stack. If those timers could be mocked, then everything would break
11218:17 <pinheadmz> are there timers in the code we'd never want to test?
11318:17 <jnewbery> *break when you call setmocktime
11418:17 <pinheadmz> how low?
11518:17 <pinheadmz> like, before seeing this PR i thought ping timeouts would be included in that
11618:17 <jnewbery> generally stuff in net, I'd hazard
11718:17 <jnewbery> pinheadmz: that's a reasonable assumption, since the ping timeouts were in net
11818:18 <pinheadmz> i was thinking nack at first, since keeping connections alive seems like a low level (net) function but since `ping` is defined as a bitcoin p2p protocol message I changed my mind, it does belong up in the applciation level
11918:18 <jnewbery> but if we want to test the behaviour of a specific timer, then we need to be able to mock it
12018:18 <jnewbery> that leads us nicely on to the second question:
12118:18 <jnewbery> 2. In CConnman::InactivityCheck(), why do we have a ping timeout? Why not just rely on the send buffer timeout and receive buffer timeout?
12218:19 <dhruvm> To occasionaly check connection health even if no data is being sent/received on it.
12318:19 <pinheadmz> jnewbery i think specifically so it can be adjsuted / customized ?
12418:19 <AnthonyRonning90> My guess: because pings have a different timeout threshold. Send/receive could take awhile depending on the size of the data while pings should be somewhat fast.
12518:19 <amiti> so the idea is if the low level network stuff could be mocked, when we set mocktime to try to test higher level stuff, the low level stuff wouldn't work as expected? ... so we shouldn't use mocktime too far down?
12618:20 <jnewbery> amiti: I think that's right
12718:20 <pinheadmz> jnewbery and to be clear, this PR removes ping timeout from inactivtyCheck() right?
12818:20 <jnewbery> generally the mockable stuff is up in the application layer. sipa wrote a nice review comment on it somewhere, but I can't quite put my finger on where it was
12918:21 <felixweis> when are members being renamed to m_snake_case?
13018:21 <jnewbery> ah yes, here it is: https://github.com/bitcoin/bitcoin/pull/20027#discussion_r501350411
13118:21 <glozow> how often do we just not really send our peers anything for 20 minutes?
13218:21 <jnewbery> pinheadmz: absolutely correct. This PR moves that logic up into the application layer (PeerManager in net processing)
13318:22 <jnewbery> glozow: never! We expect pings to be responded to pretty quickly, and they get sent out within a minute of receiving the last one I think
13418:23 <jnewbery> two minutes, sorry
13518:23 <glozow> o okie, but if we didn't ping to check health, would there be occasions where we just don't really talk much?
13618:23 <michaelfolkson> If a peer has disconnected for some reason do we stop pinging them immediately?
13718:24 <glozow> wdym, i don't think we can ping them if we're disconnected?
13818:24 <jnewbery> felixweis: if I move a data member, I'll add a scripted diff at the end to make the naming comply with current code style. All of the lines that the variable touches are being updated anyway, so it's no additional churn
13918:24 <felixweis> thanks makes sense
14018:24 <dhruvm> if a connection does not have much chatter/is dead, isn't that when we need the ping/pong most?
14118:25 <michaelfolkson> glozow: If they are online we could attempt to open a connection up again but we can't ping them until that connection is established right?
14218:25 <lightlike> glozow: if the connection is blocks-only and there are no incoming blocks for a while, what else would there be to send?
14318:25 <michaelfolkson> glozow: We have to start the handshake again
14418:25 <dhruvm> lightlike: that's a good one
14518:25 <glozow> sorree, I was trying to answer the question "why do ping timeouts at all" and my guess was that if we didn't do that, we could accidentally be conflating "nothing to talk about" with "inactivity"
14618:26 <glozow> lightlike: that makes sense
14718:26 <jnewbery> so the ping timeout does seem at first glance to be slightly redundant. If the receive buffer timeout is hit (20 minutes of inactivity) then, by definition, we can't have received a pong in 20 minutes.
14818:27 <jnewbery> however, I think they're both useful, and having timeouts on both the net/data layer and the application layer is a good separation of responsibilities
14918:27 <jnewbery> ping timeouts are net processing's way to ensure that it has an active logical connection with a peer. It must do, since the peer is sending pongs
15018:28 <jnewbery> socket timeouts are net's way to ensure that it has an active connection on the transport layer. It must do, since the connection is receiving bytes
15118:29 <jnewbery> I hope that makes sense!
15218:29 <glozow> so, we distinguish between a networking-level "healthy/active connection" and an application-level "we want our peers to be following protocol and responding to pings?"
15318:29 <pinheadmz> also ping/pong nonces are needed to avoid connection to self :-)
15418:29 <troygiorshev> glozow: ah that's a great way to put it
15518:29 <pinheadmz> dunno if the low level socket stuff could figure that out
15618:30 <jnewbery> glozow: I distinguish between those things! This PR is an attempt to clarify that distinction :)
15718:30 <jnewbery> pinheadmz: you're thinking of the nonce in the version message
15818:30 <glozow> cool cool, just wanna make sure i follow
15918:30 <pinheadmz> oh right thank oyu
16018:30 <jnewbery> the nonce in the ping/pong is to correlate the pong response to the ping request
16118:30 <jnewbery> onwards!
16218:30 <jnewbery> 3. Why do certain tests need to be updated in the second commit Use -pingtimeout in tests that might have pings time out? Why could pings start timing out when the ping timeout logic is moved into net processing?
16318:31 <sishir> because tests run for less than the peer connect timeout
16418:31 <murtyjones> I'm guessing that those tests didn't previously execute ping-checking logic before the first commit ended up doing that as a side effect, so timeouts could occur if not mocked.
16518:31 <AnthonyRonning90> Instead of only checking ping timeout when `InactivityCheck` runs & the peer has been connected longer than the threshold, pings timeouts are now checked during the `SendMessage` flow. Tests before would not have hit the 20 minute default, but now hit the ping check every `SendMessage`.
16618:31 <nehan> jnewbery: i'm having trouble convincing myself that the net layer "needs" timeouts if the application layer can handle it. is there an example of why this might be so?
16718:33 <pinheadmz> I think AnthonyRonning90 is close - the inactivity check loop runs less often
16818:33 <jnewbery> sishir AnthonyRonning90: yes! (but it's 60 seconds, not 20 minutes for the initial peer connection timeout)
16918:33 <jnewbery> I gave the answer away here yesterday: https://github.com/bitcoin/bitcoin/pull/20721#discussion_r568651714
17018:33 <glozow> nehan: I'm imagining a case where the socket connection is still active, but there's a problem with the application-layer? we'd want to disconnect for inactivity in that case
17118:33 <dhruvm> nehan: The redundancy is useful although not needed iiuc. For example, if we wanted to remove pings in a future protocol version, we could because the transport layer is still checking for connection health?
17218:34 <AnthonyRonning90> jnewbery: oh gotcha, i saw `static const int TIMEOUT_INTERVAL = 20 * 60;` and thought it meant a default of 20 min
17318:34 <jnewbery> if a test runs for less than 60 seconds, none of the inactivity logic is exercised at all (before this PR). After this PR, the ping timeout logic is exercised as soon as the connection is fully up (on receipt of the verack)
17418:34 <sishir> Does this imply to all the tests or just P2Ps?
17518:35 <nehan> glozow: we'd want to disconnect because the application layer is having a problem. not sure why the net layer would need a timeout in that case.
17618:35 <glozow> nehan: true, if the net layer had a problem then application layer would be irrelevant
17718:35 <jnewbery> nehan: your instinct is right. If the application layer is exchanging pings with a peer, then the lower layer connection *must* be healthy. However, I think this is just good defense in depth and separation of responsibilities: connman is responsible for maintaining good connections, and peerman is responsible for maintaining good peers
17818:37 <jnewbery> I also don't think it's worth spending energy convincing ourselves and each other that we should remove socket inactivity checks.
17918:37 <nehan> part of the reason i bring this up is because of the challenge of which times to mock. that seems pretty hard and not at all "settled"! it would be very tricky to make sure you're mocking/updating all the clocks appropriately
18018:38 <jnewbery> (not to be dismissive - it's a good question to ask, but not a priority to change)
18118:38 <glozow> for the "why could pings start timing out when the ping timeout logic is moved" question, I thought it boiled down to the fact that it's no longer preceded by the `m_peer_connect_timeout` check? so then even the `-peertimeout` config option wouldn't suffice
18218:38 <nehan> jnewbery: definitely not arguing for removing them. more trying to understand if they are required or not.
18318:38 <jnewbery> those socket inactivity checks use non-mockable time, so they should be just fine in all tests
18418:38 <jnewbery> sishir: unit tests and functional tests
18518:38 <sishir> Got it!
18618:39 <jnewbery> glozow: eeeeexactly!
18718:39 <jnewbery> I'm going to ask the next question, but if there's something unclear in what we've covered before, always feel free to go back and ask more questions about it
18818:40 <jnewbery> 4. The first commit, Add -pingtimeout to configure ping timeout, adds a new configuration option. Is this desirable? When should we add new configuration options, and when should we avoid adding new options?
18918:40 <AnthonyRonning90> My initial impression is this was more so added for tests to better support tests (I’ve done that myself before)
19018:40 <AnthonyRonning90> better support time*
19118:40 <lightlike> nehan, jnewbery: would it maybe make sense to make all times mockable, but have different mocktime groups (one for net processing, another one for net) that could be adjusted separately?
19218:41 <dhruvm> We are going it here for tests. Could be a cli arg, or a regtest rpc - but cli arg is better because users in high latency regions could maybe use it?
19318:41 <michaelfolkson> On the configuration option question I would guess it should have an important use case. We don't want to keep piling up configuration options that aren't needed
19418:42 <pinheadmz> jnewbery perhaps if we are on a low bandwidth connection it makes sense to allow longer pings ?
19518:42 <glozow> Adding it made sense to me since it was debug-only and existing `-peertimeout` doesn't work. idk what the good practice would be, interested in hearing about that. However, since the only option ever used is pretty magic-number-y, `-pingtimeout=9999` how come it isn't just a flag option to extend/ignore the ping timeout?
19618:42 <michaelfolkson> Some configure options need to be set before starting the daemon and some can be set after
19718:43 <jnewbery> lightlike: If it makes testing specific functionality easier and there's no other way to do that, then maybe. That needs to be weighed up against the confusion of having multiple timestamps. It's already a bit of a mess
19818:43 <nehan> lightlike: i worry it might be tricky to figure out how to set them all appropriately. like maybe you want to fast forward a few days in blocktime, but don't want to trigger inactivity checks. and maybe something else besides inactivity checks will come to rely on net time being accurate
19918:44 <jnewbery> glozow: yeah, I think a binary flag is a reasonable alternative. I just copied what was already there for peertimeout
20018:45 <jnewbery> AnthonyRonning90: exactly. Here, we've used it to allow testing. The new -pingtimeout option is a debug option, so it won't show up when running bitcoind --help.
20118:46 <murtyjones> Could you get your node into a bad state by configuring the timeout to be a really low number where you end up disconnecting peers frequently?
20218:46 <jnewbery> In general, we should try to avoid adding more and more (publicly visible) command line options. It leads to a combinatorial explosion of different configurations to test
20318:46 <glozow> what would be considered good/bad idea for debug option? are we cool with just adding whatever options are helpful for testing?
20418:46 <AnthonyRonning90> Could `MaybeSendPing` instead also apply a similar “min peer connection time” before checking ping? Like one that the activity check made?
20518:46 <jnewbery> murtyjones: sure
20618:46 <pinheadmz> murtyjones i tried running with pingtimeout=1
20718:47 <pinheadmz> surprinalgy not that many disconnects
20818:47 <murtyjones> i guess if you have a good connection maybe that's not likely. but since this isn't publicly visible you'd have to know what you were doing to even find it anyways
20918:48 <jnewbery> AnthonyRonning90: It could. But logically I don't think that makes sense. We want pings to be sent/tested as soon as we're fully connected. I don't like adding special case logic to the production code to work around difficulties in our testing
21018:48 <AnthonyRonning90> jnewbery: yeah makes sense. Thought of it more like a way to mimic existing behavior, not as a testing workaround.
21118:48 <dhruvm> murtyjones: https://github.com/bitcoin/bitcoin/pull/20721/files#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R4324
21218:49 <jnewbery> pinheadmz: your earlier comment about setting a high pingtimeout for low bandwidth connections is interesting, but I think if you're not receiving ping responses is 20 minutes, you're probably not going to have a good time :)
21318:49 <pinheadmz> 20 minutes is the default?
21418:50 <lightlike> I just built the PR, and for me wallet_resendwallettransactions.py fails intermittently (~40% of the time)
21518:50 <pinheadmz> oh maybe then the idea is to crank it lower and only connect to other high bandwaidth nodes
21618:51 <jnewbery> lightlike: oh no! Do you definitely have the latest version (commit 4180381f)
21718:51 <lightlike> jnewbery: yes, just downloaded at the beginning of the session.
21818:51 <glozow> hm. resendwallettransactions sets mock time 12 hours ahead, that's 43200 seconds > 9999?
21918:51 <jnewbery> lightlike: thanks, I'll investigate
22018:52 <jnewbery> glozow: oh! Maybe you've found my bug. Thanks!
22118:52 <glozow> :salute:
22218:52 <AnthonyRonning90> i didn't look very hard, but are there separate tests for testing if ping timeout works? I only saw the test updates making ping timeout ignored (w/ 9999 timeout)
22318:52 <glozow> p2p_pingtimeouts.py
22418:54 <jnewbery> pinheadmz: before this PR, there were two default timeouts: DEFAULT_PEER_CONNECT_TIMEOUT (60 seconds) - we don't do any inactivity timeout checks until the peer has been connected for this long. TIMEOUT_INTERVAL (20 minutes) - we'll disconnect a peer if there's no activity on their send/recv buffers or they haven't sent a pong for this long
22518:54 <AnthonyRonning> *curses my ping timeouts today* :(
22618:54 <jnewbery> ok, final question
22718:54 <jnewbery> 5. If the goal of the PR is to move ping data into net processing, why is there still a PingReceived function in CNode (in net) after this PR?
22818:54 <murtyjones> Ping data is used for eviction logic, which is a "Net" responsibility, so the data has to be kept up to date. It can also be used for debugging purposes.
22918:54 <AnthonyRonning> the `CNode::m_min_ping_time` is used during `CConnman` eviction logic, so the ping processing data still needs to make it through to `Cnode` (as accomplished via `CNode::PingReceived`).
23018:55 <jnewbery> murtyjones AnthonyRonning: YES!
23118:55 <jnewbery> very good
23218:55 <nehan> will the eviction logic move to net_processing?
23318:55 <jnewbery> the net layer is currently responsible for inbound peer eviction, so net processing needs to inform it of interesting events that it can use in that decision-making
23418:56 <jnewbery> nehan: that's a very good question. My vague, nebulous thought at this point is that it should be extracted into a separate module.
23518:56 <jnewbery> it's not fully net and not fully net processing, since it requires data from both
23618:57 <jnewbery> it's trying to make an assessment of the quality of the peer based on connection-level and application-level metrics
23718:57 <jnewbery> ok, three minutes left. Any final questions?
23818:57 <jonatack> nehan asks the *best* questions. it's a super power. (hi)
23918:58 <glozow> ya, what's a good way to decide whether or not something warrants a new debug config option?
24018:58 <jnewbery> jonatack: nehan does indeed ask excellent questions :)
24118:58 <troygiorshev> nehan: who exactly owns the connection does see to be a little in flux, I'm excited to see where it lands
24218:59 <nehan> aw thanks, but i think jnewbery and others have been pondering this for quite some time
24318:59 <jnewbery> glozow: I think probably "avoid it if you can" is the best guidance I could give. I wouldn't be surprised if I got some pushback somewhere for adding -pingtimeout
24419:00 <murtyjones> dropping off. thanks for hosting jnewbery!
24519:00 <shafiunmiraz0> Thank you jnewbery Thank you everyone
24619:00 <jnewbery> #endmeeting
24719:00 <michaelfolkson> pinging thanks to jnewbery
24819:00 <nehan> thanks!
24919:00 <norisgOG> thanks
25019:00 <troygiorshev> thanks jnewbery!
25119:00 <AnthonyRonning> thank you jnewbery & all!
25219:00 <lightlike> thanks jnewbery!
25319:00 <dhruvm> thank you, jnewbery !
25419:00 <maqusat> cheers!
25519:00 <emzy> thanks jnewbery!