Make all of net_processing (and some of net) use std::chrono types #21015

pull dhruv wants to merge 4 commits into bitcoin:master from dhruv:pr-20044 changing 13 files +141 −141
  1. dhruv commented at 10:24 pm on January 26, 2021: member

    (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.

  2. DrahtBot added the label GUI on Jan 26, 2021
  3. DrahtBot added the label P2P on Jan 26, 2021
  4. DrahtBot added the label RPC/REST/ZMQ on Jan 26, 2021
  5. DrahtBot added the label Utils/log/libs on Jan 26, 2021
  6. dhruv force-pushed on Jan 27, 2021
  7. DrahtBot commented at 1:24 am on January 27, 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:

    • #21261 (p2p, test: add inbound eviction protection for I2P peers by jonatack)
    • #21236 (Net processing: Extract addr send functionality into MaybeSendAddr() by jnewbery)
    • #21186 (Net/Net Processing: Move addr data into net_processing by jnewbery)
    • #21160 (Net/Net processing: Move tx inventory into net_processing by jnewbery)
    • #20197 (p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage by jonatack)

    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.

  8. dhruv force-pushed on Jan 27, 2021
  9. dhruv force-pushed on Jan 27, 2021
  10. dhruv commented at 6:02 am on January 27, 2021: member
    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.
  11. in test/functional/wallet_resendwallettransactions.py:73 in bbc01cf8b2 outdated 
    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)
    MarcoFalke commented at 6:21 am on January 27, 2021:
    in commit bbc01cf8b2965f716e899f413d36e2beb48c1f06: what is this?
    dhruv commented at 6:59 am on January 27, 2021:
    This was originally in d7fad1690c27bf270774da6eebc4ef8b550caf09. However, i just ran the test without the line and it seems to not be needed. Removed.
    MarcoFalke commented at 7:04 am on January 27, 2021:
    No need to keep an empty commit d5a0f24807d5423469824efe8253c8e809b2044b
    dhruv commented at 7:12 am on January 27, 2021:
    Removed.
  12. in src/net.cpp:2934 in f4bc3c6779 outdated 
    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);
    MarcoFalke commented at 6:36 am on January 27, 2021:
    you dropped the -
    dhruv commented at 7:11 am on January 27, 2021:
    Thanks for the catch!
  13. MarcoFalke removed the label GUI on Jan 27, 2021
  14. MarcoFalke removed the label RPC/REST/ZMQ on Jan 27, 2021
  15. MarcoFalke removed the label Utils/log/libs on Jan 27, 2021
  16. MarcoFalke added the label Refactoring on Jan 27, 2021
  17. MarcoFalke commented at 6:38 am on January 27, 2021: member
    Concept ACK. This refactoring should only be changing the types at compile-time, not any values at run time.
  18. dhruv force-pushed on Jan 27, 2021
  19. dhruv force-pushed on Jan 27, 2021
  20. in src/net.h:591 in 4089bc6489 outdated 
    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}};
    MarcoFalke commented at 9:04 am on January 27, 2021:
    4089bc6489255ab26a350bbadc06d1cc5cff6d35: Can use 0us, like above, which is shorter
    dhruv commented at 7:04 pm on January 27, 2021:
    Done
  21. in src/qt/guiutil.cpp:818 in 4089bc6489 outdated 
    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) ?
    MarcoFalke commented at 9:06 am on January 27, 2021:
    Same commit: Can use ping_time == 0us, which is shorter
    dhruv commented at 7:04 pm on January 27, 2021:
    Done
  22. in src/qt/guiutil.h:243 in 4089bc6489 outdated 
    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*/
    MarcoFalke commented at 9:08 am on January 27, 2021:
    4089bc6489255ab26a350bbadc06d1cc5cff6d35: Why is this changed?
    dhruv commented at 6:12 pm on January 27, 2021:
    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.
    MarcoFalke commented at 6:22 pm on January 27, 2021:
    I mean, why are you removing the doxygen comment?
    dhruv commented at 9:50 pm on January 27, 2021:
    That was unintentional. Fixed.
  23. in src/rpc/net.cpp:206 in 4089bc6489 outdated 
    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) {
    MarcoFalke commented at 9:09 am on January 27, 2021:
    4089bc6489255ab26a350bbadc06d1cc5cff6d35: Can use time > 0us, which is shorter
    dhruv commented at 7:04 pm on January 27, 2021:
    Done
  24. in src/rpc/net.cpp:212 in 4089bc6489 outdated 
    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) {
    MarcoFalke commented at 9:10 am on January 27, 2021:
    same
    dhruv commented at 7:04 pm on January 27, 2021:
    Done
  25. in src/test/fuzz/node_eviction.cpp:29 in 4089bc6489 outdated 
    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(),
    MarcoFalke commented at 9:11 am on January 27, 2021:
    unrelated style-nit: Could use named args, like in src/test/net_tests.cpp?
    dhruv commented at 7:04 pm on January 27, 2021:
    Done
  26. in src/test/net_tests.cpp:902 in 4089bc6489 outdated 
    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
    MarcoFalke commented at 9:13 am on January 27, 2021:
    if you are changing whitespace, it would be good to do it right. One space too much here
    dhruv commented at 7:05 pm on January 27, 2021:
    Oops. Done.
  27. in src/net.cpp:614 in 4089bc6489 outdated 
    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;
    MarcoFalke commented at 9:14 am on January 27, 2021:
    two spaces before =
    dhruv commented at 7:05 pm on January 27, 2021:
    Done
  28. in src/net_processing.cpp:48 in aa30fdfcc4 outdated 
    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};
    MarcoFalke commented at 9:15 am on January 27, 2021:
    in commit aa30fdfcc4e8801c3abd74468f849f6124815e10: Can use 15min, which is shorter
    dhruv commented at 7:05 pm on January 27, 2021:
    Done
  29. in src/net_processing.cpp:97 in aa30fdfcc4 outdated 
    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};
    MarcoFalke commented at 9:17 am on January 27, 2021:
    aa30fdfcc4e8801c3abd74468f849f6124815e10: 2s
    dhruv commented at 7:05 pm on January 27, 2021:
    Done
  30. in src/net_processing.cpp:577 in aa30fdfcc4 outdated 
    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};
    MarcoFalke commented at 9:33 am on January 27, 2021:
    aa30fdfcc4e8801c3abd74468f849f6124815e10: 0us on all three
    dhruv commented at 7:05 pm on January 27, 2021:
    Done
  31. in src/net_processing.cpp:631 in aa30fdfcc4 outdated 
    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};
    MarcoFalke commented at 9:34 am on January 27, 2021:
    same
    dhruv commented at 7:05 pm on January 27, 2021:
    Done
  32. in src/net_processing.cpp:4386 in aa30fdfcc4 outdated 
    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));
    MarcoFalke commented at 9:37 am on January 27, 2021:
    why ((foo)) instead of (foo)?
    dhruv commented at 7:05 pm on January 27, 2021:
    Done
  33. in src/net_processing.cpp:4741 in aa30fdfcc4 outdated 
    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};
    MarcoFalke commented at 9:40 am on January 27, 2021:
    0us
    dhruv commented at 7:05 pm on January 27, 2021:
    Done
  34. in src/net_processing.cpp:4771 in aa30fdfcc4 outdated 
    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) {
    MarcoFalke commented at 9:41 am on January 27, 2021:
    == 0us
    dhruv commented at 7:05 pm on January 27, 2021:
    Done
  35. in src/net.cpp:1791 in 64f3fb1a96 outdated 
    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})) {
    MarcoFalke commented at 9:44 am on January 27, 2021:
    64f3fb1a966cfee47e6cfccdd5e9c9540bfebecc: > 1min
    dhruv commented at 7:05 pm on January 27, 2021:
    Done
  36. in src/net.cpp:2933 in 64f3fb1a96 outdated 
    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 */);
    MarcoFalke commented at 9:49 am on January 27, 2021:
    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.
    dhruv commented at 7:06 pm on January 27, 2021:
    That makes more sense. Done.
  37. in src/net.h:52 in 64f3fb1a96 outdated 
    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};
    MarcoFalke commented at 9:49 am on January 27, 2021:
    2min
    dhruv commented at 7:06 pm on January 27, 2021:
    Done
    vasild commented at 6:37 am on January 28, 2021:
    Not done yet in c0822a71b.
  38. in src/net.h:53 in 64f3fb1a96 outdated 
    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};
    MarcoFalke commented at 9:49 am on January 27, 2021:
    5min
    dhruv commented at 7:06 pm on January 27, 2021:
    Done
  39. in src/net.h:1222 in 64f3fb1a96 outdated 
    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}};
    MarcoFalke commented at 9:50 am on January 27, 2021:
    0us
    dhruv commented at 7:06 pm on January 27, 2021:
    Done
  40. in src/net_processing.cpp:122 in 64f3fb1a96 outdated 
    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};
    MarcoFalke commented at 9:51 am on January 27, 2021:
    24h
    dhruv commented at 7:06 pm on January 27, 2021:
    Done
  41. in src/net_processing.cpp:124 in 64f3fb1a96 outdated 
    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};
    MarcoFalke commented at 9:51 am on January 27, 2021:
    30s
    dhruv commented at 7:06 pm on January 27, 2021:
    Done
  42. in src/net_processing.cpp:127 in 64f3fb1a96 outdated 
    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};
    MarcoFalke commented at 9:51 am on January 27, 2021:
    5s
    dhruv commented at 7:06 pm on January 27, 2021:
    Done
  43. in src/net_processing.cpp:143 in 64f3fb1a96 outdated 
    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};
    MarcoFalke commented at 9:51 am on January 27, 2021:
    same for those two
    dhruv commented at 7:06 pm on January 27, 2021:
    Done
  44. in src/net_processing.cpp:4570 in 64f3fb1a96 outdated 
    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);
    MarcoFalke commented at 9:57 am on January 27, 2021:
    Note to other reviewers: Division does not have any truncation warnings at compile time
    ajtowns commented at 5:08 pm on February 1, 2021:
    Might be clearer to have separate constants for inbound/outbound inventory broadcast intervals so it’s clear one’s 5s and one is 2s?
    dhruv commented at 5:07 pm on February 4, 2021:
    Yes, that’s indeed clearer. Updated.
  45. in src/net_processing.cpp:4825 in 64f3fb1a96 outdated 
    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};
    MarcoFalke commented at 10:11 am on January 27, 2021:
    I think you can simply assign this to 0, as the goal is to send immediately
    dhruv commented at 7:06 pm on January 27, 2021:
    Done
  46. in src/net_processing.cpp:43 in c9a8cbc64e outdated 
    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};
    MarcoFalke commented at 10:15 am on January 27, 2021:
    in commit c9a8cbc64eddd98d2183380148a3d9c79c1bd7d2: 15min
    dhruv commented at 7:06 pm on January 27, 2021:
    Done
  47. MarcoFalke approved
  48. MarcoFalke commented at 10:19 am on January 27, 2021: member

    left some style-nits. Feel free to ignore.

    review ACK c9a8cbc64eddd98d2183380148a3d9c79c1bd7d2 🐉

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK c9a8cbc64eddd98d2183380148a3d9c79c1bd7d2 🐉
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjXQAwAmlnqHxcZHCTykusiZ3462hXgmsVzJZFoEXtZWK0hJqI4Dic/WgQ6gd+n
 8+AJlwBfJ56FmyVHXPa61h1njES+9cZfUvq4Ty5DbgVHAF3M7e0xz0WrUq2KUzRxf
 9xpKgVuWN1/aeHXGn6qRzvGAvjAA/6ozQ8H5HC0jnru5EWZX7FBDDgef1ApkIOd4I
10YRx7D2iqHvcVPS/Di82+2UHjvBN789NMwB3MQDsB6lbEYzNaE2FVYrcdtXiepYk7
11kqXhqWavBlubcPfRyXLRXbbVv/A0z0ZUhv2laaJuOMv/ssk/LXiOCE37aytLCnvV
12AiLIlA64AFp8BDrykUtqaFqCTRbSQSELsHldAmIIe+gV9rF+6cVp8ClWMl/QRWjS
13nZnChOANGW08/+t4KeiDKxRUHlwko29Y4NX4p3pVR99beMmTagqiN0Rflbrv6okA
14aoKZdDLz0jUnARvfbib7vqbiNxvZWA6WBlfrnWSXpaTRohPWW3wWTaQc09yF2Qgw
15Q8SDUsfg
16=09jz
17-----END PGP SIGNATURE-----

    Timestamp of file with hash 21b8f1b48c83567a179a56af130b796e2cfb94ca44d7acdf169927f865176af6 -

  49. dhruv force-pushed on Jan 27, 2021
  50. dhruv commented at 7:08 pm on January 27, 2021: member
    Thanks for the review, @MarcoFalke. Comments addressed. Ready for further review.
  51. felipsoarez commented at 7:19 pm on January 27, 2021: none
    Concept ACK
  52. dhruv force-pushed on Jan 27, 2021
  53. dhruv commented at 9:54 pm on January 27, 2021: member
    Pushed to fix doxygen comment.
  54. in src/test/fuzz/node_eviction.cpp:34 in c0822a71b5 outdated 
    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(),
    vasild commented at 4:21 am on January 28, 2021:

    nit:

     0            .id = fuzzed_data_provider.ConsumeIntegral<NodeId>(),
 1            .nTimeConnected = fuzzed_data_provider.ConsumeIntegral<int64_t>(),
 2            .m_min_ping_time = std::chrono::microseconds{fuzzed_data_provider.ConsumeIntegral<int64_t>()},
 3            .nLastBlockTime = fuzzed_data_provider.ConsumeIntegral<int64_t>(),
 4            .nLastTXTime = fuzzed_data_provider.ConsumeIntegral<int64_t>(),
 5            .fRelevantServices = fuzzed_data_provider.ConsumeBool(),
 6            .fRelayTxes = fuzzed_data_provider.ConsumeBool(),
 7            .fBloomFilter = fuzzed_data_provider.ConsumeBool(),
 8            .nKeyedNetGroup = fuzzed_data_provider.ConsumeIntegral<uint64_t>(),
 9            .prefer_evict = fuzzed_data_provider.ConsumeBool(),
10            .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.

    MarcoFalke commented at 8:07 am on January 28, 2021:
    pls no. This is a bug in the upstream compilers, we shouldn’t rely on that.
  55. in src/test/net_tests.cpp:781 in c0822a71b5 outdated 
    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)),
    vasild commented at 4:29 am on January 28, 2021:

    nit, it is shorter than static_cast and consistent with the code afterwards (std::chrono::microseconds{candidate.id}):

    0            .m_min_ping_time = std::chrono::microseconds{random_context.randrange(100)},
    MarcoFalke commented at 8:21 am on January 28, 2021:
    Seems fine to remove the cast, but please don’t use the C++20 feature
    dhruv commented at 11:24 pm on January 29, 2021:
    Done.
  56. in src/net_processing.cpp:4386 in c0822a71b5 outdated 
    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;
    vasild commented at 5:18 am on January 28, 2021:

    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?

    0                state.m_headers_sync_timeout =
1                    current_time + HEADERS_DOWNLOAD_TIMEOUT_BASE +
2                    HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER *
3                        // Expected number of headers [unitless].
4                        (GetAdjustedTime() - pindexBestHeader->GetBlockTime()) /
5                        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.

    martinus commented at 7:24 am on January 28, 2021:
    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
    MarcoFalke commented at 8:15 am on January 28, 2021:

    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

    MarcoFalke commented at 8:16 am on January 28, 2021:

    Could add ( and ) to clarify this:

    0                state.m_headers_sync_timeout = current_time + HEADERS_DOWNLOAD_TIMEOUT_BASE + HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER * ((GetAdjustedTime() - pindexBestHeader->GetBlockTime())/consensusParams.nPowTargetSpacing);
    martinus commented at 8:30 am on January 28, 2021:
    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
    MarcoFalke commented at 8:42 am on January 28, 2021:
    not if you assume that x/y is in ℕ
    MarcoFalke commented at 8:43 am on January 28, 2021:
    oh wait, it isn’t. Missed that (edit: :sweat_smile: )
    dhruv commented at 11:18 pm on January 29, 2021:
    Awesome catch, @martinus ! I’ve changed HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER to be 1000us instead of 1ms to keep things consistent. The change may not be a problem, but I don’t see a reason to risk it for moving to chrono types.
    ajtowns commented at 5:28 pm on January 31, 2021:

    I think making it:

    0
1state.m_headers_sync_timeout = current_time + HEADERS_DOWNLOAD_TIMEOUT_BASE +
2     (
3         std::chrono::microseconds(HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER) *
4         (GetAdjustedTime() - pindexBestHeader->GetBlockTime())
5     ) / consensusParams.nPowTargetSpacing;
6     // GetAdjustedTime/GetBlockTime/nPowTargetSpacing return time in seconds as an int,
7     // 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.

    dhruv commented at 5:04 pm on February 4, 2021:
    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.
    ajtowns commented at 4:43 am on February 12, 2021:
    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.
    dhruv commented at 3:22 pm on February 14, 2021:
    Done.
  57. in src/net_processing.cpp:46 in c0822a71b5 outdated 
    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
    vasild commented at 5:20 am on January 28, 2021: 
    0/** Headers download timeout.
    dhruv commented at 11:18 pm on January 29, 2021:
    Done
  58. in src/net_processing.cpp:49 in c0822a71b5 outdated 
    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};
    vasild commented at 5:20 am on January 28, 2021: 
    0static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER = 1ms;
    dhruv commented at 11:19 pm on January 29, 2021:
    Done
  59. in src/net_processing.cpp:4704 in c0822a71b5 outdated 
    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
    vasild commented at 5:59 am on January 28, 2021:

    nit: this uses * and times to denote multiplication:

    0        // In case there is a block that has been in flight from this peer for (the block interval) * (1 + 0.5 * N)
    dhruv commented at 11:19 pm on January 29, 2021:
    Done
  60. in src/net.cpp:2998 in c0822a71b5 outdated 
    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 */);
    vasild commented at 6:10 am on January 28, 2021:

    -x * -y should be the same as x * y:

    0    double unscaled = log1p(GetRand(1ULL << 48) * 0.0000000000000035527136788 /* -1/2^48 */);
    MarcoFalke commented at 8:10 am on January 28, 2021:
    no. log1p(-a) is not equal to -log1p(a)
    vasild commented at 10:58 am on January 28, 2021:
    Doh, I misread this as -log1p(a) * -b.
  61. in src/net_processing.cpp:125 in c0822a71b5 outdated 
    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.
    vasild commented at 6:15 am on January 28, 2021: 
    0/** Average delay between trickled inventory transmissions.
    dhruv commented at 11:19 pm on January 29, 2021:
    Done.
  62. in src/test/fuzz/connman.cpp:109 in c0822a71b5 outdated 
    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>()});
    vasild commented at 6:28 am on January 28, 2021:

    The meaning of the second argument was “seconds” before, to keep the behavior:

    0                (void)connman.PoissonNextSendInbound(
1                        std::chrono::microseconds{fuzzed_data_provider.ConsumeIntegral<int32_t>()},
2                        std::chrono::seconds{fuzzed_data_provider.ConsumeIntegral<int>()});
    MarcoFalke commented at 8:20 am on January 28, 2021:
    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.
    dhruv commented at 11:19 pm on January 29, 2021:
    Left it as-is.
  63. in src/net_processing.cpp:45 in c0822a71b5 outdated 
    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};
    vasild commented at 6:30 am on January 28, 2021: 
    0static constexpr auto UNCONDITIONAL_RELAY_DELAY = 2min;
    dhruv commented at 11:20 pm on January 29, 2021:
    Done
  64. vasild commented at 6:33 am on January 28, 2021: member
    Approach ACK c0822a71b
  65. in src/rpc/net.cpp:207 in c0822a71b5 outdated 
    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);
    martinus commented at 9:08 am on January 28, 2021:

    Maybe use this to get rid of the hardcoded constant:

    0            obj.pushKV("pingtime", std::chrono::duration<double>{stats.m_ping_time}.count());
    MarcoFalke commented at 9:32 am on January 28, 2021:
    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.
    vasild commented at 11:06 am on January 28, 2021: 
    0    auto us = 500000us;
1    auto ns = 500000000ns;
2
3    std::cout << us.count() * 0.000001 << std::endl;
4    std::cout << std::chrono::duration<double>{us}.count() << std::endl;
5    std::cout << std::chrono::duration<double>{ns}.count() << std::endl;

    all 3 print 0.5.

    MarcoFalke commented at 11:14 am on January 28, 2021:
    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.
    MarcoFalke commented at 11:18 am on January 28, 2021:
    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)
    martinus commented at 11:37 am on January 28, 2021:

    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

    vasild commented at 12:16 pm on January 28, 2021:
    no millis, no nanos, no kilos, no megas, just seconds :)
    MarcoFalke commented at 12:50 pm on January 28, 2021:

    What about adding SecondsFloat, similar to the std::chrono::seconds helper type?

    0using SecondsFloat = std::chrono::duration<double>;
1inline auto count_seconds_f(SecondsFloat t) { return t.count(); }
    vasild commented at 1:44 pm on January 28, 2021:
    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.
    martinus commented at 6:58 pm on January 28, 2021:
    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:
    dhruv commented at 11:22 pm on January 29, 2021:
    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.
  66. in src/net.cpp:2935 in c0822a71b5 outdated 
    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)};
    martinus commented at 9:17 am on January 28, 2021:

    This could directly use chrono features for this:

    0    return now + std::chrono::round<std::chrono::microseconds>(unscaled * average_interval);
    dhruv commented at 11:23 pm on January 29, 2021:
    Thanks! Done.
  67. martinus commented at 9:48 am on January 28, 2021: contributor
    Review ACK c0822a71b5fabb39e6bbf7355b69f9f1b9264fde, I have not tested the code, looked through it and found only nits
  68. in src/net.cpp:1764 in c0822a71b5 outdated 
    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>();
    martinus commented at 12:00 pm on January 28, 2021:
    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)
    ajtowns commented at 4:04 pm on February 1, 2021:
    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.
  69. dhruv force-pushed on Jan 29, 2021
  70. dhruv commented at 0:23 am on January 30, 2021: member

    Thank you for the thorough review, @vasild @martinus and @MarcoFalke!

    Comments addressed. Ready for review.

  71. in src/net_processing.cpp:4933 in e5fb29086d outdated 
    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;
    martinus commented at 8:54 am on January 30, 2021:
    Was the logic changed on purpose?
    MarcoFalke commented at 9:25 am on January 30, 2021:
    Behaviour shouldn’t change: #21015 (review)
    vasild commented at 2:38 pm on February 1, 2021:
    Yeah, in the way it is used it will behave the same as long as m_next_send_feefilter is in the past.
  72. martinus commented at 9:38 am on January 30, 2021: contributor
    ACK e5fb29086d3c0502ec63c1890b6b56be5cb7b1d8 looked through all changes and looks good, did not run the code though.
  73. in src/net.cpp:618 in 0b7cf5889c outdated 
    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;
    ajtowns commented at 4:55 pm on January 31, 2021:
    X(m_ping_time); X(m_min_ping_time); ?
    dhruv commented at 4:58 pm on February 4, 2021:
    Done.
  74. in src/util/time.h:38 in 0b7cf5889c outdated 
    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(); }
    ajtowns commented at 5:02 pm on January 31, 2021:
    Why is this CountSeconds not count_seconds_double to match the existing count_microseconds and count_seconds?
    vasild commented at 3:21 pm on February 2, 2021:

    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.

    dhruv commented at 4:58 pm on February 4, 2021:
    That was my reasoning as well.
    ajtowns commented at 8:30 am on February 12, 2021:
    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?
    martinus commented at 11:42 am on February 12, 2021:
    I’d personally just prefer std::chrono::duration<double>(t).count() instead of a helper.
    dhruv commented at 3:22 pm on February 14, 2021:
    Renamed to CountSecondsDouble
  75. in src/net_processing.cpp:49 in bc5f977be8 outdated 
    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;
    ajtowns commented at 5:07 pm on January 31, 2021:

    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.

    MarcoFalke commented at 10:08 am on February 2, 2021:
    See also #21015 (review)
    dhruv commented at 4:58 pm on February 4, 2021:
    Thanks! Done.
  76. MarcoFalke commented at 10:50 am on February 1, 2021: member

    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

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK e5fb29086d3c0502ec63c1890b6b56be5cb7b1d8 changes since last review: 🦐
 4
 5* Restore doxygen comment
 6* Use chrono literals where possible
 7* Add "named initializer comments" in fuzz tests
 8* Adding missing negation of log1p result
 9* Using std::chrono::round instead of own round implementation
10-----BEGIN PGP SIGNATURE-----
11
12iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
13pUiNOgv/esnDzAD/Kt43N+9AyPE/F2YIwzavHnuWrj/SXUud1w6esEg2hIyXtLQV
144XaR+pDY04odSoU/ug7hIDf6YuMjP+/9GP7H0NOmVdNpVRtmTwrlRyQWj0C7ywz4
15zwmU5cymrTY72AdR9kY4KqDbSoYoUfsVduxRfJc4zV94TvddXKjHkDEx5dK1lM7I
16ErYKjiXovXU92giHP9WB9VEwNrQ0I771whBYtQGceGMUMCq6Yv1LhCG3YUzK1ywN
17YLdjr6dKO4h4xl19aRu4NXh6g6c70k9o8Jdw9slska93nMT+hNH9EOVnhpU1Oc5w
18kUC3CVZlsJFx+ko/wtYJNnETyRiHkkFrxnCf7RdoS7zSaeIdoZ/Ib7cfXj11CxX3
19CNEkJ1lBzaWt80zmAaFOnvRHlcnPBY0njOcydJB86ZZMu5/NUo0iKIopCl61MDtB
20ApbnZZ0i1u/+P6DGdlC+MsSCjeG3FZjFvP6iPWv18bximVEeRNZc2FXT0usZspSl
21H7g3SvAP
22=VrHQ
23-----END PGP SIGNATURE-----

    Timestamp of file with hash 4921d55322dc9a709473933b73c325e72d8e67b146d2eb0c9da16764d7486ae0 -

  77. in src/net_processing.cpp:49 in e5fb29086d outdated 
    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;
    MarcoFalke commented at 10:52 am on February 1, 2021:
    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.
    dhruv commented at 4:56 pm on February 4, 2021:
    Done.
  78. in src/util/time.h:33 in e5fb29086d outdated 
    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>>;
    MarcoFalke commented at 11:00 am on February 1, 2021:

    nit: Would be nice to inherit the period from “upstream”

     0diff --git a/src/util/time.h b/src/util/time.h
 1index deb9ceefe2..f1b2beb0f3 100644
 2--- a/src/util/time.h
 3+++ b/src/util/time.h
 4@@ -30,7 +30,7 @@ constexpr inline int64_t count_microseconds(std::chrono::microseconds t) { retur
 5 /**
 6  * Helper to count the seconds in any std::chrono::duration type
 7  */
 8-using SecondsDouble = std::chrono::duration<double, std::ratio<1, 1>>;
 9+using SecondsDouble = std::chrono::duration<double, std::chrono::seconds::period>;
10 inline double CountSeconds(SecondsDouble t) { return t.count(); }
11 
12 /**
    dhruv commented at 4:57 pm on February 4, 2021:
    Done
  79. MarcoFalke approved
  80. MarcoFalke commented at 11:00 am on February 1, 2021: member
    left to style-nits. Feel free to ignore
  81. in src/util/time.h:38 in e5fb29086d outdated 
    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(); }
    vasild commented at 3:11 pm on February 1, 2021:

    nit: in the doxygen-generated documentation the comment will be assigned to SecondsDouble and CountSeconds will be left undocumented.

    0/** These are the coordinates of the buzz point in the hyper plane. */
1int x;
2int y;

    this looks ok in the source code but will produce unexpected result in the doxygen docs, leaving y undocumented.

    dhruv commented at 4:57 pm on February 4, 2021:
    Thanks! Done.
  82. vasild approved
  83. vasild commented at 3:12 pm on February 1, 2021: member
    ACK e5fb29086d3c0502ec63c1890b6b56be5cb7b1d8
  84. in src/net_processing.cpp:4820 in bc5f977be8 outdated 
    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)) {
    ajtowns commented at 3:37 pm on February 1, 2021:

    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.

    MarcoFalke commented at 2:06 pm on February 4, 2021:

    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:

    • Before 600 * ( 1000000 + 500000 * nOtherPeersWithValidatedDownloads) (unit is us, implicitly)
    • After 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:

    https://www.wolframalpha.com/input/?i=%28+600++%28+1000000+%2B+500000++x+%29+%29+-+%28+1000000++%28+600++%28+1+%2B+0.5+*+x+%29+%29+%29

    dhruv commented at 5:00 pm on February 4, 2021:
    As @MarcoFalke mentioned, the units do make sense to me. Also BLOCK_DOWNLOAD_TIMEOUT_BASE in multiples of block intervals seems easier to understand?
    ajtowns commented at 5:33 am on February 12, 2021:

    “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:

    • converting the chrono::seconds version of nPowTargetSpacing to duration<double,ratio=1,1> when multiplying by a double
    • converting both m_downloading_since and the result of the above to duration<double,ratio=1,1000000> (converting one to a double, multiplying the other by 1M)
    • converting 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 two

    Are 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.

    ajtowns commented at 3:49 pm on February 18, 2021:

    What I’d prefer is something more like:

    0constexpr Fraction BLOCK_DOWNLOAD_TIMEOUT_BASE{1};
1constexpr 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:

    0class Fraction {
1public:
2    constexpr Fraction(int64_t n, int64_t d) : num{n}, den{d} { }
3    explicit constexpr Fraction(int64_t n) : num{n}, den{1} { }
4    int64_t num = 0, den = 1;
5};
6
7constexpr Fraction operator+(Fraction l, Fraction r) { return Fraction{l.num*r.den + r.num*l.den, l.den*r.den}; }
8constexpr Fraction operator*(Fraction l, int64_t n) { return Fraction{l.num*n, l.den}; }
9constexpr 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.

  85. in src/net.cpp:2958 in 0473d80bca outdated 
    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);
    ajtowns commented at 4:33 pm on February 1, 2021:

    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:

    0std::chrono::microseconds PoissonNextSend(std::chrono::microseconds now, std::chrono::seconds average_interval)
1{
2    int64_t delta = (int64_t)(log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */) * (-1.0 * count_microseconds(average_interval)) + 0.5);
3    return now + std::chrono::microseconds{ delta };
4}

    seems like a simpler way to switch the Poisson intervals to chrono types.

    dhruv commented at 5:07 pm on February 4, 2021:

    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.

    ajtowns commented at 6:04 am on February 12, 2021:

    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:

    0    double unscaled = -log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */);
1    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)

    dhruv commented at 3:41 pm on February 14, 2021:
    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.
    MarcoFalke commented at 12:30 pm on February 15, 2021:

    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)

    ajtowns commented at 2:09 am on February 16, 2021:

    It seems we’re adopting std::chrono, so std::chrono::round is 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.

    martinus commented at 7:16 am on February 16, 2021:

    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:

    0inline uint64_t myRand(uint64_t maxnum) {
1    static uint64_t x = 1;
2    auto tmp = x % maxnum;
3    x *= 123123121;
4    return tmp;
5}

    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.

    dhruv commented at 5:47 pm on February 16, 2021:
    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.
    MarcoFalke commented at 7:11 am on February 18, 2021:

    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.

    ajtowns commented at 9:39 am on February 18, 2021:

    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:

    0template<typename Dur, typename Dur2>
1constexpr Dur chrono_round_half_up(Dur2 dur)
2{
3    return std::chrono::duration_cast<Dur>(Dur{1}*0.5 + dur);
4}

    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]

  86. ajtowns commented at 6:04 pm on February 1, 2021: member
    Looks good, not there yet though imo.
  87. dhruv force-pushed on Feb 4, 2021
  88. dhruv commented at 6:50 pm on February 4, 2021: member

    Thanks, @MarcoFalke @ajtowns @vasild.

    Rebased. Comments addressed. Ready for further review.

  89. dhruv force-pushed on Feb 4, 2021
  90. dhruv commented at 1:54 am on February 5, 2021: member
    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.
  91. vasild approved
  92. vasild commented at 3:49 pm on February 5, 2021: member
    ACK 054c6c29366a7f503554d9c85e6fd231008f2f20
  93. DrahtBot added the label Needs rebase on Feb 11, 2021
  94. in src/util/time.h:28 in 054c6c2936 outdated 
    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(); }
    MarcoFalke commented at 5:26 pm on February 11, 2021: 
    0constexpr int64_t count_microseconds(std::chrono::microseconds t) { return t.count(); }

    nit: constexpr implies inline, so no need to specify twice

    dhruv commented at 11:57 pm on February 11, 2021:
    Done.
  95. MarcoFalke commented at 5:34 pm on February 11, 2021: member

    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

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 054c6c29366a7f503554d9c85e6fd231008f2f20 changes since last review: 👺
 4
 5* Use X macro where possible
 6* Use std::chrono::seconds::period where possible
 7* Replace 1000us with 1ms and cast to us where needed
 8* Use seconds as type for poisson average interval
 9* Split up inbound/outbound avg interval
10-----BEGIN PGP SIGNATURE-----
11
12iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
13pUgN/gv9H2XeUVZtB1zj0HIuOVbmki5BuvT6oGnWiKgnzZERJ/zBpbLghWP0snBG
1405IunLcL/atno/SNQ6w7B0IXZOtCHiCBgA8nbbEhTngkLY8oLn+rgyAlpc14NGHQ
15X3Du9itIuVBjVfVt9v6qK1ywYSdUexgg38XItqgq2UwANXV9CP2koavvZfeiEs48
16im/aE92SDUo9cGpx1SjKHFAOc4fnCuWfyuElgnfe3kahYwOoygpdCemQhfo1vyHO
17MHMs/EVjhWM9xdpGJ2fPCjrHQbfCz4UdVcouxnqdubM79/7O77EFPMPYMMm+4RhT
18yxumFZGX4iSacrwHKdaGB1bh0kA4Lor+zFqu+q5nF13Nj3CI91scaHUGIXc0pqJG
191zbvr/Uks2g1lcJCRoeO04WaZxNKWe5xGHYPWdrdXs9OC+UPnKWAC8PkLdVwHfKH
20rN+EUHfsARfLVIYDfq3247k4DCZUM4zipp/B03Nndo1QwqBFvBxpcpcgqaE8eehE
21fUIk46tK
22=2UZp
23-----END PGP SIGNATURE-----

    Timestamp of file with hash 2a4b9786606b664d2c4b408f943adf429fbf9af7568a63e4d184c0ab0674bedf -

  96. dhruv force-pushed on Feb 11, 2021
  97. dhruv commented at 0:48 am on February 12, 2021: member
    Rebased. Comment addressed. Ready for further review.
  98. DrahtBot removed the label Needs rebase on Feb 12, 2021
  99. martinus commented at 6:39 am on February 12, 2021: contributor
    Codereview ACK 7b57704
  100. MarcoFalke commented at 11:44 am on February 12, 2021: member

    re-ACK 7b577049fe87a20f6d12fb7ce4e7d9941d971de1 only change is removing redundant inline 🎥

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 7b577049fe87a20f6d12fb7ce4e7d9941d971de1 only change is removing redundant inline 🎥
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjaCQv/eNRybP+jarAixi/2KGfs6eWdq5SLdk9Vrgz5WQg9o0IW4QMh5Hcs0515
 8OGgZ2bI6MtSr/BioiFWsfbfvu0OyLonkfKAuQ4TBhkhlRCTXPsG0pq2RiGfaevWC
 9h79FhANZdnyYpDAAdOvQ2DmPsN6qnymMq3XjWDlgUZAXeSpS2hG5Df0EKsIGicx4
10d9EUOH2safkGaXQDwlQpKRlmlIO7OoXYvHhg6JnJy/WrGB3aiyFw+OBs5EdlhGV3
11PxwCDbkWBtKqTXtyt/Qvrbzg5tFTzh4x0YF70NxMuKO0SJ2MXOmCDnQ3JjnZqO/U
12KoNKcDHn+JSIHducBWVuDcLjTBOa6/RWs9kfZJsCx/zGoqBHbCPfenn6DHZS0g4a
1372k6Qu9Nax14WdsF6iejGIebCVmgD3NR1MYu+FPvzuTGwzfcXM7EhdCNl2QLGRET
14IFP0YsVwlPNjhHT2suOi70Ccxa2DfTYP5QhEY4sD0BJjl+WnTWeQnE2c4KNqmjfA
15bvyLfPAi
16=zPnG
17-----END PGP SIGNATURE-----

    Timestamp of file with hash f12ad6bf5ab4e44d86abf6dc1d7b5e4d1055bbe1d0a8c92f66c2328ae35c674f -

  101. DrahtBot added the label Needs rebase on Feb 12, 2021
  102. vasild approved
  103. vasild commented at 3:26 pm on February 12, 2021: member
    ACK 7b577049fe87a20f6d12fb7ce4e7d9941d971de1
  104. dhruv force-pushed on Feb 14, 2021
  105. dhruv commented at 3:43 pm on February 14, 2021: member
    Rebased. Ready for further review.
  106. DrahtBot removed the label Needs rebase on Feb 14, 2021
  107. dhruv commented at 5:45 am on February 15, 2021: member
    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.
  108. MarcoFalke commented at 12:36 pm on February 15, 2021: member

    ci timeout is unrelated and can be ignored

    re-ACK 7b6775ba7e2303128743db55e769e3339b3fc28f only changes are rename to CountSecondsDouble, adding a comment, rebase 👛

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 7b6775ba7e2303128743db55e769e3339b3fc28f only changes are rename to CountSecondsDouble, adding a comment, rebase 👛
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhRhgwArVe9YuYCz8vG/HmuQ1fTwAMMq4rn+C3DYUi2xXGzKJvkESpCCeXfuR6v
 8rrJF1Jec0Oa4okln8GoNtnweBDMpJKUZ+hecuCF1zkz8QP/Krifps/HdE95cpDEr
 9fZD1Y6PtpARGQZ4fLs4AF1XixA5iwT7HemwQ7r1c2AIx4AqNogGts4ysOd7+aRTd
10N94vnABZqiXnwFrwosMfXGTvsmnnxe2UpjI7oZV8zUOeJTna4GZnsxTwKrgaqPLD
118igcQJmLmfy9bvxab7nyCzfgU1CziA+L9RXov0JExh5rj8gKYwt93zoHanu4XsMf
12hgTlPihTkfktMUaddBelhrX1RHsljS4eSiFZpHQkglO3CCX8xpsFSs4gJIkN3u+V
13OYwnvMBHFZqcau1zXrDExXRc/FMwdcJ8/NNTcm1dXqFMf7JF7Z79NYFwABIrWT6u
14semQaAMGVTmFY6edIcIU6ub6TkHl3+LPZKXeiqaiep4kwZ02YdJikaQ9lx71peAR
15zyaVKgcx
16=1zZf
17-----END PGP SIGNATURE-----

    Timestamp of file with hash 56cb64cc51974c4141f8d6d3e56dc029e55880ad9edea70a140e1d4e5d83605f -

  109. vasild approved
  110. vasild commented at 2:29 pm on February 15, 2021: member
    ACK 7b6775ba7e2303128743db55e769e3339b3fc28f
  111. DrahtBot added the label Needs rebase on Feb 16, 2021
  112. dhruv force-pushed on Feb 16, 2021
  113. dhruv commented at 5:47 pm on February 16, 2021: member
    Rebased. Ready for further review.
  114. DrahtBot removed the label Needs rebase on Feb 16, 2021
  115. vasild approved
  116. vasild commented at 3:48 pm on February 17, 2021: member

    ACK f4450e105781441020964af1e488a033ec1c4be4

    (what a painful rebase!)

  117. in src/net.h:594 in 3e99b4c23a outdated 
    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};
    MarcoFalke commented at 6:36 am on February 18, 2021:
    in the first commit: why is this renamed?
    sipa commented at 6:39 am on February 18, 2021:
    I did that to make it more consistent (and as the type changes, all call sites have to be touched anyway).
    MarcoFalke commented at 6:45 am on February 18, 2021:
    I mean rename from m_last_ping_time to m_ping_time (removing the “last”), which happened in the latest rebase.
    vasild commented at 8:58 am on February 18, 2021:

    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.

    MarcoFalke commented at 9:00 am on February 18, 2021:

    Yes, I know :)

    My opinion is just that it doesn’t make sense to rename this after the rebase

    jnewbery commented at 8:46 am on February 26, 2021:
    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.
    dhruv commented at 5:45 pm on March 3, 2021:
    Done. Using m_last_ping_time now.
    jnewbery commented at 7:31 pm on March 3, 2021:
    Thanks!
  118. MarcoFalke approved
  119. MarcoFalke commented at 6:44 am on February 18, 2021: member

    re-ACK f4450e105781441020964af1e488a033ec1c4be4 only change is rebase and replacing round with cast 🥈

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK f4450e105781441020964af1e488a033ec1c4be4 only change is rebase and replacing round with cast 🥈
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhWmwwAxL1AxtKfLrkQG1JwbvU1nqjLFPPh21d1VbMPv6AcBa3dNgDgvVN+zhXt
 8SzZg6Xq97fpxKg+dElnrYMfJjtoUGFWMlMGg5EXvBcCNAF4m+Nse7wRUYdWU/aKD
 9hTltetKiRDhrD5hfNzy8eWA/ATjvdj7e/P5L4zA9TMvHT2HEYpO3Cxhat4KJ3oky
10nR0nkb0E8nGH5Wsj+/iiBwE2GPq7gUCUvxyNgmmwDk85Ue908I9ZkG6XTxK6SHIJ
11aKq5CwgNciiBvzm7FDEXWNed4yfQPOezrhofrtIDyX4RhY0iKYF+5Qjgc8Figq0C
12Y89vsIyTHVFviExvvD16c5CKznvwSHJOXtT+zAMibwPJnbhs4zUsFk0/JAf86c1P
131rLaeUaJDiQHXDwK9X+VdA/ut3aDZsUDo9qj9ti//7s3RkLaiIsICC0XZswgnWUK
14uUjk1A+HhL6Enel4Gn6ROMZrAD3UaEHsfTER/7g32G+0c7FMgc4087Kkn0QVk3uZ
15GNTjHYt0
16=31oD
17-----END PGP SIGNATURE-----

    Timestamp of file with hash e074c33ed68514a9ea03925c2afa9f30b80c656ab5d0cf97a31ed6c37918737f -

  120. in src/net.h:52 in f4450e1057 outdated 
    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};
    MarcoFalke commented at 7:12 am on February 18, 2021:
    Still not addressed
    dhruv commented at 5:45 pm on March 3, 2021:
    Done.
  121. DrahtBot added the label Needs rebase on Feb 25, 2021
  122. MarcoFalke commented at 8:56 am on February 26, 2021: member
    @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).
  123. in src/net_processing.cpp:628 in f4450e1057 outdated 
    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;
    jnewbery commented at 10:55 am on March 3, 2021:

    Since you’re changing these lines already, it might be a nice opportunity to remove them from the ctor and use default initialization:

     0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index 3c8ef345e1..caac75b72e 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -537,12 +537,12 @@ struct CNodeState {
 5     //! Whether we've started headers synchronization with this peer.
 6     bool fSyncStarted;
 7     //! When to potentially disconnect peer for stalling headers download
 8-    std::chrono::microseconds m_headers_sync_timeout;
 9+    std::chrono::microseconds m_headers_sync_timeout{0us};
10     //! Since when we're stalling block download progress (in microseconds), or 0.
11-    std::chrono::microseconds m_stalling_since;
12+    std::chrono::microseconds m_stalling_since{0us};
13     std::list<QueuedBlock> vBlocksInFlight;
14     //! When the first entry in vBlocksInFlight started downloading. Don't care when vBlocksInFlight is empty.
15-    std::chrono::microseconds m_downloading_since;
16+    std::chrono::microseconds m_downloading_since{0us};
17     int nBlocksInFlight;
18     int nBlocksInFlightValidHeaders;
19     //! Whether we consider this a preferred download peer.
20@@ -625,9 +625,6 @@ struct CNodeState {
21         pindexBestHeaderSent = nullptr;
22         nUnconnectingHeaders = 0;
23         fSyncStarted = false;
24-        m_headers_sync_timeout = 0us;
25-        m_stalling_since = 0us;
26-        m_downloading_since = 0us;
27         nBlocksInFlight = 0;
28         nBlocksInFlightValidHeaders = 0;
29         fPreferredDownload = false;
    dhruv commented at 5:44 pm on March 3, 2021:
    Done.
    jnewbery commented at 7:31 pm on March 3, 2021:
    Thanks!
  124. jnewbery commented at 11:18 am on March 3, 2021: member

    Thanks for all your work in rebasing this and keeping it current, @dhruv!

    utACK f4450e105781441020964af1e488a033ec1c4be4

  125. dhruv force-pushed on Mar 3, 2021
  126. Change all ping times to std::chrono types 4d98b401fb
  127. Convert block/header sync timeouts to std::chrono types c733ac4d8a
  128. Make all Poisson delays use std::chrono types 55e82881a1
  129. Make tx relay data structure use std::chrono types 0eaea66e8b
  130. dhruv force-pushed on Mar 3, 2021
  131. dhruv commented at 5:57 pm on March 3, 2021: member

    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.

  132. DrahtBot removed the label Needs rebase on Mar 3, 2021
  133. jnewbery commented at 7:36 pm on March 3, 2021: member
    utACK 0eaea66e8bfdfb23ff86c0f0924c2d75f5aca75f
  134. in src/qt/guiutil.cpp:713 in 4d98b401fb outdated 
    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) ?
    ajtowns commented at 10:53 am on March 4, 2021:
    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.
  135. vasild commented at 11:02 am on March 4, 2021: member
    ACK 0eaea66e8bfdfb23ff86c0f0924c2d75f5aca75f
  136. MarcoFalke approved
  137. MarcoFalke commented at 11:14 am on March 4, 2021: member

    re-ACK 0eaea66e8bfdfb23ff86c0f0924c2d75f5aca75f, only changes: minor rename, using C++11 member initializer, using 2min chrono literal, rebase 🤚

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 0eaea66e8bfdfb23ff86c0f0924c2d75f5aca75f, only changes: minor rename, using C++11 member initializer, using 2min chrono literal, rebase 🤚
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgJigwAlGgnegNU2yOZwT9wE9VjciuGc8zXxc4Su130pNLHYd3yirTDUUKwbk4l
 8pzjUIP6GGSFNHWKjMkGTXgIqzlKQ3YUWfBqCBZKt60G+bwAPfm+tri4lBxBsVXk6
 9D2e16A8c/mdFfyRPeFl59qtfBE3Wdu7AtHL47u77jLywCUnAPopm9PaHz/xp+w70
10O0w5FEr2cazU8tp0u2CY6rMufqjyCZZ/qR4fqNwmOFSR+luBma1ZW7Y0VbmTkjVy
11JDrlsITpSyQy9yTosRD0RZ+86BfuGuCR5M3wWsKoepuVpX886pHl/jZEyi9C+eG1
12b6ijElFEcU79COu+zocyThQqAkemegbPUBQYttpPtI9d6FZuBUVMK7P/siBVUUPi
1375xu76QgF2wuUkCFXDtBobLOGTUlUiKair0lcy/vBWjy/oT12LCh1YZTeYuPwYle
14a51LXkhN+dyNUsfYOrN0NkR58VPcKPRk26sbEEzXS750uZx4SIdJ+qSsy5w5LNZw
15518vxGAE
16=d/DI
17-----END PGP SIGNATURE-----

    Timestamp of file with hash ba138de68b952c6d098d60b477ecdf0b15cc363ee617f9836e4dafe46273e159 -

  138. ajtowns commented at 11:42 am on March 4, 2021: member
    utACK 0eaea66e8bfdfb23ff86c0f0924c2d75f5aca75f
  139. fanquake merged this on Mar 4, 2021
  140. fanquake closed this on Mar 4, 2021
  141. sidhujag referenced this in commit 150658305e on Mar 4, 2021
  142. Fabcien referenced this in commit c05aac013d on Jan 26, 2022
  143. Fabcien referenced this in commit a2222e5090 on Jan 26, 2022
  144. Fabcien referenced this in commit f68eea2897 on Jan 27, 2022
  145. Fabcien referenced this in commit 01468a99ea on Jan 27, 2022
  146. DrahtBot locked this on Aug 16, 2022


dhruv DrahtBot MarcoFalke vasild ajtowns felipsoarez martinus sipa jnewbery

Labels
Refactoring P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me