test: Disable automatic connections per default in the functional tests

[mzumsande]

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.

[DrahtBot]

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

Conflicts

No conflicts as of last run.

[jnewbery]

Concept ACK

[maflcko]

Any reason to not disable this globally?

See also fa730e9

[mzumsande]

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.

[maflcko]

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

[mzumsande]

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.

[amitiuttarwar]

concept ACK

[practicalswift]

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

[mzumsande]

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.

[tryphe]

Concept ACK, light code review ACK 8ca51af

[amitiuttarwar]

@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

        if not any("-connect" in s for s in self.extra_args):
            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?

[amitiuttarwar]

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.

[mzumsande]

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.

[amitiuttarwar]

@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!