bitcoin
bitcoin copied to clipboard
bitcoin
p2p: return `CSubNet` in `LookupSubNet`
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.
rpc/net.cpp:717:9: error: no matching function for call to 'LookupSubNet'
LookupSubNet(request.params[0].get_str(), subNet);
^~~~~~~~~~~~
./netbase.h:177:9: note: candidate function not viable: requires single argument 'subnet_str', but 2 arguments were provided
CSubNet LookupSubNet(const std::string& subnet_str);
^
1 error generated.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Code Coverage
For detailed information about the code coverage, see the test coverage report.
Reviews
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.
Conflicts
No conflicts as of last run.
std::optional seems redundant here. I think it's better to just return the CSubNet?
Force-pushed to make LookupSubnet not return an invalid but existing CSubNet, now it returns a valid CSubNet or std::nullopt. Thanks, @luke-jr and @mzumsande.
I still think adding a 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()).
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.
