Skip to content

p2p, refactor: tidy up LookupSubNet() #23219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
laanwj merged 2 commits into from
Dec 18, 2021
Merged

p2p, refactor: tidy up LookupSubNet() #23219

laanwj merged 2 commits into from
Dec 18, 2021

Conversation

@jonatack
Copy link
Member

@jonatack jonatack commented Oct 7, 2021 •
edited
Loading

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.

@jonatack
Copy link
Member Author

jonatack commented Oct 7, 2021

The resolved TODO is from c7f6ce7 by @dongcarl.

    // TODO: Use LookupHost(const char *, CNetAddr&, bool) instead to just get
    //       one CNetAddr.

vasild
vasild reviewed Oct 7, 2021
View reviewed changes
@shaavan
Copy link
Contributor

shaavan commented Oct 7, 2021

Concept ACK

@jonatack jonatack force-pushed the LookupSubNet-todo-fix branch from 7f146ab to eb66966 Compare October 7, 2021 14:19
@jonatack
Copy link
Member Author

jonatack commented Oct 7, 2021 •
edited
Loading

Thanks @vasild, updated with your feedback per git diff 7f146ab aa71b3c

@jonatack jonatack changed the title p2p: remove unneeded CNetAddr vector in LookupSubNet() and update/tidy up p2p: rm CNetAddr vector and copy in LookupSubNet() and update/tidy up Oct 7, 2021
@jonatack jonatack force-pushed the LookupSubNet-todo-fix branch from eb66966 to aa71b3c Compare October 7, 2021 14:31
vasild
vasild approved these changes Oct 7, 2021
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK aa71b3c88ffadf7250a5bacd20d65bf9cc2f1267

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 7, 2021 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@DrahtBot DrahtBot added the P2P label Oct 7, 2021
@DrahtBot DrahtBot mentioned this pull request Oct 8, 2021
Merged
stratospher
stratospher reviewed Oct 8, 2021
View reviewed changes
Copy link
Contributor

@stratospher stratospher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested ACK aa71b3c

Agree that the CNetAddr vector is unnecessary in src/netbase.cpp. The comments and variable name updates look great too! Confirmed that the changes don’t affect the unit tests which include the netbase.h header file. The tests run successfully:

  • addrman_tests.cpp
  • net_tests.cpp
  • netbase_tests.cpp

@fanquake
Copy link
Member

fanquake commented Oct 11, 2021

The same TODO is being solved in #17160.

@laanwj
Copy link
Member

laanwj commented Dec 6, 2021

Needs rebase after #17160

@jonatack jonatack force-pushed the LookupSubNet-todo-fix branch from aa71b3c to 49183e4 Compare December 7, 2021 10:55
@jonatack jonatack changed the title p2p: rm CNetAddr vector and copy in LookupSubNet() and update/tidy up p2p: tidy up LookupSubNet() Dec 7, 2021
@jonatack
Copy link
Member Author

jonatack commented Dec 7, 2021

Thanks, rebased and updated the title and description.

vasild
vasild approved these changes Dec 7, 2021
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 49183e465100715d44f91ca865432ddf71de14fe

@jonatack jonatack changed the title p2p: tidy up LookupSubNet() p2p, refactor: tidy up LookupSubNet() Dec 7, 2021
@jonatack
Copy link
Member Author

jonatack commented Dec 7, 2021

Rebased before the merge causing OOM in the CI and dropped an uneeded param per #23219 (comment).

@jonatack jonatack force-pushed the LookupSubNet-todo-fix branch from 49183e4 to d0e0335 Compare December 7, 2021 12:12
jonatack and others added 2 commits December 7, 2021 13:13
@jonatack
p2p, refactor: tidy up LookupSubNet()
f0c9e68 
- consistent param naming between function declaration and definition
- brackets, param naming and localvar naming per current standards
  in doc/developer-notes.md
- update/improve doxygen documentation in the declaration
- improve comments and other localvar names
- constness
- named args
@vasild @jonatack
p2p, refactor: drop unused DNSLookupFn param in LookupSubnet()
c44c201
@jonatack jonatack force-pushed the LookupSubNet-todo-fix branch from d0e0335 to c44c201 Compare December 7, 2021 12:13
vasild
vasild approved these changes Dec 7, 2021
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK c44c201

laanwj
laanwj reviewed Dec 8, 2021
View reviewed changes
laanwj
laanwj reviewed Dec 8, 2021
View reviewed changes
src/netbase.cpp
}

bool LookupSubNet(const std::string& strSubnet, CSubNet& ret, DNSLookupFn dns_lookup_function)
bool LookupSubNet(const std::string& subnet_str, CSubNet& subnet_out)
Copy link
Member

@laanwj laanwj Dec 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking of it, a better API would be to return option<CSubNet> instead of a bool and output parameter. But not necessarily in this PR. It's more consistent with the other functions in netbase.h as it is now.

Copy link
Member Author

@jonatack jonatack Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I have a branch of netbase follow-ups, could do it there.

@dunxen
Copy link
Contributor

dunxen commented Dec 16, 2021

ACK c44c201

shaavan
shaavan approved these changes Dec 16, 2021
View reviewed changes
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crACK c44c201

@laanwj laanwj merged commit c006ab2 into bitcoin:master Dec 18, 2021
@jonatack jonatack deleted the LookupSubNet-todo-fix branch December 18, 2021 18:28
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 18, 2021
@laanwj
Merge bitcoin#23219: p2p, refactor: tidy up LookupSubNet()
08c502f 
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
@bitcoin bitcoin locked and limited conversation to collaborators Dec 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

+3 more reviewers

@vasild vasild vasild approved these changes

@stratospher stratospher stratospher left review comments

@shaavan shaavan shaavan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@jonatack @shaavan @DrahtBot @fanquake @laanwj @dunxen @vasild @stratospher