test: Disable automatic connections per default in the functional tests #22490

1. 
   mzumsande commented at 11:30 pm on July 18, 2021: contributor

   A node normally doesn’t make automatic connections to peers in the functional tests because neither DNS seeds nor hardcoded peers are available on regtest. However, when random entries are inserted into addrman as part of a functional test (e.g. while testing addr relay), ThreadOpenConnections will periodically try to connect to them, resulting in log entries such as: [opencon] [net.cpp:400] [ConnectNode] trying connection 18.166.1.1:8333 lastseen=0.0hrs

   I don’t think it’s desirable that functional tests try to connect to random computers on the internet, aside from the possibility that at some point in time someone out there might actually answer in a way to ruin a test.

   This PR fixes this problem by disabling ThreadOpenConnections by adding -connect=0 to the default args, and adding exceptions only when needed for the test to pass.

2. 
   DrahtBot commented at 11:53 pm on July 18, 2021: contributor

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

   Conflicts

   No conflicts as of last run.

3. DrahtBot added the label Tests on Jul 19, 2021
4. DrahtBot added the label Needs rebase on Jul 19, 2021
5. 
   jnewbery commented at 12:26 pm on July 19, 2021: contributor

   Concept ACK
6. mzumsande force-pushed on Jul 19, 2021
7. mzumsande force-pushed on Jul 19, 2021
8. DrahtBot removed the label Needs rebase on Jul 19, 2021
9. 
   maflcko commented at 4:31 pm on July 19, 2021: member

   Any reason to not disable this globally?

   See also fa730e91576

10. 
    mzumsande commented at 5:25 pm on July 19, 2021: contributor

    Any reason to not disable this globally?

    To expand on the last sentence of the OP (I tried disabling this globally by adding f.write("connect=0\n") to write_config in util.py):

    • With ThreadOpenConnections disabled completely in the functional tests, possible future problems with it would not be noticed there
    • Covering any logic from ThreadOpenConnections in functional tests would not be possible. Tricks like neutralizing a global -connect=0 with a local -noconnect=0 in a single test do not seem to restore the default behavior - am I missing some other way to do this?
    • feature_config_args.py already covers parts from ThreadOpenConnections (log messages), so these tests would have to be removed as well.
11. 
    maflcko commented at 5:36 pm on July 19, 2021: member

    With ThreadOpenConnections disabled completely in the functional tests, possible future problems with it would not be noticed there

    There could be one (unit or function) test to call it?

    Covering any logic from ThreadOpenConnections in functional tests would not be possible. Tricks like neutralizing a global -connect=0 with a local -noconnect=0 in a single test do not seem to restore the default behavior - am I missing some other way to do this?

    There could be a option passed to write_config, similar to extra_config to skip noconnect

    feature_config_args.py already covers parts from ThreadOpenConnections (log messages), so these tests would have to be removed as well.

    See above

12. 
    mzumsande commented at 10:11 am on July 20, 2021: contributor

    Thanks, good idea! I’ll try this, might be a couple of days until I’ll get to it, will change to draft until then.
13. mzumsande marked this as a draft on Jul 20, 2021
14. 
    amitiuttarwar commented at 6:55 pm on July 20, 2021: contributor

    concept ACK
15. 
    practicalswift commented at 8:14 pm on July 24, 2021: contributor

    Super Strong Concept ACK

    The act of running the test suite should never cause any communication with the outside world, and certainly not cause TCP connections to random computers on the Internet :)

16. test: Disable automatic connections by default

    This prevents the node from trying to connect to random IPs on the internet
    while running the functional tests. Exceptions are added when required for
    the test to pass.

    8ca51af1ec
17. mzumsande force-pushed on Jul 26, 2021
18. mzumsande renamed this:
    test: Disable automatic connections when addrman is non-empty
    test: Disable automatic connections per default in the functional tests
    on Jul 26, 2021
19. 
    mzumsande commented at 5:22 pm on July 26, 2021: contributor

    As suggested by @MarcoFalke, I now reversed the logic to disable automatic connections by default and add exceptions only when required (feature_config_args.py and feature_anchors.py). I changed OP and title to reflect this.
20. mzumsande marked this as ready for review on Jul 26, 2021
21. 
    tryphe commented at 10:05 am on July 30, 2021: contributor

    Concept ACK, light code review ACK 8ca51af1ece371b6f1bdb88b96f16020cc787d13
22. maflcko merged this on Jul 30, 2021
23. maflcko closed this on Jul 30, 2021

    ---

24. 
    amitiuttarwar commented at 5:20 pm on July 30, 2021: contributor

    @mzumsande- did you consider the approach of adding -connect=0 as a command line arg in the TestNode initializer (aka around here)?

    I’m thinking something like

    0        if not any("-connect" in s for s in self.extra_args):
    1            self.args.append("-connect=0")
    

    When looking at #22098 (comment), I realized that test doesn’t actually need rebase or an exception added, because passing in something like self.start_node(0, ["-connect=fakenodeaddr"]) from the individual test appends the -connect arg as a command-line argument, which overrules what’s been set in the .conf file.

    So this means the tests setting self.disable_autoconnect would sometimes be unnecessary?

    I think my proposed alternative could simplify the test framework code (don’t have to pass around the disable_autoconnect param), and reduce overhead when implementing an individual test (don’t have to set an additional field, can just pass -connect=whatever). But I didn’t fully hash this out, so I easily could be missing something.

    What do you think?

25. 
    amitiuttarwar commented at 5:42 pm on July 30, 2021: contributor

    hm, I think my suggestion ☝️ does not work for the tests that hit normal automatic connections logic in ThreadOpenConnections. The DNS seeding can be enabled using -dnsseed, but if we pass -connect=anything, ThreadOpenConnections skips using addrman. darn.

    so setting as a command line arg instead of in the conf file is still an option, but we’d need an additional field regardless. still could be nice to have more straightforward logs (instead of having two -connect=x, we’d just have one), but the benefit is definitely reduced compared to what I was thinking.

26. sidhujag referenced this in commit d4463a61cf on Aug 1, 2021
27. 
    mzumsande commented at 2:05 pm on August 22, 2021: contributor

    Sorry for not answering sooner - I had shortly thought about this option before creating this PR, but thought it would fit in better in util.py where similar networking parameters such as dnsseed are set. I didn’t think about the log issue.

    hm, I think my suggestion point_up does not work for the tests that hit normal automatic connections logic in ThreadOpenConnections.

    I agree, it wouldn’t work for tests like feature_anchors.py without a parameter.

    So with the upside of having the networking command-line args in one place, and the downside of having two log items, I don’t have a strong preference here.

28. 
    amitiuttarwar commented at 7:36 pm on August 22, 2021: contributor

    @mzumsande -

    So with the upside of having the networking command-line args in one place, and the downside of having two log items, I don’t have a strong preference here.

    Agreed, let’s leave it as is. Thanks!

29. Fabcien referenced this in commit c58c98c444 on Feb 2, 2022
30. bitcoin locked this on Aug 22, 2022
31. mzumsande deleted the branch on Oct 13, 2023

Contributors
mzumsande DrahtBot jnewbery maflcko amitiuttarwar practicalswift tryphe

Labels
Tests