The goal of this PR is to be able to depend on m_inbound_onion in AttemptToEvictConnection in #20197:
CNode::m_inbound_onion is inbound in the CNode ctor to have a validity check at the class boundaryCNode::ConnectedThroughNetwork() for its inbound statusIsInboundOnion() that also allows unit testing it
Concept ACK
Thanks for improving testing!
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
1094@@ -1095,7 +1095,7 @@ class CNode
1095 CService addrLocal GUARDED_BY(cs_addrLocal);
1096 mutable RecursiveMutex cs_addrLocal;
1097
1098- //! Whether this peer connected via our Tor onion service.
1099+ //! Whether this peer is an inbound onion, e.g. connected via our Tor onion service.
1100 const bool m_inbound_onion{false};
Hm, good eye.
-> In this case, ISTM we could remove the default false argument in the constructor declaration
-> As to your last point, it suggests the default member m_inbound_onion initializer not set a value, since we are doing it dynamically in the ctor. It’s true that I see a build error in this case, but not without the change in net.h:1099
0/src/net.cpp
1- nMyStartingHeight(nMyStartingHeightIn),
2- m_inbound_onion(inbound_onion && conn_type_in == ConnectionType::INBOUND)
3+ nMyStartingHeight(nMyStartingHeightIn)
4
5/src/net.h
6 //! Whether this peer is an inbound onion, e.g. connected via our Tor onion service.
7- const bool m_inbound_onion{false};
8+ const bool m_inbound_onion;
0net.cpp:2947:1: error: uninitialized const member in ‘const bool’ [-fpermissive]
1 2947 | CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion)
2 | ^~~~~
3In file included from net.cpp:10:
4./net.h:1099:16: note: ‘const bool CNode::m_inbound_onion’ should be initialized
5 1099 | const bool m_inbound_onion;
6 | ^~~~~~~~~~~~~~~
I don’t mind pulling in a commit from your branch if you’d like to adjust this.
More generally, from what I can gather, the ideas behind in-class default member initializers added in C++11 were (a) to allow a non-static data member to be initialized where it is declared, e.g. in its class. A constructor can then use the initializer when run-time initialization is needed, which can then (b) tidy up the code up by avoiding doing it in the member initializer list of the constructor. This provides extra benefits in classes with multiple constructors.
I guess one can see the default member initializer as the general default, particularly useful for constant initializers, that can be overridden by any specific ones in member initializer lists in the constructors.
Some resources (send me any good ones you might suggest; my understanding on this is evolving):
not sure if we have any guidelines (or dominant opinions) on this subject, but isn’t it overkill to have two initializations for a class member: once via in-class initialization and once via initializer list in the ctor? (And yes, I’m aware that initializer lists have higher priority in C++). As reader of the class interface I’d not assume an in-class initialization is overruled in the ctor. Also, without the in-class initialization the compiler has the chance to throw an error if the const member was forgot to be initialized in the ctor. @theStack: done in #21167
2956@@ -2957,7 +2957,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn
2957 m_conn_type(conn_type_in),
2958 nLocalServices(nLocalServicesIn),
2959 nMyStartingHeight(nMyStartingHeightIn),
2960- m_inbound_onion(inbound_onion)
2961+ m_inbound_onion(inbound_onion && conn_type_in == ConnectionType::INBOUND)
inbound_onion parameter as true but lie about this, as it is an outbound connection? Is this a code bug?
Wouldn’t this be better as an assertion?
Assume, if Assert is too strong and risky
inbound_onion should only be true when a new CNode is instantiated from CConnman::AcceptConnection(), so this should only fail if a bug is added. Made it an assert.
in what case would the caller pass
inbound_onionparameter as true but lie about this
So, adding the assertion found one! Our net_tests had a case that lied; updating it.
as CNode ctor should only be passed inbound_onion = true
when the connection is inbound
and drop an unneeded check in CNode::ConnectedThroughNetwork()
297@@ -298,7 +298,7 @@ CNode ConsumeNode(FuzzedDataProvider& fuzzed_data_provider) noexcept
298 const CAddress addr_bind = ConsumeAddress(fuzzed_data_provider);
299 const std::string addr_name = fuzzed_data_provider.ConsumeRandomLengthString(64);
300 const ConnectionType conn_type = fuzzed_data_provider.PickValueInArray({ConnectionType::INBOUND, ConnectionType::OUTBOUND_FULL_RELAY, ConnectionType::MANUAL, ConnectionType::FEELER, ConnectionType::BLOCK_RELAY, ConnectionType::ADDR_FETCH});
301- const bool inbound_onion = fuzzed_data_provider.ConsumeBool();
302+ const bool inbound_onion{conn_type == ConnectionType::INBOUND ? fuzzed_data_provider.ConsumeBool() : false};
ConnectionType::INBOUND && fuzzed_data_provider.ConsumeBool()
1233@@ -1234,6 +1234,9 @@ class CNode
1234 void MaybeSetAddrName(const std::string& addrNameIn);
1235
1236 std::string ConnectionTypeAsString() const;
1237+
1238+ /** Whether this peer is an inbound onion, e.g. connected via our Tor onion service. */
1239+ bool IsInboundOnion() const { return m_inbound_onion; }
pnode->IsInboundConn() && pnode->ConnectedThroughNetwork() == NET_ONION which is equivalent to the newly added pnode->IsInboundOnion()?
IsInboundConn(), so this would shift the coverage from m_inbound_onion directly, to indirectly via ConnectedThroughNetwork() == NET_ONION, which would avoid the getter but seems less robust? In AttemptToEvictConnection, outbound peers return early, so technically we could use the latter…but I like it less.)
ACK 86c495223f048e5ca2cf0d8730af7db3b76f7aba
Two minor comments below, feel free to ignore.
1233@@ -1234,6 +1234,9 @@ class CNode
1234 void MaybeSetAddrName(const std::string& addrNameIn);
1235
1236 std::string ConnectionTypeAsString() const;
1237+
1238+ /** Whether this peer is an inbound onion, e.g. connected via our Tor onion service. */
review ACK 86c495223f048e5ca2cf0d8730af7db3b76f7aba 🐍
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3review ACK 86c495223f048e5ca2cf0d8730af7db3b76f7aba 🐍
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUjcBAwAyH/ZAIoBmnAriDbYf2IdudHCi55YS3Gb2Viv4ujVLCCt+dTDj6OOg6RR
8fsTKJe3f0g3eHdss1KkMxTGQm1WP6MDl7Iqpkuuar7eH4F7WslFSamnnRStDZXcG
9vUQvrGSKNakc20PT0wT8bP7UxA2+z8/gVS9V40+lKma8VmwZgYE0EcKztdfUiPL5
100v+whP/aUPxjEleeG4aec0FkOEwELAdoC6gTtn2+hBBv2x+Kj6KmeheusTNBpP7P
11jYbMqz5K+qEd10yFc+4yH7gdVyKmAuNunToURLz2SZcQmla2iMinRIXboLEhRcvm
12pS4YuY62sQ+3bOxxTi3Td7vRMKFmDJfy92vkXgDcQvvpHw72M+k+3ieuzYNvJFUq
13maOT4trYzF79M5F4Pd18ywIkgz8v50uddPVEVhl4LGeZyAN47WOH0vIoSZBhmuNN
14IdGom1GuBOMsztoXEZDLYEPr9X9HDB8CgGW5t02g4cf/EoqbtZHzIM2VOXDkiVYe
15Y5IbMXDV
16=O80S
17-----END PGP SIGNATURE-----
Timestamp of file with hash 683c8f1c6754ede192ead999e54c0440e14b98b058b5f69d6e0b9817be2c17bb -
