CSubNet in LookupSubNet
#26078
Analyzing the usage of LookupSubNet, noticed that most cases uses check if the subnet is valid by calling subnet.IsValid(), and the boolean returned by LookupSubNet hasn’t been used so much, see:
https://github.com/bitcoin/bitcoin/blob/29d540b7ada890dd588c4825d40c27c5e6f20061/src/httpserver.cpp#L172-L174
https://github.com/bitcoin/bitcoin/blob/29d540b7ada890dd588c4825d40c27c5e6f20061/src/net_permissions.cpp#L114-L116
It makes sense to return CSubNet instead of bool.
Interesting simplification and looks reasonable at first glance. A possible alternative would be to retain the checks and make LookupSubNet return std::optional<CSubNet> instead.
0rpc/net.cpp:717:9: error: no matching function for call to 'LookupSubNet'
1 LookupSubNet(request.params[0].get_str(), subNet);
2 ^~~~~~~~~~~~
3./netbase.h:177:9: note: candidate function not viable: requires single argument 'subnet_str', but 2 arguments were provided
4CSubNet LookupSubNet(const std::string& subnet_str);
5 ^
61 error generated.
171@@ -172,11 +172,9 @@ CService LookupNumeric(const std::string& name, uint16_t portDefault = 0, DNSLoo
172 * @param[in] subnet_str A string representation of a subnet of the form
173 * `network address [ "/", ( CIDR-style suffix | netmask ) ]`
174 * e.g. "2001:db8::/32", "192.0.2.0/255.255.255.0" or "8.8.8.8".
175- * @param[out] subnet_out Internal subnet representation, if parsable/resolvable
176- * from `subnet_str`.
177- * @returns whether the operation succeeded or not.
178+ * @returns CSubNet object.
Perhaps mention possible non-valid result here
0 * [@returns](/bitcoin-bitcoin/contributor/returns/) a CSubNet object (that may or may not be valid).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | vasild, theStack, achow101 |
| Concept ACK | stickies-v |
| Stale ACK | w0xlt |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
62@@ -63,9 +63,9 @@ FUZZ_TARGET(netbase_dns_lookup)
63 assert(!resolved_service.IsInternal());
64 }
65 {
66- CSubNet resolved_subnet;
67- if (LookupSubNet(name, resolved_subnet)) {
68- assert(resolved_subnet.IsValid());
69+ std::optional<CSubNet> resolved_subnet = LookupSubNet(name);
70+ if (resolved_subnet.has_value()) {
71+ assert(resolved_subnet.value().IsValid() || !resolved_subnet.value().IsValid());
assert looks tautological.
More general, if LookupSubNet is changed to return an optional, would it be possible to either return a valid CSubNet, or std::nullopt? Or is there a need to distinguish between std::nullopt and an invalid but existing CSubNet?
CSubNet. Just changed the approach to return only a valid one otherwise it returns std::nullopt.
std::optional seems redundant here. I think it’s better to just return the CSubNet?
697@@ -698,23 +698,20 @@ bool LookupSubNet(const std::string& subnet_str, CSubNet& subnet_out)
698 uint8_t netmask;
699 if (ParseUInt8(netmask_str, &netmask)) {
700 // Valid number; assume CIDR variable-length subnet masking.
701- subnet_out = CSubNet{addr, netmask};
702- return subnet_out.IsValid();
703+ return CSubNet{addr, netmask};
LookupSubnet not return an invalid but existing CSubNet, now it returns a valid CSubNet or std::nullopt. Thanks, @luke-jr and @mzumsande.
std::optional here is asking for bugs. We’re already seeing this kind of thing with the new util::Result stuff. It’s important to only have one “no value” state.
I don´t think std::optional is a problem per se, but CSubNet has the bool CSubNet::valid which can be used instead of std::optional in this case.
The problem in util::Result has more to do with the complexity of the class than in the use of std::optional.
The problem is having two ways to represent an invalid state, especially when they can be at odds with each other (in that case, .has_value() && !->IsValid()).
Fair enough, it’s better to return CSubNet instead. I’m going to change it.
reACK https://github.com/bitcoin/bitcoin/pull/26078/commits/fc60ced518c7d5e1696018e1378c457b70c6909b
nit: Maybe an explicit initialization could be added?
0class CSubNet
1{
2protected:
3 // ...
4 /// Is this value valid? (only used to signal parse errors)
5 bool valid{false};
168@@ -169,8 +169,7 @@ static bool InitHTTPAllowList()
169 rpc_allow_subnets.push_back(CSubNet(localv4, 8)); // always allow IPv4 local subnet
170 rpc_allow_subnets.push_back(CSubNet(localv6)); // always allow IPv6 localhost
171 for (const std::string& strAllow : gArgs.GetArgs("-rpcallowip")) {
172- CSubNet subnet;
173- LookupSubNet(strAllow, subnet);
174+ CSubNet subnet = LookupSubNet(strAllow);
nit, here and elsewhere:
0 const CSubNet subnet = LookupSubNet(strAllow);
686+CSubNet LookupSubNet(const std::string& subnet_str)
687 {
688+ CSubNet subnet;
689 if (!ContainsNoNUL(subnet_str)) {
690- return false;
691+ return subnet;
assert(!subnet.IsValid()); after CSubNet subnet; to make it obvious that we return an invalid object and to guard against future changes that construct a valid CSubNet by default.
1312-
1313- LookupSubNet(strNode.toStdString(), possibleSubnet);
1314- if (possibleSubnet.IsValid() && m_node.unban(possibleSubnet))
1315- {
1316+ std::optional<CSubNet> possibleSubnet = LookupSubNet(strNode.toStdString());
1317+ if (possibleSubnet.has_value() && m_node.unban(possibleSubnet.value())) {
possibleSubnet.has_value() will always be true. I guess this is an artifact from a previous incarnation of this PR that used std::optional.
62@@ -63,9 +63,7 @@ FUZZ_TARGET(netbase_dns_lookup)
63 assert(!resolved_service.IsInternal());
64 }
65 {
66- CSubNet resolved_subnet;
67- if (LookupSubNet(name, resolved_subnet)) {
68- assert(resolved_subnet.IsValid());
69- }
70+ CSubNet resolved_subnet = LookupSubNet(name);
71+ assert(resolved_subnet.IsValid() || !resolved_subnet.IsValid());
x || !x will never be false. It suffices to change this to (void)LookupSubNet(name);.
30@@ -31,9 +31,7 @@ static CNetAddr ResolveIP(const std::string& ip)
31
32 static CSubNet ResolveSubNet(const std::string& subnet)
33 {
34- CSubNet ret;
35- LookupSubNet(subnet, ret);
36- return ret;
37+ return LookupSubNet(subnet);
38 }
ResolveSubNet() is identical to LookupSubNet(). Maybe ditch the former and change the callers to use directly LookupSubNet()?
LookupSubNet().
1316-
1317- LookupSubNet(strNode.toStdString(), possibleSubnet);
1318- if (possibleSubnet.IsValid() && m_node.unban(possibleSubnet))
1319- {
1320+ const CSubNet possibleSubnet{LookupSubNet(strNode.toStdString())};
1321+ if (m_node.unban(possibleSubnet)) {
The new code could call unban() with an invalid subnet whereas the previous one would not. unban() would call m_banned.erase() which would use operator==(const CSubNet& a, const CSubNet& b) which could match another invalid entry, I guess. Maybe that is ok, but to keep this change “non-functional”, the new code should be:
0if (possibleSubnet.IsValid() && m_node.unban(possibleSubnet)) {
ACK 9fb86661cf81e27131eb5f8d901397a25e2cd4b4
Thanks!
239@@ -247,85 +240,85 @@ BOOST_AUTO_TEST_CASE(subnet_test)
240 BOOST_CHECK(!CSubNet(tor_addr, 200).IsValid());
241 BOOST_CHECK(!CSubNet(tor_addr, ResolveIP("255.0.0.0")).IsValid());
242
243- subnet = ResolveSubNet("1.2.3.4/255.255.255.255");
244+ subnet = LookupSubNet("1.2.3.4/255.255.255.255");
245 BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.3.4/32");
CheckSubnetEquals to make it even more readable?
Concept ACK, but Approach ~0 (for now). Reviewed the code (fb3e812277041f239b97b88689a5076796d75b9b) and it all looks good to me.
Generally supportive of reducing out-parameters in our function signatures, and this PR offers a modest improvement in readability and uniformizes how we check the return value of LookupSubNet, which is nice.
I think luke-jr raised a good point that having the return value be falsy in two different ways (through std::optional wrapper and .IsValid() is dangerous and should be avoided, and I agree. I’m approach ~0 as I think this PR improves things, but I think (but did not yet fully implement/verify) the real improvement would be to enforce CSubNet validity in the constructor (wrapped with std::optional factory methods) so we can improve the interface by handling bad input data early and guarantee that all CSubNet instances are valid, and wrap them in a std::optional when validity is not guaranteed. This requires a more substantial refactoring, though, so I’m not opposed to merging this first (and my suggested approach may turn out infeasible/not worth it in practice) - but I think it would make for a much cleaner interface.
