refactor: net: subnet lookup: use single-result LookupHost() #17160

1. 
   theStack commented at 11:51 am on October 16, 2019: member

   plus describe single IP subnet case for more clarity
2. fanquake added the label P2P on Oct 16, 2019
3. MarcoFalke added the label Refactoring on Oct 16, 2019
4. 
   MarcoFalke commented at 12:11 pm on October 16, 2019: member

   This is refactor, right?
5. theStack force-pushed on Oct 16, 2019
6. theStack renamed this:
   net: subnet lookup: use single-result LookupHost()
   refactor: net: subnet lookup: use single-result LookupHost()
   on Oct 16, 2019
7. 
   theStack commented at 12:33 pm on October 16, 2019: member

   This is refactor, right?

   True, I added the refactor prefix to the pull request title and commit message.

8. 
   laanwj commented at 2:34 pm on October 16, 2019: member

   as this concerns DNS: @TheBlueMatt
9. 
   dongcarl commented at 2:50 pm on October 16, 2019: member

   I added this TODO, this change merely changes our use of “the std::vector<CNetAddr>& out-parameter version of LookupHost” to “the CNetAddr& out-parameter version of LookupHost”.

   Here’s the CNetAddr& version: https://github.com/bitcoin/bitcoin/blob/c34b88620dc8435b83e6744895f2ecd3c9ec8de7/src/netbase.cpp#L158-L172

   And here’s the std::vector<CNetAddr>& version: https://github.com/bitcoin/bitcoin/blob/c34b88620dc8435b83e6744895f2ecd3c9ec8de7/src/netbase.cpp#L131-L156

10. 
    practicalswift commented at 12:35 pm on October 17, 2019: contributor

    FWIW - LookupHost(…) usages:

    0CConnman::ThreadDNSAddressSeed() → LookupHost(char const*, std::vector<CNetAddr>&, unsigned int, bool)
    1main → AppInit(int, char**) → AppInitMain(InitInterfaces&) → InitHTTPServer() → LookupHost(char const*, CNetAddr&, bool)
    2main → AppInit(int, char**) → AppInitMain(InitInterfaces&) → InitHTTPServer() → LookupHost(char const*, CNetAddr&, bool) → LookupHost(char const*, std::vector<CNetAddr>&, unsigned int, bool)
    3main → AppInit(int, char**) → AppInitMain(InitInterfaces&) → InitHTTPServer() → LookupSubNet(char const*, CSubNet&) → LookupHost(char const*, std::vector<CNetAddr>&, unsigned int, bool)
    4main → AppInit(int, char**) → AppInitMain(InitInterfaces&) → LookupSubNet(char const*, CSubNet&) → LookupHost(char const*, std::vector<CNetAddr>&, unsigned int, bool)
    5setban(JSONRPCRequest const&) → LookupHost(char const*, CNetAddr&, bool)
    6setban(JSONRPCRequest const&) → LookupHost(char const*, CNetAddr&, bool) → LookupHost(char const*, std::vector<CNetAddr>&, unsigned int, bool)
    7setban(JSONRPCRequest const&) → LookupSubNet(char const*, CSubNet&) → LookupHost(char const*, std::vector<CNetAddr>&, unsigned int, bool)
    8ThreadMapPort() → LookupHost(char const*, CNetAddr&, bool)
    9ThreadMapPort() → LookupHost(char const*, CNetAddr&, bool) → LookupHost(char const*, std::vector<CNetAddr>&, unsigned int, bool)
    

11. 
    theStack commented at 1:11 pm on October 17, 2019: member

    as this concerns DNS: @TheBlueMatt

    Note that no real lookup (resolving) is performed in LookupSubNet(), as the calls to LookupHost() pass the flag bool fAllowLookup = false. To quote the further called function LookupIntern():

    https://github.com/bitcoin/bitcoin/blob/c34b88620dc8435b83e6744895f2ecd3c9ec8de7/src/netbase.cpp#L91-L93

    Hence the calls to LookupHost() in LookupSubNet() are only used to parse an IP-Address string and convert it to a CNetAddr structure.

12. 
    DrahtBot commented at 4:31 pm on December 16, 2019: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #23219 (p2p: rm CNetAddr vector and copy in LookupSubNet() and update/tidy up by jonatack)

    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.

13. DrahtBot added the label Needs rebase on Jan 22, 2020
14. theStack force-pushed on Feb 28, 2020
15. 
    theStack commented at 9:04 am on February 28, 2020: member

    Rebased.
16. fanquake removed the label Needs rebase on Feb 28, 2020
17. DrahtBot added the label Needs rebase on Mar 15, 2021
18. refactor: net: subnet lookup: use single-result LookupHost()

    plus describe single IP subnet case for more clarity

    a989f98d24
19. in src/netbase.cpp:829 in 8509cbad6d outdated

    825@@ -826,14 +826,11 @@ bool LookupSubNet(const std::string& strSubnet, CSubNet& ret)
    826         return false;
    827     }
    828     size_t slash = strSubnet.find_last_of('/');
    829-    std::vector<CNetAddr> vIP;
    830+    CNetAddr network, netmask;
    

    ---

    ---

    

    practicalswift commented at 7:02 pm on March 15, 2021:

    The scope of netmask could be reduce to make review somewhat easier?

    ---

    

    theStack commented at 0:27 am on March 16, 2021:

    Good idea! Done.
20. theStack force-pushed on Mar 16, 2021
21. 
    theStack commented at 0:27 am on March 16, 2021: member

    Rebased on master again and limited the scope of netmask, as suggested by practicalswift.
22. DrahtBot removed the label Needs rebase on Mar 16, 2021
23. fanquake requested review from dongcarl on Oct 11, 2021
24. jonatack approved
25. 
    jonatack commented at 2:18 pm on October 20, 2021: member

    This looks like it is one of your first pulls!

    utACK a989f98d240a84b5c798252acaa4a316ac711189 the patch rebases cleanly to master, the debug build is green, and it is essentially the same patch as c8991f0251dd2a modulo local variable naming, braced initialization, and a comment

26. vasild approved
27. 
    vasild commented at 11:56 am on November 19, 2021: member

    ACK a989f98d240a84b5c798252acaa4a316ac711189
28. laanwj merged this on Dec 6, 2021
29. laanwj closed this on Dec 6, 2021

    ---

30. sidhujag referenced this in commit 75dd73382d on Dec 7, 2021
31. laanwj referenced this in commit c006ab29ce on Dec 18, 2021
32. sidhujag referenced this in commit 08c502ff3c on Dec 18, 2021
33. RandyMcMillan referenced this in commit 8428fc2363 on Dec 23, 2021
34. DrahtBot locked this on Dec 6, 2022

Contributors
theStack MarcoFalke laanwj dongcarl practicalswift DrahtBot jonatack vasild

Review Requested
dongcarl

Labels
Refactoring P2P