Add vConnect to CConnman::Options #10596

pull benma wants to merge 1 commits into bitcoin:master from benma:connmanoptions_connect changing 3 files +24 −8
  1. benma commented at 8:18 AM on June 15, 2017: none

    Split the "-connect" argument parsing out of CConnman and put it into AppInitMain().

  2. fanquake added the label P2P on Jun 15, 2017
  3. benma commented at 8:59 AM on June 15, 2017: none

    @theuni please take a look if you can.

  4. in src/net.h:298 in 846660920b outdated 
     293 | @@ -292,7 +294,7 @@ class CConnman
 294 |      void ThreadOpenAddedConnections();
 295 |      void AddOneShot(const std::string& strDest);
 296 |      void ProcessOneShot();
 297 | -    void ThreadOpenConnections();
 298 | +    void ThreadOpenConnections(std::vector<std::string> connect);
    sipa commented at 11:28 PM on June 16, 2017:

    Pass by const reference?

    theuni commented at 11:52 PM on June 16, 2017:

    std::thread takes arguments by value/rvalue only. Taking a reference would be dangerous as the arg could go out of scope before it's used in the thread.

    Edit: Thinking more about this, std::thread's constructor turns references into copies anyway, so I suppose a const reference here would just extend the lifetime of the copy as usual.

    benma commented at 11:52 AM on June 17, 2017:

    I also read that the reference is turned into a copy, but I figured it's better to be explicit about it being a copy. I also didn't use a pointer because dealing with its lifetime is not worth it.

    laanwj commented at 11:38 AM on June 21, 2017:

    I also prefer it this way, explicitly.

  5. theuni commented at 11:43 PM on June 16, 2017: member

    @benma Thanks, will review asap.

  6. in src/init.cpp:1670 in 846660920b outdated 
    1663 | @@ -1664,6 +1664,13 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
1664 |      if (gArgs.IsArgSet("-seednode")) {
1665 |          connOptions.vSeedNodes = gArgs.GetArgs("-seednode");
1666 |      }
1667 | +    if (gArgs.IsArgSet("-connect")) {
1668 | +        // Initiate outbound connections unless connect=0
1669 | +        const auto connect = gArgs.GetArgs("-connect");
1670 | +        connOptions.bNoConnect = connect.size() == 1 && connect[0] == "0";
    theuni commented at 8:48 PM on June 20, 2017:

    I didn't mention it on your first PRs because I didn't want to scare you away, but we've just agreed to change our code style from hungarian to snake_case. Mind updating? See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md.

    Don't worry about changing everything around this code, just what you're touching.

    benma commented at 10:54 PM on July 17, 2017:

    Done (I originally thought it was better to keep the local style and fix them all in batches)

  7. in src/init.cpp:1671 in 846660920b outdated 
    1663 | @@ -1664,6 +1664,13 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
1664 |      if (gArgs.IsArgSet("-seednode")) {
1665 |          connOptions.vSeedNodes = gArgs.GetArgs("-seednode");
1666 |      }
1667 | +    if (gArgs.IsArgSet("-connect")) {
1668 | +        // Initiate outbound connections unless connect=0
1669 | +        const auto connect = gArgs.GetArgs("-connect");
1670 | +        connOptions.bNoConnect = connect.size() == 1 && connect[0] == "0";
1671 | +        if (!connOptions.bNoConnect)
    theuni commented at 9:00 PM on June 20, 2017:

    Please rename this so that they're more clear, since this is exposed outside of CConnman now. connOptions.vConnect -> connOptions.m_specified_outgoing connOptions.bNoConnect -> (!) connOptions.m_use_addrman_outgoing

    benma commented at 10:54 PM on July 17, 2017:

    Done.

  8. in src/net.cpp:2326 in 846660920b outdated 
    2322 | @@ -2323,9 +2323,8 @@ bool CConnman::Start(CScheduler& scheduler, std::string& strNodeError, Options c
2323 |      // Initiate outbound connections from -addnode
2324 |      threadOpenAddedConnections = std::thread(&TraceThread<std::function<void()> >, "addcon", std::function<void()>(std::bind(&CConnman::ThreadOpenAddedConnections, this)));
2325 |  
2326 | -    // Initiate outbound connections unless connect=0
2327 | -    if (!gArgs.IsArgSet("-connect") || gArgs.GetArgs("-connect").size() != 1 || gArgs.GetArgs("-connect")[0] != "0")
2328 | -        threadOpenConnections = std::thread(&TraceThread<std::function<void()> >, "opencon", std::function<void()>(std::bind(&CConnman::ThreadOpenConnections, this)));
2329 | +    if (!connOptions.bNoConnect)
    theuni commented at 9:08 PM on June 20, 2017:

    All outbound connections are moving to one thread soon, but for now, we should return an error if connOptions.m_use_addrman_outgoing && !connOptions.m_specified_outgoing.empty().

    Also, If addrman isn't being used for outgoing and there are no specified connections (incoming connections only), we can just skip creating the thread completely.

    benma commented at 10:56 PM on July 17, 2017:

    Done, but should the error maybe be an assert? That code path cannot be triggered and is not meaningful to the user. If other things want to use Connman and mess it up, it's a coding error and they'll see the assert.

    theuni commented at 12:35 AM on July 18, 2017:

    It will be possible (and reasonable) when vAddedNodes is added as an connman option like the others. It might be a good next target :)

    benma commented at 9:31 PM on July 31, 2017:

    What does it have to do with vAddedNodes? The specifc new error that is introduced doesn't depend on it.

  9. theuni commented at 9:13 PM on June 20, 2017: member

    Concept ACK. Looks good, just a few nits.

  10. benma commented at 9:58 AM on June 22, 2017: none

    @theuni just to let you know that I'll address everything in about two weeks (will be pretty much offline until then). Cheers.

  11. benma force-pushed on Jul 17, 2017
  12. in src/init.cpp:1657 in 89bee2456b outdated 
    1650 | @@ -1651,7 +1651,14 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
1651 |      if (gArgs.IsArgSet("-seednode")) {
1652 |          connOptions.vSeedNodes = gArgs.GetArgs("-seednode");
1653 |      }
1654 | -
1655 | +    if (gArgs.IsArgSet("-connect")) {
1656 | +        // Initiate outbound connections unless connect=0
1657 | +        const auto connect = gArgs.GetArgs("-connect");
1658 | +        connOptions.m_use_addrman_outgoing = connect.empty();
    theuni commented at 12:46 AM on July 18, 2017:

    connOptions.m_use_addrman_outgoing should be false if !connect.size() == 1 && connect[0] == "0"

    benma commented at 10:05 AM on July 18, 2017:

    Right! Fixed.

  13. benma force-pushed on Jul 18, 2017
  14. Add vConnect to CConnman::Options
    Split the "-connect" argument parsing out of CConnman and put it into
AppInitMain().
    352d582ba2
  15. in src/net.h:148 in 4286fd4271 outdated 
     144 | @@ -145,6 +145,8 @@ class CConnman
 145 |          std::vector<std::string> vSeedNodes;
 146 |          std::vector<CSubNet> vWhitelistedRange;
 147 |          std::vector<CService> vBinds, vWhiteBinds;
 148 | +        bool m_use_addrman_outgoing = true;
    theuni commented at 5:52 PM on July 19, 2017:

    I'd rather not do this, as everything else here is set to null, false, or uninitialized. A value of true is unexpected.

    benma commented at 9:36 PM on July 19, 2017:

    Done. Note that now, the default options are to not make any outbound connections (I put it to true to have it make outbound connections by default, like bitcoind does).

  16. benma force-pushed on Jul 19, 2017
  17. in src/init.cpp:1659 in 352d582ba2 
    1655 | +    // Initiate outbound connections unless connect=0
1656 | +    connOptions.m_use_addrman_outgoing = !gArgs.IsArgSet("-connect");
1657 | +    if (!connOptions.m_use_addrman_outgoing) {
1658 | +        const auto connect = gArgs.GetArgs("-connect");
1659 | +        if (connect.size() != 1 || connect[0] != "0") {
1660 | +            connOptions.m_specified_outgoing = connect;
    theuni commented at 10:10 PM on July 19, 2017:

    this is broken again if connect.size() == 1 && connect[0] == "0" :)

    benma commented at 7:23 AM on July 20, 2017:

    How? If connect.size() == 1 && connect[0] == "0", then m_use_addrman_outgoing = false and m_specified_outgoing is empty, and the thread is not started.

    theuni commented at 9:24 PM on July 20, 2017:

    Yes sorry, nevermind.

  18. benma commented at 9:35 PM on July 26, 2017: none

    @theuni does the PR look good to you?

  19. theuni commented at 2:38 PM on July 31, 2017: member

    utACK 352d582ba240b825cb834cdde041864bafca0e21. This will need to go in after the 0.15 split though, due to the new strings.

  20. benma commented at 9:32 PM on July 31, 2017: none

    @theuni thanks. Did you mean this string? If so, maybe we can just remove it (resp. replace it with an assertion)?

    See #10596 (review) (sorry for the late response, somehow I missed it).

  21. laanwj merged this on Sep 6, 2017
  22. laanwj closed this on Sep 6, 2017
  23. laanwj referenced this in commit 6866b4912b on Sep 6, 2017
  24. chefsaroar commented at 5:56 PM on March 24, 2019: none

    Add BTC my wallet

  25. PastaPastaPasta referenced this in commit dfb03d6c9c on Sep 20, 2019
  26. PastaPastaPasta referenced this in commit f67bc54d30 on Sep 23, 2019
  27. PastaPastaPasta referenced this in commit fd6f830e3c on Sep 23, 2019
  28. PastaPastaPasta referenced this in commit 13ddb23367 on Sep 24, 2019
  29. PastaPastaPasta referenced this in commit e89a0f2697 on Nov 19, 2019
  30. PastaPastaPasta referenced this in commit aa4ce3dfd7 on Nov 21, 2019
  31. PastaPastaPasta referenced this in commit 36595a725b on Dec 9, 2019
  32. PastaPastaPasta referenced this in commit 277b70205a on Jan 1, 2020
  33. PastaPastaPasta referenced this in commit a9ffdd6d8c on Jan 2, 2020
  34. PastaPastaPasta referenced this in commit b922d60d81 on Jan 2, 2020
  35. PastaPastaPasta referenced this in commit 781c68af1b on Jan 2, 2020
  36. PastaPastaPasta referenced this in commit b404a2a0df on Jan 2, 2020
  37. PastaPastaPasta referenced this in commit c3ab28848b on Jan 2, 2020
  38. PastaPastaPasta referenced this in commit f6c0d153f7 on Jan 2, 2020
  39. PastaPastaPasta referenced this in commit 39fc297eed on Jan 2, 2020
  40. PastaPastaPasta referenced this in commit e4fef1fd33 on Jan 3, 2020
  41. PastaPastaPasta referenced this in commit 2a6edc19ad on Jan 10, 2020
  42. ckti referenced this in commit 1f732e4fba on Mar 28, 2021
  43. furszy referenced this in commit 97728e5673 on Dec 7, 2021
  44. DrahtBot locked this on Dec 16, 2021
Contributors
benmasipatheunilaanwjchefsaroar
Labels
P2P
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-22 06:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me