cc @practicalswift @MarcoFalke ; related: #20188 (comment)

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• net: use std::chrono throughout maxOutbound logic #20253 (net: use std::chrono throughout maxOutbound logic by fanquake)
• net: don't bind on 0.0.0.0 if binds are restricted to Tor #20234 (net: don't extra bind for Tor if binds are restricted by vasild)

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.

review ACK c401a1d83f119f11fe0e446193458c5de224adbf

Is there any downside to just using default initialization for these data members?

-    uint64_t nMaxOutboundTotalBytesSentInCycle GUARDED_BY(cs_totalBytesSent);
+    uint64_t nMaxOutboundTotalBytesSentInCycle GUARDED_BY(cs_totalBytesSent) {0};
-    uint64_t nMaxOutboundCycleStartTime GUARDED_BY(cs_totalBytesSent);
+    uint64_t nMaxOutboundCycleStartTime GUARDED_BY(cs_totalBytesSent) {0};

It seems to me that Init() should only initialize members that require a data from the connOptions object.

Thanks for noticing and fixing this @ajtowns!

Any reason not to use in-class member initialisation?

Like this:

diff --git a/src/net.h b/src/net.h
index 77649247d..d8d7daf64 100644
--- a/src/net.h
+++ b/src/net.h
@@ -478,10 +478,10 @@ private:
     uint64_t nTotalBytesSent GUARDED_BY(cs_totalBytesSent) {0};

     // outbound limit & stats
-    uint64_t nMaxOutboundTotalBytesSentInCycle GUARDED_BY(cs_totalBytesSent);
-    uint64_t nMaxOutboundCycleStartTime GUARDED_BY(cs_totalBytesSent);
-    uint64_t nMaxOutboundLimit GUARDED_BY(cs_totalBytesSent);
-    uint64_t nMaxOutboundTimeframe GUARDED_BY(cs_totalBytesSent);
+    uint64_t nMaxOutboundTotalBytesSentInCycle GUARDED_BY(cs_totalBytesSent) {0};
+    uint64_t nMaxOutboundCycleStartTime GUARDED_BY(cs_totalBytesSent) {0};
+    uint64_t nMaxOutboundLimit GUARDED_BY(cs_totalBytesSent) {0};
+    uint64_t nMaxOutboundTimeframe GUARDED_BY(cs_totalBytesSent) {0};

     // P2P timeout in seconds
     int64_t m_peer_connect_timeout;

Rationale provided in the C++ Core Guidelines: C.48: Prefer in-class initializers to member initializers in constructors for constant initializers :)

What @jnewbery said literally 4 seconds before me :)

I presumed that the Start method was provided so that connman could be stopped and started while core is running. Members would be re-initialized. Though, we don't do that right now and I don't think we will, so either approach is fine by me.

I think it would be a large change to make it reasonable to call Start() twice, so moved init to declaration, without resetting them in Start().

ACK 9d09132: patch looks correct!

review ACK 9d09132 , checked that we call Start only once and in the same scope where connman is constructed (AppInitMain) 💸

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK 9d09132be4ff99f98ca905c342347d5f35f13350 , checked that we call Start only once and in the same scope where connman is constructed (AppInitMain) 💸
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiqVQv/UC5Tg9YLXIPXSm3OwF820sVE+O+s6Ks4t71B/pzcwfUzq4llOxTx3TDW
nvpgLlCVeJkW79tUby/5YBVzrrgZ80KaGldO3H2eGvdi9ARSD3cX6OtokYNUHmty
ZJXijsEs6zlli3LK3wlBXZ30HNjkSzyLOK9CvnU9/k+MNwKvqSDYQRzHHMRungUU
+fBuc2CVW16WVhgmXMWfQb7QWjMxuqe5JwD6PAuGPftgXYB48uHR8Uys36X1o6fn
TQ1WqoZcDDULp4XIyRxqcC4FhTCBaacLE3ePZC/AzcP7Ga51brW9XuNZC8yFW4su
omaUNR2Sw0XGTqtXD1BeIT00ZbkLyX7BtVi7VMgXVCIrjZBqdPXIK4PBqtydMk7g
S5ocg3QVSLE8lZGPXg3va8u0cs7cu4ylxIyx1iIwDJmItFSv48EVGjk7CVdbWv+c
5zMC9m3XbyYQP8IP59Z2LVstQ8ga+dWCxECTTtT260gwl2CsSCFl59fL4jBWNHvu
hUIVVCRj
=EJrI
-----END PGP SIGNATURE-----

Code review ACK 9d09132