net: use std::chrono throughout maxOutbound logic

fanquake commented at 11:54 am on October 27, 2020: member

Switch to using std::chrono types for the max outbound related logic. Removes some unnecessary code from init.

fanquake added the label Refactoring on Oct 27, 2020

fanquake added the label P2P on Oct 27, 2020

practicalswift commented at 7:04 am on October 28, 2020: contributor

Concept ACK

theStack commented at 9:14 pm on November 15, 2020: member

Concept ACK Note that the functions SetMaxOutbound{Target,Timeframe} are also used in the fuzz test test/fuzz/connman.cpp, hence the uses should also be removed there.

fanquake commented at 6:22 am on November 16, 2020: member

Note that the functions SetMaxOutbound{Target,Timeframe} are also used in the fuzz test test/fuzz/connman.cpp, hence the uses should also be removed there.

Thanks. I’ve dropped them from the fuzz tests now.

fanquake force-pushed on Nov 16, 2020

DrahtBot commented at 2:18 pm on November 17, 2020: 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:

#20649 (refactor: Remove nMyStartingHeight from CNode/Connman by MarcoFalke)
#20228 (addrman: Make addrman a top-level component 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.

jnewbery commented at 11:29 am on November 18, 2020: member

Concept ACK

DrahtBot commented at 1:10 pm on November 18, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Nov 18, 2020

fanquake force-pushed on Nov 18, 2020

fanquake removed the label Needs rebase on Nov 18, 2020

fanquake commented at 1:15 pm on November 18, 2020: member

Rebased for #20408 and fixed an oversight in the fuzz changes.

in src/net.cpp:2857 in 803212cfb9 outdated

2859-    uint64_t cycleEndTime = nMaxOutboundCycleStartTime + nMaxOutboundTimeframe;
2860-    uint64_t now = GetTime();
2861-    return (cycleEndTime < now) ? 0 : cycleEndTime - GetTime();
2862+    std::chrono::seconds cycleEndTime = nMaxOutboundCycleStartTime + nMaxOutboundTimeframe;
2863+    auto now = GetTime<std::chrono::seconds>();
2864+    return (cycleEndTime < now) ? std::chrono::seconds::zero() : cycleEndTime - GetTime<std::chrono::seconds>();

theStack commented at 8:50 am on November 22, 2020:

Could reuse now here instead of calling GetTime() again: return (cycleEndTime < now) ? std::chrono::seconds::zero() : cycleEndTime - now;

fanquake commented at 3:53 am on November 23, 2020:

Thanks. I was going to leave this as a straight refactor, but have taken your suggestion.

fanquake force-pushed on Nov 23, 2020

in src/net.cpp:2844 in 2404f787f8 outdated

2864-        nMaxOutboundCycleStartTime = GetTime();
2865-    }
2866-    nMaxOutboundTimeframe = timeframe;
2867+    std::chrono::seconds cycleEndTime = nMaxOutboundCycleStartTime + nMaxOutboundTimeframe;
2868+    auto now = GetTime<std::chrono::seconds>();
2869+    return (cycleEndTime < now) ? std::chrono::seconds::zero() : cycleEndTime - now;

MarcoFalke commented at 6:37 am on November 23, 2020:

I think 0 can be written shorter with chrono literals?

fanquake commented at 12:53 pm on December 9, 2020:

Done

MarcoFalke approved

in src/net.h:208 in 2404f787f8 outdated

208@@ -209,7 +209,7 @@ class CConnman
209         BanMan* m_banman = nullptr;
210         unsigned int nSendBufferMaxSize = 0;
211         unsigned int nReceiveFloodSize = 0;
212-        uint64_t nMaxOutboundTimeframe = 0;
213+        std::chrono::seconds nMaxOutboundTimeframe{0};

jnewbery commented at 9:45 am on November 23, 2020:

Perhaps just remove this as an Options member entirely. If there’s no way to change the value in init.cpp, and its not used in the test, then this is just indirect, obfuscating code. Just use the constant internally in net.

fanquake commented at 1:11 pm on December 9, 2020:

I forgot to do this in the last push, but should have addressed it now.

in src/net.h:366 in 2404f787f8 outdated

366     uint64_t GetMaxOutboundTarget();
367-
368-    //!set the timeframe for the max outbound target
369-    void SetMaxOutboundTimeframe(uint64_t timeframe);
370-    uint64_t GetMaxOutboundTimeframe();
371+    std::chrono::seconds GetMaxOutboundTimeframe();

jnewbery commented at 10:00 am on November 23, 2020:

It’s a good idea to use std::chrono internally in net, but I think you can leave these interface functions GetMaxOutboundTimeframe() and GetMaxOutboundTimeLeftInCycle() the same and continue to just send a count of seconds to the caller. The only place these are used is in rpc/net, where the count of seconds is returned directly to the user.

fanquake commented at 12:53 pm on December 9, 2020:

The only place these are used is in rpc/net, where the count of seconds is returned directly to the user.

GetMaxOutboundTimeLeftInCycle is called in CConnman::OutboundTargetReached.

jnewbery commented at 10:52 am on December 10, 2020:

Ah, of course.

jnewbery commented at 10:03 am on November 23, 2020: member

Concept ACK, especially cleaning up init.

Longer term, it’d be nice to move initialization of some of these CConnman parameters into the constructor and make them const. That clarifies the initialization and means that they don’t need to be guarded by a mutex.

fanquake force-pushed on Dec 9, 2020

fanquake commented at 12:53 pm on December 9, 2020: member

I’ve rebased this, cherry-picked in fa11110bff6288f63e0c487e2e4b4079fb0f4569 from 20602 and updated some std::chrono usage to use literals as suggested.

fanquake force-pushed on Dec 9, 2020

in src/net.cpp:2842 in 832fc10e9e outdated

2865-        // the timeframe
2866-        nMaxOutboundCycleStartTime = GetTime();
2867-    }
2868-    nMaxOutboundTimeframe = timeframe;
2869+    std::chrono::seconds cycleEndTime = nMaxOutboundCycleStartTime + MAX_UPLOAD_TIMEFRAME;
2870+    auto now = GetTime<std::chrono::seconds>();

jonatack commented at 3:51 pm on December 9, 2020:

832fc10e9e45abf92a perhaps use explicit typing here, const, and universal initialization to check for narrowing

0-    std::chrono::seconds cycleEndTime = nMaxOutboundCycleStartTime + MAX_UPLOAD_TIMEFRAME;
1-    auto now = GetTime<std::chrono::seconds>();
2+    const std::chrono::seconds cycleEndTime{nMaxOutboundCycleStartTime + MAX_UPLOAD_TIMEFRAME};
3+    const std::chrono::seconds now{GetTime<std::chrono::seconds>()};

in src/net.cpp:2856 in 832fc10e9e outdated

2851@@ -2871,8 +2852,8 @@ bool CConnman::OutboundTargetReached(bool historicalBlockServingLimit)
2852     if (historicalBlockServingLimit)
2853     {
2854         // keep a large enough buffer to at least relay each block once
2855-        uint64_t timeLeftInCycle = GetMaxOutboundTimeLeftInCycle();
2856-        uint64_t buffer = timeLeftInCycle / 600 * MAX_BLOCK_SERIALIZED_SIZE;
2857+        std::chrono::seconds timeLeftInCycle = GetMaxOutboundTimeLeftInCycle();
2858+        uint64_t buffer = timeLeftInCycle / std::chrono::minutes{10} * MAX_BLOCK_SERIALIZED_SIZE;

jonatack commented at 3:55 pm on December 9, 2020:

832fc10e9e45abf92a4f04012c918156 maybe prefer to be explicit about types and narrowing conversions in the net code

0-        std::chrono::seconds timeLeftInCycle = GetMaxOutboundTimeLeftInCycle();
1-        uint64_t buffer = timeLeftInCycle / std::chrono::minutes{10} * MAX_BLOCK_SERIALIZED_SIZE;
2+        const std::chrono::seconds timeLeftInCycle{GetMaxOutboundTimeLeftInCycle()};
3+        const uint64_t buffer{timeLeftInCycle / std::chrono::minutes{10} * static_cast<uint64_t>(MAX_BLOCK_SERIALIZED_SIZE)};

in src/net.cpp:2809 in 832fc10e9e outdated

2805@@ -2806,8 +2806,8 @@ void CConnman::RecordBytesSent(uint64_t bytes)
2806     LOCK(cs_totalBytesSent);
2807     nTotalBytesSent += bytes;
2808 
2809-    uint64_t now = GetTime();
2810-    if (nMaxOutboundCycleStartTime + nMaxOutboundTimeframe < now)
2811+    auto now = GetTime<std::chrono::seconds>();

jonatack commented at 4:01 pm on December 9, 2020:

832fc10e9e45abf9 const and maybe explicit typing + narrowing check

0    const std::chrono::seconds now{GetTime<std::chrono::seconds>()};

fanquake commented at 3:43 am on December 13, 2020:

Having std::chrono::seconds twice on the same line is pretty verbose. I’d like to think we can just use auto here because the type is clear from the call to GetTime<>().

jonatack commented at 8:57 pm on December 13, 2020:

Yes, the reason for the comment here was constness.

jonatack commented at 9:05 pm on December 13, 2020:

Note that existing code in the same file specifies the type explicitly, line 1646: const std::chrono::seconds now = GetTime<std::chrono::seconds>(); introduced in commit b24a17f0398

jonatack commented at 4:04 pm on December 9, 2020: member

ACK 832fc10e9e45abf92a4f04012c918156d92a3649

with some optional style ideas below

in src/net.h:78 in 832fc10e9e outdated

72@@ -73,9 +73,9 @@ static const bool DEFAULT_UPNP = false;
73 /** The maximum number of peer connections to maintain. */
74 static const unsigned int DEFAULT_MAX_PEER_CONNECTIONS = 125;
75 /** The default for -maxuploadtarget. 0 = Unlimited */
76-static const uint64_t DEFAULT_MAX_UPLOAD_TARGET = 0;
77+static constexpr uint64_t DEFAULT_MAX_UPLOAD_TARGET = 0;
78 /** The default timeframe for -maxuploadtarget. 1 day. */
79-static const uint64_t MAX_UPLOAD_TIMEFRAME = 60 * 60 * 24;
80+static constexpr std::chrono::seconds MAX_UPLOAD_TIMEFRAME{60 * 60 * 24};

jnewbery commented at 10:40 am on December 10, 2020:

Since this is only used internally in net, it could now be moved to the implementation cpp file to avoid exporting this symbol to other translation units.

fanquake commented at 3:43 am on December 13, 2020:

Done in the latest push.

jnewbery commented at 10:53 am on December 10, 2020: member

utACK 832fc10e9e

In case you touch this branch again: In commit net: remove nMaxOutboundTimeframe from connection options I think you need to change “DEFAULT_MAX_UPLOAD_TARGET is a compile time constant.” to “DEFAULT_MAX_UPLOAD_TIMEFRAME is a compile time constant.”

net: remove SetMaxOutboundTarget

This has been unused since f3552da81393a8e78ce3e2afed0b9c9d1ff5cee0.

2f3f1aec1f

net: remove SetMaxOutboundTimeframe

This was introduced in 872fee3fccc8b33b9af0a401b5f85ac5504b57eb and it's unclear
if it's ever been used.

b117eb1486

net: remove nMaxOutboundTimeframe from connection options

It's not actually possible to change this value, so remove the
indirection of it being a conn option.

DEFAULT_MAX_UPLOAD_TIMEFRAME is a compile time constant.

173d0d35f1

init: set nMaxOutboundLimit connection option directly

DEFAULT_MAX_UPLOAD_TARGET is a compile time constant.

f805933e70

net: use std::chrono throughout maxOutbound logic 0475c8ba4d

fanquake force-pushed on Dec 13, 2020

fanquake commented at 3:45 am on December 13, 2020: member

Rebased & addressed some comments.

In case you touch this branch again:

I’ve fixed the commit message.

MarcoFalke approved

MarcoFalke commented at 8:41 am on December 15, 2020: member

review ACK 0475c8ba4d10dce79092361bc4c23c11dadba39a 🎭

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 0475c8ba4d10dce79092361bc4c23c11dadba39a 🎭
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjthwwAzVabd9gXhamR9ER43crYv/VUAp95aBP6//eaBjC5dwfvq8UzeJtoP9aF
 8NiVZ14+MDWyLQyUBZF+DYU3veWMHYgaIGSpOQ+ZlgVYCva2ZmkhqnlCk4de4jjcL
 91WMJV9zxauGhITHd+AVqy2SyKd/6ljAMWn3v89KYqDApw7zzEIe5Jpbfq7f7kbbh
106d9X1XqQoSCNS+SwCouEu192Y9f+44jAo08FupUshCHMUMV+MZs87Hs3rD+N4DsX
1195AZV1gO48HgDldPvUC7LZIieTIJFBpI4Y7dr/Re186h1csWc++8FbXOTLN12gg2
12+KR4/XgqYVBNKBhv/X60Hc113MReXc3gbGqXtFMu1vVXRJnEXM5E/bk9qxYzsdFX
13n5D97DMHOotQh1W61f7Mqlqk9N+qP28P2BfJwxamyVC1DQ4PiLvlY79kxm3UkWGx
14BzFD1qqn7M67Twm2z1SO0nBgQZCXMPsmFI0dR+abUT4o5jUXMIovVef26reBTS8v
15ReieyZiR
16=+TOK
17-----END PGP SIGNATURE-----

Timestamp of file with hash 963300e8377971b0f423721ee34e6ab50d44d071f41bb4f7f3598935d178a88d -

jnewbery commented at 10:04 am on December 15, 2020: member

utACK 0475c8ba4d10dce79092361bc4c23c11dadba39a

fanquake merged this on Dec 15, 2020

fanquake closed this on Dec 15, 2020

sidhujag referenced this in commit 83106bdc9c on Dec 15, 2020

Fabcien referenced this in commit 250f648f76 on Jan 26, 2022

DrahtBot locked this on Feb 15, 2022

fanquake deleted the branch on Nov 9, 2022

net: use std::chrono throughout maxOutbound logic #20253

Conflicts