Make the global flag *fDiscover* an instance variable of CConnman #16548

1. 
   mmachicao commented at 4:48 PM on August 5, 2019: contributor

   REFERS TO: #14210

   #14210

   • net.h, net.cpp : Removed global flag bool fDiscover

   • net.h, net.cpp : Added field bool fDiscover = true to struct CConnman.Options

   • net.h, net.cpp : Added instance variable bool fDiscover and method bool isDiscover() to CConnman class

   • net.h, net.cpp : Added bool fDiscover to signatures of AddLocal, AdvertiseLocal and IsPeerAddrLocalGood

   • net.h, net.cpp : ThreadMapPort reads fDiscover from the command line, which defaults to true

   • test/net_tests.cpp : Test case LocalAddress_BasicLifecycle runs with fDiscovery = true

   • net_processing.cpp : Made PeerLogicValidation use CConnman.isDiscover()

   • torcontrol.cpp : Added instance variable bool fDiscover to TorController class

   • torcontrol.cpp : TorControllerThread reads fDiscover from the command line, which defaults to true

   • init.h : AppInitMain reads bool fDiscover from the command line and sets CConnman.Option.fDiscovery

2. GOAL: Make the flag *fDiscover* an instance variable of CConnman

    REFEERS TO: #14210
   
    https://github.com/bitcoin/bitcoin/issues/14210
   
    - net.h, net.cpp : Removed global flag bool fDiscover
    - net.h, net.cpp : Added field bool fDiscover = true to struct CConnman.Options
    - net.h, net.cpp : Added instance variable bool fDiscover and method bool isDiscover() to CConnman class
    - net.h, net.cpp : Added bool fDiscover to signatures of AddLocal, AdvertiseLocal and IsPeerAddrLocalGood
    - net.h, net.cpp : ThreadMapPort reads fDiscover from the command line, which defaults to *true*
   
    - test/net_tests.cpp : Test case LocalAddress_BasicLifecycle runs with fDiscovery = true
   
    - net_processing.cpp : Made PeerLogicValidation use CConnman.isDiscover()
   
    - torcontrol.cpp : Added instance variable bool fDiscover to TorController class
    - torcontrol.cpp : TorControllerThread reads fDiscover from the command line, which defaults to *true*
   
    - init.h : AppInitMain reads bool fDiscover from the command line and sets CConnman.Option.fDiscovery

   f46c5a7eed
3. 
   dongcarl commented at 5:18 PM on August 5, 2019: member

   Perhaps you want to take a look at our developer notes for some naming conventions. Note that the naming convention has changed, and newly added code doesn't need to matching surrounding code. :smiley:

4. DrahtBot added the label P2P on Aug 5, 2019
5. DrahtBot added the label Tests on Aug 5, 2019
6. MarcoFalke removed the label Tests on Aug 5, 2019
7. 
   DrahtBot commented at 9:05 PM on August 5, 2019: member

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #17812 (config, test: asmap functional tests and feature refinements by jonatack)
   • #17754 (net: Don't allow resolving of std::string with embedded NUL characters. Add tests. by practicalswift)
   • #17428 (p2p: Try to preserve outbound block-relay-only connections during restart by hebasto)
   • #16702 (p2p: supplying and using asmap to improve IP bucketing in addrman by naumenkogs)
   • #15606 ([experimental] UTXO snapshots by jamesob)
   • #15112 (build: Optionally enable -Wzero-as-null-pointer-constant by Empact)

   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.

8. fanquake requested review from theuni on Aug 6, 2019
9. 
   MarcoFalke commented at 1:20 PM on August 6, 2019: member

   It is not really clear what this is doing and why. Also, it is not clear what this refactoring achieves in the long term.

10. 
    mmachicao commented at 4:17 PM on August 6, 2019: contributor

    It is not really clear what this is doing and why. Also, it is not clear what this refactoring achieves in the long term.

    This PR refers to issue #14210

    The issue being that it is difficult to test the network interface thoroughly.

    Quotes:

    The idea, again, is to be able to run multiple networking instances inside of one bitcoind process.

    CConnman being instantiable like this would allow us to inspect why a CConnman instance might have stopped receiving data rather than just guessing based on pings and timing checks.

    So, I am removing the global dependencies (one by one) until this can be achieved.

11. DrahtBot added the label Needs rebase on Aug 14, 2019
12. 
    laanwj commented at 6:09 PM on October 24, 2019: member

    Concept ACK, I think if it's possible to have certain variables in a smaller scope, instead of globally external throughout the program, that's preferable.

    Perhaps you want to take a look at our developer notes for some naming conventions.

    Yes, please check the naming / coding conventions. We don't use names like fDiscover anymore, so this would be a good opportunity to modernize the naming. Just discover is fine, and m_discover for the instance field.

13. applied coding conventions 485b429c40
14. resolved PR #16548 merge conflicts 6ad22d081d
15. in src/net.cpp:185 in f46c5a7eed outdated

     183 |  {
     184 |      if (fListen && pnode->fSuccessfullyConnected)
     185 |      {
     186 |          CAddress addrLocal = GetLocalAddress(&pnode->addr, pnode->GetLocalServices());
     187 | -        if (gArgs.GetBoolArg("-addrmantest", false)) {
     188 | +        if (gArgs.GetBoolArg("-addrmantest", false))
    

    ---

    ---

    

    laanwj commented at 6:09 PM on October 24, 2019:

    Keep this { at the end of the line

16. DrahtBot removed the label Needs rebase on Nov 13, 2019
17. resolved PR #16548 merge conflicts 3cd4afa7a3
18. DrahtBot added the label Needs rebase on Jan 22, 2020
19. 
    DrahtBot commented at 9:17 PM on January 22, 2020: member

    <!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

20. 
    adamjonas commented at 1:26 AM on May 19, 2020: member

    Hi @mmachicao - pinging for rebase and squashing the merge conflict commits consistent with the notes found in the CONTRIBUTING.md doc. Thanks!

21. 
    MarcoFalke commented at 7:13 AM on August 13, 2020: member

    Closing for now. Let me know when you want to work on this again

22. MarcoFalke closed this on Aug 13, 2020

    ---

23. DrahtBot locked this on Feb 15, 2022

Review Requested
theuni