(Picking up #20044. Rebased against master.)
This changes various uses of integers to represent timestamps and durations to std::chrono duration types with type-safe conversions, getting rid of various .count(), constructors, and conversion factors.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
addr send functionality into MaybeSendAddr() 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.
I am still working on figuring out why test/functional/wallet_groups.py is failing with an off-by-one error only on some platforms. In the meantime, ready for initial review.
69 | @@ -70,6 +70,7 @@ def run_test(self): 70 | # Tell scheduler to call MaybeResendWalletTxn now. 71 | node.mockscheduler(1) 72 | # Give some time for trickle to occur 73 | + time.sleep(1)
in commit bbc01cf8b2965f716e899f413d36e2beb48c1f06: what is this?
This was originally in d7fad1690c27bf270774da6eebc4ef8b550caf09. However, i just ran the test without the line and it seems to not be needed. Removed.
No need to keep an empty commit d5a0f24807d5423469824efe8253c8e809b2044b
Removed.
2934 | -int64_t PoissonNextSend(int64_t now, int average_interval_seconds) 2935 | +std::chrono::microseconds PoissonNextSend(std::chrono::microseconds now, std::chrono::microseconds average_interval) 2936 | { 2937 | - return now + (int64_t)(log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */) * average_interval_seconds * -1000000.0 + 0.5); 2938 | + double unscaled = log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */); 2939 | + double scaled = unscaled * count_microseconds(average_interval);
you dropped the -
Thanks for the catch!
Concept ACK. This refactoring should only be changing the types at compile-time, not any values at run time.
587 | @@ -588,9 +588,9 @@ class CNode 588 | /** When the last ping was sent, or 0 if no ping was ever sent */ 589 | std::atomic<std::chrono::microseconds> m_ping_start{0us}; 590 | // Last measured round-trip time. 591 | - std::atomic<int64_t> nPingUsecTime{0}; 592 | + std::atomic<std::chrono::microseconds> m_ping_time{std::chrono::microseconds{0}};
4089bc6489255ab26a350bbadc06d1cc5cff6d35: Can use 0us, like above, which is shorter
Done
812 | @@ -811,9 +813,11 @@ QString formatServicesStr(quint64 mask) 813 | return QObject::tr("None"); 814 | } 815 | 816 | -QString formatPingTime(int64_t ping_usec) 817 | +QString formatPingTime(std::chrono::microseconds ping_time) 818 | { 819 | - return (ping_usec == std::numeric_limits<int64_t>::max() || ping_usec == 0) ? QObject::tr("N/A") : QString(QObject::tr("%1 ms")).arg(QString::number((int)(ping_usec / 1000), 10)); 820 | + return (ping_time == std::chrono::microseconds::max() || ping_time.count() == 0) ?
Same commit: Can use ping_time == 0us, which is shorter
Done
239 | @@ -238,8 +240,8 @@ namespace GUIUtil 240 | /** Format CNodeStats.nServices bitmask into a user-readable string */ 241 | QString formatServicesStr(quint64 mask); 242 | 243 | - /** Format a CNodeStats.m_ping_usec into a user-readable string or display N/A, if 0 */ 244 | - QString formatPingTime(int64_t ping_usec); 245 | + /* Format a CNodeStats.m_ping_time into a user-readable string or display N/A, if 0*/
4089bc6489255ab26a350bbadc06d1cc5cff6d35: Why is this changed?
This function is invoked with CNodeStats:m_.*ping.* variables in (for example) rpcconsole.cpp:1105. Those member variables are now chrono, so we updated this function.
I mean, why are you removing the doxygen comment?
That was unintentional. Fixed.
202 | @@ -203,14 +203,14 @@ static RPCHelpMan getpeerinfo() 203 | obj.pushKV("bytesrecv", stats.nRecvBytes); 204 | obj.pushKV("conntime", stats.nTimeConnected); 205 | obj.pushKV("timeoffset", stats.nTimeOffset); 206 | - if (stats.m_ping_usec > 0) { 207 | - obj.pushKV("pingtime", ((double)stats.m_ping_usec) / 1e6); 208 | + if (stats.m_ping_time.count() > 0) {
4089bc6489255ab26a350bbadc06d1cc5cff6d35: Can use time > 0us, which is shorter
Done
213 | + if (stats.m_min_ping_time < std::chrono::microseconds::max()) { 214 | + obj.pushKV("minping", count_microseconds(stats.m_min_ping_time) * 0.000001); 215 | } 216 | - if (stats.m_ping_wait_usec > 0) { 217 | - obj.pushKV("pingwait", ((double)stats.m_ping_wait_usec) / 1e6); 218 | + if (stats.m_ping_wait.count() > 0) {
same
Done
22 | @@ -23,7 +23,7 @@ FUZZ_TARGET(node_eviction) 23 | eviction_candidates.push_back({ 24 | fuzzed_data_provider.ConsumeIntegral<NodeId>(), 25 | fuzzed_data_provider.ConsumeIntegral<int64_t>(), 26 | - fuzzed_data_provider.ConsumeIntegral<int64_t>(), 27 | + std::chrono::microseconds{fuzzed_data_provider.ConsumeIntegral<int64_t>()}, 28 | fuzzed_data_provider.ConsumeIntegral<int64_t>(), 29 | fuzzed_data_provider.ConsumeIntegral<int64_t>(), 30 | fuzzed_data_provider.ConsumeBool(),
unrelated style-nit: Could use named args, like in src/test/net_tests.cpp?
Done
898 | @@ -899,10 +899,10 @@ BOOST_AUTO_TEST_CASE(node_eviction_test) 899 | // Combination of all tests above. 900 | BOOST_CHECK(!IsEvicted( 901 | number_of_nodes, [number_of_nodes](NodeEvictionCandidate& candidate) { 902 | - candidate.nKeyedNetGroup = number_of_nodes - candidate.id; // 4 protected 903 | - candidate.nMinPingUsecTime = candidate.id; // 8 protected 904 | - candidate.nLastTXTime = number_of_nodes - candidate.id; // 4 protected 905 | - candidate.nLastBlockTime = number_of_nodes - candidate.id; // 4 protected 906 | + candidate.nKeyedNetGroup = number_of_nodes - candidate.id; // 4 protected
if you are changing whitespace, it would be good to do it right. One space too much here
Oops. Done.
609 | @@ -610,9 +610,9 @@ void CNode::copyStats(CNodeStats &stats, const std::vector<bool> &m_asmap) 610 | } 611 | 612 | // Raw ping time is in microseconds, but show it to user as whole seconds (Bitcoin users should be well used to small numbers with many decimal places by now :) 613 | - stats.m_ping_usec = nPingUsecTime; 614 | - stats.m_min_ping_usec = nMinPingUsecTime; 615 | - stats.m_ping_wait_usec = count_microseconds(ping_wait); 616 | + stats.m_ping_time = m_ping_time; 617 | + stats.m_min_ping_time = m_min_ping_time;
two spaces before =
Done
44 | @@ -45,8 +45,8 @@ static constexpr std::chrono::seconds RELAY_TX_CACHE_TIME = std::chrono::minutes 45 | static constexpr std::chrono::seconds UNCONDITIONAL_RELAY_DELAY = std::chrono::minutes{2}; 46 | /** Headers download timeout expressed in microseconds 47 | * Timeout = base + per_header * (expected number of headers) */ 48 | -static constexpr int64_t HEADERS_DOWNLOAD_TIMEOUT_BASE = 15 * 60 * 1000000; // 15 minutes 49 | -static constexpr int64_t HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER = 1000; // 1ms/header 50 | +static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_BASE = std::chrono::minutes{15};
in commit aa30fdfcc4e8801c3abd74468f849f6124815e10: Can use 15min, which is shorter
Done
92 | @@ -93,8 +93,8 @@ static constexpr std::chrono::microseconds GETDATA_TX_INTERVAL{std::chrono::seco 93 | static const unsigned int MAX_GETDATA_SZ = 1000; 94 | /** Number of blocks that can be requested at any given time from a single peer. */ 95 | static const int MAX_BLOCKS_IN_TRANSIT_PER_PEER = 16; 96 | -/** Timeout in seconds during which a peer must stall block download progress before being disconnected. */ 97 | -static const unsigned int BLOCK_STALLING_TIMEOUT = 2; 98 | +/** Time during which a peer must stall block download progress before being disconnected. */ 99 | +static constexpr auto BLOCK_STALLING_TIMEOUT = std::chrono::seconds{2};
aa30fdfcc4e8801c3abd74468f849f6124815e10: 2s
Done
571 | @@ -572,9 +572,9 @@ struct CNodeState { 572 | pindexBestHeaderSent = nullptr; 573 | nUnconnectingHeaders = 0; 574 | fSyncStarted = false; 575 | - nHeadersSyncTimeout = 0; 576 | - nStallingSince = 0; 577 | - nDownloadingSince = 0; 578 | + m_headers_sync_timeout = std::chrono::microseconds{0}; 579 | + m_stalling_since = std::chrono::microseconds{0}; 580 | + m_downloading_since = std::chrono::microseconds{0};
aa30fdfcc4e8801c3abd74468f849f6124815e10: 0us on all three
Done
628 | + state->m_downloading_since = std::max(state->m_downloading_since, GetTime<std::chrono::microseconds>()); 629 | } 630 | state->vBlocksInFlight.erase(itInFlight->second.second); 631 | state->nBlocksInFlight--; 632 | - state->nStallingSince = 0; 633 | + state->m_stalling_since = std::chrono::microseconds{0};
same
Done
4382 | @@ -4383,7 +4383,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) 4383 | // Only actively request headers from a single peer, unless we're close to today. 4384 | if ((nSyncStarted == 0 && fFetch) || pindexBestHeader->GetBlockTime() > GetAdjustedTime() - 24 * 60 * 60) { 4385 | state.fSyncStarted = true; 4386 | - state.nHeadersSyncTimeout = count_microseconds(current_time) + HEADERS_DOWNLOAD_TIMEOUT_BASE + HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER * (GetAdjustedTime() - pindexBestHeader->GetBlockTime())/(consensusParams.nPowTargetSpacing); 4387 | + state.m_headers_sync_timeout = current_time + HEADERS_DOWNLOAD_TIMEOUT_BASE + HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER * ((GetAdjustedTime() - pindexBestHeader->GetBlockTime())/(consensusParams.nPowTargetSpacing));
why ((foo)) instead of (foo)?
Done
4737 | @@ -4738,13 +4738,13 @@ bool PeerManagerImpl::SendMessages(CNode* pto) 4738 | // this peer (eventually). 4739 | state.fSyncStarted = false; 4740 | nSyncStarted--; 4741 | - state.nHeadersSyncTimeout = 0; 4742 | + state.m_headers_sync_timeout = std::chrono::microseconds{0};
0us
Done
4767 | @@ -4768,8 +4768,8 @@ bool PeerManagerImpl::SendMessages(CNode* pto) 4768 | pindex->nHeight, pto->GetId()); 4769 | } 4770 | if (state.nBlocksInFlight == 0 && staller != -1) { 4771 | - if (State(staller)->nStallingSince == 0) { 4772 | - State(staller)->nStallingSince = count_microseconds(current_time); 4773 | + if (State(staller)->m_stalling_since.count() == 0) {
== 0us
Done
1787 | @@ -1788,7 +1788,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect) 1788 | // Note that we only do this if we started with an empty peers.dat, 1789 | // (in which case we will query DNS seeds immediately) *and* the DNS 1790 | // seeds have not returned any results. 1791 | - if (addrman.size() == 0 && (GetTime() - nStart > 60)) { 1792 | + if (addrman.size() == 0 && (GetTime<std::chrono::microseconds>() - start > std::chrono::minutes{1})) {
64f3fb1a966cfee47e6cfccdd5e9c9540bfebecc: > 1min
Done
2933 | 2934 | -int64_t PoissonNextSend(int64_t now, int average_interval_seconds) 2935 | +std::chrono::microseconds PoissonNextSend(std::chrono::microseconds now, std::chrono::microseconds average_interval) 2936 | { 2937 | - return now + (int64_t)(log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */) * average_interval_seconds * -1000000.0 + 0.5); 2938 | + double unscaled = log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */);
64f3fb1a966cfee47e6cfccdd5e9c9540bfebecc: Style nit: I'd say the minus goes conceptually before log1p: -log1p(...). Putting it in the round-to-integer step seems confusing, because the negation is unrelated to rounding.
That makes more sense. Done.
46 | @@ -47,10 +47,10 @@ static const bool DEFAULT_WHITELISTFORCERELAY = false; 47 | 48 | /** Time after which to disconnect, after waiting for a ping response (or inactivity). */ 49 | static const int TIMEOUT_INTERVAL = 20 * 60; 50 | -/** Run the feeler connection loop once every 2 minutes or 120 seconds. **/ 51 | -static const int FEELER_INTERVAL = 120; 52 | +/** Run the feeler connection loop once every 2 minutes. **/ 53 | +static constexpr auto FEELER_INTERVAL = std::chrono::minutes{2};
2min
Done
Not done yet in c0822a71b.
51 | -static const int FEELER_INTERVAL = 120; 52 | +/** Run the feeler connection loop once every 2 minutes. **/ 53 | +static constexpr auto FEELER_INTERVAL = std::chrono::minutes{2}; 54 | /** Run the extra block-relay-only connection loop once every 5 minutes. **/ 55 | -static const int EXTRA_BLOCK_RELAY_ONLY_PEER_INTERVAL = 300; 56 | +static constexpr auto EXTRA_BLOCK_RELAY_ONLY_PEER_INTERVAL = std::chrono::minutes{5};
5min
Done
1218 | @@ -1219,7 +1219,7 @@ class CConnman 1219 | */ 1220 | std::atomic_bool m_start_extra_block_relay_peers{false}; 1221 | 1222 | - std::atomic<int64_t> m_next_send_inv_to_incoming{0}; 1223 | + std::atomic<std::chrono::microseconds> m_next_send_inv_to_incoming{std::chrono::microseconds{0}};
0us
Done
118 | @@ -119,17 +119,17 @@ static const int MAX_UNCONNECTING_HEADERS = 10; 119 | /** Minimum blocks required to signal NODE_NETWORK_LIMITED */ 120 | static const unsigned int NODE_NETWORK_LIMITED_MIN_BLOCKS = 288; 121 | /** Average delay between local address broadcasts */ 122 | -static constexpr std::chrono::hours AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL{24}; 123 | +static constexpr auto AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL = std::chrono::hours{24};
24h
Done
118 | @@ -119,17 +119,17 @@ static const int MAX_UNCONNECTING_HEADERS = 10; 119 | /** Minimum blocks required to signal NODE_NETWORK_LIMITED */ 120 | static const unsigned int NODE_NETWORK_LIMITED_MIN_BLOCKS = 288; 121 | /** Average delay between local address broadcasts */ 122 | -static constexpr std::chrono::hours AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL{24}; 123 | +static constexpr auto AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL = std::chrono::hours{24}; 124 | /** Average delay between peer address broadcasts */ 125 | -static constexpr std::chrono::seconds AVG_ADDRESS_BROADCAST_INTERVAL{30}; 126 | +static constexpr auto AVG_ADDRESS_BROADCAST_INTERVAL = std::chrono::seconds{30};
30s
Done
125 | -static constexpr std::chrono::seconds AVG_ADDRESS_BROADCAST_INTERVAL{30}; 126 | +static constexpr auto AVG_ADDRESS_BROADCAST_INTERVAL = std::chrono::seconds{30}; 127 | /** Average delay between trickled inventory transmissions in seconds. 128 | * Blocks and peers with noban permission bypass this, outbound peers get half this delay. */ 129 | -static const unsigned int INVENTORY_BROADCAST_INTERVAL = 5; 130 | +static constexpr auto INVENTORY_BROADCAST_INTERVAL = std::chrono::seconds{5};
5s
Done
137 | @@ -138,9 +138,9 @@ static constexpr unsigned int INVENTORY_MAX_RECENT_RELAY = 3500; 138 | * peers, and random variations in the broadcast mechanism. */ 139 | static_assert(INVENTORY_MAX_RECENT_RELAY >= INVENTORY_BROADCAST_PER_SECOND * UNCONDITIONAL_RELAY_DELAY / std::chrono::seconds{1}, "INVENTORY_RELAY_MAX too low"); 140 | /** Average delay between feefilter broadcasts in seconds. */ 141 | -static constexpr unsigned int AVG_FEEFILTER_BROADCAST_INTERVAL = 10 * 60; 142 | +static constexpr auto AVG_FEEFILTER_BROADCAST_INTERVAL = std::chrono::minutes{10}; 143 | /** Maximum feefilter broadcast delay after significant change. */ 144 | -static constexpr unsigned int MAX_FEEFILTER_CHANGE_DELAY = 5 * 60; 145 | +static constexpr auto MAX_FEEFILTER_CHANGE_DELAY = std::chrono::minutes{5};
same for those two
Done
4563 | @@ -4564,10 +4564,10 @@ bool PeerManagerImpl::SendMessages(CNode* pto) 4564 | if (pto->m_tx_relay->nNextInvSend < current_time) { 4565 | fSendTrickle = true; 4566 | if (pto->IsInboundConn()) { 4567 | - pto->m_tx_relay->nNextInvSend = std::chrono::microseconds{m_connman.PoissonNextSendInbound(count_microseconds(current_time), INVENTORY_BROADCAST_INTERVAL)}; 4568 | + pto->m_tx_relay->nNextInvSend = m_connman.PoissonNextSendInbound(current_time, INVENTORY_BROADCAST_INTERVAL); 4569 | } else { 4570 | // Use half the delay for outbound peers, as there is less privacy concern for them. 4571 | - pto->m_tx_relay->nNextInvSend = PoissonNextSend(current_time, std::chrono::seconds{INVENTORY_BROADCAST_INTERVAL >> 1}); 4572 | + pto->m_tx_relay->nNextInvSend = PoissonNextSend(current_time, INVENTORY_BROADCAST_INTERVAL / 2);
Note to other reviewers: Division does not have any truncation warnings at compile time
Might be clearer to have separate constants for inbound/outbound inventory broadcast intervals so it's clear one's 5s and one is 2s?
Yes, that's indeed clearer. Updated.
4821 | @@ -4822,24 +4822,24 @@ bool PeerManagerImpl::SendMessages(CNode* pto) 4822 | if (pto->m_tx_relay->lastSentFeeFilter == MAX_FILTER) { 4823 | // Send the current filter if we sent MAX_FILTER previously 4824 | // and made it out of IBD. 4825 | - pto->m_tx_relay->nextSendTimeFeeFilter = count_microseconds(current_time) - 1; 4826 | + pto->m_tx_relay->m_next_send_feefilter = current_time - std::chrono::microseconds{1};
I think you can simply assign this to 0, as the goal is to send immediately
Done
39 | @@ -40,9 +40,9 @@ static constexpr int64_t ORPHAN_TX_EXPIRE_TIME = 20 * 60; 40 | /** Minimum time between orphan transactions expire time checks in seconds */ 41 | static constexpr int64_t ORPHAN_TX_EXPIRE_INTERVAL = 5 * 60; 42 | /** How long to cache transactions in mapRelay for normal relay */ 43 | -static constexpr std::chrono::seconds RELAY_TX_CACHE_TIME = std::chrono::minutes{15}; 44 | +static constexpr auto RELAY_TX_CACHE_TIME = std::chrono::minutes{15};
in commit c9a8cbc64eddd98d2183380148a3d9c79c1bd7d2: 15min
Done
left some style-nits. Feel free to ignore.
review ACK c9a8cbc64eddd98d2183380148a3d9c79c1bd7d2 🐉
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
review ACK c9a8cbc64eddd98d2183380148a3d9c79c1bd7d2 🐉
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjXQAwAmlnqHxcZHCTykusiZ3462hXgmsVzJZFoEXtZWK0hJqI4Dic/WgQ6gd+n
+AJlwBfJ56FmyVHXPa61h1njES+9cZfUvq4Ty5DbgVHAF3M7e0xz0WrUq2KUzRxf
xpKgVuWN1/aeHXGn6qRzvGAvjAA/6ozQ8H5HC0jnru5EWZX7FBDDgef1ApkIOd4I
YRx7D2iqHvcVPS/Di82+2UHjvBN789NMwB3MQDsB6lbEYzNaE2FVYrcdtXiepYk7
kqXhqWavBlubcPfRyXLRXbbVv/A0z0ZUhv2laaJuOMv/ssk/LXiOCE37aytLCnvV
AiLIlA64AFp8BDrykUtqaFqCTRbSQSELsHldAmIIe+gV9rF+6cVp8ClWMl/QRWjS
nZnChOANGW08/+t4KeiDKxRUHlwko29Y4NX4p3pVR99beMmTagqiN0Rflbrv6okA
aoKZdDLz0jUnARvfbib7vqbiNxvZWA6WBlfrnWSXpaTRohPWW3wWTaQc09yF2Qgw
Q8SDUsfg
=09jz
-----END PGP SIGNATURE-----
Timestamp of file with hash 21b8f1b48c83567a179a56af130b796e2cfb94ca44d7acdf169927f865176af6 -
</details>
Thanks for the review, @MarcoFalke. Comments addressed. Ready for further review.
Concept ACK
Pushed to fix doxygen comment.
40 | + /* fRelevantServices */ fuzzed_data_provider.ConsumeBool(), 41 | + /* fRelayTxes */ fuzzed_data_provider.ConsumeBool(), 42 | + /* fBloomFilter */ fuzzed_data_provider.ConsumeBool(), 43 | + /* nKeyedNetGroup */ fuzzed_data_provider.ConsumeIntegral<uint64_t>(), 44 | + /* prefer_evict */ fuzzed_data_provider.ConsumeBool(), 45 | + /* m_is_local */ fuzzed_data_provider.ConsumeBool(),
nit:
.id = fuzzed_data_provider.ConsumeIntegral<NodeId>(),
.nTimeConnected = fuzzed_data_provider.ConsumeIntegral<int64_t>(),
.m_min_ping_time = std::chrono::microseconds{fuzzed_data_provider.ConsumeIntegral<int64_t>()},
.nLastBlockTime = fuzzed_data_provider.ConsumeIntegral<int64_t>(),
.nLastTXTime = fuzzed_data_provider.ConsumeIntegral<int64_t>(),
.fRelevantServices = fuzzed_data_provider.ConsumeBool(),
.fRelayTxes = fuzzed_data_provider.ConsumeBool(),
.fBloomFilter = fuzzed_data_provider.ConsumeBool(),
.nKeyedNetGroup = fuzzed_data_provider.ConsumeIntegral<uint64_t>(),
.prefer_evict = fuzzed_data_provider.ConsumeBool(),
.m_is_local = fuzzed_data_provider.ConsumeBool(),
Designated initializers are already used elsewhere in the fuzz tests. Should be ok, even though it is a c++20 feature.
pls no. This is a bug in the upstream compilers, we shouldn't rely on that.
792 | for (int id = 0; id < n_candidates; ++id) {
793 | candidates.push_back({
794 | /* id */ id,
795 | /* nTimeConnected */ static_cast<int64_t>(random_context.randrange(100)),
796 | - /* nMinPingUsecTime */ static_cast<int64_t>(random_context.randrange(100)),
797 | + /* m_min_ping_time */ static_cast<std::chrono::microseconds>(random_context.randrange(100)),
nit, it is shorter than static_cast and consistent with the code afterwards (std::chrono::microseconds{candidate.id}):
.m_min_ping_time = std::chrono::microseconds{random_context.randrange(100)},
Seems fine to remove the cast, but please don't use the C++20 feature
Done.
4382 | @@ -4383,7 +4383,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) 4383 | // Only actively request headers from a single peer, unless we're close to today. 4384 | if ((nSyncStarted == 0 && fFetch) || pindexBestHeader->GetBlockTime() > GetAdjustedTime() - 24 * 60 * 60) { 4385 | state.fSyncStarted = true; 4386 | - state.nHeadersSyncTimeout = count_microseconds(current_time) + HEADERS_DOWNLOAD_TIMEOUT_BASE + HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER * (GetAdjustedTime() - pindexBestHeader->GetBlockTime())/(consensusParams.nPowTargetSpacing); 4387 | + state.m_headers_sync_timeout = current_time + HEADERS_DOWNLOAD_TIMEOUT_BASE + HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER * (GetAdjustedTime() - pindexBestHeader->GetBlockTime())/consensusParams.nPowTargetSpacing;
This looks wrong, but it is not! HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER is std::chrono::milliseconds while GetAdjustedTime() is int64_t seconds and we seem to be doing arithmetic with them. Maybe some comment would make it clearer?
state.m_headers_sync_timeout =
current_time + HEADERS_DOWNLOAD_TIMEOUT_BASE +
HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER *
// Expected number of headers [unitless].
(GetAdjustedTime() - pindexBestHeader->GetBlockTime()) /
consensusParams.nPowTargetSpacing;
Note: if we put the unitless expression in a variable, then it will do the division first and after that the multiplication which may produce different results.
Actually I think this changes the rounding calculation: HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER used to be 1000, in us. Now its 1 in milliseconds. The calculation HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER * x / y used to be in us precision, but now it is only in ms precision. So there is now a bit more of a rounding error than there was before
How so? GetAdjustedTime is divided by the time it takes for a header, so the result it the number of headers (no unit).
Multiplying a (no unit) with 1000us will give exactly (a)ms, just like multplying a (no unit) with 1ms will give exactly (a)ms.
This is correct
Could add ( and ) to clarify this:
state.m_headers_sync_timeout = current_time + HEADERS_DOWNLOAD_TIMEOUT_BASE + HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER * ((GetAdjustedTime() - pindexBestHeader->GetBlockTime())/consensusParams.nPowTargetSpacing);
The original calculation order was (HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER * ((GetAdjustedTime() - pindexBestHeader->GetBlockTime())) / consensusParams.nPowTargetSpacing, so (1000 * x) / y. The PR has basically changed this into (1 * x) / y. I don't think this is a problem, but the result might be off by up to 999us: https://godbolt.org/z/TEYood
not if you assume that x/y is in ℕ
oh wait, it isn't. Missed that (edit: :sweat_smile: )
I think making it:
state.m_headers_sync_timeout = current_time + HEADERS_DOWNLOAD_TIMEOUT_BASE +
(
std::chrono::microseconds(HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER) *
(GetAdjustedTime() - pindexBestHeader->GetBlockTime())
) / consensusParams.nPowTargetSpacing;
// GetAdjustedTime/GetBlockTime/nPowTargetSpacing return time in seconds as an int,
// so we get maximum precision by working in microseconds and multiplying first, then dividing.
would let you have the constexpr be 1ms while maintaining precision by still doing the calculation as ((HDTPH * (GAT-GBT)) / nPowTS) (even though that involves an intermediate value whose unit is "microseconds * seconds"). Converting GAT/GBT/nPowTS to use chrono::seconds would mean you'd need to wrap them with count_seconds() in addition, I think.
I've converted to microseconds, but left the parenthesis the same as before because it felt more intuitive to be multiplying HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER with something that was a measure of number of headers. C++ operator precedence for * and / is left -to-right so the math shouldn't change afaict.
Hmm, I'd still suggest adding a comment explaining the conversion to std::chrono::microseconds -- the usual point of chrono types is that it does the right conversions behind the scenes, so the fact it's getting done manually is weird and could use an explanation I think.
Done.
39 | @@ -40,13 +40,13 @@ static constexpr int64_t ORPHAN_TX_EXPIRE_TIME = 20 * 60; 40 | /** Minimum time between orphan transactions expire time checks in seconds */ 41 | static constexpr int64_t ORPHAN_TX_EXPIRE_INTERVAL = 5 * 60; 42 | /** How long to cache transactions in mapRelay for normal relay */ 43 | -static constexpr std::chrono::seconds RELAY_TX_CACHE_TIME = std::chrono::minutes{15}; 44 | +static constexpr auto RELAY_TX_CACHE_TIME = 15min; 45 | /** How long a transaction has to be in the mempool before it can unconditionally be relayed (even when not in mapRelay). */ 46 | -static constexpr std::chrono::seconds UNCONDITIONAL_RELAY_DELAY = std::chrono::minutes{2}; 47 | +static constexpr auto UNCONDITIONAL_RELAY_DELAY = std::chrono::minutes{2}; 48 | /** Headers download timeout expressed in microseconds
/** Headers download timeout.
Done
48 | /** Headers download timeout expressed in microseconds 49 | * Timeout = base + per_header * (expected number of headers) */ 50 | -static constexpr int64_t HEADERS_DOWNLOAD_TIMEOUT_BASE = 15 * 60 * 1000000; // 15 minutes 51 | -static constexpr int64_t HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER = 1000; // 1ms/header 52 | +static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_BASE = 15min; 53 | +static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER = std::chrono::milliseconds{1};
static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER = 1ms;
Done
4701 | LogPrintf("Peer=%d is stalling block download, disconnecting\n", pto->GetId());
4702 | pto->fDisconnect = true;
4703 | return true;
4704 | }
4705 | - // In case there is a block that has been in flight from this peer for 2 + 0.5 * N times the block interval
4706 | + // In case there is a block that has been in flight from this peer for 1 + 0.5 * N times the block interval
nit: this uses * and times to denote multiplication:
// In case there is a block that has been in flight from this peer for (the block interval) * (1 + 0.5 * N)
Done
2933 | 2934 | -int64_t PoissonNextSend(int64_t now, int average_interval_seconds) 2935 | +std::chrono::microseconds PoissonNextSend(std::chrono::microseconds now, std::chrono::microseconds average_interval) 2936 | { 2937 | - return now + (int64_t)(log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */) * average_interval_seconds * -1000000.0 + 0.5); 2938 | + double unscaled = -log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */);
-x * -y should be the same as x * y:
double unscaled = log1p(GetRand(1ULL << 48) * 0.0000000000000035527136788 /* -1/2^48 */);
no. log1p(-a) is not equal to -log1p(a)
Doh, I misread this as -log1p(a) * -b.
125 | -static constexpr std::chrono::hours AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL{24}; 126 | +static constexpr auto AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL = 24h; 127 | /** Average delay between peer address broadcasts */ 128 | -static constexpr std::chrono::seconds AVG_ADDRESS_BROADCAST_INTERVAL{30}; 129 | +static constexpr auto AVG_ADDRESS_BROADCAST_INTERVAL = 30s; 130 | /** Average delay between trickled inventory transmissions in seconds.
/** Average delay between trickled inventory transmissions.
Done.
103 | @@ -104,7 +104,9 @@ FUZZ_TARGET_INIT(connman, initialize_connman) 104 | }, 105 | [&] { 106 | // Limit now to int32_t to avoid signed integer overflow 107 | - (void)connman.PoissonNextSendInbound(fuzzed_data_provider.ConsumeIntegral<int32_t>(), fuzzed_data_provider.ConsumeIntegral<int>()); 108 | + (void)connman.PoissonNextSendInbound( 109 | + std::chrono::microseconds{fuzzed_data_provider.ConsumeIntegral<int32_t>()}, 110 | + std::chrono::microseconds{fuzzed_data_provider.ConsumeIntegral<int>()});
The meaning of the second argument was "seconds" before, to keep the behavior:
(void)connman.PoissonNextSendInbound(
std::chrono::microseconds{fuzzed_data_provider.ConsumeIntegral<int32_t>()},
std::chrono::seconds{fuzzed_data_provider.ConsumeIntegral<int>()});
I think it is fine to change the meaning with the benefit that the fuzz test will be also grinding at a smaller resolution, which is more likely to be used. I don't think we have a poisson delay of int::max() seconds anywhere. And I don't think it makes sense in practice.
Left it as-is.
39 | @@ -40,13 +40,13 @@ static constexpr int64_t ORPHAN_TX_EXPIRE_TIME = 20 * 60; 40 | /** Minimum time between orphan transactions expire time checks in seconds */ 41 | static constexpr int64_t ORPHAN_TX_EXPIRE_INTERVAL = 5 * 60; 42 | /** How long to cache transactions in mapRelay for normal relay */ 43 | -static constexpr std::chrono::seconds RELAY_TX_CACHE_TIME = std::chrono::minutes{15}; 44 | +static constexpr auto RELAY_TX_CACHE_TIME = 15min; 45 | /** How long a transaction has to be in the mempool before it can unconditionally be relayed (even when not in mapRelay). */ 46 | -static constexpr std::chrono::seconds UNCONDITIONAL_RELAY_DELAY = std::chrono::minutes{2}; 47 | +static constexpr auto UNCONDITIONAL_RELAY_DELAY = std::chrono::minutes{2};
static constexpr auto UNCONDITIONAL_RELAY_DELAY = 2min;
Done
Approach ACK c0822a71b
202 | @@ -203,14 +203,14 @@ static RPCHelpMan getpeerinfo() 203 | obj.pushKV("bytesrecv", stats.nRecvBytes); 204 | obj.pushKV("conntime", stats.nTimeConnected); 205 | obj.pushKV("timeoffset", stats.nTimeOffset); 206 | - if (stats.m_ping_usec > 0) { 207 | - obj.pushKV("pingtime", ((double)stats.m_ping_usec) / 1e6); 208 | + if (stats.m_ping_time > 0us) { 209 | + obj.pushKV("pingtime", count_microseconds(stats.m_ping_time) * 0.000001);
Maybe use this to get rid of the hardcoded constant:
obj.pushKV("pingtime", std::chrono::duration<double>{stats.m_ping_time}.count());
That'd be a breaking RPC change if someone changed the type of m_ping_time to nanoseconds. Unlikely here, but plausible for other durations, so I'd rather not encourage such code.
auto us = 500000us;
auto ns = 500000000ns;
std::cout << us.count() * 0.000001 << std::endl;
std::cout << std::chrono::duration<double>{us}.count() << std::endl;
std::cout << std::chrono::duration<double>{ns}.count() << std::endl;
all 3 print 0.5.
Hmm, then it must be broken in another way. There is no way the compiler can figure out what time unit double should represent in the Univalue object.
Oh, it will always represent the result in seconds? Seems fair enough, but it would be good to wrap that into a helper method, so that the C++ docs don't have to be looked up every time this is used. (This relies on the default ration being 1)
all 3 print
0.5.
yes, the only difference I see should be that count_microseconds won't accept a nanoseconds argument and you'd get a compile error in that case. But the std::chrono::duration<double>(...) should always work. Also the round() should be more correct than + 0.5 for negative durations, but that shouldn't matter here
Oh, it will always represent the result in seconds
yes, std::chrono duration's default to std::ratio<1> which is seconds
no millis, no nanos, no kilos, no megas, just seconds :)
What about adding SecondsFloat, similar to the std::chrono::seconds helper type?
using SecondsFloat = std::chrono::duration<double>;
inline auto count_seconds_f(SecondsFloat t) { return t.count(); }
I like the idea. Shouldn't it be CountSeconds() to follow the naming convention? I think Float in the name could be confusing because it is using double, not float.
I'd personally prefer just std::chrono::duration<double>, but most likely because I've recently done a lot of chrono related refactoring at work so I'm already used to what this means :smile:
Thanks for teaching me through code review! I essentially used @MarcoFalke's approach, but made the ratio and the return type explicit for the reader.
2935 | +std::chrono::microseconds PoissonNextSend(std::chrono::microseconds now, std::chrono::microseconds average_interval) 2936 | { 2937 | - return now + (int64_t)(log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */) * average_interval_seconds * -1000000.0 + 0.5); 2938 | + double unscaled = -log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */); 2939 | + double scaled = unscaled * count_microseconds(average_interval); 2940 | + return now + std::chrono::microseconds{int64_t(scaled + 0.5)};
This could directly use chrono features for this:
return now + std::chrono::round<std::chrono::microseconds>(unscaled * average_interval);
Thanks! Done.
Review ACK c0822a71b5fabb39e6bbf7355b69f9f1b9264fde, I have not tested the code, looked through it and found only nits
1767 | @@ -1768,11 +1768,11 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect) 1768 | } 1769 | 1770 | // Initiate network connections 1771 | - int64_t nStart = GetTime(); 1772 | + auto start = GetTime<std::chrono::microseconds>();
I think GetTime should actually return a std::chrono::time_point instead of a duration. But that's probably out of scope for this PR. Then we'd have a few more compile time checks concerning the allowed operations (e.g. you can't add two timepoints, you can't accidentally compare a timepoints with a duration and you can't accidentally mix timepoints from different clocks)
I've had a look at this a while ago: https://github.com/ajtowns/bitcoin/commits/201908-systime It's particularly nice that your types can then encode what sort of time you've got -- if it's a high precision clock, if it's a stable clock, or, for us, if it's a mockable clock. One drawback is that time_point's aren't compatible with atomic, so you have to hack around that. I think we'd want to think some more about exactly what sorts of times we care about before doing that conversion seriously.
Thank you for the thorough review, @vasild @martinus and @MarcoFalke!
Comments addressed. Ready for review.
4821 | @@ -4822,24 +4822,24 @@ bool PeerManagerImpl::SendMessages(CNode* pto) 4822 | if (pto->m_tx_relay->lastSentFeeFilter == MAX_FILTER) { 4823 | // Send the current filter if we sent MAX_FILTER previously 4824 | // and made it out of IBD. 4825 | - pto->m_tx_relay->nextSendTimeFeeFilter = count_microseconds(current_time) - 1; 4826 | + pto->m_tx_relay->m_next_send_feefilter = 0us;
Was the logic changed on purpose?
Behaviour shouldn't change: #21015 (review)
Yeah, in the way it is used it will behave the same as long as m_next_send_feefilter is in the past.
ACK e5fb29086d3c0502ec63c1890b6b56be5cb7b1d8 looked through all changes and looks good, did not run the code though.
609 | @@ -610,9 +610,9 @@ void CNode::copyStats(CNodeStats &stats, const std::vector<bool> &m_asmap) 610 | } 611 | 612 | // Raw ping time is in microseconds, but show it to user as whole seconds (Bitcoin users should be well used to small numbers with many decimal places by now :) 613 | - stats.m_ping_usec = nPingUsecTime; 614 | - stats.m_min_ping_usec = nMinPingUsecTime; 615 | - stats.m_ping_wait_usec = count_microseconds(ping_wait); 616 | + stats.m_ping_time = m_ping_time; 617 | + stats.m_min_ping_time = m_min_ping_time; 618 | + stats.m_ping_wait = ping_wait;
X(m_ping_time); X(m_min_ping_time); ?
Done.
26 | @@ -27,6 +27,12 @@ void UninterruptibleSleep(const std::chrono::microseconds& n); 27 | inline int64_t count_seconds(std::chrono::seconds t) { return t.count(); } 28 | inline int64_t count_microseconds(std::chrono::microseconds t) { return t.count(); } 29 | 30 | +/** 31 | + * Helper to count the seconds in any std::chrono::duration type 32 | + */ 33 | +using SecondsDouble = std::chrono::duration<double, std::ratio<1, 1>>; 34 | +inline double CountSeconds(SecondsDouble t) { return t.count(); }
Why is this CountSeconds not count_seconds_double to match the existing count_microseconds and count_seconds?
To my understanding, new code should follow the naming convention:
Class names, function names, and method names are UpperCamelCase
instead of trying to mimic existent code.
That was my reasoning as well.
Personally I think this is an example of where the naming convention is buggy rather than the code; but especially if int64_t count_seconds(seconds) should be int64_t CountSeconds(seconds) we shouldn't be introducing double CountSeconds(SecondsDouble) with a name clash like that -- would CountSeconds(micros) return an int or double?
I'd personally just prefer std::chrono::duration<double>(t).count() instead of a helper.
Renamed to CountSecondsDouble
47 | +/** Headers download timeout. 48 | * Timeout = base + per_header * (expected number of headers) */ 49 | -static constexpr int64_t HEADERS_DOWNLOAD_TIMEOUT_BASE = 15 * 60 * 1000000; // 15 minutes 50 | -static constexpr int64_t HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER = 1000; // 1ms/header 51 | +static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_BASE = 15min; 52 | +static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER = 1000us;
Why isn't this 1ms ?
I see it's mentioned in review comments for code below, but if the precision is actually needed, it should be std::chrono::microseconds not auto and the reason should probably be commented here too.
See also #21015 (review)
Thanks! Done.
re-ACK e5fb29086d3c0502ec63c1890b6b56be5cb7b1d8 changes since last review: 🦐
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
re-ACK e5fb29086d3c0502ec63c1890b6b56be5cb7b1d8 changes since last review: 🦐
* Restore doxygen comment
* Use chrono literals where possible
* Add "named initializer comments" in fuzz tests
* Adding missing negation of log1p result
* Using std::chrono::round instead of own round implementation
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiNOgv/esnDzAD/Kt43N+9AyPE/F2YIwzavHnuWrj/SXUud1w6esEg2hIyXtLQV
4XaR+pDY04odSoU/ug7hIDf6YuMjP+/9GP7H0NOmVdNpVRtmTwrlRyQWj0C7ywz4
zwmU5cymrTY72AdR9kY4KqDbSoYoUfsVduxRfJc4zV94TvddXKjHkDEx5dK1lM7I
ErYKjiXovXU92giHP9WB9VEwNrQ0I771whBYtQGceGMUMCq6Yv1LhCG3YUzK1ywN
YLdjr6dKO4h4xl19aRu4NXh6g6c70k9o8Jdw9slska93nMT+hNH9EOVnhpU1Oc5w
kUC3CVZlsJFx+ko/wtYJNnETyRiHkkFrxnCf7RdoS7zSaeIdoZ/Ib7cfXj11CxX3
CNEkJ1lBzaWt80zmAaFOnvRHlcnPBY0njOcydJB86ZZMu5/NUo0iKIopCl61MDtB
ApbnZZ0i1u/+P6DGdlC+MsSCjeG3FZjFvP6iPWv18bximVEeRNZc2FXT0usZspSl
H7g3SvAP
=VrHQ
-----END PGP SIGNATURE-----
Timestamp of file with hash 4921d55322dc9a709473933b73c325e72d8e67b146d2eb0c9da16764d7486ae0 -
</details>
49 | +/** Headers download timeout. 50 | * Timeout = base + per_header * (expected number of headers) */ 51 | -static constexpr int64_t HEADERS_DOWNLOAD_TIMEOUT_BASE = 15 * 60 * 1000000; // 15 minutes 52 | -static constexpr int64_t HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER = 1000; // 1ms/header 53 | +static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_BASE = 15min; 54 | +static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER = 1000us;
it would be nicer to cast to the desired and wanted resolution where needed in the source code. Otherwise the move to chrono doesn't feel like an improvement. Image this being changed to 200ms, you'd have to type 200*1000 here again.
Done.
30 | +constexpr inline int64_t count_microseconds(std::chrono::microseconds t) { return t.count(); } 31 | + 32 | +/** 33 | + * Helper to count the seconds in any std::chrono::duration type 34 | + */ 35 | +using SecondsDouble = std::chrono::duration<double, std::ratio<1, 1>>;
nit: Would be nice to inherit the period from "upstream"
diff --git a/src/util/time.h b/src/util/time.h
index deb9ceefe2..f1b2beb0f3 100644
--- a/src/util/time.h
+++ b/src/util/time.h
@@ -30,7 +30,7 @@ constexpr inline int64_t count_microseconds(std::chrono::microseconds t) { retur
/**
* Helper to count the seconds in any std::chrono::duration type
*/
-using SecondsDouble = std::chrono::duration<double, std::ratio<1, 1>>;
+using SecondsDouble = std::chrono::duration<double, std::chrono::seconds::period>;
inline double CountSeconds(SecondsDouble t) { return t.count(); }
/**
Done
left to style-nits. Feel free to ignore
31 | + 32 | +/** 33 | + * Helper to count the seconds in any std::chrono::duration type 34 | + */ 35 | +using SecondsDouble = std::chrono::duration<double, std::ratio<1, 1>>; 36 | +inline double CountSeconds(SecondsDouble t) { return t.count(); }
nit: in the doxygen-generated documentation the comment will be assigned to SecondsDouble and CountSeconds will be left undocumented.
/** These are the coordinates of the buzz point in the hyper plane. */
int x;
int y;
this looks ok in the source code but will produce unexpected result in the doxygen docs, leaving y undocumented.
Thanks! Done.
ACK e5fb29086d3c0502ec63c1890b6b56be5cb7b1d8
4710 | // to unreasonably increase our timeout.
4711 | if (state.vBlocksInFlight.size() > 0) {
4712 | QueuedBlock &queuedBlock = state.vBlocksInFlight.front();
4713 | int nOtherPeersWithValidatedDownloads = nPeersWithValidatedDownloads - (state.nBlocksInFlightValidHeaders > 0);
4714 | - if (count_microseconds(current_time) > state.nDownloadingSince + consensusParams.nPowTargetSpacing * (BLOCK_DOWNLOAD_TIMEOUT_BASE + BLOCK_DOWNLOAD_TIMEOUT_PER_PEER * nOtherPeersWithValidatedDownloads)) {
4715 | + if (current_time > state.m_downloading_since + std::chrono::seconds{consensusParams.nPowTargetSpacing} * (BLOCK_DOWNLOAD_TIMEOUT_BASE + BLOCK_DOWNLOAD_TIMEOUT_PER_PEER * nOtherPeersWithValidatedDownloads)) {
I think this is changing: micros > micros + seconds * (millions + millions * count) to micros > micros + (seconds * (double + double * count))*scale which doesn't really seem right to me.
Leaving BLOCK_DOWNLOAD_TIMEOUT_{BASE,PER_PEER} as they are, documenting that they're measured in microseconds-per-second (and thus are int's not chrono::durations), and wrapping the entire nPowTargetSpacing * (...) in a std::chrono::microseconds{ .. } cast/constructor seems like a more accurate refactor to me.
which doesn't really seem right to me.
Do you mean mathematically or conceptually on a meta level?
Mathematically, it seems correct. Since all variables are constants, we have:
600 * ( 1000000 + 500000 * nOtherPeersWithValidatedDownloads) (unit is us, implicitly)600 * (1. + .5 * nOtherPeersWithValidatedDownloads), (unit is s, explicitly. Converted by the compiler to us by multiplying with 1kk)The difference between the two is zero:
As @MarcoFalke mentioned, the units do make sense to me. Also BLOCK_DOWNLOAD_TIMEOUT_BASE in multiples of block intervals seems easier to understand?
"doesn't really seem right" -- yeah, more along the lines of "morally right" vs "mathematically correct" :)
In general using doubles where they're not needed just seems like a bad idea to me -- there's way more possible special cases and pitfalls with them, and the code already works fine without them, so why change it? Changing behaviour in potentially subtle ways in refactors bothers me in general -- to me that seems like the easiest way for a potential attacker to slip vulnerabilities into the codebase.
For example, in this line, I think behind the scenes chrono is:
chrono::seconds version of nPowTargetSpacing to duration<double,ratio=1,1> when multiplying by a doublem_downloading_since and the result of the above to duration<double,ratio=1,1000000> (converting one to a double, multiplying the other by 1M)current_time to the duration<double,ratio=1,1000000> (ie conveting to a double) as well and then doing a floating point comparison between the twoAre those all fine? Yeah, I think so -- but why add the burden of reviewing all that just to use "0.5" instead of "500000"? In general, figuring out what the code actually does is the tricky part, so making it easy to see what the plan was and hard to see what the compiler's going to do seems backwards to me in general. After all, if it's not clear from the code what the plan was, you can add a comment.
What I'd prefer is something more like:
constexpr Fraction BLOCK_DOWNLOAD_TIMEOUT_BASE{1};
constexpr Fraction BLOCK_DOWNLOAD_TIMEOUT_PER_PEER{1,2};
so that you can define the constants meaningfully, but still have all the calculations done with integer math. You can make that actually work relatively easily:
class Fraction {
public:
constexpr Fraction(int64_t n, int64_t d) : num{n}, den{d} { }
explicit constexpr Fraction(int64_t n) : num{n}, den{1} { }
int64_t num = 0, den = 1;
};
constexpr Fraction operator+(Fraction l, Fraction r) { return Fraction{l.num*r.den + r.num*l.den, l.den*r.den}; }
constexpr Fraction operator*(Fraction l, int64_t n) { return Fraction{l.num*n, l.den}; }
constexpr std::chrono::microseconds operator*(std::chrono::microseconds t, Fraction f) { return t*f.num/f.den; }
but maintaining a special class for this seems crazy, and C++ stdlib seems to only provide compile-time rationals (via std::ratio) which is useless here, and boost::rational seems to insist on normalising the denominator for no particular reason (and the fixedpoint stuff seems even more overkill). Still not convinced having floating point math is a win compared to just having 1M and 500k as constants, but there doesn't seem to be any better alternatives than those.
2934 | -int64_t PoissonNextSend(int64_t now, int average_interval_seconds) 2935 | +std::chrono::microseconds PoissonNextSend(std::chrono::microseconds now, std::chrono::microseconds average_interval) 2936 | { 2937 | - return now + (int64_t)(log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */) * average_interval_seconds * -1000000.0 + 0.5); 2938 | + double unscaled = -log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */); 2939 | + return now + std::chrono::round<std::chrono::microseconds>(unscaled * average_interval);
I'm not sure it makes much sense to have the average interval measured in microseconds rather than seconds -- poisson timings are for things that don't happen too frequently. With the different types, you get compile time errors if you swap the arguments or if you try setting the poisson interval far too low.
Looking at godbolt, using chrono::round seems to produce much more complicated assembly than doing the math by hand (perhaps due to rounding towards an even value, rather than [0,0.5) going down and [0.5,1.0) going up?). Having it be:
std::chrono::microseconds PoissonNextSend(std::chrono::microseconds now, std::chrono::seconds average_interval)
{
int64_t delta = (int64_t)(log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */) * (-1.0 * count_microseconds(average_interval)) + 0.5);
return now + std::chrono::microseconds{ delta };
}
seems like a simpler way to switch the Poisson intervals to chrono types.
It does seem better to have the average interval in seconds. Updated.
I felt that the code reads cleaner with chrono::round and have left it in for now. If other reviewers also think more optimal assembly is necessary here, I'll be happy to change it.
Optimal assembly doesn't matter so much (we're calculating log1p at the same time which presumably isn't that quick), it's more that it's not particularly obvious to me that using chrono::round doesn't introduce a bug, and I don't see any need for the change. Obviously if it does introduce a bug, that would be the compiler's fault rather than yours per se, but that happens too.
I think an alternative way of writing what we currently do would be:
double unscaled = -log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */);
return now + std::chrono::duration_cast<std::chrono::microseconds>(unscaled * average_interval + 0.5us);
It doesn't generate precisely the same asm as the current code because the "-" isn't attached to the factor of 1M anymore but I think that's the only difference, so it seems easier to be sure that's actually not changing any behaviour.
(I don't think there's any real value in rounding to the nearest microsecond rather than just truncating -- everything we do ends up being at intervals of 50,000 or 100,000 microseconds anyway)
EDIT: (0.5*1us -> 0.5us)
There's definitely a tradeoff between the probability of introducing a bug and modernizing our codebase. When we choose to embrace a new stdlib or language feature, I'd propose we should do it relatively consistently (lower level code will be more resistant). It seems we're adopting std::chrono, so std::chrono::round is not that different.
I think if we can't trust the compiler to produce correct code, and can't trust our test to catch the issue either, there is nothing we can do preemptively other than write more tests. Choosing one version of the code over another in the absence of any known compiler miscompilation bugs doesn't seem too helpful to me, because we don't know if any of our dependencies use that piece of code in either a header or library.
(I am not saying which version to pick here. I am happy to review and ACK any version that does the right thing)
It seems we're adopting
std::chrono, sostd::chrono::roundis not that different.
I just explained why std::chrono::round is different -- it does different, substantially more complicated logic than the current code [0]. Converting to std::chrono::duration doesn't do different logic to just having an int64_t, it just adds type safety to help avoid mixing different scales. We don't need to use ::round to use std::chrono-- we can do duration_cast to truncate instead. Rounding to even has its place (https://mathematica.stackexchange.com/questions/2116/why-round-to-even-integers) but none of those reasons apply here.
[0] It's kind-of weird that it does more logic -- round to even is a standard IEEE754 rounding mode so it ought not to require custom template code or result in complicated assembly.
Choosing one version of the code over another in the absence of any known compiler miscompilation bugs
In the absence of a good reason to make a change, don't make a change. If there's two approaches that achieve the same result, choose the simpler one.
I have now actually benchmarked the PoissonNextSend in 3 different variants: original, round, duration_cast. Here are the results:
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark | |
|---|---|---|---|---|---|---|---|---|---|---|
| 2,338.00 | 427,716.00 | 0.4% | 6,721.29 | 7,467.00 | 0.900 | 193.00 | 0.5% | 0.00 | old |
|
| 2,380.33 | 420,109.23 | 1.8% | 6,749.57 | 7,603.17 | 0.888 | 199.29 | 0.4% | 0.00 | round |
|
| 2,338.12 | 427,693.13 | 0.3% | 6,749.00 | 7,486.00 | 0.902 | 199.29 | 0.4% | 0.00 | duration_cast |
The runtime difference here unmeasurable. The overhead is completely dwarfed by GetRand. When I then replaced GetRand with the simplest possible random generator I could think of:
inline uint64_t myRand(uint64_t maxnum) {
static uint64_t x = 1;
auto tmp = x % maxnum;
x *= 123123121;
return tmp;
}
Ran the benchmark again:
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark | |
|---|---|---|---|---|---|---|---|---|---|---|
| 18.43 | 54,269,715.35 | 0.4% | 110.27 | 58.90 | 1.872 | 13.78 | 4.5% | 0.00 | original |
|
| 24.60 | 40,642,221.28 | 0.3% | 143.80 | 78.60 | 1.830 | 20.85 | 3.1% | 0.00 | round |
|
| 24.55 | 40,724,958.20 | 0.3% | 143.82 | 78.57 | 1.831 | 20.85 | 3.0% | 0.00 | duration_cast |
There is a slight difference between duration_cast and round, but hardly significant.
So at least on my Intel CPU there is no performance argument for duration_cast in this case.
With no material performance difference, my preference aside, I do not see a reason to hold up this PR. Updated the code to use duration_cast.
In the absence of a good reason to make a change, don't make a change. If there's two approaches that achieve the same result, choose the simpler one.
Obviously I agree with that statement, but it doesn't seem applicable here. First, it doesn't look like the duration cast produces the same binary for me: https://godbolt.org/z/nejf46. And second, the round variant is simpler and shorter code.
I believe the differences are that average_interval goes from int to int64_t precision, and 0.5us is a long double while 0.5 is a double. I think replacing chrono::round<chrono::microseconds> with:
template<typename Dur, typename Dur2>
constexpr Dur chrono_round_half_up(Dur2 dur)
{
return std::chrono::duration_cast<Dur>(Dur{1}*0.5 + dur);
}
will generate the same code that we currently get on clang (except for average_interval_seconds being int vs int64_t): https://godbolt.org/z/rxjdc8 . gcc seems to generate different code due to negating the log1p result then multiplying by 1M rather than just multiplying by -1M.
[EDIT: function can be constexpr]
Looks good, not there yet though imo.
Thanks, @MarcoFalke @ajtowns @vasild.
Rebased. Comments addressed. Ready for further review.
Fixed a test and pushed - ready for further review. Test failure for test/fuzz/system.cpp seems to be unrelated and happening on other recent PRs as well.
ACK 054c6c29366a7f503554d9c85e6fd231008f2f20
23 | @@ -24,8 +24,15 @@ void UninterruptibleSleep(const std::chrono::microseconds& n); 24 | * This helper is used to convert durations before passing them over an 25 | * interface that doesn't support std::chrono (e.g. RPC, debug log, or the GUI) 26 | */ 27 | -inline int64_t count_seconds(std::chrono::seconds t) { return t.count(); } 28 | -inline int64_t count_microseconds(std::chrono::microseconds t) { return t.count(); } 29 | +constexpr inline int64_t count_seconds(std::chrono::seconds t) { return t.count(); } 30 | +constexpr inline int64_t count_microseconds(std::chrono::microseconds t) { return t.count(); }
constexpr int64_t count_microseconds(std::chrono::microseconds t) { return t.count(); }
nit: constexpr implies inline, so no need to specify twice
Done.
re-ACK 054c6c29366a7f503554d9c85e6fd231008f2f20 changes since last review: 👺
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
re-ACK 054c6c29366a7f503554d9c85e6fd231008f2f20 changes since last review: 👺
* Use X macro where possible
* Use std::chrono::seconds::period where possible
* Replace 1000us with 1ms and cast to us where needed
* Use seconds as type for poisson average interval
* Split up inbound/outbound avg interval
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgN/gv9H2XeUVZtB1zj0HIuOVbmki5BuvT6oGnWiKgnzZERJ/zBpbLghWP0snBG
05IunLcL/atno/SNQ6w7B0IXZOtCHiCBgA8nbbEhTngkLY8oLn+rgyAlpc14NGHQ
X3Du9itIuVBjVfVt9v6qK1ywYSdUexgg38XItqgq2UwANXV9CP2koavvZfeiEs48
im/aE92SDUo9cGpx1SjKHFAOc4fnCuWfyuElgnfe3kahYwOoygpdCemQhfo1vyHO
MHMs/EVjhWM9xdpGJ2fPCjrHQbfCz4UdVcouxnqdubM79/7O77EFPMPYMMm+4RhT
yxumFZGX4iSacrwHKdaGB1bh0kA4Lor+zFqu+q5nF13Nj3CI91scaHUGIXc0pqJG
1zbvr/Uks2g1lcJCRoeO04WaZxNKWe5xGHYPWdrdXs9OC+UPnKWAC8PkLdVwHfKH
rN+EUHfsARfLVIYDfq3247k4DCZUM4zipp/B03Nndo1QwqBFvBxpcpcgqaE8eehE
fUIk46tK
=2UZp
-----END PGP SIGNATURE-----
Timestamp of file with hash 2a4b9786606b664d2c4b408f943adf429fbf9af7568a63e4d184c0ab0674bedf -
</details>
Rebased. Comment addressed. Ready for further review.
Codereview ACK 7b57704
re-ACK 7b577049fe87a20f6d12fb7ce4e7d9941d971de1 only change is removing redundant inline 🎥
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
re-ACK 7b577049fe87a20f6d12fb7ce4e7d9941d971de1 only change is removing redundant inline 🎥
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjaCQv/eNRybP+jarAixi/2KGfs6eWdq5SLdk9Vrgz5WQg9o0IW4QMh5Hcs0515
OGgZ2bI6MtSr/BioiFWsfbfvu0OyLonkfKAuQ4TBhkhlRCTXPsG0pq2RiGfaevWC
h79FhANZdnyYpDAAdOvQ2DmPsN6qnymMq3XjWDlgUZAXeSpS2hG5Df0EKsIGicx4
d9EUOH2safkGaXQDwlQpKRlmlIO7OoXYvHhg6JnJy/WrGB3aiyFw+OBs5EdlhGV3
PxwCDbkWBtKqTXtyt/Qvrbzg5tFTzh4x0YF70NxMuKO0SJ2MXOmCDnQ3JjnZqO/U
KoNKcDHn+JSIHducBWVuDcLjTBOa6/RWs9kfZJsCx/zGoqBHbCPfenn6DHZS0g4a
72k6Qu9Nax14WdsF6iejGIebCVmgD3NR1MYu+FPvzuTGwzfcXM7EhdCNl2QLGRET
IFP0YsVwlPNjhHT2suOi70Ccxa2DfTYP5QhEY4sD0BJjl+WnTWeQnE2c4KNqmjfA
bvyLfPAi
=zPnG
-----END PGP SIGNATURE-----
Timestamp of file with hash f12ad6bf5ab4e44d86abf6dc1d7b5e4d1055bbe1d0a8c92f66c2328ae35c674f -
</details>
ACK 7b577049fe87a20f6d12fb7ce4e7d9941d971de1
Rebased. Ready for further review.
Fuzz tests on CI are timing out. I tried re-running but that did not help and I do not believe the failure to be related to this PR.
ci timeout is unrelated and can be ignored
re-ACK 7b6775ba7e2303128743db55e769e3339b3fc28f only changes are rename to CountSecondsDouble, adding a comment, rebase 👛
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
re-ACK 7b6775ba7e2303128743db55e769e3339b3fc28f only changes are rename to CountSecondsDouble, adding a comment, rebase 👛
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhRhgwArVe9YuYCz8vG/HmuQ1fTwAMMq4rn+C3DYUi2xXGzKJvkESpCCeXfuR6v
rrJF1Jec0Oa4okln8GoNtnweBDMpJKUZ+hecuCF1zkz8QP/Krifps/HdE95cpDEr
fZD1Y6PtpARGQZ4fLs4AF1XixA5iwT7HemwQ7r1c2AIx4AqNogGts4ysOd7+aRTd
N94vnABZqiXnwFrwosMfXGTvsmnnxe2UpjI7oZV8zUOeJTna4GZnsxTwKrgaqPLD
8igcQJmLmfy9bvxab7nyCzfgU1CziA+L9RXov0JExh5rj8gKYwt93zoHanu4XsMf
hgTlPihTkfktMUaddBelhrX1RHsljS4eSiFZpHQkglO3CCX8xpsFSs4gJIkN3u+V
OYwnvMBHFZqcau1zXrDExXRc/FMwdcJ8/NNTcm1dXqFMf7JF7Z79NYFwABIrWT6u
semQaAMGVTmFY6edIcIU6ub6TkHl3+LPZKXeiqaiep4kwZ02YdJikaQ9lx71peAR
zyaVKgcx
=1zZf
-----END PGP SIGNATURE-----
Timestamp of file with hash 56cb64cc51974c4141f8d6d3e56dc029e55880ad9edea70a140e1d4e5d83605f -
</details>
ACK 7b6775ba7e2303128743db55e769e3339b3fc28f
Rebased. Ready for further review.
ACK f4450e105781441020964af1e488a033ec1c4be4
(what a painful rebase!)
590 | @@ -591,11 +591,11 @@ class CNode 591 | std::atomic<int64_t> nLastTXTime{0}; 592 | 593 | /** Last measured round-trip time. Used only for RPC/GUI stats/debugging.*/ 594 | - std::atomic<int64_t> m_last_ping_time{0}; 595 | + std::atomic<std::chrono::microseconds> m_ping_time{0us};
in the first commit: why is this renamed?
I did that to make it more consistent (and as the type changes, all call sites have to be touched anyway).
I mean rename from m_last_ping_time to m_ping_time (removing the "last"), which happened in the latest rebase.
Notice - it was renamed to m_ping_time in previous incarnations of this PR:
df8892dc9..7b6775ba7: nPingUsecTime -> m_ping_time
9bbf08bf9..f4450e105: m_last_ping_time -> m_ping_time
So, it is all consistent from this PR's point of view. Just master changed from nPingUsecTime to m_last_ping_time. I think either one of m_last_ping_time or m_ping_time is fine.
Yes, I know :)
My opinion is just that it doesn't make sense to rename this after the rebase
I renamed this from nPingUsecTime to m_last_ping_time in #20721. I think m_last_ping_time is slightly better than m_ping_time because it makes it clear that this is the time from the most recent ping/pong round trip. The next field is m_min_ping_time, which is the minimum time of all the ping/pong round trips.
Done. Using m_last_ping_time now.
Thanks!
re-ACK f4450e105781441020964af1e488a033ec1c4be4 only change is rebase and replacing round with cast 🥈
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
re-ACK f4450e105781441020964af1e488a033ec1c4be4 only change is rebase and replacing round with cast 🥈
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhWmwwAxL1AxtKfLrkQG1JwbvU1nqjLFPPh21d1VbMPv6AcBa3dNgDgvVN+zhXt
SzZg6Xq97fpxKg+dElnrYMfJjtoUGFWMlMGg5EXvBcCNAF4m+Nse7wRUYdWU/aKD
hTltetKiRDhrD5hfNzy8eWA/ATjvdj7e/P5L4zA9TMvHT2HEYpO3Cxhat4KJ3oky
nR0nkb0E8nGH5Wsj+/iiBwE2GPq7gUCUvxyNgmmwDk85Ue908I9ZkG6XTxK6SHIJ
aKq5CwgNciiBvzm7FDEXWNed4yfQPOezrhofrtIDyX4RhY0iKYF+5Qjgc8Figq0C
Y89vsIyTHVFviExvvD16c5CKznvwSHJOXtT+zAMibwPJnbhs4zUsFk0/JAf86c1P
1rLaeUaJDiQHXDwK9X+VdA/ut3aDZsUDo9qj9ti//7s3RkLaiIsICC0XZswgnWUK
uUjk1A+HhL6Enel4Gn6ROMZrAD3UaEHsfTER/7g32G+0c7FMgc4087Kkn0QVk3uZ
GNTjHYt0
=31oD
-----END PGP SIGNATURE-----
Timestamp of file with hash e074c33ed68514a9ea03925c2afa9f30b80c656ab5d0cf97a31ed6c37918737f -
</details>
47 | @@ -48,10 +48,10 @@ static const bool DEFAULT_WHITELISTFORCERELAY = false; 48 | 49 | /** Time after which to disconnect, after waiting for a ping response (or inactivity). */ 50 | static const int TIMEOUT_INTERVAL = 20 * 60; 51 | -/** Run the feeler connection loop once every 2 minutes or 120 seconds. **/ 52 | -static const int FEELER_INTERVAL = 120; 53 | +/** Run the feeler connection loop once every 2 minutes. **/ 54 | +static constexpr auto FEELER_INTERVAL = std::chrono::minutes{2};
Still not addressed
Done.
@ajtowns if there are specifics that you want to see addressed, it might help to post a patch which compiles and that can be taken in the next rebase. Otherwise I think this is ready for merge (mod the nits, which can be fixed in the next rebase as well).
624 | @@ -621,9 +625,9 @@ struct CNodeState { 625 | pindexBestHeaderSent = nullptr; 626 | nUnconnectingHeaders = 0; 627 | fSyncStarted = false; 628 | - nHeadersSyncTimeout = 0; 629 | - nStallingSince = 0; 630 | - nDownloadingSince = 0; 631 | + m_headers_sync_timeout = 0us;
Since you're changing these lines already, it might be a nice opportunity to remove them from the ctor and use default initialization:
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 3c8ef345e1..caac75b72e 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -537,12 +537,12 @@ struct CNodeState {
//! Whether we've started headers synchronization with this peer.
bool fSyncStarted;
//! When to potentially disconnect peer for stalling headers download
- std::chrono::microseconds m_headers_sync_timeout;
+ std::chrono::microseconds m_headers_sync_timeout{0us};
//! Since when we're stalling block download progress (in microseconds), or 0.
- std::chrono::microseconds m_stalling_since;
+ std::chrono::microseconds m_stalling_since{0us};
std::list<QueuedBlock> vBlocksInFlight;
//! When the first entry in vBlocksInFlight started downloading. Don't care when vBlocksInFlight is empty.
- std::chrono::microseconds m_downloading_since;
+ std::chrono::microseconds m_downloading_since{0us};
int nBlocksInFlight;
int nBlocksInFlightValidHeaders;
//! Whether we consider this a preferred download peer.
@@ -625,9 +625,6 @@ struct CNodeState {
pindexBestHeaderSent = nullptr;
nUnconnectingHeaders = 0;
fSyncStarted = false;
- m_headers_sync_timeout = 0us;
- m_stalling_since = 0us;
- m_downloading_since = 0us;
nBlocksInFlight = 0;
nBlocksInFlightValidHeaders = 0;
fPreferredDownload = false;
Done.
Thanks!
Rebased. master changed while I was rebasing, so we see 2 force pushes. Reviewers can use git range-diff cabe637 f4450e1 0eaea66 for convenience.
Comments addressed:
Ready for further review.
utACK 0eaea66e8bfdfb23ff86c0f0924c2d75f5aca75f
707 | @@ -706,9 +708,11 @@ QString formatServicesStr(quint64 mask) 708 | return QObject::tr("None"); 709 | } 710 | 711 | -QString formatPingTime(int64_t ping_usec) 712 | +QString formatPingTime(std::chrono::microseconds ping_time) 713 | { 714 | - return (ping_usec == std::numeric_limits<int64_t>::max() || ping_usec == 0) ? QObject::tr("N/A") : QString(QObject::tr("%1 ms")).arg(QString::number((int)(ping_usec / 1000), 10)); 715 | + return (ping_time == std::chrono::microseconds::max() || ping_time == 0us) ?
Having this be ping_time == ping_time.max() would ensure you don't accidently get the wrong type (eg if ping_time was changed to milliseconds, and set to max it would no longer compare equal to microseconds max). There's a few places where the code might be improved by that change.
ACK 0eaea66e8bfdfb23ff86c0f0924c2d75f5aca75f
re-ACK 0eaea66e8bfdfb23ff86c0f0924c2d75f5aca75f, only changes: minor rename, using C++11 member initializer, using 2min chrono literal, rebase 🤚
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
re-ACK 0eaea66e8bfdfb23ff86c0f0924c2d75f5aca75f, only changes: minor rename, using C++11 member initializer, using 2min chrono literal, rebase 🤚
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgJigwAlGgnegNU2yOZwT9wE9VjciuGc8zXxc4Su130pNLHYd3yirTDUUKwbk4l
pzjUIP6GGSFNHWKjMkGTXgIqzlKQ3YUWfBqCBZKt60G+bwAPfm+tri4lBxBsVXk6
D2e16A8c/mdFfyRPeFl59qtfBE3Wdu7AtHL47u77jLywCUnAPopm9PaHz/xp+w70
O0w5FEr2cazU8tp0u2CY6rMufqjyCZZ/qR4fqNwmOFSR+luBma1ZW7Y0VbmTkjVy
JDrlsITpSyQy9yTosRD0RZ+86BfuGuCR5M3wWsKoepuVpX886pHl/jZEyi9C+eG1
b6ijElFEcU79COu+zocyThQqAkemegbPUBQYttpPtI9d6FZuBUVMK7P/siBVUUPi
75xu76QgF2wuUkCFXDtBobLOGTUlUiKair0lcy/vBWjy/oT12LCh1YZTeYuPwYle
a51LXkhN+dyNUsfYOrN0NkR58VPcKPRk26sbEEzXS750uZx4SIdJ+qSsy5w5LNZw
518vxGAE
=d/DI
-----END PGP SIGNATURE-----
Timestamp of file with hash ba138de68b952c6d098d60b477ecdf0b15cc363ee617f9836e4dafe46273e159 -
</details>
utACK 0eaea66e8bfdfb23ff86c0f0924c2d75f5aca75f