p2p: return CSubNet in LookupSubNet
#26078
Conversation
|
Approach ACK |
|
Interesting simplification and looks reasonable at first glance. A possible alternative would be to retain the checks and make |
3007397 to
0b558d3
Compare
0b558d3 to
60a0283
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
210cb2d to
0b3db2f
Compare
|
Addressed @jonatack's review and now it returns |
|
|
0b3db2f to
07c6053
Compare
|
Force-pushed to make |
|
I still think adding a |
07c6053 to
b7e197c
Compare
|
I don´t think The problem in |
|
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 |
b7e197c to
fc60ced
Compare
5c832c3 p2p, refactor: return `std::optional<CNetAddr>` in `LookupHost` (brunoerg) 34bcdfc p2p, refactor: return vector/optional<CService> in `Lookup` (brunoerg) 7799eb1 p2p, refactor: return `std::vector<CNetAddr>` in `LookupHost` (brunoerg) 5c1774a p2p, refactor: return `std::vector<CNetAddr>` in `LookupIntern` (brunoerg) Pull request description: Continuation of #26078. To improve readability instead of returning a bool and passing stuff by reference, this PR changes: - `LookupHost` to return `std::vector<CNetAddr>` - `LookupHost` to return `std::optional<CNetAddr>` - `Lookup` to return `std::vector<CService>` - `Lookup` to return `std::optional<CService>`. - `LookupIntern` to return `std::vector<CNetAddr>` As discussed in #26078, it would be better to avoid using `optional` in some cases, but for specific `Lookup` and `LookupHost` functions it's necessary to use `optional` to verify if they were able to catch some data from their overloaded function. ACKs for top commit: achow101: ACK 5c832c3 stickies-v: re-ACK 5c832c3 - just addressing two nits, no other changes theStack: re-ACK 5c832c3 Tree-SHA512: ea346fdc54463999646269bd600cd4a1590ef958001d2f0fc2be608ca51e1b4365efccca76dd4972b023e12fcc6e67d226608b0df7beb901bdeadd19948df840
…pHost` 5c832c3 p2p, refactor: return `std::optional<CNetAddr>` in `LookupHost` (brunoerg) 34bcdfc p2p, refactor: return vector/optional<CService> in `Lookup` (brunoerg) 7799eb1 p2p, refactor: return `std::vector<CNetAddr>` in `LookupHost` (brunoerg) 5c1774a p2p, refactor: return `std::vector<CNetAddr>` in `LookupIntern` (brunoerg) Pull request description: Continuation of bitcoin#26078. To improve readability instead of returning a bool and passing stuff by reference, this PR changes: - `LookupHost` to return `std::vector<CNetAddr>` - `LookupHost` to return `std::optional<CNetAddr>` - `Lookup` to return `std::vector<CService>` - `Lookup` to return `std::optional<CService>`. - `LookupIntern` to return `std::vector<CNetAddr>` As discussed in bitcoin#26078, it would be better to avoid using `optional` in some cases, but for specific `Lookup` and `LookupHost` functions it's necessary to use `optional` to verify if they were able to catch some data from their overloaded function. ACKs for top commit: achow101: ACK 5c832c3 stickies-v: re-ACK 5c832c3 - just addressing two nits, no other changes theStack: re-ACK 5c832c3 Tree-SHA512: ea346fdc54463999646269bd600cd4a1590ef958001d2f0fc2be608ca51e1b4365efccca76dd4972b023e12fcc6e67d226608b0df7beb901bdeadd19948df840
4e377d8 to
fb3e812
Compare
There was a problem hiding this comment.
Concept ACK, but Approach ~0 (for now). Reviewed the code (fb3e812) 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.
| subnet = LookupSubNet("1.2.3.4/255.255.255.255"); | ||
| BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.3.4/32"); |
There was a problem hiding this comment.
nit: would make sense to make these one-liners, I think, ideally w/ scripted diff? Or a fn/lambda CheckSubnetEquals to make it even more readable?
|
ACK fb3e812 |
Analyzing the usage of
LookupSubNet, noticed that most cases uses check if the subnet is valid by callingsubnet.IsValid(), and the boolean returned byLookupSubNethasn't been used so much, see:bitcoin/src/httpserver.cpp
Lines 172 to 174 in 29d540b
bitcoin/src/net_permissions.cpp
Lines 114 to 116 in 29d540b
It makes sense to return
CSubNetinstead ofbool.