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

theStack · 2019-10-16T11:51:21Z

plus describe single IP subnet case for more clarity

maflcko · 2019-10-16T12:11:10Z

This is refactor, right?

theStack · 2019-10-16T12:33:51Z

This is refactor, right?

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

laanwj · 2019-10-16T14:34:56Z

as this concerns DNS: @TheBlueMatt

dongcarl · 2019-10-16T14:50:47Z

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:

bitcoin/src/netbase.cpp

Lines 158 to 172 in c34b886

    
            /** 
        
            * Resolve a host string to its first corresponding network address. 
        
            * 
        
            * @see LookupHost(const char *, std::vector<CNetAddr>&, unsigned int, bool) for 
        
            *      additional parameter descriptions. 
        
            */ 
        
           bool LookupHost(const char *pszName, CNetAddr& addr, bool fAllowLookup) 
        
           { 
        
               std::vector<CNetAddr> vIP; 
        
               LookupHost(pszName, vIP, 1, fAllowLookup); 
        
               if(vIP.empty()) 
        
                   return false; 
        
               addr = vIP.front(); 
        
               return true; 
        
           }

And here's the std::vector<CNetAddr>& version:

bitcoin/src/netbase.cpp

Lines 131 to 156 in c34b886

    
           /** 
        
            * Resolve a host string to its corresponding network addresses. 
        
            * 
        
            * @param pszName The string representing a host. Could be a name or a numerical 
        
            *                IP address (IPv6 addresses in their bracketed form are 
        
            *                allowed). 
        
            * @param[out] vIP The resulting network addresses to which the specified host 
        
            *                 string resolved. 
        
            * 
        
            * @returns Whether or not the specified host string successfully resolved to 
        
            *          any resulting network addresses. 
        
            * 
        
            * @see Lookup(const char *, std::vector<CService>&, int, bool, unsigned int) 
        
            *      for additional parameter descriptions. 
        
            */ 
        
           bool LookupHost(const char *pszName, std::vector<CNetAddr>& vIP, unsigned int nMaxSolutions, bool fAllowLookup) 
        
           { 
        
               std::string strHost(pszName); 
        
               if (strHost.empty()) 
        
                   return false; 
        
               if (strHost.front() == '[' && strHost.back() == ']') { 
        
                   strHost = strHost.substr(1, strHost.size() - 2); 
        
               } 
        
               return LookupIntern(strHost.c_str(), vIP, nMaxSolutions, fAllowLookup); 
        
           }

practicalswift · 2019-10-17T12:35:31Z

FWIW - LookupHost(…) usages:

CConnman::ThreadDNSAddressSeed() → LookupHost(char const*, std::vector<CNetAddr>&, unsigned int, bool)
main → AppInit(int, char**) → AppInitMain(InitInterfaces&) → InitHTTPServer() → LookupHost(char const*, CNetAddr&, bool)
main → AppInit(int, char**) → AppInitMain(InitInterfaces&) → InitHTTPServer() → LookupHost(char const*, CNetAddr&, bool) → LookupHost(char const*, std::vector<CNetAddr>&, unsigned int, bool)
main → AppInit(int, char**) → AppInitMain(InitInterfaces&) → InitHTTPServer() → LookupSubNet(char const*, CSubNet&) → LookupHost(char const*, std::vector<CNetAddr>&, unsigned int, bool)
main → AppInit(int, char**) → AppInitMain(InitInterfaces&) → LookupSubNet(char const*, CSubNet&) → LookupHost(char const*, std::vector<CNetAddr>&, unsigned int, bool)
setban(JSONRPCRequest const&) → LookupHost(char const*, CNetAddr&, bool)
setban(JSONRPCRequest const&) → LookupHost(char const*, CNetAddr&, bool) → LookupHost(char const*, std::vector<CNetAddr>&, unsigned int, bool)
setban(JSONRPCRequest const&) → LookupSubNet(char const*, CSubNet&) → LookupHost(char const*, std::vector<CNetAddr>&, unsigned int, bool)
ThreadMapPort() → LookupHost(char const*, CNetAddr&, bool)
ThreadMapPort() → LookupHost(char const*, CNetAddr&, bool) → LookupHost(char const*, std::vector<CNetAddr>&, unsigned int, bool)

theStack · 2019-10-17T13:11:34Z

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():

bitcoin/src/netbase.cpp

Lines 91 to 93 in c34b886

    
           // If we don't allow lookups, then use the AI_NUMERICHOST flag for 
        
           // getaddrinfo to only decode numerical network addresses and suppress 
        
           // hostname lookups.

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

DrahtBot · 2019-12-16T16:31:49Z

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.

theStack · 2020-02-28T09:04:16Z

Rebased.

src/netbase.cpp

plus describe single IP subnet case for more clarity

theStack · 2021-03-16T00:27:33Z

Rebased on master again and limited the scope of netmask, as suggested by practicalswift.

jonatack

This looks like it is one of your first pulls!

utACK a989f98 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

vasild

ACK a989f98

…LookupHost() a989f98 refactor: net: subnet lookup: use single-result LookupHost() (Sebastian Falbesoner) Pull request description: plus describe single IP subnet case for more clarity ACKs for top commit: jonatack: utACK a989f98 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 vasild: ACK a989f98 Tree-SHA512: 082d3481b1fa5e5f3267b7c4a812954b67b36d1f94c5296fe20110699f053e5042dfa13f728ae20249e9b8d71e930c3b119410125d0faeccdfbdc259223ee3a6

c44c201 p2p, refactor: drop unused DNSLookupFn param in LookupSubnet() (Vasil Dimov) f0c9e68 p2p, refactor: tidy up LookupSubNet() (Jon Atack) Pull request description: This pull originally resolved a code `TO-DO`, as well as fixing different param names between the function declaration and definition, updating the function to current style standards, clearer variable naming, and improving the Doxygen documentation. Following the merge of #17160, it now does the non-`TODO` changes and also now drops an unused param to simplify the function. ACKs for top commit: dunxen: ACK c44c201 vasild: ACK c44c201 shaavan: crACK c44c201 Tree-SHA512: 55f64c7f403819dec84f4da06e63db50f7c0601a2d9a1ec196fda667c220ec6f5ad2a3c95e0e02275da9f6da6b984275d1dc10e19ed82005c5e13da5c5ecab02

c44c201 p2p, refactor: drop unused DNSLookupFn param in LookupSubnet() (Vasil Dimov) f0c9e68 p2p, refactor: tidy up LookupSubNet() (Jon Atack) Pull request description: This pull originally resolved a code `TO-DO`, as well as fixing different param names between the function declaration and definition, updating the function to current style standards, clearer variable naming, and improving the Doxygen documentation. Following the merge of bitcoin#17160, it now does the non-`TODO` changes and also now drops an unused param to simplify the function. ACKs for top commit: dunxen: ACK c44c201 vasild: ACK c44c201 shaavan: crACK c44c201 Tree-SHA512: 55f64c7f403819dec84f4da06e63db50f7c0601a2d9a1ec196fda667c220ec6f5ad2a3c95e0e02275da9f6da6b984275d1dc10e19ed82005c5e13da5c5ecab02

…-result LookupHost() a156de3 refactor: net: subnet lookup: use single-result LookupHost() (Sebastian Falbesoner) Pull request description: plus describe single IP subnet case for more clarity ACKs for top commit: jonatack: utACK a156de3 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 vasild: ACK a156de3 Tree-SHA512: 082d3481b1fa5e5f3267b7c4a812954b67b36d1f94c5296fe20110699f053e5042dfa13f728ae20249e9b8d71e930c3b119410125d0faeccdfbdc259223ee3a6

fanquake added the P2P label Oct 16, 2019

maflcko added the Refactoring label Oct 16, 2019

theStack force-pushed the 20191016-net-simplify_function_lookupsubnet branch from 24e1b7f to 5b1f9f6 Compare October 16, 2019 12:32

theStack changed the title ~~net: subnet lookup: use single-result LookupHost()~~ refactor: net: subnet lookup: use single-result LookupHost() Oct 16, 2019

DrahtBot added the Needs rebase label Jan 22, 2020

theStack force-pushed the 20191016-net-simplify_function_lookupsubnet branch from 5b1f9f6 to 8509cba Compare February 28, 2020 09:04

fanquake removed the Needs rebase label Feb 28, 2020

DrahtBot mentioned this pull request Jun 30, 2020

net: Make DNS lookup mockable, add fuzzing harness #19415

Merged

DrahtBot added the Needs rebase label Mar 15, 2021

practicalswift reviewed Mar 15, 2021

View reviewed changes

src/netbase.cpp Outdated Show resolved Hide resolved

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

a989f98

plus describe single IP subnet case for more clarity

theStack force-pushed the 20191016-net-simplify_function_lookupsubnet branch from 8509cba to a989f98 Compare March 16, 2021 00:26

DrahtBot removed the Needs rebase label Mar 16, 2021

DrahtBot mentioned this pull request Oct 7, 2021

p2p, refactor: tidy up LookupSubNet() #23219

Merged

fanquake requested a review from dongcarl October 11, 2021 01:22

jonatack approved these changes Oct 20, 2021

View reviewed changes

vasild approved these changes Nov 19, 2021

View reviewed changes

laanwj merged commit 786ffb3 into bitcoin:master Dec 6, 2021

bitcoin locked and limited conversation to collaborators Dec 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

theStack commented Oct 16, 2019

Uh oh!

maflcko commented Oct 16, 2019

Uh oh!

theStack commented Oct 16, 2019

Uh oh!

laanwj commented Oct 16, 2019

Uh oh!

dongcarl commented Oct 16, 2019

Uh oh!

practicalswift commented Oct 17, 2019

Uh oh!

theStack commented Oct 17, 2019

Uh oh!

DrahtBot commented Dec 16, 2019 •

edited

Loading

Uh oh!

theStack commented Feb 28, 2020

Uh oh!

Uh oh!

theStack commented Mar 16, 2021

Uh oh!

jonatack left a comment

Uh oh!

vasild left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

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

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

Uh oh!

Conversation

theStack commented Oct 16, 2019

Uh oh!

maflcko commented Oct 16, 2019

Uh oh!

theStack commented Oct 16, 2019

Uh oh!

laanwj commented Oct 16, 2019

Uh oh!

dongcarl commented Oct 16, 2019

Uh oh!

practicalswift commented Oct 17, 2019

Uh oh!

theStack commented Oct 17, 2019

Uh oh!

DrahtBot commented Dec 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

theStack commented Feb 28, 2020

Uh oh!

Uh oh!

theStack commented Mar 16, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

DrahtBot commented Dec 16, 2019 •

edited

Loading