doc: clarify -timeout and -peertimeout config options #21077

The debug-only option -peertimeout is used to delay InactivityCheck(), whereas the -timeout option specifies socket timeouts (nConnectTimeout). The current descriptions are a bit misleading and hard to tell apart. I think it would save dev/review time to update them 🤷

@glozow

This PR has a broader scope than docs - maybe add Utils/Log/Libs label or cli ?

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #20721 (Net: Move ping data 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.

ACK f2306cbb9f564815b015fdbdb80410299fd22dee although I agree with @ghost that "P2P" could be added back.

There was no issue with any of the text. It was written very clearly @ghost: I disagree. The text as currently written states that if an p2p connection stops responding to messages for -peertimeout seconds, then we'll disconnect. This isn't what the config option does. It controls how long we'll wait after we connect before we start running inactivity checks. Those inactivity checks always use the hardcoded TIMEOUT_INTERVAL, which is 20 minutes.

ACK f2306cbb9f564815b015fdbdb80410299fd22dee

I see this PR as a kind of follow up of #20927.

Currently, on master:

$ src/bitcoind -help-debug | grep -A 2 -e '-timeout'
  -timeout=<n>
       Specify connection timeout in milliseconds (minimum: 1, default: 5000)

$ src/bitcoind -help-debug | grep -A 4 -e '-peertimeout'
  -peertimeout=<n>
       Specify p2p connection timeout in seconds. This option determines the
       amount of time a peer may be inactive before the connection to it
       is dropped. (minimum: 1, default: 60)

With this PR (f2306cbb9f564815b015fdbdb80410299fd22dee):

$ src/bitcoind -help-debug | grep -A 4 -e '-peertimeout'
  -peertimeout=<n>
       Specify a timeout delay in seconds. After connecting to a peer, wait
       this amount of time before deciding to disconnect based on
       inactivity. (minimum: 1, default: 60)

that describes the logic of CConnman::InactivityCheck much better: https://github.com/bitcoin/bitcoin/blob/bf100f8170770544fb39ae6802175c564cde532f/src/net.cpp#L1225-L1229

Currently, on master:

$ src/bitcoind -help-debug | grep -A 2 -e '-timeout'
  -timeout=<n>
       Specify connection timeout in milliseconds (minimum: 1, default: 5000)```

This config option is also under-documented! The timeout this is referring to is the socket timeout on initial connection (i.e. if poll or select don't return a file descriptor because the initial connection timed out)

Also, nConnectTimeout is a global (bad!!) and could be a const member of connman instead

Also, nConnectTimeout is a global (bad!!) and could be a const member of connman instead

I'm not sure if connman is the right home for this. It's a low-level socket configuration, and could live entirely in netbase. It seems strange that connman takes a global from netbase.cpp and then passes it to netbase through ConnectThroughProxy() or ConnectSocketDirectly().

In any case, I think we've wandered off topic a little bit :)

Thanks @hebasto and @jnewbery for your kind and thoughtful feedback ☺️ I've addressed your comments and edited the -timeout help string a little bit so that they're more distinguishable.

ACK eecb7ab105a4a59d09cd55b124c5ad563846fe11 nice doc fixup

ACK eecb7ab105a4a59d09cd55b124c5ad563846fe11