Refactor: Changes time variables from int to chrono #23832

shaavan commented at 12:08 PM on December 21, 2021: contributor

This PR is a follow-up to #23801. This PR aims to make the following changes to all the time variables in net_processing.cpp wherever possible.

Convert all time variables to std::chrono.
Use chorno::literals wherever possible.
Use auto keywords wherever possible.
Use var{val} convention of initialization.

This PR also minimizes the number of times, serialization of time count_seconds(..) occurs.

in src/net_processing.cpp:67 in 17af89fdaf outdated

  65 |  static constexpr auto STALE_CHECK_INTERVAL{10min};
  66 |  /** How frequently to check for extra outbound peers and disconnect */
  67 |  static constexpr auto EXTRA_PEER_CHECK_INTERVAL{45s};
  68 |  /** Minimum time an outbound-peer-eviction candidate must be connected for, in order to evict */
  69 | -static constexpr std::chrono::seconds MINIMUM_CONNECT_TIME{30};
  70 | +static constexpr auto MINIMUM_CONNECT_TIME{30s};

MarcoFalke commented at 12:14 PM on December 21, 2021:

This is already chrono. I think those changes should be in a separate commit (or left as is)

shaavan commented at 2:30 PM on December 21, 2021:

Let me post these changes as a separate commit.

shaavan commented at 11:43 AM on December 22, 2021:

Addressed in 16d0b619d72971b23eabcb0a5849370720f93909

in src/net_processing.cpp:4351 in 17af89fdaf outdated

4347 | @@ -4348,7 +4348,7 @@ void PeerManagerImpl::CheckForStaleTipAndEvictPeers()
4348 |          // Check whether our tip is stale, and if so, allow using an extra
4349 |          // outbound peer
4350 |          if (!fImporting && !fReindex && m_connman.GetNetworkActive() && m_connman.GetUseAddrmanOutgoing() && TipMayBeStale()) {
4351 | -            LogPrintf("Potential stale tip detected, will try using extra outbound peer (last tip update: %d seconds ago)\n", count_seconds(now) - count_seconds(m_last_tip_update));
4352 | +            LogPrintf("Potential stale tip detected, will try using extra outbound peer (last tip update: %d seconds ago)\n", count_seconds(now - m_last_tip_update.load()));

MarcoFalke commented at 12:15 PM on December 21, 2021:

nit: (Feel free to ignore). Maybe add a line break while touching this?

            LogPrintf("Potential stale tip detected, will try using extra outbound peer (last tip update: %d seconds ago)\n",
count_seconds(now - m_last_tip_update.load()));

shaavan commented at 11:43 AM on December 22, 2021:

Addressed in e7535d44cbd4273283202d95c31dbd29e9945115

MarcoFalke commented at 12:17 PM on December 21, 2021: member

Copncept ACK. I haven't reviewed this, but I left comments

DrahtBot added the label P2P on Dec 21, 2021

DrahtBot added the label Refactoring on Dec 21, 2021

theStack commented at 12:53 PM on December 21, 2021: member

Concept ACK

DrahtBot commented at 7:52 PM on December 21, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22778 (net processing: Reduce resource usage for inbound block-relay-only connections by jnewbery)
#21515 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)
#21160 (net/net processing: Move tx inventory into net_processing by jnewbery)
#18642 (Use std::chrono for the time to rotate destination of addr messages + tests by naumenkogs)

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.

in src/net_processing.cpp:97 in 17af89fdaf outdated

  96 |  /** How long to delay requesting transactions from overloaded peers (see MAX_PEER_TX_REQUEST_IN_FLIGHT). */
  97 | -static constexpr auto OVERLOADED_PEER_TX_DELAY = std::chrono::seconds{2};
  98 | +static constexpr auto OVERLOADED_PEER_TX_DELAY{2s};
  99 |  /** How long to wait (in microseconds) before downloading a transaction from an additional peer */
 100 | -static constexpr std::chrono::microseconds GETDATA_TX_INTERVAL{std::chrono::seconds{60}};
 101 | +static constexpr auto GETDATA_TX_INTERVAL{60us};

MarcoFalke commented at 9:52 AM on December 22, 2021:

This is wrong 60s is not equal to 60us

shaavan commented at 11:41 AM on December 22, 2021:

Sorry. I made an error interpreting this line of code earlier.

In the updated PR (commit 16d0b619d72971b23eabcb0a5849370720f93909), I have changed the variable to std::chrono::seconds.

std::chrono::microseconds GETDATA_TX_INTERVAL{std::chrono::seconds{60}}; -> auto GETDATA_TX_INTERVAL{60s};

As I could not see any apparent reason to keep it in microseconds.

shaavan force-pushed on Dec 22, 2021

shaavan commented at 11:38 AM on December 22, 2021: contributor

Updated from 17af89fdaf0f95a21f36d54421d3bef745af13be to 16d0b619d72971b23eabcb0a5849370720f93909 (pr23832.01 -> pr23832.02)

Addressed @MarcoFalke comments

Changes:

Split PR into two commits.
- First one converting remaining time variables from int to Chrono
- The second one makes the other changes done in this PR.
Added line break between a long LogPrintf line.
Corrected one error in variable initialization.

w0xlt approved

w0xlt commented at 10:48 AM on December 23, 2021: contributor

crACK 16d0b61

shaavan requested review from MarcoFalke on Dec 25, 2021

klementtan commented at 6:41 AM on December 27, 2021: contributor

crACK 16d0b619d72971b23eabcb0a5849370720f93909

verified that the correct std::chrono::* is used when converting from int to std::chrono
verified that the correct chorno::literals is used

klementtan approved

in src/net_processing.cpp:952 in e7535d44cb outdated

 948 | @@ -949,10 +949,10 @@ bool PeerManagerImpl::TipMayBeStale()
 949 |  {
 950 |      AssertLockHeld(cs_main);
 951 |      const Consensus::Params& consensusParams = m_chainparams.GetConsensus();
 952 | -    if (count_seconds(m_last_tip_update) == 0) {
 953 | +    if (m_last_tip_update.load() == 0s) {

MarcoFalke commented at 11:27 AM on January 3, 2022:

Wrong commit? This has nothing to do with switching types?

Edit: Ok, I see it doesn't switch the type of m_last_tip_update, but of the literal 0. Feel free to ignore.

in src/net_processing.cpp:60 in e7535d44cb outdated

  56 | @@ -57,8 +57,8 @@ static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER = 1ms;
  57 |   * behind headers chain.
  58 |   */
  59 |  static constexpr int32_t MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT = 4;
  60 | -/** Timeout for (unprotected) outbound peers to sync to our chainwork, in seconds */
  61 | -static constexpr int64_t CHAIN_SYNC_TIMEOUT = 20 * 60; // 20 minutes
  62 | +/** Timeout for (unprotected) outbound peers to sync to our chainwork */

MarcoFalke commented at 11:28 AM on January 3, 2022:

Will also need to remove "seconds" from where CHAIN_SYNC_TIMEOUT is used in docs?

shaavan commented at 11:47 AM on January 3, 2022:

Just to confirm. Are you talking about this line?

https://github.com/bitcoin/bitcoin/blob/d69af93223c4008c3255f7e4848ff05d78c514fa/src/net_processing.cpp#L729-L730

MarcoFalke commented at 12:13 PM on January 3, 2022:

Yes

shaavan commented at 2:26 PM on January 3, 2022:

This line could use a fixing.

However, doing so will revoke the current 3 ACKs on the current top commit of this PR. So would you suggest I should update this PR for this change?

MarcoFalke commented at 3:25 PM on January 3, 2022:

If it is fixed, it should be fixed here to avoid another follow-up pull. If not, it might be best to leave it as-is.

A re-ACK is less overhead and effort than a new pull + fresh ACK.

shaavan commented at 5:56 AM on January 4, 2022:

Fixed in fe86eb50c986f7b5ccce63f984d8a51cd9ee9e2c

MarcoFalke approved

MarcoFalke commented at 11:36 AM on January 3, 2022: member

ACK 16d0b619d72971b23eabcb0a5849370720f9390 🔠

<details><summary>Show signature</summary>

Signature:

</details>

Refactor: Changes remaining time variable type from int to chrono 6111b0d7fa

Refactor: Uses c++ init convention for time variables

This commit does following changes to time variables in net_processing.h:
- Used {} initialization.
- Uses universal initializer auto.
- Uses chrono::literals

The reason for these changes is to make code simpler, and easier to
understand and rationalize.

fe86eb50c9

shaavan force-pushed on Jan 4, 2022

shaavan commented at 5:56 AM on January 4, 2022: contributor

Updated from 16d0b619d72971b23eabcb0a5849370720f93909 to fe86eb50c986f7b5ccce63f984d8a51cd9ee9e2c (pr23832.02 -> pr23832.03, diff) Addressed @MarcoFalke suggestion.

Removed the term second for CHAIN_SYNC_TIMEOUT in docs.

Should be trivial to reACK.

MarcoFalke approved

MarcoFalke commented at 12:37 PM on January 6, 2022: member

re-ACK fe86eb50c986f7b5ccce63f984d8a51cd9ee9e2c 🏕

only minor doc change

<details><summary>Show signature</summary>

Signature:

</details>

MarcoFalke merged this on Jan 6, 2022

MarcoFalke closed this on Jan 6, 2022

shaavan deleted the branch on Jan 6, 2022

sidhujag referenced this in commit f313d0ad94 on Jan 6, 2022

DrahtBot locked this on Jan 6, 2023