Refactor: Change time variable type from int64_t to std::chrono::seconds in net

shaavan commented at 11:57 AM on December 17, 2021: contributor

This is a follow-up to PR #23758
This changes the remaining time variable in net_processing.cpp from int64_t to std::chrono::seconds

shaavan commented at 11:58 AM on December 17, 2021: contributor

in src/net_processing.cpp:61 in f8b810b8e1 outdated

  57 | @@ -58,9 +58,9 @@ static constexpr int32_t MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT = 4;
  58 |  /** Timeout for (unprotected) outbound peers to sync to our chainwork, in seconds */
  59 |  static constexpr int64_t CHAIN_SYNC_TIMEOUT = 20 * 60; // 20 minutes
  60 |  /** How frequently to check for stale tips, in seconds */
  61 | -static constexpr int64_t STALE_CHECK_INTERVAL = 10 * 60; // 10 minutes
  62 | +static constexpr std::chrono::seconds STALE_CHECK_INTERVAL{10 * 60}; // 10 minutes

MarcoFalke commented at 12:23 PM on December 17, 2021:

static constexpr std::chrono::minutes STALE_CHECK_INTERVAL{10};

nit. Also:

The subject line of commit hash f8b810b8e15b72f5cff4c6e27bea3f6a64fd0c3d is followed by a non-empty line. Subject lines should always be followed by a blank line.

shaavan commented at 1:14 PM on December 17, 2021:

Addressed in 8e0ba04b2e6e9ecb11b541ecc361c3a9fe5e9fde

in src/net_processing.cpp:62 in f8b810b8e1 outdated

  57 | @@ -58,9 +58,9 @@ static constexpr int32_t MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT = 4;
  58 |  /** Timeout for (unprotected) outbound peers to sync to our chainwork, in seconds */
  59 |  static constexpr int64_t CHAIN_SYNC_TIMEOUT = 20 * 60; // 20 minutes
  60 |  /** How frequently to check for stale tips, in seconds */
  61 | -static constexpr int64_t STALE_CHECK_INTERVAL = 10 * 60; // 10 minutes
  62 | +static constexpr std::chrono::seconds STALE_CHECK_INTERVAL{10 * 60}; // 10 minutes
  63 |  /** How frequently to check for extra outbound peers and disconnect, in seconds */

MarcoFalke commented at 12:23 PM on December 17, 2021:

/** How frequently to check for extra outbound peers and disconnect */

nit

shaavan commented at 1:14 PM on December 17, 2021:

Addressed in 8e0ba04

in src/net_processing.cpp:4346 in f8b810b8e1 outdated

4344 |      if (time_in_seconds > m_stale_tip_check_time) {
4345 |          // Check whether our tip is stale, and if so, allow using an extra
4346 |          // outbound peer
4347 |          if (!fImporting && !fReindex && m_connman.GetNetworkActive() && m_connman.GetUseAddrmanOutgoing() && TipMayBeStale()) {
4348 | -            LogPrintf("Potential stale tip detected, will try using extra outbound peer (last tip update: %d seconds ago)\n", time_in_seconds - m_last_tip_update);
4349 | +            LogPrintf("Potential stale tip detected, will try using extra outbound peer (last tip update: %d seconds ago)\n", (time_in_seconds - m_last_tip_update).count());

MarcoFalke commented at 12:24 PM on December 17, 2021:

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

nit: Could clarify the unit

shaavan commented at 1:15 PM on December 17, 2021:

Addressed in 8e0ba04

in src/net_processing.cpp:950 in f8b810b8e1 outdated

 946 | @@ -947,10 +947,10 @@ bool PeerManagerImpl::TipMayBeStale()
 947 |  {
 948 |      AssertLockHeld(cs_main);
 949 |      const Consensus::Params& consensusParams = m_chainparams.GetConsensus();
 950 | -    if (m_last_tip_update == 0) {
 951 | -        m_last_tip_update = GetTime();
 952 | +    if (m_last_tip_update == std::chrono::seconds::zero()) {

MarcoFalke commented at 12:25 PM on December 17, 2021:

    if (m_last_tip_update.count() == 0) {

I'd use this, but there is really no difference, so both are fine.

shaavan commented at 1:16 PM on December 17, 2021:

Better to keep the coding usage throughout the codebase consistent.

Addressed in 8e0ba04

MarcoFalke approved

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

ack. left some nits

DrahtBot added the label P2P on Dec 17, 2021

DrahtBot added the label Refactoring on Dec 17, 2021

shaavan force-pushed on Dec 17, 2021

shaavan commented at 1:14 PM on December 17, 2021: contributor

Updated from f8b810b8e15b72f5cff4c6e27bea3f6a64fd0c3d to 8e0ba04b2e6e9ecb11b541ecc361c3a9fe5e9fde (pr23801.01 -> pr23801.02)

Changes

Addressed nits, suggested by @MarcoFalke

in src/net_processing.cpp:60 in 8e0ba04b2e outdated

  57 | @@ -58,9 +58,9 @@ static constexpr int32_t MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT = 4;
  58 |  /** Timeout for (unprotected) outbound peers to sync to our chainwork, in seconds */
  59 |  static constexpr int64_t CHAIN_SYNC_TIMEOUT = 20 * 60; // 20 minutes
  60 |  /** How frequently to check for stale tips, in seconds */

MarcoFalke commented at 1:24 PM on December 17, 2021:

/** How frequently to check for stale tips */

shaavan commented at 1:26 PM on December 19, 2021:

Addressed in 1a1fafa4fdcbe7b0d1ef8df466a71bd3e65195d3

in src/net_processing.cpp:544 in 8e0ba04b2e outdated

 540 | @@ -541,7 +541,7 @@ class PeerManagerImpl final : public PeerManager
 541 |      std::map<uint256, std::pair<NodeId, std::list<QueuedBlock>::iterator> > mapBlocksInFlight GUARDED_BY(cs_main);
 542 |  
 543 |      /** When our tip was last updated. */
 544 | -    std::atomic<int64_t> m_last_tip_update{0};
 545 | +    std::chrono::seconds m_last_tip_update{0};

MarcoFalke commented at 1:24 PM on December 17, 2021:

is atomic no longer needed?

shaavan commented at 6:36 PM on December 17, 2021:

Initially, I removed it by mistake. But when I tried to initialize var as

std::atomic<std::chrono::seconds> m_last_tip_update{0};

It is causing a compilation error.

I tried other ways to initialize it, but they don't seem to work. I searched for a solution on the internet, and I came around this bug report which is in a very similar spirit to this problem: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54025

So how do you suggest I should proceed now:

Revert this variable type to std::atomic<int64_t>
Keep the current implementation, which the std::atomic

laanwj commented at 1:16 PM on December 18, 2021:

atomic works for specific concrete integer types, it doesn't work for arbitrary data structures, you'll have to use a full mutex. I don't think this is worth it here so might as well stick with a int64_t.

MarcoFalke commented at 1:18 PM on December 18, 2021:

chronos are plain integers (they only have a bit of type safety added at compile time).

atomic<seconds> a{0s};

should compile, like in all other places where atomic<seconds> is used.

shaavan commented at 1:27 PM on December 19, 2021:

atomic<seconds> a{0} was not working but for some reason atomic<seconds> a{0s} worked.

Addressed in 1a1fafa4fdcbe7b0d1ef8df466a71bd3e65195d3

in src/net_processing.cpp:4338 in 8e0ba04b2e outdated

4334 | @@ -4335,15 +4335,15 @@ void PeerManagerImpl::CheckForStaleTipAndEvictPeers()
4335 |  {
4336 |      LOCK(cs_main);
4337 |  
4338 | -    int64_t time_in_seconds = GetTime();
4339 | +    auto time_in_seconds{GetTime<std::chrono::seconds>()};

MarcoFalke commented at 1:25 PM on December 17, 2021:

Maybe rename to now for brevity?

shaavan commented at 6:37 PM on December 17, 2021:

Makes sense. I am going to address it in the next update of PR.

shaavan commented at 1:27 PM on December 19, 2021:

Addressed in 1a1fafa4fdcbe7b0d1ef8df466a71bd3e65195d3

MarcoFalke approved

MarcoFalke commented at 1:54 PM on December 17, 2021: member

more nits

w0xlt approved

w0xlt commented at 7:32 PM on December 17, 2021: contributor

crACK 8e0ba04

hebasto commented at 1:25 PM on December 18, 2021: member

Concept ACK.

hebasto commented at 1:55 PM on December 18, 2021: member

#include <chrono>

?

shaavan force-pushed on Dec 19, 2021

shaavan commented at 1:25 PM on December 19, 2021: contributor

Updated from 8e0ba04b2e6e9ecb11b541ecc361c3a9fe5e9fde to 1a1fafa4fdcbe7b0d1ef8df466a71bd3e65195d3 (pr23801.02 -> pr23801.03)

Addressed comment by @MarcoFalke

Changes:

Addressed one nit. Removed unnecessary “in seconds.”
Changed m_last_tip_update time from std::chrono::seconds -> std::atomic<std::chrono::seconds>. Made other related changes, accordingly.
Renamed time_in_seconds to now under void PeerManagerImpl::CheckForStaleTipAndEvictPeers() definition. @Hebasto. I think including <chrono> in this file would be redundant, as the header banman.h, which this file imports, also imports chrono in it.

hebasto commented at 1:29 PM on December 19, 2021: member

@shaavan

@hebasto. I think including <chrono> in this file would be redundant, as the header banman.h, which this file imports, also imports chrono in it.

https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#source-code-organization:

Every .cpp and .h file should #include every header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers.

Rationale: Excluding headers because they are already indirectly included results in compilation failures when those indirect dependencies change. Furthermore, it obscures what the real code dependencies are.

shaavan force-pushed on Dec 19, 2021

shaavan commented at 1:39 PM on December 19, 2021: contributor

Updated from 1a1fafa4fdcbe7b0d1ef8df466a71bd3e65195d3 to d5207c2b75c33a720879ed1e43f3db1156cdcce8 Addressed @hebasto comment.

Changes:

Added <chrono> in the included header as it is a direct dependency in this file.

Thanks, @hebasto, for enlightening me. I will take care of this in the future.

stratospher commented at 3:42 PM on December 19, 2021: contributor

ACK d5207c2.

nit: On the same note, wouldn't #include <atomic> be needed too?

in src/net_processing.cpp:427 in d5207c2b75 outdated

 422 | @@ -422,7 +423,7 @@ class PeerManagerImpl final : public PeerManager
 423 |      std::atomic<int> m_best_height{-1};
 424 |  
 425 |      /** Next time to check for stale tip */
 426 | -    int64_t m_stale_tip_check_time{0};
 427 | +    std::chrono::seconds m_stale_tip_check_time{0s};

hebasto commented at 7:00 PM on December 19, 2021:

While using std::literals::chrono_literals why not using auto with universal initialization:

    auto m_stale_tip_check_time{0s};

It looks concise and expressive, no? Also could be applied in other places above.

shaavan commented at 6:08 AM on December 20, 2021:

That’s a good suggestion. I have used the auto keyword at all the places where it could be used. See 92082ea0bb62ab6ec723b57ca4f2b2c5f6b28469 However, I could not use auto with the particular example you had taken. Doing so would cause the following error:

error: non-static data member declared with placeholder ‘auto’

The rationale for this behavior is discussed here.

hebasto approved

hebasto commented at 7:02 PM on December 19, 2021: member

ACK d5207c2b75c33a720879ed1e43f3db1156cdcce8

Change time variable type to std::chrono::seconds in src/net_processing.cpp

- This commit is a followup to commit: 60b579
- This changes the remaining time variable in net_processing.cpp from
int64_t to std::chrono

92082ea0bb

shaavan force-pushed on Dec 20, 2021

shaavan commented at 6:06 AM on December 20, 2021: contributor

Updated from d5207c2b75c33a720879ed1e43f3db1156cdcce8 to 92082ea0bb62ab6ec723b57ca4f2b2c5f6b28469 (pr23801.03 -> pr23801.04) Addressed @Stratospher and @Hebasto comments.

Changes:

Added atomic in the included header, as it is a direct dependency.
Used the universal initializer auto, wherever chrono_literals were used.

naumenkogs commented at 7:50 AM on December 20, 2021: member

ACK 92082ea0bb62ab6ec723b57ca4f2b2c5f6b28469

hebasto approved

hebasto commented at 10:07 AM on December 20, 2021: member

re-ACK 92082ea0bb62ab6ec723b57ca4f2b2c5f6b28469

hebasto commented at 10:09 AM on December 20, 2021: member

I've restarted the "macOS 12 native [gui, system sqlite only] [no depends]" Cirrus CI task.

in src/net_processing.cpp:61 in 92082ea0bb

  58 | @@ -57,10 +59,10 @@ static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER = 1ms;
  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

MarcoFalke commented at 10:48 AM on December 20, 2021:

follow-up idea: I think this can also be switched to chrono? It would probably be the last occurrence and conclude the chrono-refactors?

shaavan commented at 11:05 AM on December 20, 2021:

Let's take it in a follow-up!

MarcoFalke merged this on Dec 20, 2021

MarcoFalke closed this on Dec 20, 2021

in src/net_processing.cpp:955 in 92082ea0bb

 953 | -        m_last_tip_update = GetTime();
 954 | +    if (count_seconds(m_last_tip_update) == 0) {
 955 | +        m_last_tip_update = GetTime<std::chrono::seconds>();
 956 |      }
 957 | -    return m_last_tip_update < GetTime() - consensusParams.nPowTargetSpacing * 3 && mapBlocksInFlight.empty();
 958 | +    return count_seconds(m_last_tip_update) < GetTime() - consensusParams.nPowTargetSpacing * 3 && mapBlocksInFlight.empty();

MarcoFalke commented at 10:55 AM on December 20, 2021:

nit: I think it would have been better to use type-safe time here (the goal of this pull), because:

GetTime should eventually be removed
count_seconds should generally only be used to "serialize" time values (for example in RPC results or P2P messages)

(see comments in src/util/time.h)

shaavan commented at 11:05 AM on December 20, 2021:

I think it would have been better to use the type-safe time here

My first approach was to do precisely this. But since m_last_tip_update was atomic<seconds> type. There were two issues I faced:

Converting RHS to match the type
For some reason, subtraction between atomic types is not possible

I tried to find some way to convert atomic<seconds> to <seconds>, but I could not find a satisfactory result. That's why I went with the current implementation.

MarcoFalke commented at 12:28 PM on December 20, 2021:

I think m_last_tip_update.load() will load the value wrapped by the atomic

shaavan commented at 5:56 PM on December 20, 2021:

I think m_last_tip_update.load() will load the value wrapped by the atomic

It worked! Let me open PR with all the suggested changes, and wrap up this chain of PR of going from int -> chrono as soon as possible.

Thanks a lot!

shaavan deleted the branch on Dec 20, 2021

in src/net_processing.cpp:4348 in 92082ea0bb

4347 | +    if (now > m_stale_tip_check_time) {
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", time_in_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) - count_seconds(m_last_tip_update));

MarcoFalke commented at 10:59 AM on December 20, 2021:

nit: I think it might have been better to do the calculation type-safe and then only "serialize" the result of the addition. This would also result in less code.

shaavan commented at 11:11 AM on December 20, 2021:

One doubt:

Is there a way to go from atomic<seconds> to <seconds> Or do subtraction between two atomic<seconds> type variables?

Thanks.

shaavan commented at 11:17 AM on December 20, 2021:

subtraction between two atomic<seconds> type variables?

For reference, doing so would cause the following error:

no match for ‘operator-’ (operand types are ‘std::atomic<std::chrono::duration<long int> >’ and ‘std::atomic<std::chrono::duration<long int> >’)

cc @MarcoFalke

MarcoFalke commented at 11:00 AM on December 20, 2021: member

re-ack. Left two nits and a follow-up idea.

sidhujag referenced this in commit 9eee2a9dff on Dec 20, 2021

MarcoFalke referenced this in commit 06209574da on Jan 6, 2022

DrahtBot locked this on Dec 20, 2022

Refactor: Change time variable type from int64_t to std::chrono::seconds in net_processing.cpp #23801